diff --git a/packages/core/src/discovery.js b/packages/core/src/discovery.js index cf4f5c6c0..797ee8d81 100644 --- a/packages/core/src/discovery.js +++ b/packages/core/src/discovery.js @@ -8,6 +8,7 @@ import { createRootResource, createPercyCSSResource, createLogResource, + redactSecrets, yieldAll, snapshotLogName, waitForTimeout, @@ -218,8 +219,10 @@ function processSnapshotResources({ domSnapshot, resources, ...snapshot }) { // For multi dom root resources are stored as array resources = resources.flat(); - // include associated snapshot logs matched by meta information - resources.push(createLogResource(logger.snapshotLogs(snapshot.meta.snapshot))); + // include associated snapshot logs matched by meta information. Redact + // secrets before egress — this per-snapshot log resource is a parallel + // egress path to sendBuildLogs and must scrub tokens/credentials too (CWE-532). + resources.push(createLogResource(redactSecrets(logger.snapshotLogs(snapshot.meta.snapshot)))); logger.evictSnapshot(snapshot.meta.snapshot); if (process.env.PERCY_GZIP) { diff --git a/packages/core/src/percy.js b/packages/core/src/percy.js index 1bab43d7a..910befc0f 100644 --- a/packages/core/src/percy.js +++ b/packages/core/src/percy.js @@ -863,7 +863,10 @@ export class Percy { if (!process.env.PERCY_TOKEN) return; try { const logsObject = { - clilogs: logger.query(log => !['ci'].includes(log.debug)) + // Redact secrets from CLI logs before egress to the Percy API — these + // can contain tokens or URLs with embedded credentials (CWE-532). The + // cilogs below were already redacted; clilogs were not. + clilogs: redactSecrets(logger.query(log => !['ci'].includes(log.debug))) }; // Only add CI logs if not disabled voluntarily. diff --git a/packages/core/src/secretPatterns.yml b/packages/core/src/secretPatterns.yml index 12bc3e089..9e82b19d6 100644 --- a/packages/core/src/secretPatterns.yml +++ b/packages/core/src/secretPatterns.yml @@ -7021,4 +7021,8 @@ patterns: - pattern: name: Bitcoin Address regex: '^[13][a-km-zA-HJ-NP-Z0-9]{26,33}$' - confidence: high \ No newline at end of file + confidence: high + - pattern: + name: Percy Token + regex: '\b(web|app|auto|ss|vmw|res)_[A-Za-z0-9]{20,}' + confidence: high diff --git a/packages/core/src/snapshot.js b/packages/core/src/snapshot.js index 8d23e9352..703f86ab5 100644 --- a/packages/core/src/snapshot.js +++ b/packages/core/src/snapshot.js @@ -45,21 +45,31 @@ function validateAndFixSnapshotUrl(snapshot) { // used to deserialize regular expression strings const RE_REGEXP = /^\/(.+)\/(\w+)?$/; +// Upper bound on the snapshot name length we will run user-controllable +// regex/glob matching against. A crafted, very long snapshot name reaching this +// matcher (e.g. via the local API) combined with a backtracking-prone pattern +// could otherwise trigger catastrophic backtracking / ReDoS (CWE-1333). Real +// snapshot names are short; an over-long name simply does not match patterns. +const MAX_MATCH_INPUT_LENGTH = 2048; + // Returns true or false if a snapshot matches the provided include and exclude predicates. A // predicate can be an array of predicates, a regular expression, a glob pattern, or a function. function snapshotMatches(snapshot, include, exclude) { // support an options object as the second argument if (include?.include || include?.exclude) ({ include, exclude } = include); + // guard pattern matching against pathologically long inputs (ReDoS) + let patternSafe = typeof snapshot.name === 'string' && snapshot.name.length <= MAX_MATCH_INPUT_LENGTH; + // recursive predicate test function let test = (predicate, fallback) => { if (predicate && typeof predicate === 'string') { - // snapshot name matches exactly or matches a glob + // exact match is always safe; glob matching is only run on bounded input let result = snapshot.name === predicate || - micromatch.isMatch(snapshot.name, predicate); + (patternSafe && micromatch.isMatch(snapshot.name, predicate)); - // snapshot might match a string-based regexp pattern - if (!result) { + // snapshot might match a string-based regexp pattern (bounded input only) + if (!result && patternSafe) { try { let [, parsed, flags] = RE_REGEXP.exec(predicate) || []; result = !!parsed && new RegExp(parsed, flags).test(snapshot.name); @@ -68,8 +78,8 @@ function snapshotMatches(snapshot, include, exclude) { return result; } else if (predicate instanceof RegExp) { - // snapshot matches a regular expression - return predicate.test(snapshot.name); + // snapshot matches a regular expression (bounded input only) + return patternSafe && predicate.test(snapshot.name); } else if (typeof predicate === 'function') { // advanced matching return predicate(snapshot); diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index bdc206587..45670c17d 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -615,22 +615,50 @@ export async function withRetries(fn, { count, onRetry, signal, throwOn }) { } } -export function redactSecrets(data) { - const filepath = path.resolve(url.fileURLToPath(import.meta.url), '../secretPatterns.yml'); - const secretPatterns = YAML.parse(readFileSync(filepath, 'utf-8')); +// Lazily load and compile the secret patterns once. The pattern file holds +// ~1.7k regexes; parsing the YAML and compiling every RegExp on each call made +// redactSecrets O(patterns) per string and re-read the file for every recursive +// call. Since redactSecrets now runs over the full CLI log array on egress +// (sendBuildLogs), that per-call cost is paid hundreds of times and could blow +// past test/runtime timeouts. Compile once and reuse. +let _compiledSecretPatterns; +function getSecretPatterns() { + if (!_compiledSecretPatterns) { + const filepath = path.resolve(url.fileURLToPath(import.meta.url), '../secretPatterns.yml'); + const secretPatterns = YAML.parse(readFileSync(filepath, 'utf-8')); + // Regex sources come from the first-party, bundled secretPatterns.yml that + // ships in this package - never from remote or attacker-controlled input. + // nosemgrep: javascript.lang.security.audit.detect-non-literal-regexp.detect-non-literal-regexp + _compiledSecretPatterns = secretPatterns.patterns.map(p => new RegExp(p.pattern.regex, 'g')); + } + return _compiledSecretPatterns; +} - if (Array.isArray(data)) { - // Process each item in the array - return data.map(item => redactSecrets(item)); - } else if (typeof data === 'object' && data !== null) { - // Process each key-value pair in the object - data.message = redactSecrets(data.message); - } +export function redactSecrets(data) { + // Strings are redacted against every compiled secret pattern. if (typeof data === 'string') { - for (const pattern of secretPatterns.patterns) { - data = data.replace(new RegExp(pattern.pattern.regex, 'g'), '[REDACTED]'); + for (const pattern of getSecretPatterns()) { + data = data.replace(pattern, '[REDACTED]'); } + return data; + } + // Arrays (the CLI-log entry list) are redacted element-wise, in place. + if (Array.isArray(data)) { + for (let i = 0; i < data.length; i++) data[i] = redactSecrets(data[i]); + return data; + } + // Log entries: redact the human-readable `message` in place and return the + // same reference (sendBuildLogs reads the return value; the CI-log path reads + // the same memory-mode entry back — both see the redacted message). We do NOT + // recurse over `meta`: it holds structured instrumentation (e.g. a numeric + // `size`, ids) and a broad secret pattern matches plain digit strings, so a + // blanket meta sweep clobbers legitimate diagnostic data. Log-line secrets + // surface in `message`. + if (typeof data === 'object' && data !== null) { + data.message = redactSecrets(data.message); + return data; } + // Any other primitive (number, boolean, null, undefined) passes through. return data; } diff --git a/packages/core/test/unit/utils.test.js b/packages/core/test/unit/utils.test.js index 7b143cd61..cbd708f78 100644 --- a/packages/core/test/unit/utils.test.js +++ b/packages/core/test/unit/utils.test.js @@ -218,6 +218,83 @@ describe('Unit / Utils', () => { expect(redactSecrets(msg)).toContain('[REDACTED]'); }); }); + + // No-false-positive cases: benign text must pass through unchanged, and the + // anchored Percy-token pattern must not fire on token-ish substrings. + describe('no false positives', () => { + it('leaves a plain URL with no secret unchanged', () => { + let url = 'https://percy.io/dashboard/builds'; + expect(redactSecrets(url)).toEqual(url); + }); + + it('leaves a normal log message unchanged', () => { + let msg = 'Snapshot taken for homepage at width 1280'; + expect(redactSecrets(msg)).toEqual(msg); + }); + + it('does not redact an access_ substring (ss_ leg must not fire mid-word)', () => { + let text = 'access_ABCDEFGHIJKLMNOPQRSTUV'; + expect(redactSecrets(text)).toEqual(text); + }); + + it('does not clobber a crossapp_ substring at the app_ leg', () => { + let text = 'crossapp_ABCDEFGHIJKLMNOPQRSTUV'; + expect(redactSecrets(text)).toEqual(text); + }); + }); + + // Redaction is scoped to the entry's `message` and is applied in place. + describe('log-entry redaction', () => { + // Built by concatenation so the contiguous token literal never appears in + // source (a Percy-token-shaped literal would trip GitHub secret scanning); + // at runtime it still matches the pattern and must be redacted. + const secret = 'web_' + 'aB3dE7gH1jK4mN6pQ9sTuVwXyZ012345'; + + it('redacts the entry message in place and returns the same reference', () => { + // memory-mode logger.query returns live entry refs; the CI-log path + // reads the entry back after redaction, so redaction must mutate in + // place (not return a detached copy) as well as return the value. + let entry = { message: `token ${secret}`, type: 'ci' }; + let redacted = redactSecrets(entry); + expect(redacted).toBe(entry); + expect(entry.message).toEqual('token [REDACTED]'); + }); + + it('does not touch structured meta (avoids clobbering instrumentation data)', () => { + // `meta` holds diagnostic fields (e.g. a numeric size); a broad secret + // pattern matches plain digit strings, so meta is intentionally not swept. + let entry = { message: 'Resource too large', meta: { size: 30000000, url: 'http://localhost:8000/huge.css' } }; + redactSecrets(entry); + expect(entry.meta.size).toBe(30000000); + expect(entry.meta.url).toBe('http://localhost:8000/huge.css'); + }); + + it('redacts each message across an array of entries, in place', () => { + let entries = [{ message: `a ${secret}` }, { message: 'clean' }]; + let out = redactSecrets(entries); + expect(out).toBe(entries); + expect(entries[0].message).toEqual('a [REDACTED]'); + expect(entries[1].message).toEqual('clean'); + }); + + it('passes null, undefined and non-string primitives through unchanged', () => { + expect(redactSecrets(null)).toBeNull(); + expect(redactSecrets(undefined)).toBeUndefined(); + expect(redactSecrets(42)).toBe(42); + expect(redactSecrets(true)).toBe(true); + }); + }); + }); + + describe('Percy token prefixes', () => { + for (const prefix of ['web', 'app', 'auto', 'ss', 'vmw', 'res']) { + it(`redacts a ${prefix}_ Percy token`, () => { + let token = `${prefix}_aB3dE7gH1jK4mN6pQ9sTuVwXyZ012345`; + let redacted = redactSecrets(`Authenticated build using ${token} now`); + expect(redacted).toContain('[REDACTED]'); + expect(redacted).not.toContain(token); + }); + } }); describe('base64encode', () => {