Repository navigation
fix(desktop-test): do not redact short secret-named environment values - #4401
Open
Jason Morse (JasonVMo) wants to merge 1 commit into
Open
Jason Morse (JasonVMo) wants to merge 1 commit into
Jason Morse (JasonVMo) wants to merge 1 commit into
Conversation
NodeDesktopCommandRunner registered the value of every environment variable named like a token, secret, password, nonce, or authorization header as a secret. A value such as `true` then rewrote machine-readable output (`"delegated":true` became `"delegated":[REDACTED]`). That fails the delegated-cli contract in the Office main pipeline, where the environment carries such a variable, and stops the stages after the tests, including Component Governance scanning. Register only values of at least 8 characters. Explicit addSecret calls are unchanged. The new command-secrets contract covers a flag, a 7-character value, and an 8-character value, and fails without the change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6f34858-06bf-4230-8be7-1f65402b208a
Contributor
Bundle size reportTree-shaken, minified production esbuild bundles with React and React Native runtimes externalized.
The job is advisory: size changes are reported but do not fail the pull request. Bundle or analysis errors still fail. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Platforms Impacted
No platform runtime code changes. This fixes
@fluentui-react-native/desktop-test, the development-time desktop test harness.Description of changes
NodeDesktopCommandRunnerregisters the value of every environment variable whose name containsnonce,token,password,secret, orauthorizationas a secret and redacts it from all command output. When such a variable holds a flag-like value such astrue, everytruein the output is rewritten. A nested CLI's machine output{"runId":"...","delegated":true,"exitCode":0}becomes"delegated":[REDACTED]and no longer parses, which fails thedelegated-clicontract test added in #4389. The test passes locally and on GitHub-hosted checks but fails in the Officemainpipeline, whose environment evidently has such a variable. The rewrite also reaches the realevents.jsonljournal andcommands/*.logfiles, so this is a product bug and not only a test problem.Why it matters now. Of the 12
mainbuilds since #4389 landed on 2026-10-23, 4 failed on this test and 8 failed earlier atyarn install(the Office feed returned 404 forstorybook@10.6.3). Installs have succeeded since 2026-10-27, and each of those builds then failed on this test. A failedmainbuild skips the stages after the tests, including Component Governance scanning, so alerts that earlier PRs already fixed cannot clear. That includes the legal-information alert for@fluentui-react-native/tester-win32@0.1.0, which #4399 addressed (version0.0.0and"license": "MIT") and which only a successful scan ofmaincan close.Changes:
packages/agentic/desktop-test/src/commands.ts: register an environment value as a secret only when it is at least 8 characters long (MIN_ENVIRONMENT_SECRET_LENGTH). Real credentials, passwords, and nonces are long; flags and counters such astrueor0are not. ExplicitaddSecretcalls are unchanged and still apply to values of any length.packages/agentic/desktop-test/src/contracts.contract.cjsandcontracts.test.ts: newcommand-secretscontract. It runs a real child throughNodeDesktopCommandRunnerwith secret-named variables set totrue, a 7-character value, and an 8-character value, and asserts that the JSON output stays parseable, the 7-character value is untouched, and the 8-character value is redacted.packages/agentic/desktop-test/references/reporting.md: documents the rule..changeset/desktop-test-env-secret-length.md: patch changeset for@fluentui-react-native/desktop-test.Trade-off: a real secret shorter than 8 characters that reaches a child only through a secret-named environment variable is no longer registered automatically. Pattern-based redaction of
key=valueassignments forpassword,authorization,access-token, and the nonce keys still applies, and callers can register any value explicitly withaddSecret.Merge note: this is expected to turn the
mainpipeline green, which will run the stages after the tests (pack, publish, Component Governance) for the first time since 2026-10-22. Other issues behind this one are possible, and any pending changesets will be published by that run.Verification
Run on Windows against the final tree:
yarn format,yarn lint,yarn buildinpackages/agentic/desktop-testyarn testinpackages/agentic/desktop-testcommand-secretscontract anddelegated-cli.yarn change:checkyarn clean-lage-cache, thenyarn lage buildci --continue --concurrency 4check-package-contentsreportsspawnSync yarn ENOENTon Windows becausescripts/src/tasks/checkPackageContentsTask.tscallsexecFileSync('yarn')without a shell. That predates this PR; with a session-only preload that launchesyarn.cmd,node ./scripts/src/cli.ts check-package-contentsverified all 8 guided packages.Manual check, run in
packages/agentic/desktop-testafteryarn build:0(the previous behavior),node src/contracts.contract.cjs command-secretsfails, and so doesnode src/contracts.contract.cjs delegated-cliwithZZ_ACCESS_TOKEN_ENABLED=trueset. Both reportSyntaxError: Unexpected token 'R', ..."legated":[REDACTED],"... is not valid JSON, the same error as themainpipeline. With the fix, both pass.true, the output{"delegated":true}is rewritten to{"delegated":[REDACTED]}and fails to parse.Pull request checklist
This PR has considered (when applicable):