security: redact CLI logs, bound regex matching (ReDoS), validate Chromium base URL (PER-8609/8615/8616)

[Shivanshu-07]

Summary

Second focused percy-cli security PR — three contained runtime findings (deadline 2026-06-16) 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	CWE-918	SSRF via unvalidated PERCY_CHROMIUM_BASE_URL download target

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.

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).

install.js (PER-8616): Added resolveChromiumBaseUrl() — PERCY_CHROMIUM_BASE_URL must be a well-formed HTTPS URL, otherwise it warns and falls back to the trusted default host. Private HTTPS mirrors remain supported (the doctor check documents them), so no host allowlist.

Verification (against real source)

• resolveChromiumBaseUrl: https mirror kept; http://, ftp://, and malformed values rejected → default. ✅
• redactSecrets on the clilogs array shape: GitHub token + bearer redacted to [REDACTED]. ✅
• 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); the Chromium with custom URL test uses an HTTPS URL (compatible).

PER-8605 (Chromium binary integrity) is partially addressed here — HTTPS is now enforced, giving transport integrity from the trusted host. Full checksum pinning of the downloaded binary (per platform/revision) is a separate follow-up since it needs canonical hashes for all 5 platform builds.

Closes PER-8609, PER-8615, PER-8616. Partially addresses PER-8605; mitigates the ReDoS leg of PER-8627.

🤖 Generated with Claude Code

[Shivanshu-07]

Claude Code PR Review

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

Summary

Three runtime-hardening measures in @percy/core:

1. 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.
2. PER-8615 (CWE-1333 / ReDoS): bound snapshot-name pattern matching to a max input length in snapshot.js.
3. 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