fix: strip prototype pollution keys from FormData JSON parsing#1850
fix: strip prototype pollution keys from FormData JSON parsing#1850eddieran wants to merge 2 commits intoelysiajs:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughFormData JSON parsing now strips prototype-pollution keys ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Adapter as WebStandard Adapter
participant Handler as DynamicHandler
participant Sanitizer as stripDangerousKeys
participant App as Application Route
Client->>Adapter: POST multipart/form-data
Adapter->>Handler: normalized form fields (strings/JSON)
Handler->>Sanitizer: JSON.parse(...) results
Sanitizer-->>Handler: sanitized objects (dangerous keys removed)
Handler->>App: constructed request body (safe)
App-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/adapter/web-standard/index.ts (1)
61-69: Security logic is duplicated in two places — extract a shared source, baka~
stripDangerousKeys/dangerous-key rules here mirrorsrc/dynamic-handle.ts(Line 40-56). Since this is security-sensitive, duplication risks drift in future patches. Please centralize the literal generation or shared sanitizer definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapter/web-standard/index.ts` around lines 61 - 69, The sanitizer logic is duplicated: the inline function stripDangerousKeys and its dangerousKeys set here mirror the implementation in src/dynamic-handle.ts; centralize them by extracting the shared sanitizer (e.g., export a sanitize/stripDangerousKeys function and the dangerousKeys definition from src/dynamic-handle.ts) and import and use that single exported symbol in this file (index.ts) instead of redefining the function/string template, ensuring both places call the same implementation to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/core/formdata-prototype-pollution.test.ts`:
- Around line 24-29: The tests currently try to send an own "__proto__" property
but use a bare __proto__ literal which becomes a prototype setter and is omitted
by JSON.stringify/Object.entries, so update the test payloads (the calls to
formPost and any JSON.stringify in formdata-prototype-pollution.test.ts) to
actually include an own "__proto__" key — either build objects with computed
property names (e.g., { ['__proto__']: { polluted: true }, safe: 'value' }) or
pass raw JSON strings that include "__proto__" — apply this change to all
occurrences around the JSON.stringify calls used in formPost and related tests
so the sanitizer receives the intended payload.
---
Nitpick comments:
In `@src/adapter/web-standard/index.ts`:
- Around line 61-69: The sanitizer logic is duplicated: the inline function
stripDangerousKeys and its dangerousKeys set here mirror the implementation in
src/dynamic-handle.ts; centralize them by extracting the shared sanitizer (e.g.,
export a sanitize/stripDangerousKeys function and the dangerousKeys definition
from src/dynamic-handle.ts) and import and use that single exported symbol in
this file (index.ts) instead of redefining the function/string template,
ensuring both places call the same implementation to avoid drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8165c26b-4d7a-4592-bfd8-4a688bd4d35f
📒 Files selected for processing (3)
src/adapter/web-standard/index.tssrc/dynamic-handle.tstest/core/formdata-prototype-pollution.test.ts
In JS object literals, `__proto__` acts as a prototype setter rather
than an own enumerable property, so `JSON.stringify({ __proto__: ... })`
drops it and `FormData` never sees it either. The tests therefore
weren't actually exercising the sanitization path. Build the JSON
strings and FormData instances manually so the dangerous keys show up
in the parsed input. Caught by @coderabbitai in elysiajs#1850.
|
Thanks @coderabbitai — valid catch. In an object literal, |
|
Tip For best results, initiate chat on the files or code changes.
Raw JSON strings like ✏️ Learnings added
|
Summary
Fixes #1848 — FormData auto-parse prototype pollution.
This replaces #1849 (which was auto-closed when the fork was deleted) and addresses the gap identified in the CodeRabbit review: the nested key path parsing code was calling
JSON.parseon intermediate values without stripping dangerous keys.Changes
stripDangerousKeysfunction that recursively removes__proto__,constructor, andprototypekeys from parsed objectsstripDangerousKeysto allJSON.parseresults in FormData processing:normalizeFormValueinsrc/dynamic-handle.ts(single-value JSON parse and file+JSON mixed parse)parseObjectStringinsrc/dynamic-handle.ts(nested key path intermediate object parsing)JSON.parsecall sites in the inlinedformDataparser insrc/adapter/web-standard/index.ts— including the nested key pathJSON.parse(existing)that the review flagged__proto__,constructor,prototypestripping at top-level, nested, and deeply nested JSON, as well as nested key path (user.profile) and dangerous key path (constructor.prototype) scenariosTest plan
test/core/formdata-prototype-pollution.test.tspasstest/core/formdata.test.tstests continue to pass (no regressions)Summary by CodeRabbit
Bug Fixes
Tests