fix: preserve raw:true Buffer headers instead of nulling them (#251)#252
Open
Banhhmii wants to merge 1 commit into
Open
fix: preserve raw:true Buffer headers instead of nulling them (#251)#252Banhhmii wants to merge 1 commit into
Banhhmii wants to merge 1 commit into
Conversation
…osh#251) The mafintosh#250 prototype-pollution fix (783a7ab) added sanitizeHeader() to reject any non-string header, but raw:true mode produces Buffer cells for every column including headers, so every header was silently nulled and writeRow() dropped every column, yielding {} for all rows. sanitizeHeader() now special-cases Buffer: compares the decoded utf-8 string against DANGEROUS_KEYS but returns the original Buffer when safe. Buffer.prototype.toString() is a fixed, non-overridable decode of the same bytes, so there's no check-time/use-time gap for it to disagree with itself the way an arbitrary object's toString() could. Other non-string types still return null, preserving mafintosh#250's security posture. Also adds regression tests for mafintosh#250 itself, which shipped with none, and updates index.d.ts/README.md to document Buffer headers.
There was a problem hiding this comment.
Pull request overview
Fixes a regression introduced by the prototype-pollution hardening (#250) where raw: true caused header cells (Buffers) to be rejected and nulled, resulting in all columns being dropped (empty {} rows). The PR updates header sanitization to safely handle Buffer headers while keeping the dangerous-key protections, and documents the Buffer header behavior in typings and README.
Changes:
- Update
sanitizeHeader()to acceptBufferheaders by checking the decoded string againstDANGEROUS_KEYSwhile returning the original Buffer when safe. - Add regression fixtures and issue tests covering
raw: trueheaders/rows and prototype-pollution stripping. - Update TypeScript definitions and README to document that
mapHeadersmay receive/returnBufferwhenraw: true.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/issues.test.js | Adds regression tests for #251 and #250 behaviors. |
| test/fixtures/raw-headers.csv | Fixture for validating raw: true parsing with normal headers. |
| test/fixtures/prototype-pollution.csv | Fixture for validating stripping of dangerous header keys. |
| README.md | Documents Buffer headers under raw: true and mapHeaders. |
| index.test-d.ts | Updates tsd expectations for mapHeaders header type under raw. |
| index.js | Fixes sanitizeHeader() to handle Buffer headers safely. |
| index.d.ts | Updates mapHeaders signature to allow `string |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+41
| test.cb('raw headers and rows are preserved (#251)', (t) => { | ||
| const verify = (err, lines) => { | ||
| t.false(err, 'no err') | ||
| t.is(lines.length, 2, '2 rows') | ||
| t.true(Buffer.isBuffer(lines[0].a), 'header a is a Buffer') | ||
| t.true(Buffer.isBuffer(lines[0].b), 'header b is a Buffer') | ||
| t.is(lines[0].a.toString(), '1') | ||
| t.is(lines[0].b.toString(), '2') | ||
| t.is(lines[1].a.toString(), '3') | ||
| t.is(lines[1].b.toString(), '4') | ||
| t.end() | ||
| } | ||
|
|
||
| collect('raw-headers', { raw: true }, verify) | ||
| }) |
Comment on lines
+56
to
+66
| test.cb('prototype pollution keys are stripped from raw Buffer headers (#250 + #251)', (t) => { | ||
| const verify = (err, lines) => { | ||
| t.false(err, 'no err') | ||
| t.is(Object.keys(lines[0]).length, 2, 'only safe headers survive') | ||
| t.true(Buffer.isBuffer(lines[0].a), 'header a is a Buffer') | ||
| t.true(Buffer.isBuffer(lines[0].b), 'header b is a Buffer') | ||
| t.end() | ||
| } | ||
|
|
||
| collect('prototype-pollution', { raw: true }, verify) | ||
| }) |
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
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.
TITLE:
Fix raw:true headers being nulled, dropping every row (#251)
This PR contains:
Breaking Changes?
If yes, please describe the breakage.
Please Describe Your Changes
Fixes #251. The #250 prototype-pollution fix added
sanitizeHeader(), which rejects any header whosetypeofisn't'string'. That didn't account forraw: truemode, where every parsed cell — including header-row cells — is aBuffer, not a string. Every header was silently nulled, andwriteRow()treats anullheader as "drop this column," so every row came back as{}.Changes
sanitizeHeader()now special-casesBuffer: it compares the decoded (utf-8) string form againstDANGEROUS_KEYS, but returns the original Buffer when safe, soraw: truekeeps producing real Buffer values instead ofnull.mapHeadersreturning a number or object) are still rejected tonull— this isn't broadened to a generalString()coercion, since that would let an object with a craftedtoString()defeat the dangerous-key check on the check call and return something else on a later call, reopening the exact hole Fix prototype pollution #250 closed.Buffer.prototype.toString()doesn't have that problem: it's a fixed decode of the same bytes, not attacker-overridable.test/issues.test.js, including 2 for Fix prototype pollution #250 itself, which shipped with no test coverage.index.d.ts(mapHeadersheader type is nowstring | Buffer) and README (mapHeadersandrawsections) to document that headers can be Buffers underraw: true.Testing
npm test(ava && tsd) — full suite green, including the 3 new testsnpm run lint— cleanraw: trueon'a,b\n1,2\n3,4\n') before and after the fix, and confirmed the Fix prototype pollution #250 regression case (__proto__header) still gets stripped underraw: true