fix(csv-parse): prototype replacement reachable via columns path (#496) - #497
Merged
Merged
Conversation
spencer0124
added a commit
to spencer0124/skkuverse-server
that referenced
this pull request
Aug 7, 2026
Phase 1 of spencer0124/skkuverse#13, the half that gets data in. Parsing is csv-parse@7.0.2, not a hand-rolled scanner. The ops sheet is edited in a spreadsheet, so three separate things occur that a split(",") gets wrong: five committed rows already carry a comma inside a quoted note_ko, doubled quotes appear the moment anyone types one, and a spreadsheet export prepends a UTF-8 BOM. bom:true is the one that would have cost the most to debug — without it the first header key parses as U+FEFF + "placeId" and all 62 rows fail for a missing id, which reads as a data problem when it is an encoding problem. The patch level is deliberate: upstream says 7.0.0 "was published by mistake, there is no breaking changes", and 7.0.2 fixes prototype replacement reachable via `columns` (adaltas/node-csv#497) — `columns: true` is exactly the option this importer uses. devDependency, because the runtime stage runs npm ci --omit=dev and copies only dist/; if src/ ever imports it, it must move to dependencies in the same commit. Coordinate validation exists because Number("") === 0. An isNaN-only guard admits a blank cell as 0, the pair becomes [0,0], and 2dsphere accepts it because the Gulf of Guinea is a real place (spencer0124/skkuverse#12). Cells must be non-empty AND a plain decimal AND in range — the range check is what catches a lat/lng swap, where both values are perfectly finite. parseFloat is never used: parseFloat("37,29") is 37. Any rejected row aborts the whole import. A half-imported map hides its own gaps: the missing plots are invisible and the ones that made it look authoritative. Two invariants are pinned by tests and stated in the code: - updatedAt is written only when a place actually changed. Phase 2's contentHash reads the contributors' [_id, updatedAt], so stamping every document on every import would publish a new snapshot version after a re-import that changed nothing (spencer0124/skkuverse#11 R4). - Only the demo seed flips `enabled`; the importer's activation write is $setOnInsert. That makes run order irrelevant and, more to the point, means re-importing a corrected sheet mid-festival cannot take the live map down. The demo seed departs from the issue's "~12 places": spencer0124/skkuverse#12 surveyed all 62 coordinates, so it seeds sessions over the real layout instead of inventing a second set. Times are relative to now, so one run shows open, upcoming and closed at once and crosses a boundary every couple of minutes. It refuses a non-_dev database without --force. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GJKCFR8LZA8ZThg1aCWf68
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
2 participants
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.
Fixes #496.
The #479 fix guarded only the
objnamecollector (lib/index.js). The same class ofprototype replacement remained reachable via the
columnsrecord builder inlib/api/index.js, whose keys come from the CSV header row whencolumns: true.With
group_columns_by_name: trueand a duplicate__proto__column, the duplicatebranch assigned an array to
obj['__proto__'], invoking the__proto__setter andreplacing the parsed record's prototype with an attacker-controlled array.
This PR:
Object.hasOwn(obj, name)for the duplicate-column check (so an inherited__proto__is not treated as already present).Object.defineProperty(...)so a__proto__column becomes an ordinaryown property instead of invoking the setter.
test/option.columns.tscovering the#496case.Verified: parsing
__proto__,__proto__,role\nEVIL1,EVIL2,adminyields{"__proto__":["EVIL1","EVIL2"],"role":"admin"}, the record's prototype is unchanged,and
rec.length/rec["1"]/rec["2"]areundefined.Diff authored by @wdavidw; opening the PR per coordination on #496.