From 241d78f4b4c44d5d6d9f8013423075dac5c03abd Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Mon, 25 May 2026 23:37:45 +0530 Subject: [PATCH 1/3] fix(core): block security-sensitive launchOptions over HTTP API (PER-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. --- packages/core/src/api.js | 38 +++++++++++++++++++++++--- packages/core/test/api.test.js | 49 ++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index dc40e415b..40514051d 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -37,6 +37,33 @@ function encodeURLSearchParams(subj, prefix) { )).join('&') : `${prefix}=${encodeURIComponent(subj)}`; } +// Fields under discovery.launchOptions that can only be set via the static config file or CLI +// args at startup — never over HTTP. They control the browser binary and flags (e.g. +// --renderer-cmd-prefix, --gpu-launcher, --utility-cmd-prefix) so accepting them over the +// local API would let any process on the machine execute arbitrary code as the Percy user. +const BLOCKED_LAUNCH_OPTION_KEYS = ['executable', 'args']; + +// Returns a body with blocked launchOptions keys removed; logs a warning for each stripped +// field. Returns the original body unchanged when nothing needs stripping so we don't pay +// for a clone on every config request. Caller guarantees `body` is truthy. +function stripBlockedConfigFields(body, log) { + let launchOptions = body.discovery?.launchOptions; + let present = launchOptions && BLOCKED_LAUNCH_OPTION_KEYS.filter( + k => Object.prototype.hasOwnProperty.call(launchOptions, k) + ); + if (!present?.length) return body; + + let stripped = { + ...body, + discovery: { ...body.discovery, launchOptions: { ...launchOptions } } + }; + for (let k of present) { + delete stripped.discovery.launchOptions[k]; + log.warn(`Ignoring \`discovery.launchOptions.${k}\` from /percy/config request: this field can only be set via the config file or CLI at startup.`); + } + return stripped; +} + // Create a Percy CLI API server instance export function createPercyServer(percy, port) { let pkg = getPackageJSON(import.meta.url); @@ -113,10 +140,13 @@ export function createPercyServer(percy, port) { }); }) // get or set config options - .route(['get', 'post'], '/percy/config', async (req, res) => res.json(200, { - config: req.body ? percy.set(req.body) : percy.config, - success: true - })) + .route(['get', 'post'], '/percy/config', async (req, res) => { + let body = req.body && stripBlockedConfigFields(req.body, logger('core:server')); + return res.json(200, { + config: body ? percy.set(body) : percy.config, + success: true + }); + }) // responds once idle (may take a long time) .route('get', '/percy/idle', async (req, res) => res.json(200, { success: await percy.idle().then(() => true) diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index a1db6bbe7..fe905d8c6 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -111,6 +111,55 @@ describe('API Server', () => { expect(percy.config).toEqual(expected); }); + it('does not warn when /config POST contains launchOptions without blocked keys', async () => { + await percy.start(); + + await request('/percy/config', { + method: 'POST', + body: { discovery: { launchOptions: { headless: false, closeBrowser: false } } } + }); + + expect(percy.config.discovery.launchOptions.headless).toBe(false); + expect(percy.config.discovery.launchOptions.closeBrowser).toBe(false); + expect(logger.stderr).not.toEqual(jasmine.arrayContaining([ + jasmine.stringMatching(/Ignoring `discovery\.launchOptions/) + ])); + }); + + it('strips security-sensitive launchOptions fields from /config POST', async () => { + await percy.start(); + + let before = percy.config.discovery.launchOptions; + expect(before.executable).toBeUndefined(); + expect(before.args).toBeUndefined(); + + await request('/percy/config', { + method: 'POST', + body: { + discovery: { + launchOptions: { + executable: '/tmp/evil-binary', + args: ['--renderer-cmd-prefix=/tmp/payload'], + headless: false, + closeBrowser: false + } + } + } + }); + + // dangerous fields ignored + expect(percy.config.discovery.launchOptions.executable).toBeUndefined(); + expect(percy.config.discovery.launchOptions.args).toBeUndefined(); + // benign fields still settable + expect(percy.config.discovery.launchOptions.headless).toBe(false); + expect(percy.config.discovery.launchOptions.closeBrowser).toBe(false); + + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + jasmine.stringMatching(/Ignoring `discovery\.launchOptions\.executable`/), + jasmine.stringMatching(/Ignoring `discovery\.launchOptions\.args`/) + ])); + }); + it('has an /idle endpoint that calls #idle()', async () => { spyOn(percy, 'idle').and.resolveTo(); await percy.start(); From 7e5f2c9d376a8f20b8a6f588c1905808ed638c8e Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Tue, 26 May 2026 17:10:07 +0530 Subject: [PATCH 2/3] refactor(core): drive HTTP-blocked config fields from schema annotation Per review feedback, replace the hardcoded BLOCKED_LAUNCH_OPTION_KEYS list with a generic schema walker. Sensitive fields are now marked `httpReadOnly: true` next to their schema definition, so adding a new HTTP-blocked field is a one-line annotation rather than touching api.js. --- packages/core/src/api.js | 57 +++++++++++++++++++++++-------------- packages/core/src/config.js | 7 +++-- 2 files changed, 41 insertions(+), 23 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 40514051d..5620f0c54 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -12,6 +12,7 @@ import { handleSyncJob } from './snapshot.js'; // This change ensures better compatibility and avoids relying on Node.js-specific APIs that might cause issues in ESM environments. import { fileURLToPath } from 'url'; import { createRequire } from 'module'; +import { configSchema } from './config.js'; export const getPercyDomPath = (url) => { try { @@ -37,29 +38,43 @@ function encodeURLSearchParams(subj, prefix) { )).join('&') : `${prefix}=${encodeURIComponent(subj)}`; } -// Fields under discovery.launchOptions that can only be set via the static config file or CLI -// args at startup — never over HTTP. They control the browser binary and flags (e.g. -// --renderer-cmd-prefix, --gpu-launcher, --utility-cmd-prefix) so accepting them over the -// local API would let any process on the machine execute arbitrary code as the Percy user. -const BLOCKED_LAUNCH_OPTION_KEYS = ['executable', 'args']; +// Walks the config schema and collects dot-paths of any fields marked `httpReadOnly: true` +// that are present in `body`. Driving this from the schema means new HTTP-blocked fields +// only need a one-line annotation next to their definition — no list to keep in sync here. +function findHttpReadOnlyPaths(body, schema, path = '') { + if (!body || typeof body !== 'object' || !schema?.properties) return []; + let paths = []; + for (let [key, propSchema] of Object.entries(schema.properties)) { + if (!Object.prototype.hasOwnProperty.call(body, key)) continue; + let childPath = path ? `${path}.${key}` : key; + if (propSchema?.httpReadOnly) { + paths.push(childPath); + } else { + paths.push(...findHttpReadOnlyPaths(body[key], propSchema, childPath)); + } + } + return paths; +} + +// Top-level configSchema is a map of subschemas keyed by top-level config namespace +// (`discovery`, `snapshot`, …). Wrap it as a single object schema so the walker can recurse +// uniformly from the root. +const ROOT_CONFIG_SCHEMA = { type: 'object', properties: configSchema }; -// Returns a body with blocked launchOptions keys removed; logs a warning for each stripped -// field. Returns the original body unchanged when nothing needs stripping so we don't pay -// for a clone on every config request. Caller guarantees `body` is truthy. +// Returns a body with `httpReadOnly` fields removed; logs a warning for each stripped field. +// Returns the original body unchanged when nothing needs stripping so we don't pay for a +// clone on every config request. Caller guarantees `body` is truthy. function stripBlockedConfigFields(body, log) { - let launchOptions = body.discovery?.launchOptions; - let present = launchOptions && BLOCKED_LAUNCH_OPTION_KEYS.filter( - k => Object.prototype.hasOwnProperty.call(launchOptions, k) - ); - if (!present?.length) return body; - - let stripped = { - ...body, - discovery: { ...body.discovery, launchOptions: { ...launchOptions } } - }; - for (let k of present) { - delete stripped.discovery.launchOptions[k]; - log.warn(`Ignoring \`discovery.launchOptions.${k}\` from /percy/config request: this field can only be set via the config file or CLI at startup.`); + let paths = findHttpReadOnlyPaths(body, ROOT_CONFIG_SCHEMA); + if (!paths.length) return body; + + let stripped = JSON.parse(JSON.stringify(body)); + for (let p of paths) { + let parts = p.split('.'); + let leaf = parts.pop(); + let parent = parts.reduce((o, k) => o?.[k], stripped); + if (parent && typeof parent === 'object') delete parent[leaf]; + log.warn(`Ignoring \`${p}\` from /percy/config request: this field can only be set via the config file or CLI at startup.`); } return stripped; } diff --git a/packages/core/src/config.js b/packages/core/src/config.js index ad860e2fd..167d028ba 100644 --- a/packages/core/src/config.js +++ b/packages/core/src/config.js @@ -489,9 +489,12 @@ export const configSchema = { type: 'object', additionalProperties: false, properties: { - executable: { type: 'string' }, + // httpReadOnly: never settable over /percy/config — accepting these would let any + // local process execute arbitrary code as the Percy user via flags like + // --renderer-cmd-prefix / --gpu-launcher. Must be set via static config or CLI. + executable: { type: 'string', httpReadOnly: true }, timeout: { type: 'integer' }, - args: { type: 'array', items: { type: 'string' } }, + args: { type: 'array', items: { type: 'string' }, httpReadOnly: true }, headless: { type: 'boolean' }, closeBrowser: { type: 'boolean', default: true } } From 36d05c6e31e986b7ae98bc0e4d2dd6adcdad0129 Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Wed, 27 May 2026 00:10:07 +0530 Subject: [PATCH 3/3] test(core): cover defensive guard in _applyHttpReadOnlyStripping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the path-stripping step out of stripBlockedConfigFields and export it so the `o?.[k]` short-circuit (defensive against paths whose ancestor is missing from body) can be unit-tested directly. Through the production caller every intermediate is verified by findHttpReadOnlyPaths, so the guard is unreachable via the HTTP /config endpoint and only reachable through the new exported helper. Bumps branch coverage on api.js from 99.6% to 100% by pinning the guard's behavior — no production behavior change. --- packages/core/src/api.js | 18 ++++++++++++----- packages/core/test/api.test.js | 36 +++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 567a70b07..6a4ec27e6 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -65,11 +65,14 @@ function findHttpReadOnlyPaths(body, schema, path = '') { // uniformly from the root. const ROOT_CONFIG_SCHEMA = { type: 'object', properties: configSchema }; -// Returns a body with `httpReadOnly` fields removed; logs a warning for each stripped field. -// Returns the original body unchanged when nothing needs stripping so we don't pay for a -// clone on every config request. Caller guarantees `body` is truthy. -function stripBlockedConfigFields(body, log) { - let paths = findHttpReadOnlyPaths(body, ROOT_CONFIG_SCHEMA); +// Removes each dot-path's leaf from a deep clone of `body` and logs a warning per path. +// Returns the original `body` unchanged when `paths` is empty so we don't pay for a clone +// on every config request. Exported for unit testing: the `?.` chain in the reduce is a +// defensive guard for paths whose ancestor is absent from `body`. Through the production +// caller (stripBlockedConfigFields → findHttpReadOnlyPaths) every intermediate is verified +// present, so the guard is unreachable in normal use — but the explicit paths parameter +// lets a unit test exercise it without contorting the schema. +export function _applyHttpReadOnlyStripping(body, paths, log) { if (!paths.length) return body; let stripped = JSON.parse(JSON.stringify(body)); @@ -83,6 +86,11 @@ function stripBlockedConfigFields(body, log) { return stripped; } +// Returns a body with `httpReadOnly` fields removed. Caller guarantees `body` is truthy. +function stripBlockedConfigFields(body, log) { + return _applyHttpReadOnlyStripping(body, findHttpReadOnlyPaths(body, ROOT_CONFIG_SCHEMA), log); +} + // Parse PNG IHDR chunk for the screenshot's actual rendered dimensions. // Returns { width, height } when the buffer is a valid PNG with non-zero // dimensions, or null otherwise (non-PNG signature, truncated file, zero diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 0aaacb56a..e32f910fa 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -3,7 +3,7 @@ import PercyConfig from '@percy/config'; import { logger, setupTest, fs } from './helpers/index.js'; import Percy from '@percy/core'; import WebdriverUtils from '@percy/webdriver-utils'; -import { getPercyDomPath } from '../src/api.js'; +import { getPercyDomPath, _applyHttpReadOnlyStripping } from '../src/api.js'; describe('API Server', () => { let percy; @@ -1798,3 +1798,37 @@ describe('API Server', () => { }); }); }); + +// Pure unit tests for the stripping helper — kept in their own describe so they don't +// drag the API Server's beforeEach (Percy instance + Chromium setup). Through the +// production caller every intermediate of a returned path is verified present, so the +// `o?.[k]` defensive guard inside _applyHttpReadOnlyStripping is unreachable in normal +// use. These tests pin that guard directly so a refactor can't silently lose it. +describe('_applyHttpReadOnlyStripping', () => { + it('tolerates paths whose ancestor is absent from body', () => { + let log = { warn: jasmine.createSpy('warn') }; + let body = { unrelated: { keep: true } }; + let result = _applyHttpReadOnlyStripping( + body, + ['discovery.launchOptions.executable'], + log + ); + + // Body is deep-cloned (not mutated); the missing path is a no-op delete. + expect(result).toEqual({ unrelated: { keep: true } }); + expect(result).not.toBe(body); + // The warning still fires — caller is told the field was rejected. + expect(log.warn).toHaveBeenCalledWith( + jasmine.stringMatching(/Ignoring `discovery\.launchOptions\.executable`/) + ); + }); + + it('returns body unchanged (no clone) when paths is empty', () => { + let log = { warn: jasmine.createSpy('warn') }; + let body = { discovery: { launchOptions: { headless: true } } }; + let result = _applyHttpReadOnlyStripping(body, [], log); + + expect(result).toBe(body); + expect(log.warn).not.toHaveBeenCalled(); + }); +});