From eb1779d5c19a5d5f3653cac1381120f65192bda6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 28 Jun 2026 23:48:43 +0200 Subject: [PATCH] fix(documents): close v1.25.1 library QA findings (backend) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Default documentDate at store time. A store-only upload no longer lands with a NULL filing date: when the client omits one, default it to the upload day so the column is always set on a stored row. The library list sorts, groups, and range-filters on the same effective date the UI shows, so undated uploads no longer mis-sort under nulls-last, break date-group headers, or fall through the range filter. The date stays user-editable and may be cleared back to NULL via PATCH. Block re-extraction once any fact is APPROVED. A partially-confirmed document stays EXTRACTED, which the CONFIRMED guard does not catch; re-extracting would delete the approved facts (severing the committed record's provenance) and re-stage them as PENDING, letting a second approval duplicate the committed lab / condition / medication. The extract route now refuses with 409 (documents.inbound.alreadyPartlyConfirmed) when approved facts exist; the user must finish reviewing or discard first. RFC 5987-encode the original-download filename. The Content-Disposition now carries both an ASCII filename= fallback and a filename*=UTF-8'' value so a non-ASCII name survives intact; the stored set is inline-safe by construction, so the dead attachment branch is removed. Add a light per-user rate-limit backstop to the original-read, PATCH, and DELETE routes (generous; self-DoS only — the routes are owner-scoped). Lean the list counts: tally facts for the whole page in one grouped count instead of materialising fact-id rows, and exclude REJECTED facts from factCount. Extend the documentDate index with deleted_at to cover the list scan. Document the deliberate plaintext-title PHI tradeoff at the column and the store site. --- docs/api/openapi.yaml | 22 +++-- .../0222_v125_documents_library/migration.sql | 12 +-- prisma/schema.prisma | 81 ++++++++++--------- .../inbound/[id]/__tests__/route.test.ts | 7 ++ .../[id]/extract/__tests__/route.test.ts | 27 ++++++- .../documents/inbound/[id]/extract/route.ts | 30 ++++++- .../[id]/original/__tests__/route.test.ts | 35 ++++++-- .../documents/inbound/[id]/original/route.ts | 63 +++++++++++---- src/app/api/documents/inbound/[id]/route.ts | 31 +++++++ .../documents/inbound/__tests__/route.test.ts | 27 ++++++- src/app/api/documents/inbound/route.ts | 54 ++++++++++--- src/lib/documents/store.ts | 5 +- src/lib/openapi/routes/documents.ts | 15 ++-- src/lib/validations/inbound-documents.ts | 2 + 14 files changed, 317 insertions(+), 94 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 77784d252..083cf949d 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -8714,7 +8714,8 @@ paths: - VACCINATION - OTHER documentDate: - description: Optional user filing date (YYYY-MM-DD). + description: Optional user filing date (YYYY-MM-DD). Defaults to the upload day when omitted so the library + sort/filter/display stay aligned. type: string required: - file @@ -8828,10 +8829,10 @@ paths: - Documents summary: View / download the original document description: "Decrypts and serves the raw uploaded document (the bytes stored encrypted at rest under - `InboundDocument.contentEncrypted`). Owner-scoped + gated on the opt-in `inboundDocuments` module. The response - carries the stored `mimeType`: a PDF / image is served inline (`Content-Disposition: inline`) so the browser can - render it; any other type downloads as an attachment. Fails closed (500) on a decrypt error — it never returns - the ciphertext." + `InboundDocument.contentEncrypted`). Owner-scoped + gated on the opt-in `inboundDocuments` module. Uploads are + constrained at the store path to the inline-safe set (JPEG / PNG / WebP / PDF), so the document is always served + `Content-Disposition: inline` for in-tab rendering; a non-ASCII filename is carried via RFC 5987 `filename*`. + Fails closed (500) on a decrypt error — it never returns the ciphertext." parameters: - name: id in: path @@ -8850,9 +8851,6 @@ paths: schema: type: string format: binary - text/plain: - schema: - type: string "401": *a1 "422": *a3 "429": *a4 @@ -8888,6 +8886,14 @@ paths: schema: $ref: "#/components/schemas/InboundExtractEnvelope" "401": *a1 + "409": + description: "Re-extraction refused: at least one fact on this document is already APPROVED. `meta.errorCode` = + `documents.inbound.alreadyPartlyConfirmed`. Re-extracting would sever committed-record provenance and + duplicate committed records, so the user must finish reviewing or discard the document first." + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" "422": *a3 "429": *a4 /api/documents/inbound/{id}/facts/{factId}: diff --git a/prisma/migrations/0222_v125_documents_library/migration.sql b/prisma/migrations/0222_v125_documents_library/migration.sql index 8ee63ab32..fba0ac956 100644 --- a/prisma/migrations/0222_v125_documents_library/migration.sql +++ b/prisma/migrations/0222_v125_documents_library/migration.sql @@ -13,8 +13,8 @@ -- * `document_date` — user-set filing date (editable). Distinct from -- `report_date`, which stays the model-transcribed -- value. --- * a `(user_id, document_date)` index for date-grouped browse + range --- filter. +-- * a `(user_id, deleted_at, document_date)` index for date-grouped browse + +-- range filter (the list always filters `deleted_at IS NULL`). -- * `inbound_document_kind` gains LAB_RESULT / IMAGING / PRESCRIPTION / -- INSURANCE / VACCINATION (the existing 3 stay; OTHER stays the fallback). -- * `inbound_document_status` gains STORED, made the column DEFAULT (the @@ -79,6 +79,8 @@ BEGIN END IF; END $$; --- CreateIndex: date-grouped browse + document_date range filter. -CREATE INDEX IF NOT EXISTS "inbound_documents_user_id_document_date_idx" - ON "inbound_documents"("user_id", "document_date"); +-- CreateIndex: date-grouped browse + document_date range filter. The list +-- always filters `deleted_at IS NULL` and sorts by `document_date`, so +-- deleted_at leads the sort column to keep the scan covered. +CREATE INDEX IF NOT EXISTS "inbound_documents_user_id_deleted_at_document_date_idx" + ON "inbound_documents"("user_id", "deleted_at", "document_date"); diff --git a/prisma/schema.prisma b/prisma/schema.prisma index fccb5fc20..895e694fc 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -59,10 +59,10 @@ model User { // without a home set) simply has no environment rows. `homeTimezone` anchors // the day-key an observation is stored under (it can differ from the display // `timezone` above for an account that tracks weather for another locale). - homeLat Float? @map("home_lat") - homeLon Float? @map("home_lon") - homeLabel String? @map("home_label") - homeTimezone String? @map("home_timezone") + homeLat Float? @map("home_lat") + homeLon Float? @map("home_lon") + homeLabel String? @map("home_label") + homeTimezone String? @map("home_timezone") // v1.25 — effective-from instant for the home location. Stamped the moment a // home is set/updated. Day resolution attributes a past day to home ONLY when // the day is on/after this instant; earlier days are skipped (never fabricated @@ -5581,39 +5581,44 @@ enum ExtractedFactStatus { } model InboundDocument { - id String @id @default(cuid()) - userId String @map("user_id") - kind InboundDocumentKind @default(OTHER) - /// User-given label. Plaintext (like `filename`) so the library list can - /// sort + search in SQL. The document BODY stays encrypted; only this - /// short, user-authored title is clear-text — it is what makes ILIKE - /// search and ORDER BY possible. - title String? + id String @id @default(cuid()) + userId String @map("user_id") + kind InboundDocumentKind @default(OTHER) + /// User-given label. Deliberately plaintext (like `filename`) so the library + /// list can ILIKE-search + ORDER BY in SQL. The document BODY stays + /// encrypted; only this short, user-authored title is clear-text. Tradeoff: + /// a title the user types MAY contain PHI (e.g. "HIV result") and is then + /// cleartext at rest / in backups — the same exposure the pre-existing + /// plaintext `filename` already carries. Accepted to keep server-side search + /// and sort. + title String? /// Original filename as uploaded (sanitised, optional). - filename String? + filename String? /// Magic-byte-sniffed MIME type the document was stored as. - mimeType String @map("mime_type") + mimeType String @map("mime_type") /// Stored byte length of the original (pre-encryption) document. - byteSize Int @map("byte_size") + byteSize Int @map("byte_size") /// AES-256-GCM ciphertext of the raw document bytes (base64 of the binary /// → `encrypt()` string → UTF-8 BYTEA), the `*_encrypted` Bytes convention /// shared with the note columns. The original is sensitive and never /// stored in the clear or logged. - contentEncrypted Bytes @map("content_encrypted") - status InboundDocumentStatus @default(STORED) + contentEncrypted Bytes @map("content_encrypted") + status InboundDocumentStatus @default(STORED) /// Provider type that ran the extraction (e.g. "anthropic", "openai", /// "local"). Diagnostic only. Null on a store-only document. - providerType String? @map("provider_type") + providerType String? @map("provider_type") /// The document's stated report/collection date (YYYY-MM-DD at UTC), as /// transcribed by the model — never inferred, never user-set. - reportDate DateTime? @map("report_date") - /// User-set filing date (editable). Distinct from `reportDate`, which is - /// the model-transcribed value. The library shows `documentDate`, - /// defaulting to `reportDate` then `createdAt` when the user hasn't set - /// one. - documentDate DateTime? @map("document_date") + reportDate DateTime? @map("report_date") + /// Effective filing date the library sorts, groups, and range-filters on + /// (editable). Distinct from `reportDate`, which is the model-transcribed + /// value. On store it defaults to the upload day (now) when the user does + /// not supply one, so a stored row's filing date is never NULL and display + /// == sort == filter. It stays user-editable and MAY be cleared back to + /// NULL via PATCH (the list then sorts such rows last). + documentDate DateTime? @map("document_date") /// Failure reason when status = FAILED (a short code, never the document). - errorReason String? @map("error_reason") + errorReason String? @map("error_reason") facts ExtractedFact[] @@ -5626,24 +5631,26 @@ model InboundDocument { @@index([userId, createdAt]) @@index([userId, deletedAt]) - @@index([userId, documentDate]) + // The list always filters `deletedAt: null` and sorts by `documentDate`, so + // deletedAt leads the sort column to keep the scan covered. + @@index([userId, deletedAt, documentDate]) @@map("inbound_documents") } model ExtractedFact { - id String @id @default(cuid()) - documentId String @map("document_id") - userId String @map("user_id") - factType ExtractedFactType @map("fact_type") - status ExtractedFactStatus @default(PENDING) + id String @id @default(cuid()) + documentId String @map("document_id") + userId String @map("user_id") + factType ExtractedFactType @map("fact_type") + status ExtractedFactStatus @default(PENDING) /// Overall extraction confidence 0..1 (the lowest per-field score). The /// review screen gates approval of a low-confidence fact behind a manual /// edit (fail-closed). - confidence Float @default(0) + confidence Float @default(0) /// True when the fact scored below the confidence floor and must be /// reviewed/edited before it can be approved. Cleared once the user edits /// the fact (the values then become user-asserted). - needsReview Boolean @default(true) @map("needs_review") + needsReview Boolean @default(true) @map("needs_review") /// The FHIR-staged fields, STATED status only, shaped per `factType` /// (Condition / Observation / MedicationStatement). Codes (SNOMED / ICD-10 /// / LOINC / RxNorm / ATC) are present only when the document writes them. @@ -5651,20 +5658,20 @@ model ExtractedFact { /// medication names, stated codes) is clinical PHI transcribed verbatim from /// the source document, so it rides the AES-256-GCM `*_encrypted` BYTEA /// convention shared with the note columns rather than as plaintext JSONB. - dataEncrypted Bytes @map("data_encrypted") + dataEncrypted Bytes @map("data_encrypted") /// Per-field provenance: the verbatim source span the value came from, an /// optional page index, and per-field confidence. Makes "extract what's /// written" auditable against the original. The source span is a verbatim /// excerpt of the clinical document (PHI), so it is stored encrypted with the /// same codec as `dataEncrypted`. - provenanceEncrypted Bytes @map("provenance_encrypted") + provenanceEncrypted Bytes @map("provenance_encrypted") /// The id of the structured-store row created when the fact was approved /// (LabResult / IllnessEpisode / Medication), or NULL while PENDING / /// REJECTED. - committedRecordId String? @map("committed_record_id") + committedRecordId String? @map("committed_record_id") /// The structured store the approved fact landed in ("labResult" / /// "illnessEpisode" / "medication"), or NULL. - committedRecordType String? @map("committed_record_type") + committedRecordType String? @map("committed_record_type") document InboundDocument @relation(fields: [documentId], references: [id], onDelete: Cascade) user User @relation(fields: [userId], references: [id], onDelete: Cascade) diff --git a/src/app/api/documents/inbound/[id]/__tests__/route.test.ts b/src/app/api/documents/inbound/[id]/__tests__/route.test.ts index 720b4a4a0..b9dd14372 100644 --- a/src/app/api/documents/inbound/[id]/__tests__/route.test.ts +++ b/src/app/api/documents/inbound/[id]/__tests__/route.test.ts @@ -34,6 +34,13 @@ vi.mock("@/lib/modules/gate", () => ({ requireModuleEnabled: vi.fn().mockResolvedValue({ enabled: true }), })); +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi + .fn() + .mockResolvedValue({ allowed: true, remaining: 239, resetAt: Date.now() }), + rateLimitHeaders: () => ({}), +})); + vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn().mockResolvedValue(undefined), diff --git a/src/app/api/documents/inbound/[id]/extract/__tests__/route.test.ts b/src/app/api/documents/inbound/[id]/extract/__tests__/route.test.ts index bdc0d2d6f..1dde37d5d 100644 --- a/src/app/api/documents/inbound/[id]/extract/__tests__/route.test.ts +++ b/src/app/api/documents/inbound/[id]/extract/__tests__/route.test.ts @@ -13,7 +13,7 @@ import { NextRequest } from "next/server"; vi.mock("@/lib/db", () => ({ prisma: { inboundDocument: { findFirst: vi.fn(), update: vi.fn() }, - extractedFact: { deleteMany: vi.fn() }, + extractedFact: { deleteMany: vi.fn(), count: vi.fn() }, user: { findUnique: vi.fn() }, $transaction: vi.fn(), }, @@ -104,6 +104,7 @@ beforeEach(() => { mimeType: "image/png", status: "STORED", } as never); + vi.mocked(prisma.extractedFact.count).mockResolvedValue(0 as never); }); describe("POST /api/documents/inbound/[id]/extract", () => { @@ -123,6 +124,30 @@ describe("POST /api/documents/inbound/[id]/extract", () => { expect(prisma.inboundDocument.update).not.toHaveBeenCalled(); }); + it("409s and refuses re-extraction when any fact is already APPROVED", async () => { + // A partially-confirmed document stays at EXTRACTED, so the CONFIRMED gate + // does not catch it; the approved-fact guard must. + vi.mocked(prisma.inboundDocument.findFirst).mockResolvedValue({ + id: "doc-1", + kind: "OTHER", + contentEncrypted: new Uint8Array([1, 2, 3]), + mimeType: "image/png", + status: "EXTRACTED", + } as never); + vi.mocked(prisma.extractedFact.count).mockResolvedValue(2 as never); + + const res = await POST(visionReq("doc-1") as never, ctx("doc-1") as never); + expect(res.status).toBe(409); + const body = await res.json(); + expect(body.meta?.errorCode).toBe( + "documents.inbound.alreadyPartlyConfirmed", + ); + + // No staging transaction, no fact deletion — committed provenance is safe. + expect(prisma.$transaction).not.toHaveBeenCalled(); + expect(prisma.extractedFact.deleteMany).not.toHaveBeenCalled(); + }); + it("404s for a document the caller does not own", async () => { vi.mocked(prisma.inboundDocument.findFirst).mockResolvedValue( null as never, diff --git a/src/app/api/documents/inbound/[id]/extract/route.ts b/src/app/api/documents/inbound/[id]/extract/route.ts index 8d382bb5c..cbd6a513f 100644 --- a/src/app/api/documents/inbound/[id]/extract/route.ts +++ b/src/app/api/documents/inbound/[id]/extract/route.ts @@ -81,9 +81,10 @@ type LoadedDocument = { /** * Replace the document's staged facts and flip it to EXTRACTED in one - * transaction. Re-extraction is allowed (a STORED / FAILED / EXTRACTED row), - * so any prior PENDING staging is cleared first; facts already APPROVED into - * the structured stores are independent rows and are not touched here. + * transaction. Re-extraction is allowed only when NO fact is APPROVED yet (the + * POST handler refuses it otherwise), so clearing every prior staged fact here + * can never drop an approved row or sever a committed-record provenance link — + * the only facts present are PENDING / REJECTED leftovers from an earlier run. */ async function stageExtraction( documentId: string, @@ -149,6 +150,29 @@ export const POST = apiHandler( }); } + // Refuse re-extraction once ANY fact on this document is APPROVED. A + // partially-confirmed document (some facts approved, the rest still pending) + // stays at status EXTRACTED, so the CONFIRMED gate above does not catch it. + // Re-extracting would `deleteMany` the APPROVED rows — severing the + // committed-record provenance link — and re-stage the same facts as PENDING, + // letting the user approve them a second time and duplicate the committed + // lab / condition / medication. Block it: the user must finish reviewing or + // discard the document first. + const approvedCount = await prisma.extractedFact.count({ + where: { documentId: document.id, userId: user.id, status: "APPROVED" }, + }); + if (approvedCount > 0) { + annotate({ + action: { name: "documents.inbound.extractRefusedApproved" }, + meta: { documentId: document.id, approvedCount }, + }); + return apiError( + "Some facts from this document are already confirmed. Finish reviewing or discard it before extracting again.", + 409, + { errorCode: "documents.inbound.alreadyPartlyConfirmed" }, + ); + } + const contentType = request.headers.get("content-type") ?? ""; if (contentType.includes("application/json")) { return handleTextExtract(request, user.id, document); diff --git a/src/app/api/documents/inbound/[id]/original/__tests__/route.test.ts b/src/app/api/documents/inbound/[id]/original/__tests__/route.test.ts index 153a3dc15..980fa8391 100644 --- a/src/app/api/documents/inbound/[id]/original/__tests__/route.test.ts +++ b/src/app/api/documents/inbound/[id]/original/__tests__/route.test.ts @@ -34,6 +34,13 @@ vi.mock("@/lib/modules/gate", async (importOriginal) => ({ resolveModuleMap: vi.fn().mockResolvedValue({}), })); +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi + .fn() + .mockResolvedValue({ allowed: true, remaining: 239, resetAt: Date.now() }), + rateLimitHeaders: () => ({}), +})); + vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn().mockResolvedValue(undefined), @@ -118,20 +125,38 @@ describe("GET /api/documents/inbound/[id]/original", () => { expect(where.deletedAt).toBeNull(); }); - it("serves a non-inline type (text) as an attachment", async () => { + it("serves inline with a generated fallback name when filename is null", async () => { vi.mocked(prisma.inboundDocument.findFirst).mockResolvedValue({ id: "doc-2", filename: null, - mimeType: "text/plain", - contentEncrypted: encryptDocumentToBytes(Buffer.from("plain text")), + mimeType: "image/png", + contentEncrypted: encryptDocumentToBytes(Buffer.from("png bytes")), } as never); const res = await callGet(makeReq("doc-2"), ctx("doc-2")); expect(res.status).toBe(200); const disposition = res.headers.get("Content-Disposition") ?? ""; - expect(disposition).toContain("attachment"); + // The stored set is inline-safe by construction → always inline. + expect(disposition).toContain("inline"); // No filename → generated fallback keyed by id + extension. - expect(disposition).toContain("document-doc-2.txt"); + expect(disposition).toContain("document-doc-2.png"); + }); + + it("RFC 5987-encodes a non-ASCII filename and keeps an ASCII fallback", async () => { + vi.mocked(prisma.inboundDocument.findFirst).mockResolvedValue({ + id: "doc-4", + filename: "Müller-Bericht.pdf", + mimeType: "application/pdf", + contentEncrypted: encryptDocumentToBytes(PDF_BYTES), + } as never); + + const res = await callGet(makeReq("doc-4"), ctx("doc-4")); + expect(res.status).toBe(200); + const disposition = res.headers.get("Content-Disposition") ?? ""; + // ASCII fallback: the non-ASCII byte is replaced so the bare `filename=` + // value is header-safe; the RFC 5987 form carries the percent-encoded UTF-8. + expect(disposition).toContain(`filename="M_ller-Bericht.pdf"`); + expect(disposition).toContain("filename*=UTF-8''M%C3%BCller-Bericht.pdf"); }); it("404s for another user's document (the userId narrows it out)", async () => { diff --git a/src/app/api/documents/inbound/[id]/original/route.ts b/src/app/api/documents/inbound/[id]/original/route.ts index c2c445e6b..97366a357 100644 --- a/src/app/api/documents/inbound/[id]/original/route.ts +++ b/src/app/api/documents/inbound/[id]/original/route.ts @@ -7,10 +7,10 @@ * extracted facts came from. Owner-scoped + module-gated; the bytes are PHI: * never logged, never served cross-user, fail-closed on a decrypt error. * - * The response carries the stored `mimeType`. A PDF / image is served inline - * (`Content-Disposition: inline`) so the browser can render it in a tab; any - * other type (e.g. the text-mode upload) is sent as an attachment. The - * filename is sanitised before it reaches the header. + * The response carries the stored `mimeType`. Uploads are constrained at the + * store path to the inline-safe set (JPEG / PNG / WebP / PDF, magic-byte + * sniffed), so the document is always served `Content-Disposition: inline` for + * in-tab rendering. The filename is sanitised before it reaches the header. */ import { NextResponse } from "next/server"; @@ -20,18 +20,19 @@ import { prisma } from "@/lib/db"; import { decryptDocumentFromBytes } from "@/lib/documents/store"; import { annotate } from "@/lib/logging/context"; import { requireModuleEnabled } from "@/lib/modules/gate"; +import { checkRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; type RouteParams = { params: Promise<{ id: string }> }; export const dynamic = "force-dynamic"; -/** MIME types we render inline; everything else downloads as an attachment. */ -const INLINE_MIME_TYPES = new Set([ - "application/pdf", - "image/jpeg", - "image/png", - "image/webp", -]); +/** + * Decrypting + serving an original is CPU work on PHI. A generous per-user + * ceiling is a backstop against self-DoS only (the route is owner-scoped, so + * there is no cross-user abuse vector). + */ +const ORIGINAL_READ_LIMIT_PER_HOUR = 240; +const ORIGINAL_READ_WINDOW_MS = 60 * 60 * 1000; /** Extension fallback when the stored filename is absent / unusable. */ const MIME_EXTENSION: Record = { @@ -39,11 +40,22 @@ const MIME_EXTENSION: Record = { "image/jpeg": "jpg", "image/png": "png", "image/webp": "webp", - "text/plain": "txt", }; /** - * Build a header-safe ASCII filename. Strips control characters (incl. CR/LF, + * Percent-encode a UTF-8 string for an RFC 5987 `filename*` parameter. Builds + * on `encodeURIComponent` and additionally escapes the characters it leaves but + * RFC 5987's `attr-char` set forbids (`' ( ) *`). + */ +function encodeRfc5987(value: string): string { + return encodeURIComponent(value).replace( + /['()*]/g, + (c) => `%${c.charCodeAt(0).toString(16).toUpperCase()}`, + ); +} + +/** + * Build a header-safe filename. Strips control characters (incl. CR/LF, * the header-injection vector), quotes, and path separators (path traversal), * caps the length, and falls back to a generated name keyed by the document * id + MIME type when nothing usable remains. @@ -71,6 +83,21 @@ export const GET = apiHandler( const gate = await requireModuleEnabled(user.id, "inboundDocuments"); if (!gate.enabled) return gate.response; + const rl = await checkRateLimit( + `documents-original:${user.id}`, + ORIGINAL_READ_LIMIT_PER_HOUR, + ORIGINAL_READ_WINDOW_MS, + ); + if (!rl.allowed) { + const response = apiError("Too many requests. Try again later.", 429, { + errorCode: "documents.inbound.rateLimited", + }); + for (const [k, v] of Object.entries(rateLimitHeaders(rl))) { + response.headers.set(k, v); + } + return response; + } + const { id } = await params; const document = await prisma.inboundDocument.findFirst({ // Owner-scoped: the userId narrows the row so a cuid guess cannot reach @@ -105,12 +132,16 @@ export const GET = apiHandler( }); } - const inline = INLINE_MIME_TYPES.has(document.mimeType); const downloadName = safeDownloadName( document.filename, document.id, document.mimeType, ); + // ASCII fallback for the bare `filename=` (replace any non-ASCII byte) plus + // an RFC 5987 `filename*` that carries the real UTF-8 name to compliant + // clients. The stored set is inline-safe by construction, so always inline. + const asciiName = downloadName.replace(/[^ -~]/g, "_"); + const disposition = `inline; filename="${asciiName}"; filename*=UTF-8''${encodeRfc5987(downloadName)}`; annotate({ action: { name: "documents.inbound.original.get" }, @@ -118,7 +149,7 @@ export const GET = apiHandler( documentId: document.id, mimeType: document.mimeType, bytes: bytes.byteLength, - disposition: inline ? "inline" : "attachment", + disposition: "inline", }, }); @@ -127,7 +158,7 @@ export const GET = apiHandler( headers: { "Content-Type": document.mimeType, "Content-Length": String(bytes.byteLength), - "Content-Disposition": `${inline ? "inline" : "attachment"}; filename="${downloadName}"`, + "Content-Disposition": disposition, // Private PHI — never cache in a shared / disk cache. "Cache-Control": "private, no-store", }, diff --git a/src/app/api/documents/inbound/[id]/route.ts b/src/app/api/documents/inbound/[id]/route.ts index b8223a1c9..3efdbd2dd 100644 --- a/src/app/api/documents/inbound/[id]/route.ts +++ b/src/app/api/documents/inbound/[id]/route.ts @@ -21,6 +21,7 @@ import { prisma } from "@/lib/db"; import { serialiseDocumentDetail } from "@/lib/documents/store"; import { annotate } from "@/lib/logging/context"; import { requireModuleEnabled } from "@/lib/modules/gate"; +import { checkRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; import { documentUpdateSchema } from "@/lib/validations/inbound-documents"; import type { Prisma } from "@/generated/prisma/client"; @@ -28,6 +29,30 @@ type RouteParams = { params: Promise<{ id: string }> }; export const dynamic = "force-dynamic"; +/** + * Edit / discard are owner-scoped, so a generous per-user ceiling is just a + * self-DoS backstop. PATCH and DELETE share one write bucket. + */ +const WRITE_LIMIT_PER_HOUR = 240; +const WRITE_WINDOW_MS = 60 * 60 * 1000; + +/** Shared 429 helper for the mutating routes. */ +async function enforceWriteRateLimit(userId: string): Promise { + const rl = await checkRateLimit( + `documents-write:${userId}`, + WRITE_LIMIT_PER_HOUR, + WRITE_WINDOW_MS, + ); + if (rl.allowed) return null; + const response = apiError("Too many requests. Try again later.", 429, { + errorCode: "documents.inbound.rateLimited", + }); + for (const [k, v] of Object.entries(rateLimitHeaders(rl))) { + response.headers.set(k, v); + } + return response; +} + export const GET = apiHandler( async (_request: NextRequest, { params }: RouteParams) => { const { user } = await requireAuth(); @@ -60,6 +85,9 @@ export const PATCH = apiHandler( const gate = await requireModuleEnabled(user.id, "inboundDocuments"); if (!gate.enabled) return gate.response; + const limited = await enforceWriteRateLimit(user.id); + if (limited) return limited; + const { id } = await params; const { data: body, error: jsonError } = await safeJson(request); @@ -127,6 +155,9 @@ export const DELETE = apiHandler( const gate = await requireModuleEnabled(user.id, "inboundDocuments"); if (!gate.enabled) return gate.response; + const limited = await enforceWriteRateLimit(user.id); + if (limited) return limited; + const { id } = await params; const document = await prisma.inboundDocument.findFirst({ where: { id, userId: user.id, deletedAt: null }, diff --git a/src/app/api/documents/inbound/__tests__/route.test.ts b/src/app/api/documents/inbound/__tests__/route.test.ts index f4d17baf0..d1be3394f 100644 --- a/src/app/api/documents/inbound/__tests__/route.test.ts +++ b/src/app/api/documents/inbound/__tests__/route.test.ts @@ -15,6 +15,9 @@ vi.mock("@/lib/db", () => ({ create: vi.fn(), findMany: vi.fn(), }, + extractedFact: { + groupBy: vi.fn(), + }, }, })); @@ -110,6 +113,7 @@ beforeEach(() => { vi.clearAllMocks(); vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(requireModuleEnabled).mockResolvedValue({ enabled: true } as never); + vi.mocked(prisma.extractedFact.groupBy).mockResolvedValue([] as never); }); describe("POST /api/documents/inbound (store-only)", () => { @@ -153,10 +157,17 @@ describe("POST /api/documents/inbound (store-only)", () => { }); describe("GET /api/documents/inbound (list)", () => { - it("applies q + kind filters and returns a keyset page", async () => { + it("applies q + kind filters and returns a keyset page with grouped counts", async () => { vi.mocked(prisma.inboundDocument.findMany).mockResolvedValue([ - { ...docRow({ id: "d1" }), _count: { facts: 0 }, facts: [] }, - { ...docRow({ id: "d2" }), _count: { facts: 2 }, facts: [{ id: "f1" }] }, + docRow({ id: "d1" }), + docRow({ id: "d2" }), + ] as never); + // factCount excludes REJECTED; pendingCount is the PENDING subset. The + // counts come from a single grouped query, not from materialised fact rows. + vi.mocked(prisma.extractedFact.groupBy).mockResolvedValue([ + { documentId: "d2", status: "PENDING", _count: { _all: 1 } }, + { documentId: "d2", status: "APPROVED", _count: { _all: 1 } }, + { documentId: "d2", status: "REJECTED", _count: { _all: 3 } }, ] as never); const res = await get( @@ -169,6 +180,14 @@ describe("GET /api/documents/inbound (list)", () => { expect(body.data.documents).toHaveLength(2); expect(body.data.nextCursor).toBeNull(); + // d1 has no facts; d2 has 1 pending + 1 approved + 3 rejected → factCount 2 + // (rejected excluded), pendingCount 1. + const [d1, d2] = body.data.documents; + expect(d1.factCount).toBe(0); + expect(d1.pendingCount).toBe(0); + expect(d2.factCount).toBe(2); + expect(d2.pendingCount).toBe(1); + const arg = vi.mocked(prisma.inboundDocument.findMany).mock.calls[0]![0]!; const where = arg.where!; expect(where.userId).toBe("user-1"); @@ -178,6 +197,8 @@ describe("GET /api/documents/inbound (list)", () => { { title: { contains: "panel", mode: "insensitive" } }, { filename: { contains: "panel", mode: "insensitive" } }, ]); + // No fact rows are materialised in the page query — counts come from groupBy. + expect(arg.include).toBeUndefined(); // limit+1 fetched for the has-more probe. expect(arg.take).toBe(11); }); diff --git a/src/app/api/documents/inbound/route.ts b/src/app/api/documents/inbound/route.ts index 5b270cc5a..eda814789 100644 --- a/src/app/api/documents/inbound/route.ts +++ b/src/app/api/documents/inbound/route.ts @@ -136,10 +136,24 @@ export const POST = apiHandler(async (request: Request) => { // No mass assignment — every column is set field-by-field; `userId` comes // from the session, never the body. `status` is STORED (the library default); // no provider / consent / budget call runs here. + // + // `documentDate` is the effective filing date the library sorts, groups, and + // range-filters on. When the user does not supply one at upload, default it to + // the upload day (now) — the same effective date the UI shows — so the column + // is never NULL on a stored row. This keeps display == sort == filter: an + // undated upload no longer sinks below older dated rows under `nulls: "last"`, + // and a "this month" range filter sees it. (At store time `reportDate` is + // still null; it only exists after an extraction, so the default resolves to + // the upload day.) The date stays user-editable via PATCH, which still allows + // clearing it back to NULL. const document = await prisma.inboundDocument.create({ data: { userId: user.id, kind: parsed.data.kind ?? "OTHER", + // Stored plaintext on purpose (mirrors `filename`) so the list can + // ILIKE-search + ORDER BY it. It MAY hold PHI the user types; that is the + // accepted tradeoff for server-side search/sort — the document body stays + // encrypted. title: parsed.data.title ?? null, filename: typeof file.name === "string" ? file.name.slice(0, 255) : null, mimeType: mime, @@ -148,7 +162,7 @@ export const POST = apiHandler(async (request: Request) => { status: "STORED", documentDate: parsed.data.documentDate ? isoDateToUtc(parsed.data.documentDate) - : null, + : new Date(), }, }); @@ -223,10 +237,6 @@ export const GET = apiHandler(async (request: Request) => { const rows = await prisma.inboundDocument.findMany({ where, orderBy, - include: { - _count: { select: { facts: true } }, - facts: { where: { status: "PENDING" }, select: { id: true } }, - }, take: limit + 1, ...(cursor ? { cursor: { id: cursor }, skip: 1 } : {}), }); @@ -235,6 +245,32 @@ export const GET = apiHandler(async (request: Request) => { const page = hasMore ? rows.slice(0, limit) : rows; const nextCursor = hasMore ? (page[page.length - 1]?.id ?? null) : null; + // Fact tallies for the whole page in ONE grouped count — no per-document + // fan-out and no materialising fact-id rows. `factCount` counts every + // non-REJECTED fact (a rejected fact is discarded, not part of the document's + // tally), `pendingCount` the PENDING subset still awaiting review. + const factCounts = new Map< + string, + { factCount: number; pendingCount: number } + >(); + if (page.length > 0) { + const groups = await prisma.extractedFact.groupBy({ + by: ["documentId", "status"], + where: { userId: user.id, documentId: { in: page.map((d) => d.id) } }, + _count: { _all: true }, + }); + for (const g of groups) { + const entry = factCounts.get(g.documentId) ?? { + factCount: 0, + pendingCount: 0, + }; + const n = g._count._all; + if (g.status !== "REJECTED") entry.factCount += n; + if (g.status === "PENDING") entry.pendingCount += n; + factCounts.set(g.documentId, entry); + } + } + annotate({ action: { name: "documents.inbound.list" }, meta: { count: page.length, sort, order, filtered: Boolean(q || kind) }, @@ -242,10 +278,10 @@ export const GET = apiHandler(async (request: Request) => { return apiSuccess({ documents: page.map((doc) => - serialiseDocument(doc, { - factCount: doc._count.facts, - pendingCount: doc.facts.length, - }), + serialiseDocument( + doc, + factCounts.get(doc.id) ?? { factCount: 0, pendingCount: 0 }, + ), ), nextCursor, }); diff --git a/src/lib/documents/store.ts b/src/lib/documents/store.ts index 77997b77f..8b9f90254 100644 --- a/src/lib/documents/store.ts +++ b/src/lib/documents/store.ts @@ -129,8 +129,11 @@ export function serialiseDocumentDetail( facts: ExtractedFact[], ): InboundDocumentDetailDto { const pendingCount = facts.filter((f) => f.status === "PENDING").length; + // `factCount` excludes REJECTED facts (a rejected fact is discarded, not part + // of the document's tally) so the badge matches the list query. + const factCount = facts.filter((f) => f.status !== "REJECTED").length; return { - ...serialiseDocument(doc, { factCount: facts.length, pendingCount }), + ...serialiseDocument(doc, { factCount, pendingCount }), facts: facts.map(serialiseFact), }; } diff --git a/src/lib/openapi/routes/documents.ts b/src/lib/openapi/routes/documents.ts index 8401545d3..b60e0c48e 100644 --- a/src/lib/openapi/routes/documents.ts +++ b/src/lib/openapi/routes/documents.ts @@ -25,7 +25,7 @@ import { INBOUND_DOCUMENT_STATUSES, } from "@/lib/validations/inbound-documents"; -import { dataEnvelope, stdResponses } from "./shared"; +import { dataEnvelope, errorEnvelope, stdResponses } from "./shared"; const kindEnum = z.enum(INBOUND_DOCUMENT_KINDS); const statusEnum = z.enum(INBOUND_DOCUMENT_STATUSES); @@ -238,7 +238,8 @@ export const inboundDocumentPaths: NonNullable = { .optional() .meta({ description: "Optional category." }), documentDate: z.string().optional().meta({ - description: "Optional user filing date (YYYY-MM-DD).", + description: + "Optional user filing date (YYYY-MM-DD). Defaults to the upload day when omitted so the library sort/filter/display stay aligned.", }), }), }, @@ -352,7 +353,7 @@ export const inboundDocumentPaths: NonNullable = { tags: ["Documents"], summary: "View / download the original document", description: - "Decrypts and serves the raw uploaded document (the bytes stored encrypted at rest under `InboundDocument.contentEncrypted`). Owner-scoped + gated on the opt-in `inboundDocuments` module. The response carries the stored `mimeType`: a PDF / image is served inline (`Content-Disposition: inline`) so the browser can render it; any other type downloads as an attachment. Fails closed (500) on a decrypt error — it never returns the ciphertext.", + "Decrypts and serves the raw uploaded document (the bytes stored encrypted at rest under `InboundDocument.contentEncrypted`). Owner-scoped + gated on the opt-in `inboundDocuments` module. Uploads are constrained at the store path to the inline-safe set (JPEG / PNG / WebP / PDF), so the document is always served `Content-Disposition: inline` for in-tab rendering; a non-ASCII filename is carried via RFC 5987 `filename*`. Fails closed (500) on a decrypt error — it never returns the ciphertext.", parameters: [ { name: "id", @@ -372,9 +373,6 @@ export const inboundDocumentPaths: NonNullable = { "image/*": { schema: { type: "string", format: "binary" }, }, - "text/plain": { - schema: { type: "string" }, - }, }, }, ...stdResponses, @@ -421,6 +419,11 @@ export const inboundDocumentPaths: NonNullable = { }, }, }, + "409": { + description: + "Re-extraction refused: at least one fact on this document is already APPROVED. `meta.errorCode` = `documents.inbound.alreadyPartlyConfirmed`. Re-extracting would sever committed-record provenance and duplicate committed records, so the user must finish reviewing or discard the document first.", + content: { "application/json": { schema: errorEnvelope } }, + }, ...stdResponses, }, }, diff --git a/src/lib/validations/inbound-documents.ts b/src/lib/validations/inbound-documents.ts index fb54dd70b..b2153902d 100644 --- a/src/lib/validations/inbound-documents.ts +++ b/src/lib/validations/inbound-documents.ts @@ -212,7 +212,9 @@ export interface InboundDocumentDto { /** User-set filing date (YYYY-MM-DD), or null. */ documentDate: string | null; errorReason: string | null; + /** Count of the document's non-REJECTED staged facts. */ factCount: number; + /** Count of staged facts still PENDING review. */ pendingCount: number; createdAt: string; updatedAt: string;