Review follow-ups: PR #542 — fix(import): any spreadsheet a seller can export either imports or says exactly why not #788

New Issue

2026-09-23T01:16:37Z

gambit-admin commented

2026-09-23 01:16:37 +00:00

Non-blocking findings from the auto code review of PR #542 (fix(import): any spreadsheet a seller can export either imports or says exactly why not) — captured for batch cleanup rather than blocking the PR.
Commit 714f5af · https://github.com/Gambit-Inc/gambit/pull/542

SHOULD-FIX:

backend/app/routes/imports.py:1104 — get_mapping (GET /intake/imports/{batch_id}/mapping) calls _all_sheets without catching import_parse.UnreadableFile, unlike both branches of update_mapping and create_import that this PR guards. No global exception handler for ValueError/UnreadableFile exists in the app (checked main.py and the rest of backend/app), so a batch whose stored file the new sniffing logic can't read (a legacy draft, or a file corrupted post-upload) 500s on a plain GET instead of getting the clean string-detail 400 this PR promises everywhere else. The PR's own new test (test_remap_of_an_unreadable_stored_file_is_a_400) proves this exact scenario is expected to occur, but only the POST paths were covered.
backend/app/services/import_parse.py:875 — the zip-member size guard (zf.getinfo(files[0]).file_size > _MAX_ZIP_MEMBER_BYTES) checks the zip's declared, attacker-controlled uncompressed size before doing a single unbounded zf.read(files[0]). A crafted zip whose one CSV/XLSX member declares a small file_size but whose compressed stream actually inflates far past that (a zip-bomb shape) passes the check and then decompresses fully into memory — the cap doesn't bound what actually gets read.
backend/app/services/import_parse.py:962 — a title-less, identity-less row is now kept (parked, not skipped) whenever cost/ask/market/exit_price is a truthy money value. This also matches a spreadsheet's trailing "Total"/"Subtotal" row (blank title, no card_number/cert/sku/barcode, but a real dollar figure in Cost). Before this PR, any blank-title row was unconditionally skipped, so totals rows were correctly dropped; now they surface as a fake parked "card" the seller has to notice and reject in the set-aside pile.

NITS:
none

Filed from a Claude Code session

Non-blocking findings from the auto code review of **PR #542** (fix(import): any spreadsheet a seller can export either imports or says exactly why not) — captured for batch cleanup rather than blocking the PR. Commit `714f5af` · https://github.com/Gambit-Inc/gambit/pull/542 SHOULD-FIX: - `backend/app/routes/imports.py:1104` — `get_mapping` (GET `/intake/imports/{batch_id}/mapping`) calls `_all_sheets` without catching `import_parse.UnreadableFile`, unlike both branches of `update_mapping` and `create_import` that this PR guards. No global exception handler for `ValueError`/`UnreadableFile` exists in the app (checked `main.py` and the rest of `backend/app`), so a batch whose stored file the new sniffing logic can't read (a legacy draft, or a file corrupted post-upload) 500s on a plain GET instead of getting the clean string-detail 400 this PR promises everywhere else. The PR's own new test (`test_remap_of_an_unreadable_stored_file_is_a_400`) proves this exact scenario is expected to occur, but only the POST paths were covered. - `backend/app/services/import_parse.py:875` — the zip-member size guard (`zf.getinfo(files[0]).file_size > _MAX_ZIP_MEMBER_BYTES`) checks the zip's declared, attacker-controlled uncompressed size before doing a single unbounded `zf.read(files[0])`. A crafted zip whose one CSV/XLSX member declares a small `file_size` but whose compressed stream actually inflates far past that (a zip-bomb shape) passes the check and then decompresses fully into memory — the cap doesn't bound what actually gets read. - `backend/app/services/import_parse.py:962` — a title-less, identity-less row is now kept (parked, not skipped) whenever `cost`/`ask`/`market`/`exit_price` is a truthy money value. This also matches a spreadsheet's trailing "Total"/"Subtotal" row (blank title, no card_number/cert/sku/barcode, but a real dollar figure in Cost). Before this PR, any blank-title row was unconditionally skipped, so totals rows were correctly dropped; now they surface as a fake parked "card" the seller has to notice and reject in the set-aside pile. NITS: none --- Filed from a Claude Code session

gambit-admin added the enhancement label 2026-09-23 01:16:37 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gambit/gambit#788