Skip to content

tools/review: bind saved decisions to the file's SHA-256 and render in linear time - #269

Open
Wenjix wants to merge 1 commit into
jaredpalmer:mainfrom
Wenjix:security/review-tool
Open

Wenjix wants to merge 1 commit into
jaredpalmer:mainfrom
Wenjix:security/review-tool

Conversation

@Wenjix

@Wenjix Wenjix commented Oct 27, 2026

Copy link
Copy Markdown

Closes #266

Summary

tools/review saves a reviewer's decisions in localStorage so a reload resumes the review, and its reviews.jsonl export feeds the spot-check gate in scripts/freeze_documents_v1.py. Three fixes (findings 2, 5 and 3 of a security scan of 2074cc4):

2, saved decisions were bound to a weak fingerprint. The storage key was two 32-bit FNV-style hashes plus the length. A file crafted to collide with one already reviewed would resume that review's decisions for every matching id, and Export would then report them for text that may have changed. The freeze gate checks ids and proposed labels, not document text, so it would not notice. The key is now the SHA-256 of the file's text (Web Crypto), under a new prefix, kev-review:sha256:. Decisions resume only for a byte-identical file. Old entries are not migrated, because the only way to find them is the weak hash this replaces, and they are not deleted either; they stay in localStorage under kev-review:<old hash>.

5, choosing the current item was quadratic. The fallback for item called step(s.cur, 1), a full queue scan, once for every candidate queue.find tried. When the filter has nothing left, which is the normal end of a review, every render cost n² checks. The fallback id is now computed once. With 5,000 items (about 20 options and 10 judges each), all decided, a render on the empty "undecided" filter took 318 and 301 ms on main and 109 ms here (production build, headless Chromium).

3, every option scanned every judge. The judges of the current item are grouped by label once per render, instead of filtered again for each option. Same badges, same order.

Behavior relative to main

Flow main this PR
Resume key kev-review: + FNV pair + length kev-review:sha256: + SHA-256 of the file
Reviews saved before this change resumed by fingerprint not resumed (left in localStorage)
Render with the filter exhausted n² filter checks one queue scan
Judge badges per option options × judges judges + options
Page opened outside a secure context (Web Crypto unavailable) works shows an error; npm run dev on localhost is a secure context

Red / green

tools/review has no test runner. A headless Chromium run of the production build covered the changed paths instead (below).

Verification

  • npm ci && npm run build (tsc, then vite) in tools/review: clean.
  • Headless Chromium (Playwright) against vite preview of this branch: opened ?file=sample.jsonl, accepted one item and reloaded; the review resumed at "1 / 24 reviewed" under the SHA-256 key. After deciding all 24, the empty "undecided" view renders.
  • 5,000-item file with every item decided, render on the exhausted filter: main 318 / 301 ms, this branch 109 / 109 ms.

Merge Danger

  • A review in progress when this lands won't resume. Its decisions are still in localStorage under the old key.
  • The page needs a secure context (localhost or https) for Web Crypto. The README's npm run dev URL is one.

Checklist

  • The why is documented in the linked issue and this description.
  • npm run build (tsc + vite) passes in tools/review.
  • tools/review/README.md describes the new key.
  • No change to the export format.

Generated with Devin

…n linear time

Saved decisions were keyed by a non-cryptographic fingerprint that a
crafted file could collide with; they are now keyed by the SHA-256 of
the file. The fallback current item no longer rescans the queue for
every candidate, and judge badges are grouped once per render.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 27, 2026 09:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(security): tools/review restores decisions by a weak file fingerprint and renders in quadratic time

2 participants