fix(core): block security-sensitive launchOptions over HTTP API (PER-…

[aryanku-dev]

…8258)

POST /percy/config previously merged any field into runtime config, including discovery.launchOptions.executable and discovery.launchOptions.args. Since args are forwarded directly to the Chrome process, a local attacker could inject flags like --renderer-cmd-prefix=... or --gpu-launcher=... to execute arbitrary code as the Percy user.

Strip executable/args from incoming /percy/config bodies and warn that these fields can only be set via the static config file or CLI args.

[amandeepsingh333]

Good fix — the schema-driven httpReadOnly annotation is exactly the right shape: future security-sensitive fields are one-line additions next to their definition, no parallel list to drift. The unit test pinning the unreachable ?. guard is also nice forward-defensive work.

Concerns inline, ordered by severity:

1. Schema walker only handles properties — items/additionalProperties/oneOf/anyOf are silent misses. Not a problem today (both blocked fields live under properties), but a future contributor annotating httpReadOnly: true on an array-item or oneOf branch would silently get no protection. The "annotation is enough" contract should be explicit, or the walker needs to cover more constructs.
2. httpReadOnly is a non-standard JSON Schema keyword — what schema validator does Percy use? AJV in strict mode rejects unknown keywords; lenient validators may emit warnings. Worth verifying it doesn't break schema-driven downstream tooling (docs gen, type gen, IDE support).
3. Silent stripping — request succeeds with 200 OK and a stripped config; an SDK consumer wouldn't see the WARN log. Should the response surface ignored_fields so callers get actionable feedback? Debatable, but worth being intentional about.
4. logger('core:server') called per request — minor; hoist to module scope.

Broader concern (not blocking): customer docs for /percy/config need updating to list which fields are HTTP-read-only. Today a customer programmatically configuring executable or args gets silent loss with only a server-side WARN; they'd reasonably assume the request worked.

[amandeepsingh333]

Schema walker only descends through properties — silent miss for arrays / additionalProperties / oneOf.

if (!body || typeof body !== 'object' || !schema?.properties) return [];

Today this works because both blocked fields (executable, args) sit directly under properties of launchOptions. But the PR sets up an open extension point: "add httpReadOnly: true to any future sensitive field." That contract breaks silently when:

• args items become objects with a nested sensitive sub-field — walker doesn't descend through items:.
• A field lives under additionalProperties (e.g., dynamic keys) — walker doesn't descend.
• A field lives in a oneOf / anyOf branch — walker doesn't descend.

The failure mode is the worst kind: a future contributor adds httpReadOnly: true to a sensitive field, no one notices the walker doesn't reach it, and the field is silently settable over HTTP. The annotation looks correct, the test for that field is green (it's never reached), and the security hole is invisible until discovered.

Two options:

• Document the limit explicitly on the function ("only descends through properties — annotating fields under items / additionalProperties will be silently ignored") and add an assertion in test setup that walks configSchema for any httpReadOnly keys not reachable from the root, OR
• Extend the walker to descend into items/additionalProperties (less code than you'd think).

Given this is a security primitive, option 2 is the safer long-term play.

[amandeepsingh333]

httpReadOnly is a non-standard JSON Schema keyword — compatibility check.

Question: what schema validator does Percy use to validate /percy/config bodies (and the static config file)?

• AJV with strict: true would reject unknown keywords with an error.
• AJV with strict: false (or default in older versions) silently ignores them — fine for stripping, but the keyword becomes invisible to any tooling that reads the schema.
• Other tools that consume configSchema (docs generation, TypeScript type generation, IDE auto-completion, schema-aware validators) won't know what httpReadOnly means.

Worth either:

• Confirming the validator is lenient and adding a comment here noting the keyword is read by findHttpReadOnlyPaths in api.js (so a reader doesn't grep for it in vain), OR
• Defining httpReadOnly as a documented custom keyword via the validator's extension API (AJV's addKeyword) so it's first-class.

Also: customer-facing docs for POST /percy/config should list which fields are HTTP-read-only. Today a customer programmatically configuring executable or args gets silent loss with only a server-side WARN log they probably won't see.

[amandeepsingh333]

Silent stripping — response shape doesn't surface what was rejected.

let body = req.body && stripBlockedConfigFields(req.body, logger('core:server'));
return res.json(200, {
  config: body ? percy.set(body) : percy.config,
  success: true
});

A caller (Percy SDK, custom HTTP client, internal tooling) sends { discovery: { launchOptions: { executable: '/foo' } } }, gets 200 OK with success: true and a config object that silently lacks their executable setting. The WARN is logged server-side but the caller never sees it. "Successfully ignored your security-sensitive setting" is not great DX.

Question: should the response include ignored_fields: ['discovery.launchOptions.executable'] so the caller can show an actionable message in their UI / log? Or even return 400 Bad Request for HTTP-only sensitive fields, matching how other validators behave?

Argument for silent: less info-leak to a hostile caller; existing pattern matches "strip unknown fields." Argument against: this isn't a malicious-caller scenario by design — /percy/config is localhost-only, the caller is presumably trusted. Surface info wins.

Also minor: logger('core:server') is invoked on every config request. If logger() does any non-trivial lookup or instance allocation, hoist it to module scope:

const configLog = logger('core:server');
// ...
let body = req.body && stripBlockedConfigFields(req.body, configLog);

[amandeepsingh333]

structuredClone is faster + handles more types if you're on Node 17+.

let stripped = JSON.parse(JSON.stringify(body));

For JSON-typed HTTP bodies this works fine — nothing in the body should be a Date/Map/Set/Function. So pure nitpick. But:

• structuredClone(body) is ~3× faster on Node 17+ (V8 native).
• It preserves types JSON loses (Dates, Maps, undefined slots in arrays), which makes the helper safer if it ever gets reused beyond HTTP request bodies.
• It throws on un-cloneable values (functions, DOM nodes) rather than silently dropping them.

Given the helper is exported and prefixed _ (i.e., "private but usable"), the wider type-correctness matters more than the perf delta. Worth a swap unless there's a stated Node-version floor below 17 that I'm missing.