feat: attach documents to firearms (receipts, warranties, ATF forms) - #68
Merged
unclesp1d3r merged 6 commits intoJul 12, 2026
Merged
unclesp1d3r merged 6 commits into
unclesp1d3r merged 6 commits into
Conversation
… downloading, and deleting documents Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Let a firearm's owner attach, view, download, and delete typed documents (receipts, warranties, ATF Form 1/4, manuals, insurance) on the firearm detail view, reusing the storage foundation from #9. Closes #12. Documents are owner-only on EVERY operation (list/upload/view/download/ delete) -- unlike photos, they never follow the firearm's grants -- and are never included in CSV export. - New `firearm_document` table + migration: firearm child record, MIME and docType CHECK constraints, FK ON DELETE CASCADE (U1). - Document constants + pure upload validator (U2); owner-only read auth gate `authorizeOwnerOnlyRead` (U4). - Single-blob delete + eager firearm-delete blob cleanup so PII blobs are never left to the bare cascade, plus orphan-sweep coverage (U3, R19). - Domain service: magic-byte content sniff (file-type), RFC-safe filename sanitization, optimistic-then-locked quota, cleanup-on-rollback, most- recent-first listing (U5). - Authenticated serving route with an inline/attachment disposition switch, RFC 6266 filenames, nosniff, CSP frame-ancestors on inline, no-store, and a 404-collapse that hides existence from non-owners (U6). - Owner-only documents UI section with a View/Download split, a hardened view modal (img / sandboxed PDF iframe with an error+Download fallback), destructive delete confirmation, empty state, and a static locked panel for non-owners (U7). - CSV-exclusion regression guard and end-to-end coverage (U8). Hardening from the multi-persona plan and code reviews is built in: the R19 blob-cleanup gap, the 250MB Server Action body-limit vs the 160MB config (raised to 270MB), the CSP directive reconciliation, content sniffing, and a guarded file-type sniff that can't orphan blobs on a corrupt upload. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
2 tasks
WalkthroughAdds owner-only firearm document management across persistence, validation, storage, serving, server actions, UI workflows, cleanup paths, and automated tests. Documents support uploads, inline viewing, downloads, deletion, authorization checks, and CSV exclusion. A separate encryption-backup planning document is also added. ChangesFirearm document feature
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Notion Comment |
coderabbitai
Bot
added
backend
dependencies
There was a problem hiding this comment.
Pull request overview
Adds an owner-only firearm document attachment feature (receipts/warranties/ATF forms/manuals/insurance) on top of the existing storage foundation, with hardened authenticated serving and explicit guarantees that documents never follow firearm sharing grants and never appear in CSV export.
Changes:
- Introduces
firearm_documentschema + migration, plus domain constants/validation and an owner-only read gate for document operations. - Implements document upload/list/delete service with magic-byte MIME sniffing (
file-type), filename sanitization, per-request/per-firearm caps, and best-effort blob cleanup (including eager cleanup on firearm delete and orphan-sweep support). - Adds an authenticated serving route with inline/attachment disposition switching and hardened headers, a new detail-view UI section, and unit/integration/e2e coverage (plus Next Server Action body size limit increase).
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test-support/factories.ts | Adds makeFirearmDocument factory helper for tests. |
| src/storage/orphan-sweep.ts | Treats firearm_document blobs as owned keys to avoid false orphan deletion. |
| src/storage/index.ts | Exposes deleteDocumentBlob from the storage module surface. |
| src/storage/document-blobs.ts | Adds best-effort single-blob deletion helper for documents. |
| src/storage/tests/document-blobs.test.ts | Covers document-blob delete behavior, firearm-delete cleanup, and orphan sweep interactions. |
| src/domain/firearms/service.ts | Extends firearm deletion to eagerly delete associated document blobs post-commit. |
| src/domain/firearm-documents/validate.ts | Adds pure validation helpers (MIME allow-list/size + batch cap + docType guard). |
| src/domain/firearm-documents/urls.ts | Adds client-safe view/download URL helpers for the serving route. |
| src/domain/firearm-documents/service.ts | Implements owner-only document upload/list/delete + serving byte resolution with MIME sniffing and rollback cleanup. |
| src/domain/firearm-documents/sanitize-filename.ts | Adds filename sanitization to prevent traversal/header injection and length-cap. |
| src/domain/firearm-documents/constants.ts | Defines allow-list, size/count caps, and controlled docType set. |
| src/domain/firearm-documents/tests/validate.test.ts | Unit tests for document upload validation + batch/docType helpers. |
| src/domain/firearm-documents/tests/serving.test.ts | Tests serving-route behavior (disposition, headers, 404-collapse, auth). |
| src/domain/firearm-documents/tests/service.test.ts | Integration tests for service behavior (auth, sniffing, quota, list order, delete/servable). |
| src/domain/csv/tests/build.test.ts | Adds regression guard that document data never appears in CSV export. |
| src/db/migrations/meta/0017_snapshot.json | Drizzle snapshot for the new firearm_document table. |
| src/db/migrations/meta/_journal.json | Adds migration journal entry for 0017. |
| src/db/migrations/0017_certain_mentallo.sql | Creates firearm_document table + constraints + FK/index. |
| src/db/inventory-schema.ts | Adds firearmDocument table definition with CHECK constraints and index. |
| src/db/tests/firearm-document.test.ts | DB-level constraint + cascade coverage for firearm_document. |
| src/auth/authorize.ts | Adds authorizeOwnerOnlyRead wrapper for owner-only read authorization. |
| src/auth/tests/authorize.test.ts | Tests the new owner-only read authorization behavior. |
| package.json | Adds file-type dependency. |
| next.config.ts | Raises Server Action bodySizeLimit for document batch uploads. |
| e2e/fixtures/user-pool.ts | Seeds users for firearm-documents e2e coverage. |
| e2e/firearm-documents.spec.ts | E2E coverage for upload/view/download/delete and grantee lockout behavior. |
| docs/plans/2026-07-12-001-feat-encryption-at-rest-backups-plan.md | Adds a requirements-only plan doc for encrypted backups/encryption posture (#67 direction). |
| docs/plans/2026-07-11-001-feat-firearm-documents-plan.md | Adds the implementation plan document for firearm documents (#12). |
| bun.lock | Updates lockfile for file-type and related dependency changes. |
| app/api/documents/[id]/route.ts | Implements authenticated, owner-only document serving with hardened headers + 404-collapse. |
| app/(app)/firearms/firearm-detail-view.tsx | Mounts the documents section owner-only; shows locked panel for non-owners. |
| app/(app)/firearms/documents-actions.ts | Adds server actions for document upload/delete + path revalidation + rate limiting. |
| app/(app)/firearms/[id]/page.tsx | Loads documents for owners only and narrows the shape sent to the client (no storageKey). |
| app/(app)/firearms/[id]/firearm-documents.tsx | Implements the owner-only documents UI, modal viewing, and delete confirmation. |
| app/(app)/firearms/tests/documents-actions.test.ts | Unit tests for server action boundaries (session gating, shaping, ActionResult mapping). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/domain/firearm-documents/constants.ts (1)
16-22: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGuard the hand-synced MIME/docType allow-lists against drift.
ALLOWED_MIME_TYPESandDOC_TYPESmust match the DBCHECKconstraints in0017_certain_mentallo.sqlexactly, and the docstring already flags this as manually maintained. A cheap regression test (or a script run in CI) asserting the two lists are identical would catch drift before it becomes a silent upload rejection or a DB-level acceptance the domain layer disallows.♻️ Example parity check
// e.g. in a db-level test, query information_schema / pg_constraint for the // CHECK constraint's allowed values and assert equality with ALLOWED_MIME_TYPES / DOC_TYPES.Also applies to: 48-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/firearm-documents/constants.ts` around lines 16 - 22, Update the validation/test coverage for ALLOWED_MIME_TYPES and DOC_TYPES to compare both lists against the allowed values in the 0017_certain_mentallo.sql DB CHECK constraints, asserting exact parity. Use an existing DB-level test or CI script and fail when either domain allow-list or constraint values drift.Source: MCP tools
src/db/inventory-schema.ts (1)
433-447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a parity test between the CHECK lists and TS constants.
firearm_document_mime_type_validandfirearm_document_doc_type_validare hand-synced copies ofALLOWED_MIME_TYPES/DOC_TYPES(per the comments at lines 436-437 and 443-444, since SQL can't import the TS constant). Nothing currently pins these two sources together, so a future edit to one list without the other fails silently until an insert with the new value hits the DB constraint in production.Consider adding a test (e.g. in the
src/db/__tests__schema suite) that introspects the constraint definition viapg_get_constraintdefand asserts it matches the TS constant arrays, so drift is caught at test time instead of at insert time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/inventory-schema.ts` around lines 433 - 447, Add a schema parity test in the database schema test suite that introspects the definitions of firearm_document_mime_type_valid and firearm_document_doc_type_valid via pg_get_constraintdef, parses their allowed values, and asserts they match ALLOWED_MIME_TYPES and DOC_TYPES. Ensure the test covers both CHECK constraints so future changes to either the SQL lists or TypeScript constants fail during testing.src/domain/firearm-documents/__tests__/service.test.ts (1)
1-283: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLGTM on the coverage provided. One gap worth noting: no test exercises a mixed valid/invalid batch near the per-firearm cap — see the corresponding comment on
service.ts's optimistic quota check, which this suite doesn't currently catch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/firearm-documents/__tests__/service.test.ts` around lines 1 - 283, Add a service test near the existing per-firearm cap coverage that pre-fills a firearm to one slot below MAX_DOCUMENTS_PER_FIREARM, then submits a mixed batch containing one valid document and one invalid document. Assert the valid item succeeds, the invalid item is rejected with its validation code, and the firearm retains only the valid new document without an optimistic quota rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/`(app)/firearms/[id]/firearm-documents.tsx:
- Around line 184-205: Update the modal flow around the viewTarget focus effects
and dialog markup to trap Tab focus within the modal and make the obscured page
inert while it is open. Reuse the established dialog primitive if available,
preserving initial focus on viewCloseRef, Escape dismissal via
setViewTarget(null), and restoration through viewRestoreFocusRef; apply the same
behavior to the additional affected modal section.
- Around line 500-507: Update the document viewer flow around the iframe and its
view-target state to perform an explicit preflight/status check on
documentViewUrl(viewTarget.id) before rendering the iframe, treating non-success
responses and unreadable/corrupt PDFs as load failures. Use the result to show
the existing fallback deterministically, and retain the iframe only for
documents that pass validation rather than relying on its onError handler.
- Around line 341-349: Define or reuse a shared maximum-length constant for
document notes, then apply it as the maxLength prop on the Textarea in the notes
field. Keep the existing notes state binding and upload behavior unchanged, and
use the same shared limit wherever document notes are validated or persisted.
In `@app/`(app)/firearms/documents-actions.ts:
- Around line 71-73: Update the success return in the document action around
createDocuments so each result’s full FirearmDocument is mapped to a client DTO
containing only the fields required by the UI, excluding storageKey and other
internal fields. Preserve the existing ok/data response shape and revalidation
behavior.
In `@docs/plans/2026-07-12-001-feat-encryption-at-rest-backups-plan.md`:
- Around line 59-64: Update the force-replace restore requirements in R7 and the
corresponding restore flow to acquire a maintenance/exclusive-write lock before
snapshotting or wiping. Snapshot both Postgres data and the upload/blob volume,
then restore the bundle atomically; on any failure, roll back both snapshots
together so database rows and blobs remain consistent and concurrent writes
cannot cause data loss.
In `@e2e/firearm-documents.spec.ts`:
- Around line 76-124: Extend the PDF test flow around the existing “download the
PDF via its Download control (F4)” step to open the PDF’s view control before
downloading it. Assert the PDF dialog is visible, contains the inline iframe,
and that the iframe has the expected pinned sandbox value; then close the dialog
and preserve the existing download assertions unchanged.
In `@src/domain/csv/__tests__/build.test.ts`:
- Around line 31-58: Update the test around buildInventoryCsv so the document is
attached to a magazine included in the exported inventory row, ensuring the
before and after CSV outputs exercise actual row serialization while retaining
the document-field leakage assertions. Use the existing magazine factory and
makeFirearmDocument flow, and keep the test’s byte-identical expectation.
In `@src/domain/firearm-documents/sanitize-filename.ts`:
- Around line 41-58: Update sanitizeFilename to prevent truncation from
returning a lone UTF-16 surrogate. After both the ordinary length limit and the
extension-preserving slice in sanitizeFilename, adjust the cut boundary backward
when it lands between a high and low surrogate, while preserving the existing
fallback, extension, and maximum-length behavior.
In `@src/domain/firearm-documents/service.ts`:
- Around line 148-159: Update the optimistic quota check around existingCount
and inputs so it short-circuits only when the firearm is already at
MAX_DOCUMENTS_PER_FIREARM capacity, rather than counting raw submitted inputs.
Leave the authoritative transaction check using prepared.length unchanged,
allowing mixed-validity batches to validate and partially succeed.
---
Nitpick comments:
In `@src/db/inventory-schema.ts`:
- Around line 433-447: Add a schema parity test in the database schema test
suite that introspects the definitions of firearm_document_mime_type_valid and
firearm_document_doc_type_valid via pg_get_constraintdef, parses their allowed
values, and asserts they match ALLOWED_MIME_TYPES and DOC_TYPES. Ensure the test
covers both CHECK constraints so future changes to either the SQL lists or
TypeScript constants fail during testing.
In `@src/domain/firearm-documents/__tests__/service.test.ts`:
- Around line 1-283: Add a service test near the existing per-firearm cap
coverage that pre-fills a firearm to one slot below MAX_DOCUMENTS_PER_FIREARM,
then submits a mixed batch containing one valid document and one invalid
document. Assert the valid item succeeds, the invalid item is rejected with its
validation code, and the firearm retains only the valid new document without an
optimistic quota rejection.
In `@src/domain/firearm-documents/constants.ts`:
- Around line 16-22: Update the validation/test coverage for ALLOWED_MIME_TYPES
and DOC_TYPES to compare both lists against the allowed values in the
0017_certain_mentallo.sql DB CHECK constraints, asserting exact parity. Use an
existing DB-level test or CI script and fail when either domain allow-list or
constraint values drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d4f51a96-19fe-4976-83d8-0d57c5b8d9e3
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lock,!bun.locke2e/fixtures/sample-document.pdfis excluded by!**/*.pdf
📒 Files selected for processing (34)
app/(app)/firearms/[id]/firearm-documents.tsxapp/(app)/firearms/[id]/page.tsxapp/(app)/firearms/__tests__/documents-actions.test.tsapp/(app)/firearms/documents-actions.tsapp/(app)/firearms/firearm-detail-view.tsxapp/api/documents/[id]/route.tsdocs/plans/2026-07-11-001-feat-firearm-documents-plan.mddocs/plans/2026-07-12-001-feat-encryption-at-rest-backups-plan.mde2e/firearm-documents.spec.tse2e/fixtures/user-pool.tsnext.config.tspackage.jsonsrc/auth/__tests__/authorize.test.tssrc/auth/authorize.tssrc/db/__tests__/firearm-document.test.tssrc/db/inventory-schema.tssrc/db/migrations/0017_certain_mentallo.sqlsrc/db/migrations/meta/0017_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/domain/csv/__tests__/build.test.tssrc/domain/firearm-documents/__tests__/service.test.tssrc/domain/firearm-documents/__tests__/serving.test.tssrc/domain/firearm-documents/__tests__/validate.test.tssrc/domain/firearm-documents/constants.tssrc/domain/firearm-documents/sanitize-filename.tssrc/domain/firearm-documents/service.tssrc/domain/firearm-documents/urls.tssrc/domain/firearm-documents/validate.tssrc/domain/firearms/service.tssrc/storage/__tests__/document-blobs.test.tssrc/storage/document-blobs.tssrc/storage/index.tssrc/storage/orphan-sweep.tssrc/test-support/factories.ts
…sanitizer Address PR review findings: - deleteFirearm's doc comment falsely claimed the owning-user-account clause of R19 "rides this same path" and runs the blob-cleanup hook. It does not: user deletion is a raw DB delete whose owner_id FK cascade drops firearm and firearm_document rows directly, bypassing deleteFirearm(). Corrected the comment to state the gap honestly (orphanSweep is the backstop; a dedicated account-deletion cleanup is follow-up; pre-existing for photos too). - sanitizeFilename now also strips the double-quote (the Content-Disposition quoted-string delimiter), completing its stated header-injection defense (KTD6) rather than relying solely on the serving route's escaping. - Add the missing dedicated unit test for the security-critical sanitizer, covering separators, C0/DEL controls, quotes, fallback, truncation, and non-ASCII preservation. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Resolve every valid finding from the multi-agent PR review — no deferrals. Code hardening: - createDocuments: split validation from blob writes so the optimistic quota check counts only files that would persist (a mixed valid/invalid batch near the cap is no longer rejected wholesale), and replace the documented- unreachable content-sniff guard with a clean `sniffAllowedMime` helper returning `AllowedMimeType | null`. - Extract the client-safe row shape + narrowing constructor into `row.ts` (`toFirearmDocumentRow`) as the single enforcement point that keeps `storageKey` off the client, and narrow the upload action's response through it too (it previously returned the full row incl. storageKey to the client). - View modal: fetch the document and render from an object URL instead of pointing <img>/<iframe> src directly at the endpoint — an <iframe> does not fire `error` for HTTP 4xx/5xx, so a failed PDF view (deleted/revoked/missing blob) now reliably shows the error + Download fallback (R21), with an explicit loading state. The `allow-scripts`-only sandbox (R15 binding control) is unchanged. - Log unmapped errors in `toActionError` and the getServableDocument blob read (context only, never filename/notes PII) so PII-feature failures leave a trail. - Correct the deleteFirearm comment (user-account deletion is a raw FK cascade that bypasses the app cleanup hook; orphanSweep is the backstop). - Harden the filename sanitizer to strip the double-quote; cap notes length; align DOWNLOAD_LINK_CLASS with the Button secondary variant. Tests (+~30): dedicated sanitizer suite; fileTypeFromBuffer throw; storage.save failure; transaction-rollback blob cleanup; declared-vs-sniffed MIME; unknown-id delete; mixed-batch quota regression; concurrent quota race; stranger→404 collapse; DOC_TYPES/MIME drift guard; multi-document firearm-delete; row serialization. E2e: invalid-upload feedback + multi-file batch. Chore: run `biome migrate` to align biome.json's schema with the pinned @biomejs/biome 2.5.2 (dependabot #60 left it at 2.5.1), unblocking lint. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/domain/firearm-documents/__tests__/service.test.ts (1)
75-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mock.module("file-type", ...)is never restored — verify it doesn't leak into sibling test files.Bun's
mock.module()overrides are not undone bymock.restore()and are documented/known to persist across test files run in the same process (oven-sh/bun#12823 is still open). This mock delegates to the realfileTypeFromBufferfor all non-poisoned buffers, so it's likely benign today, but it's an ordering-dependent risk if another spec file (e.g.serving.test.ts,validate.test.ts) also importsfile-typeand runs in the samebun testprocess. Contrast with theLocalFilesystemAdapter.prototype.savespy elsewhere in this file, which is properly scoped withmockRestore()in afinally.Consider using
--preloadfor this mock, or confirming this repo's CI runs each spec file in an isolated process/worker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/firearm-documents/__tests__/service.test.ts` around lines 75 - 90, Prevent the file-type override created near capturedFileTypeFromBuffer and FILE_TYPE_THROW_MARKER from leaking into sibling test files. Prefer moving this mock setup into the repository’s preload mechanism, or verify and enforce isolated test-file processes/workers in the test configuration; preserve the forced-throw behavior for this suite while ensuring the override is not shared across suites.src/domain/firearm-documents/__tests__/sanitize-filename.test.ts (1)
55-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd truncation coverage for surrogate-pair (emoji) filenames.
All length-cap tests use plain ASCII.
String.prototype.slice()counts UTF-16 code units, not code points, so a truncation boundary landing inside a surrogate pair (e.g. an emoji near position 200) would split it into a lone surrogate — this is a known open finding for this feature (surrogate-pair splitting during truncation can break downloads/previews for emoji-containing names). Add a case with a repeated multi-byte character crossingMAX_FILENAME_LENGTHto lock in correct (or corrected) behavior.test("does not split a surrogate pair when truncating (emoji at the boundary)", () => { const long = `${"🔥".repeat(250)}.pdf`; // 250 astral chars = 500 UTF-16 units const out = sanitizeFilename(long); expect(out.length).toBeLessThanOrEqual(200); expect(out).not.toMatch(/[\uD800-\uDBFF]$/); // no dangling high surrogate });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/firearm-documents/__tests__/sanitize-filename.test.ts` around lines 55 - 67, Add a test in the sanitizeFilename length-cap coverage using repeated emoji characters that cross MAX_FILENAME_LENGTH, and assert the result stays within the cap without ending in a dangling high surrogate. Update sanitizeFilename truncation logic as needed so truncation preserves complete surrogate pairs while retaining existing ASCII and extension behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domain/action-result.ts`:
- Around line 27-32: Update the unhandled-error logging in the action-result
flow to avoid passing the raw error object to console.error. Log only a
minimized, sanitized shape containing the error name, message, and stack, while
preserving the existing generic user-facing response.
---
Nitpick comments:
In `@src/domain/firearm-documents/__tests__/sanitize-filename.test.ts`:
- Around line 55-67: Add a test in the sanitizeFilename length-cap coverage
using repeated emoji characters that cross MAX_FILENAME_LENGTH, and assert the
result stays within the cap without ending in a dangling high surrogate. Update
sanitizeFilename truncation logic as needed so truncation preserves complete
surrogate pairs while retaining existing ASCII and extension behavior.
In `@src/domain/firearm-documents/__tests__/service.test.ts`:
- Around line 75-90: Prevent the file-type override created near
capturedFileTypeFromBuffer and FILE_TYPE_THROW_MARKER from leaking into sibling
test files. Prefer moving this mock setup into the repository’s preload
mechanism, or verify and enforce isolated test-file processes/workers in the
test configuration; preserve the forced-throw behavior for this suite while
ensuring the override is not shared across suites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1969ce70-d31f-4945-8365-f71930775b6f
📒 Files selected for processing (18)
app/(app)/firearms/[id]/firearm-documents.tsxapp/(app)/firearms/[id]/page.tsxapp/(app)/firearms/documents-actions.tsapp/(app)/firearms/firearm-detail-view.tsxbiome.jsone2e/firearm-documents.spec.tssrc/db/__tests__/firearm-document.test.tssrc/domain/action-result.tssrc/domain/firearm-documents/__tests__/row.test.tssrc/domain/firearm-documents/__tests__/sanitize-filename.test.tssrc/domain/firearm-documents/__tests__/service.test.tssrc/domain/firearm-documents/__tests__/serving.test.tssrc/domain/firearm-documents/constants.tssrc/domain/firearm-documents/row.tssrc/domain/firearm-documents/sanitize-filename.tssrc/domain/firearm-documents/service.tssrc/domain/firearms/service.tssrc/storage/__tests__/document-blobs.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/domain/firearm-documents/sanitize-filename.ts
- src/domain/firearm-documents/constants.ts
- src/domain/firearms/service.ts
- src/storage/tests/document-blobs.test.ts
- app/(app)/firearms/documents-actions.ts
- src/db/tests/firearm-document.test.ts
- app/(app)/firearms/[id]/page.tsx
- app/(app)/firearms/[id]/firearm-documents.tsx
- src/domain/firearm-documents/service.ts
- src/domain/firearm-documents/tests/serving.test.ts
- Trap Tab focus inside the View modal (WCAG 2.2 AA), mirroring ConfirmDialog; preserves initial focus, Escape, and focus restoration, sandbox unchanged. - Bound the notes Textarea with maxLength using the shared MAX_NOTES_LENGTH. - sanitizeFilename: truncate by code point (Array.from) so a surrogate pair is never split at the length cap; add astral-char tests. - firearm-document schema test: use a flat storage key matching generateKey's production shape (no docs/ prefix orphanSweep would not expect). - CSV-exclusion test: link the firearm to a magazine so it appears in an actual exported row, making the byte-identical no-leak assertion meaningful. - e2e: open the PDF in the View modal and assert the sandboxed inline iframe (sandbox="allow-scripts"), covering the highest-risk inline-serving path. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
unclesp1d3r
deleted the
12-attach-documents-to-firearms-receipts-warranties-atf-forms
branch
July 12, 2026 20:01
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.
Summary
Lets a firearm's owner attach, view, download, and delete typed documents (receipts, warranties, ATF Form 1/4, manuals, insurance) on the firearm detail view, reusing the storage foundation from #9. Documents are owner-only on every operation (list / upload / view / download / delete) — unlike photos, they never follow the firearm's grants — and are never included in CSV export. Fixes #12.
file-type)just ci-checkgreen (lint · format · typecheck · pre-commit · full unit/integration suite · full Playwright e2e, 33 passed)For Reviewers — suggested review order
Review the security spine first (top), then the mechanical mirror-of-photos work (bottom):
src/db/inventory-schema.ts(firearmDocument) +src/db/migrations/0017_*.sql. The shape: owner-less firearm child, MIME/docType CHECK constraints, FKON DELETE CASCADE.src/auth/authorize.ts(authorizeOwnerOnlyRead). The owner-only read gate; the security core.src/domain/firearm-documents/service.ts⭐ highest-logic file: magic-byte content sniff, filename sanitization, optimistic-then-locked quota, cleanup-on-rollback.app/api/documents/[id]/route.ts: 404-collapse existence-hiding, inline/attachment disposition, RFC 6266 filenames,nosniff/ CSPframe-ancestors/no-store.app/(app)/firearms/[id]/firearm-documents.tsx+firearm-detail-view.tsx:isOwnersection gating + static locked panel, thesandbox="allow-scripts"PDF iframe, the fetch-to-blob View modal with focus trap.src/storage/document-blobs.ts+ thedeleteFirearmhook insrc/domain/firearms/service.ts(R19 eager cleanup) +orphan-sweep.ts.__tests__/*ande2e/*files (mirror the sibling photo suites),constants.ts,validate.ts,urls.ts,row.ts.Request / serve flow
What changed
firearm_documenttable + migration (0017): firearm child, MIME + docType CHECK constraints,sizeBytes > 0, FKON DELETE CASCADEauthorizeOwnerOnlyRead— owner-only read gate (none existed before)file-type), filename sanitization, optimistic-then-locked quota, cleanup-on-rollback, most-recent-first listingnosniff, CSPframe-ancestorson inline,no-store, 404-collapse hiding existence from non-owners<img>/ sandboxed PDF<iframe>, error+Download fallback, focus trap), delete confirmation, empty state, static locked panel for non-ownersSecurity posture
"delimiter (header-injection defense), truncates surrogate-safe; serving uses RFC 6266 encoding.sandbox="allow-scripts"iframe (never withallow-same-origin),nosniff, anti-framing CSP,no-store; the View modal fetches then renders so HTTP failures reliably show the error+Download fallback (R21).storageKeynever reaches the client — narrowed throughtoFirearmDocumentRowon both the page load and the upload-action response.Risk Assessment
firearm-photossibling's shapefile-type) for magic-byte sniffing;bodySizeLimitraised 160→270 MB for the document batchMitigation summary: the inherent high-risk surface is reduced by (a) mirroring an already-reviewed sibling feature, (b) ≈1:1 adversarial test coverage, (c) four review passes (multi-persona plan review, ce-code-review, pr-review-toolkit, CodeRabbit + Copilot), and (d) a green
just ci-checkincluding the full e2e suite with no regressions.Test coverage
sizeBytes>0), FK cascade, DOC_TYPES/MIME drift guardstorage.savefailure, most-recent-firstnosniff/ CSP /no-store, non-owner + stranger → 404 collapse, 401, malformed/unknown idReviewer checklist
Security
nosniff+ CSP present; PDF iframe sandbox never combinesallow-scripts+allow-same-originstorageKeynever crosses to the client (page load and upload response)Code quality
console.login production code (✅ verified 0)data-testidin the app (ARIA/accessible-names only)Testing
DATABASE_URL(Testcontainers); e2e via the Playwright harnessValidation
just ci-check— Biome lint + format,tsc, pre-commit hooks, fullbun testsuite, and the full Playwright e2e suite (33 passed, 4 skipped demos) — includinge2e/firearm-documents.spec.tswith no regression to existing specs. Live browser smoke confirmed boot/serve/auth-gating.Review history — all findings resolved
Four review passes ran; every valid finding is fixed (commits
9442634/93bae78/73f7f96), nothing deferred: R19 blob-cleanup gap, the 250 MB-vs-160 MB body-limit conflict, CSP directive reconciliation, an unguardedfile-typethrow, a git-binary sanitizer, quota-overcount,storageKeyclient leak, the unreliable PDF<iframe onError>, silent-failure logging, WCAG focus trap, notesmaxLength, surrogate-safe truncation, a weak CSV assertion, and PDF-view e2e coverage.Not code-fixed (with reasons, not residuals): the NotFound-vs-NotAuthorized "leak" is the explicit R9/AE3 design (the serving route is the stricter 404-collapse); the Server-Action body-buffer-before-auth is a pre-existing Next.js property shared with photos, already mitigated by the middleware edge cookie-gate + upload rate limiter, with a streaming Route Handler tracked as a cross-cutting follow-up.
Follow-up / out of scope
Per the plan's Deferred / Open Questions: NFA-trust co-owner read access to ATF Form 1/4, document egress/backup coverage, and finer upload-interaction states. Encryption-at-rest is a design constraint here (blobs stay behind the storage seam; PII columns recorded) and is planned separately (#67).