Redact secrets from CLI logs, not just CI logs - #2412
Open
ninadbstack wants to merge 2 commits into
Open
Conversation
sendBuildLogs applied redactSecrets to cilogs but sent clilogs through untouched. CLI log entries can carry upstream response text — SDK errors interpolate remote response fields into their messages — and /logs content is retrievable via GET /api/v1/logs by anyone with build read access, so the half that most often holds foreign response data was the unredacted half. Also memoize the pattern file. redactSecrets recurses per log entry and re-read + re-parsed the ~1750-rule YAML and recompiled every regex on each call; that was tolerable for cilogs alone but not once clilogs (the larger set) goes through it. Patterns are now parsed and compiled once. Reusing the global regexes across replace() calls is safe — replace() resets lastIndex on a global pattern. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Master landed the same fix in #2279 (security: redact CLI logs, bound regex matching). Both conflicting hunks resolved in favour of master's version, which is a superset of this branch's: same redactSecrets call on clilogs in sendBuildLogs, same compile-patterns-once memoization, plus in-place entry mutation, the ReDoS bound and a semgrep annotation this branch didn't have. What remains of this branch is the integration-level sendBuildLogs redaction test; master's coverage for #2279 is unit-level in test/unit/utils.test.js. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| percy.build = { id: 1 }; | ||
| // A CLI-side log entry can carry upstream response text, so it needs the | ||
| // same redaction cilogs already gets. | ||
| percy.log.info('leaked from upstream: ASIAY34FZKBOKMUTVV7A'); |
| // A CLI-side log entry can carry upstream response text, so it needs the | ||
| // same redaction cilogs already gets. | ||
| percy.log.info('leaked from upstream: ASIAY34FZKBOKMUTVV7A'); | ||
| percy.log.info('ci side: ASIAY34FZKBOKMUTVV7A', {}, true); |
|
|
||
| const clilogs = JSON.stringify(sent.clilogs); | ||
| const cilogs = JSON.stringify(sent.cilogs); | ||
| expect(clilogs).not.toContain('ASIAY34FZKBOKMUTVV7A'); |
| const cilogs = JSON.stringify(sent.cilogs); | ||
| expect(clilogs).not.toContain('ASIAY34FZKBOKMUTVV7A'); | ||
| expect(clilogs).toContain('[REDACTED]'); | ||
| expect(cilogs).not.toContain('ASIAY34FZKBOKMUTVV7A'); |
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
2 participants
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.
What
sendBuildLogsappliedredactSecretstocilogsbut sentclilogsthrough untouched:CLI log entries are the ones that can carry foreign response text — SDK error messages interpolate fields from remote HTTP responses — and
/logscontent is retrievable viaGET /api/v1/logsby anyone who passesauthorize(build, :read?), i.e. any project member. There is no server-side redaction on that path. So the half most likely to hold upstream data was the half we weren't redacting.This routes
clilogsthrough the same existingredactSecretshelper. No new patterns, no new mechanism —secretPatterns.ymlalready ships ~1750 rules including AWS Access Key ID / cred-file shapes.Why the memoization is in the same PR
redactSecretsrecurses per log entry, and each call didreadFileSync+YAML.parseof the ~1750-rule pattern file and recompiled every regex. That was already wasteful forcilogs; sendingclilogs(the larger set) through it would have made it a real cost on big builds. Patterns are now parsed and compiled once.Reusing the
/gregexes acrossreplace()calls is safe —String.prototype.replaceresetslastIndexon a global pattern, verified:Behavior notes
[REDACTED].cilogspath and matches the contract documented atpackages/logger/src/logger.js:137-146.Testing
Added
redacts secrets from CLI logs, not just CI logstopackages/core/test/percy.test.js— decodes the actual posted/logspayload and asserts the key shape is absent fromclilogsand[REDACTED]is present.Draft because I could not run the suite —
node_modulesisn't installed in my environment and installing deps is out of scope there. Files passnode --check; the regex-reuse claim above is verified standalone. Please let CI run before marking ready.Context
Found while assessing PER-9354. That chain finding itself does not reproduce (details in the ticket) — this is an independent gap surfaced along the way, and it stands on its own merits.
🤖 Generated with Claude Code