diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 42a36385e..6a4ec27e6 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -16,6 +16,7 @@ import { Readable } from 'stream'; // 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 { @@ -41,6 +42,55 @@ function encodeURLSearchParams(subj, prefix) { )).join('&') : `${prefix}=${encodeURIComponent(subj)}`; } +// 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 }; + +// 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)); + 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; +} + +// 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 @@ -293,10 +343,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/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 } } diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index ad6d67962..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; @@ -113,6 +113,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(); @@ -1749,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(); + }); +});