Skip to content

fix(core): allowlist the /percy/log level so request input never drives dynamic dispatch (PER-8625) #2426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ninadbstack merged 1 commit into from
Sep 10, 2026
Merged

fix(core): allowlist the /percy/log level so request input never drives dynamic dispatch (PER-8625) #2426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion packages/core/src/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ function assertNotCrossOrigin(req) {
}
}

// Log levels an SDK may forward through POST /percy/log.
const SDK_LOG_LEVELS = new Set(['debug', 'info', 'warn', 'error']);

@ninadbstack ninadbstack Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Level allowlist duplicates the logger's own level list

This set repeats the levels already defined as LOG_LEVELS in packages/logger/src/logger.js, which is not exported. If a level is ever added or renamed there without updating this copy, the two drift silently and this endpoint starts rejecting a valid level.

Suggestion: Export the level list from @percy/logger and import it here rather than keeping a second hardcoded copy. Fine as a follow-up.

Reviewer: stack-code-reviewer


// Create a Percy CLI API server instance
export function createPercyServer(percy, port) {
let pkg = getPackageJSON(import.meta.url);
Expand Down Expand Up @@ -319,7 +322,20 @@ export function createPercyServer(percy, port) {
const message = req.body.message;
const meta = req.body.meta || {};

log[level](message, meta);
// `level` is attacker-controlled input on an unauthenticated endpoint, so
// it must never be used as a dynamic property key on the logger object
// (CWE-1321 / PER-8606, PER-8625). Only the four log levels are dispatched;
// anything else (`__proto__`, `constructor`, `loglevel`, ...) is rejected.
if (!SDK_LOG_LEVELS.has(level)) {
return res.json(400, { error: 'Invalid log level' });
}

switch (level) {

@ninadbstack ninadbstack Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Switch is redundant once the guard has run

After SDK_LOG_LEVELS.has(level) passes, level is provably one of four safe literals, so log[level](message, meta) would be equally safe and shorter. The comment above reads as though the guard alone is the mitigation, which leaves this switch looking redundant to a later reader.

Suggestion: Keep the switch if it is deliberate defense in depth against any dynamic property access, and say so in the comment. Otherwise collapse it back to the guarded dynamic call.

Reviewer: stack-code-reviewer

case 'debug': log.debug(message, meta); break;
case 'info': log.info(message, meta); break;
case 'warn': log.warn(message, meta); break;
default: log.error(message, meta);
}

res.json(200, { success: true });
})
Expand Down
35 changes: 35 additions & 0 deletions packages/core/test/api.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,41 @@ describe('API Server', () => {
expect(sdkLogs[1].meta).toEqual(message2.meta);
});

it('rejects /log requests whose level is not a real log level (PER-8606/8625)', async () => {
await percy.start();
let before = logger.loglevel();

for (let level of ['__proto__', 'constructor', 'loglevel', 'deprecated', 'stdout', '', null, 42]) {
let [data, res] = await request('/percy/log', {
body: { level, message: 'debug', meta: {} },
method: 'post'
}, true);

expect(res.statusCode).toBe(400);
expect(data).toEqual({ error: 'Invalid log level' });
}

// no dynamic dispatch side effects: global loglevel untouched, nothing logged
expect(logger.loglevel()).toBe(before);
expect(logger.instance.query(log => log.debug === 'sdk').length).toBe(0);
expect(Object.prototype.token).toBeUndefined();
});

it('dispatches every allowed /log level', async () => {
await percy.start();
logger.loglevel('debug');

for (let level of ['debug', 'info', 'warn', 'error']) {
await expectAsync(request('/percy/log', {
body: { level, message: `${level} message` },
method: 'post'
})).toBeResolvedTo({ success: true });
}

const sdkLogs = logger.instance.query(log => log.debug === 'sdk');
expect(sdkLogs.map(l => l.level)).toEqual(['debug', 'info', 'warn', 'error']);
});

it('returns a 500 error when an endpoint throws', async () => {
spyOn(percy, 'snapshot').and.rejectWith(new Error('test error'));
await percy.start();
Expand Down
Loading