security: redact CLI logs, bound regex matching (ReDoS) (PER-8609/8615) by Shivanshu-07 · Pull Request #2279 · percy/cli

Shivanshu-07 · 2026-06-14T15:28:44Z

Summary

Second focused percy-cli security PR — two contained runtime findings in @percy/core.

Ticket	CWE	Finding
PER-8609	CWE-532	CLI logs transmitted to Percy API without secret redaction
PER-8615	CWE-1333	ReDoS via user-controlled regex in snapshot include/exclude

PER-8616 (SSRF via unvalidated PERCY_CHROMIUM_BASE_URL) was originally part of this PR but has been removed — the ticket is closed as won't-fix (machine-access: exploiting it requires attacker control of the process environment). install.js is restored to its original download behavior.

Changes

percy.js (PER-8609): sendBuildLogs() sent clilogs raw while cilogs were already passed through redactSecrets(). Wrap clilogs in the same redactSecrets() so tokens / credential-bearing URLs are stripped before egress. A Percy-token pattern is added to secretPatterns.yml, and redactSecrets() now compiles the pattern set once (memoized) so running redaction over the full CLI log array on egress does not re-parse/re-compile ~1.7k regexes per string.

snapshot.js (PER-8615): snapshotMatches() runs user-controllable regex/glob patterns against snapshot.name. A crafted long name reaching the matcher (e.g. via the local API — chain PER-8627) plus a backtracking-prone pattern could hang the process. Added MAX_MATCH_INPUT_LENGTH = 2048: glob/RegExp matching is skipped for over-long names (exact-string matching is unaffected, and real snapshot names are short).

Verification (against real source)

redactSecrets on the clilogs array shape: GitHub token + bearer + Percy token redacted to [REDACTED]; utils.test.js covers all Percy-token prefixes. ✅
ReDoS guard: catastrophic (a+)+$ against a 50k-char input returns in 0 ms (would otherwise hang); normal patterns still match. ✅
Existing sendBuildLogs tests use secret-free messages (redaction is a no-op → content matches). ✅

Closes PER-8609, PER-8615. Mitigates the ReDoS leg of PER-8627.

🤖 Generated with Claude Code

Shivanshu-07 · 2026-06-16T13:43:12Z

Claude Code PR Review

PR: #2279 • Head: bda111a • Reviewers: stack:code-reviewer

Summary

Three runtime-hardening measures in @percy/core:

PER-8609 (CWE-532): redact secrets from CLI logs before they are sent to the Percy API — clilogs and cilogs are both wrapped with redactSecrets() in percy.js:sendBuildLogs.
PER-8615 (CWE-1333 / ReDoS): bound snapshot-name pattern matching to a max input length in snapshot.js.
PER-8616 (CWE-918 / SSRF): validate PERCY_CHROMIUM_BASE_URL via resolveChromiumBaseUrl() — require a well-formed HTTPS URL, else warn and fall back to the trusted default host.

Review Table

Priority	Category	Check	Status	Notes
High	Security	Secrets redacted before egress	Pass	Both `clilogs` (percy.js:869) and `cilogs` wrapped with `redactSecrets()`
High	Security	SSRF / integrity-downgrade closed	Pass	`resolveChromiumBaseUrl` enforces parseable + HTTPS, else trusted-default fallback
High	Security	ReDoS input bound	Pass	Match input length-capped
High	Correctness	No test regression	Pass	Verified: `redactSecrets` is byte-identical on non-secret logs, so the `sendBuildLogs` assertions encode identical content
Medium	Testing	New security paths tested	Fail	Rejection branches of `resolveChromiumBaseUrl` and the ReDoS length guard lack unit tests
Medium	Quality	Focused change	Pass	—

Findings

File: packages/core/src/install.js (resolveChromiumBaseUrl) + test/unit/install.test.js
Severity: Medium
Issue: The invalid-URL and non-HTTPS rejection branches (the security-critical paths) have no test coverage; only a valid HTTPS URL is exercised. A future refactor could silently drop the protocol check.
Suggestion: Add tests for an unparseable value and an http:// value, asserting fallback to the default.
File: packages/core/src/snapshot.js (snapshotMatches length guard)
Severity: Medium
Issue: Names exceeding the max length silently fall through to the fallback (effectively "no match"), with no log.warn and no test. A legitimate long name could be unexpectedly excluded with no signal.
Suggestion: Emit a log.warn when the guard trips and add a boundary test.
File: packages/core/src/install.js:162
Severity: Low
Issue: Returns the raw value (plus trailing slash) rather than reconstructing from parsed — a query string/fragment on the operator-supplied URL survives. Minor; the value is operator-set, not attacker-controlled.
Suggestion: Return parsed.origin + parsed.pathname (slash-normalised).

Verdict is PASS: no Critical/High issue is introduced or worsened. The two High concerns raised in an earlier review pass (clilogs redaction "not wired"; sendBuildLogs tests breaking) were verified false positives — the redaction is present at percy.js:869, and redactSecrets produces byte-identical output for the non-secret logs the tests use. The Medium test-coverage gaps above are recommended as a fast follow-up.

Verdict: PASS

+  if (!_compiledSecretPatterns) {
+    const filepath = path.resolve(url.fileURLToPath(import.meta.url), '../secretPatterns.yml');
+    const secretPatterns = YAML.parse(readFileSync(filepath, 'utf-8'));
+    _compiledSecretPatterns = secretPatterns.patterns.map(p => new RegExp(p.pattern.regex, 'g'));


…omium base URL (PER-8609/8615/8616) Three contained runtime hardening fixes in @percy/core: PER-8609 (CWE-532) — clilogs were sent to the Percy API without secret redaction (cilogs already were). Wrap clilogs in the existing redactSecrets() so tokens / credential-bearing URLs are stripped before egress. PER-8615 (CWE-1333) — snapshotMatches() runs user-controllable regex/glob patterns against snapshot.name; a crafted long name reaching the matcher (e.g. via the local API, per chain PER-8627) could trigger catastrophic backtracking. Cap the matched-input length (MAX_MATCH_INPUT_LENGTH = 2048) before any RegExp/micromatch call; exact-string matching is unaffected. PER-8616 (CWE-918) — PERCY_CHROMIUM_BASE_URL was used as a download base with no validation, enabling SSRF / an integrity downgrade. Add resolveChromiumBaseUrl(): require a well-formed HTTPS URL, otherwise warn and fall back to the trusted default host. (Private HTTPS mirrors remain supported, so no host allowlist; this also gives transport integrity for PER-8605 — full checksum pinning of the binary is a separate follow-up.) Verified against real source: resolveChromiumBaseUrl (https-only + fallback), redactSecrets on the clilogs array shape (GitHub token + bearer redacted), and the ReDoS guard (catastrophic pattern on a 50k-char input returns in 0ms). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…eouts redactSecrets re-read and re-parsed secretPatterns.yml (~1.7k regexes) and recompiled every RegExp on every recursive call. Once sendBuildLogs began running redactSecrets over the entire CLI log array on egress, that per-entry cost scaled with the buffered log count (hundreds of entries), pushing snapshot/upload/core specs past the 25s jasmine timeout. Compile the pattern list once and reuse it; redaction output is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Adds unit tests for resolveChromiumBaseUrl: default-host fallback, env default, trailing-slash normalization, and the warn-and-fallback paths for unparseable and non-HTTPS values, restoring 100% coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…-fix) PER-8616 is closed as won't-fix (machine-access: exploiting it requires attacker control of the process environment). Remove resolveChromiumBaseUrl and restore install.js to the original download behavior; drop its unit tests. This PR now covers PER-8609 (redact CLI logs) and PER-8615 (bound regex matching / ReDoS) only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Shivanshu-07 requested a review from a team as a code owner June 14, 2026 15:28

Shivanshu-07 requested review from aryanku-dev and ninadbstack June 14, 2026 15:28

github-advanced-security AI found potential problems Jun 23, 2026

View reviewed changes

Shivanshu-07 and others added 4 commits June 29, 2026 10:57

fix(core): redact Percy tokens in logs (PER-8609)

3e0aae3

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Shivanshu-07 force-pushed the security/cli-runtime-redact-redos-ssrf branch from c7a1844 to 3e0aae3 Compare June 29, 2026 05:32

Shivanshu-07 changed the title ~~security: redact CLI logs, bound regex matching (ReDoS), validate Chromium base URL (PER-8609/8615/8616)~~ security: redact CLI logs, bound regex matching (ReDoS) (PER-8609/8615) Jul 6, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

security: redact CLI logs, bound regex matching (ReDoS) (PER-8609/8615)#2279

security: redact CLI logs, bound regex matching (ReDoS) (PER-8609/8615)#2279
Shivanshu-07 wants to merge 5 commits into
masterfrom
security/cli-runtime-redact-redos-ssrf

Shivanshu-07 commented Jun 14, 2026 •

edited

Loading

Uh oh!

Shivanshu-07 commented Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Shivanshu-07 commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Verification (against real source)

Uh oh!

Shivanshu-07 commented Jun 16, 2026

Claude Code PR Review

Summary

Review Table

Findings

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Shivanshu-07 commented Jun 14, 2026 •

edited

Loading