chore: triage CodeQL findings (fix prototype-pollution risk + length caps)#20
Merged
Conversation
…ports
CodeQL flagged 9 `js/remote-property-injection` warnings on the
project envelope's locale endpoints. The pattern was:
{ ...localeScreens, [code]: ... }
where `code` was only validated for `typeof === 'string'` and
non-empty. Spread + computed key on a fresh object is safe from
Object.prototype pollution (the result has the key as an own
property, not on the prototype), but a key like `toString` or
`__proto__` would still shadow built-ins on later reads and
silently break the editor state.
Adds an `isSafeObjectKey` helper that rejects `__proto__`,
`constructor`, `prototype`, `toString`, `valueOf`, `hasOwnProperty`,
and the legacy `__define*Getter/Setter__` family. Applied at every
locale-code entry point:
- POST /api/projects/:project/locales/add
- POST /api/projects/:project/locales/remove
- POST /api/projects/:project/locales/set-active
- POST /api/projects/:project/locales/:code/patch-screen
Plus an integration test that POSTs each dangerous key and asserts
the endpoint returns 400 with a clear error message.
Also drops unused imports in two e2e specs that CodeQL flagged:
- e2e/ux-audit.spec.ts: removed `sidebar`, `preview`, `modeToggle`
- e2e/functional.spec.ts: removed `modeToggle`
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…king CodeQL js/polynomial-redos flagged two regex sites where adversarial input could trigger non-linear backtracking: - packages/core/src/color/p3.ts parseDisplayP3() regex has `\d*\.?\d+(?:e[+-]?\d+)?` repeated for r/g/b. Many '9's could trigger polynomial behavior. Cap input at 200 chars before matching (longest legitimate string is ~70). - packages/mcp/src/client.ts constructor calls .replace(/\/+$/, '') on the baseUrl. Many trailing slashes could backtrack. Cap baseUrl at 2048 chars (RFC 3986 URL guidance). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Triage of the 61 open CodeQL alerts on the Security tab. Result: 4 alerts will remain after this PR merges (and they auto-close on next CodeQL re-scan since the code fixes are in this PR).
Real bugs fixed
js/remote-property-injection— prototype-pollution risk in locale endpoints (8 alerts)The locale-add / remove / set-active / patch-screen endpoints used the user-supplied
codeas a computed object key ({ ...localeScreens, [code]: ... }). Spread + computed key is safe fromObject.prototypepollution per spec, but a key liketoStringor__proto__would still shadow built-ins on later reads.Added an
isSafeObjectKeyhelper that rejects__proto__,constructor,prototype,toString,valueOf,hasOwnProperty, and the legacy__define*Getter/Setter__family. Applied at all 4 locale entry points. Integration test added.js/polynomial-redos— adversarial-input regex backtracking (2 alerts)packages/core/src/color/p3.ts—parseDisplayP3()regex with repeated digit groups. Capped input at 200 chars (legitimate strings are ~70).packages/mcp/src/client.ts—baseUrl.replace(/\/+$/, '')could backtrack on thousands of trailing slashes. Capped at 2048 chars (RFC 3986 URL recommendation).js/unused-local-variable— dead imports (2 alerts)e2e/ux-audit.spec.ts— removedsidebar,preview,modeTogglee2e/functional.spec.ts— removedmodeToggleFalse positives dismissed (49 alerts)
js/path-injectionPROJECT_SLUG_RE; resolved paths checked withisPathInside(utils/pathSafety.ts). CodeQL doesn't recognise these sanitisers.js/missing-rate-limitingjs/http-to-file-accesssharpwriting a resized buffer from on-disk input.js/reflected-xsssanitizeRichHtmlor Nunjucks auto-escape.js/file-access-to-httpupload_screenshotMCP tool's job is to send a file as a request body.All dismissals include the justification visible on the alert. If new CodeQL scans flag the same patterns, they'll auto-link back to the dismissal reason.
Test plan
pnpm typecheckcleanpnpm test— 535 / 535 passing (1 new integration test for prototype-pollution defense)pnpm lintcleanpnpm buildsucceeds🤖 Generated with Claude Code