938c161a26
Give /scanner a md+ camera, live match inspector, and history strip (with device picker, batch scan, and tips) without regressing the mobile immersive checkout. Co-authored-by: Cursor <cursoragent@cursor.com>
30 lines
4.5 KiB
Markdown
30 lines
4.5 KiB
Markdown
## Security Audit
|
||
|
||
| Check | Status | Notes |
|
||
| --- | --- | --- |
|
||
| Auth boundary | ✅ | No new API route or auth flow. `/scanner` keeps its `useAuth` redirect, and the unchanged scan endpoint verifies the Bearer token before its per-user scan rate limit. |
|
||
| Authorization (IDOR) | ✅ | The desktop list picker uses the existing collection-card API; its server-side POST path confirms authenticated owner/editor permission before mutation. Client-held `collectionId` is not trusted as authorization. |
|
||
| Input / injection | ⚠️ | File names and server error text are rendered as React text (no HTML sink), but new batch intake has no client-side file-size or count bound before decoding images into a canvas. |
|
||
| Secrets exposure | ✅ | Device IDs are stored only in tab-scoped `sessionStorage`; no new secret, `NEXT_PUBLIC_*` value, token, or credential was found in the scanner additions. |
|
||
| Dependencies | ⚠️ | No dependency manifest change is in the supplied diff, but `npm audit --omit=dev --json` reports five pre-existing production high-severity advisories (including `next`, `jws`, `nanoid`, `postcss`, and `sharp`). |
|
||
|
||
### Findings
|
||
|
||
1. **[Severity 4 — scope expansion]** `git diff origin/main -- . ':!.convoys/.metrics.jsonl'` contains 15 tracked paths outside Briefs 1–6, including dashboard/nav work, `components/Layout.js`, `pages/dashboard.js`, `pages/my-cards.js`, and `pages/api/user-cards.js`; it also deletes the separate `dashboard-home-realignment` convoy artifacts. This violates every brief's explicit file scope and prevents a trustworthy scanner-only review. **Fix:** rebase/cherry-pick the scanner implementation onto `origin/main` (or split the unrelated dashboard/API changes into their own convoy) and re-run the audit. Note that the supplied `git diff` does not show untracked scanner files, so this audit additionally inspected their working-tree contents.
|
||
|
||
2. **[Severity 1 — file intake hardening]** `pages/scanner.js:269-300` accepts an arbitrary number of arbitrary-sized `image/*` files, and `lib/use-scanner-identification.js:20-42` decodes each selected image at full natural dimensions into a canvas before the existing server-side 6 MB request cap can apply. A logged-in user can select a very large or decompression-bomb image set, freezing their tab and creating repeated scan attempts. `accept="image/*"` is only a picker hint, not validation. **Fix:** before `runSequentialGalleryIdentify`, enforce a maximum file count, MIME allowlist, and conservative byte limit compatible with the 6 MB base64 server limit; reject failures per file before calling `readFileToImageData`.
|
||
|
||
3. **[Severity 2 — inherited production dependency advisories]** `npm audit --omit=dev --json` reports five high-severity production vulnerabilities. The most material is the direct `next` range `<16.2.11`; the audit also identifies transitive `jws`, `nanoid`, `postcss`, and `sharp`. No package manifest changed in this convoy, so this is not introduced by the scanner work, but it remains a release risk. **Fix:** open or link the dependency-upgrade/accepted-risk work before release, and verify the deployed Next.js version against the advisories.
|
||
|
||
### Layer notes
|
||
|
||
- **Layer 1:** Existing scanner endpoints retain authentication and the scan rate-limit gate; this convoy adds no server action or API route.
|
||
- **Layer 2:** The list picker is UI convenience only. `pages/api/collections/[identifier]/cards.js:90-101` independently requires an authenticated owner/editor for POST, preventing a forged picker ID from becoming an IDOR.
|
||
- **Layer 3:** React escapes the batch-derived file basename (`pages/scanner.js:226-235`) and failure strings (`components/scanner/ScannerHistoryStrip.js:91-94`); no `dangerouslySetInnerHTML`, `eval`, or shell construction was found. The file-bound finding above remains.
|
||
- **Layer 4:** `lib/use-camera-scanner.js:37-90` stores only opaque camera device IDs in `sessionStorage`, using a fixed key and guarded reads/writes. No scanner secret is exposed to the client.
|
||
- **Layer 5:** See dependency finding 3. The full audit reports 16 issues including dev dependencies; the production-only audit reports five high findings.
|
||
- **Layer 6:** No middleware, Next config, CORS, cookie, or header change was included.
|
||
|
||
### Recommendation
|
||
|
||
**Do not merge as currently scoped.** Resolve the severity-4 scope expansion first. After isolating the scanner-only diff, add the file size/count/type guard (or document an accepted risk) and re-audit; the existing dependency findings should be tracked or risk-accepted separately because they were not introduced here.
|