fix: full-app audit round 9 — cost guardrails, data loss, resilience - #25
Merged
Merged
Conversation
Addresses the four highest-severity findings of the full-app audit. SEC-1 — user-initiated AI had no request ceiling. The monthly token cap was unlimited by default and is only checked against already-recorded usage, so a loop of generation calls ran unbounded against the operator's shared provider key. Adds assertAiAllowed (30 requests / 5 min per user) across the AI actions, a per-user burst limit on /api/ai/chat, and a RATE_LIMITED error code. The default monthly budget is now 5M tokens per user; 0 still means unlimited for operators who want it. The coverage-generation loop and quiz grading on submit keep calling assertWithinLimit directly: both are already bounded, and throttling them would discard work the user has done. BE-1 — background summarization ignored the cap entirely. It is the most expensive background pass (a model call per section plus reduce rounds), and it also overwrote the "limit reached" status with "ready", hiding the skip. Gates on isOverLimit, leaves the status intact, and only enqueues the summary job when processMaterial actually produced usable text. FE-2 — an expired session silently destroyed queued offline work. The redirect to /login rejects the action promise with a NEXT_REDIRECT digest, which is not a network error, so every queued review was dropped as a permanent failure. Redirects now count as retryable, and flush reports discarded entries so the UI can say so instead of failing quietly. QA-3 — crypto.ts had no tests despite protecting every secret at rest, and a truncated payload crashed with an opaque TypeError from node:crypto. Adds a guard and a test suite (round-trip, unicode, IV uniqueness, tamper detection, version and truncation errors). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round 2 of the audit fixes — the findings that affect self-hosters. OPS-1 — the documented backup was a file copy of the running PostgreSQL data directory, which yields an inconsistent snapshot that can refuse to start on restore. Documents pg_dump/pg_restore instead, keeps the file copy only for the stopped-stack case, and separates uploads from the DB. OPS-2 — every container migrated on boot and drizzle-kit takes no lock, so an app plus a worker (or scaled replicas) starting together raced over the same DDL. Migrations now run through scripts/migrate.mjs under a Postgres advisory lock, and a container started with a custom command (the worker) no longer migrates at all. OPS-3 — POSTGRES_PASSWORD silently defaulted to "study" and was missing from .env.example entirely, so the documented quick start left the database on a known password. It is now required, documented, and the init-only semantics plus a rotation recipe are written down. OPS-4 — dev fallbacks keyed off "not production", so a worker started without NODE_ENV (systemd, cron, npm run worker) silently used the local dev database and the publicly known dev encryption key. Fallbacks now require an explicit development/test, and the .env.example placeholder is rejected outright for both secrets. Short keys only warn: ENCRYPTION_KEY cannot be rotated without losing every stored secret, so refusing to boot would strand a running instance rather than protect it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round 3 of the audit fixes. BE-3 — finalizeUpload swallowed every error, which made pg-boss's retryLimit dead code: a transient DB failure lost the upload silently and left the blob orphaned in storage. Transient failures now propagate (the staging file survives for the retry), and a quota failure is recorded as a failed material so the user sees why the upload vanished. BE-4 — reminders claimed the dedup row before sending, so a throwing send burned the claim: that reminder was gone for good, and the error escaped the loop, silencing every remaining reminder in the run. Delivery is now isolated per reminder and releases its claim on failure, turning both into a retry on the next tick. BE-5 — one failing free-text grading call rejected the whole Promise.all and discarded the entire quiz attempt. Grading failures now fall back to the existing "not graded" path, so the attempt is still recorded. BE-6, DB-5 — deck/quiz creation wrote two tables without a transaction and could leave an empty AI-generated deck behind. Module reordering fired up to 10.000 concurrent updates against a 10-connection pool, with no rollback if it failed part-way; it is now one statement per semester inside a transaction. DB-1..4, DB-8, DB-10, DB-12 — index pass (migration 0043): - material_chunk.parent_chunk_id: the self-referencing ON DELETE SET NULL ran a full-table scan per deleted row, quadratic on every material delete and re-chunk - answer_log.question_id, plan_task.goal_id, event.module_id, assignment.goal_id, generation_coverage.topic_id: unindexed cascade targets - assignment: partial index for the 5-minute reminder cron - ai_usage_log: composite (user_id, created_at) — the cap check aggregated a user's entire history on every AI call - user_ai_key: unique (user_id, provider_id), with existing duplicates cleaned up first, so a rotated key can no longer lose to a stale row - dropped material_text_trgm_idx, dead since 0042 but still maintained on every write over a column holding up to 200k characters Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round 4 of the audit fixes. FE-1 — the app had no error.tsx, not-found.tsx or global-error.tsx at all, while every route is force-dynamic and database-backed. Any DB blip dropped the user on the unstyled framework error page. Adds a localized error boundary for the app segment (using Next 16.2's unstable_retry, which re-fetches rather than just re-rendering), a localized 404, and a global-error that brings its own html/body. The strings live under a new "errorPages" namespace — "errors" is reserved for the ActionErrorCode union and a test asserts it holds nothing else. FE-3 — the outbox had a "toggle-session" handler wired up that nothing ever enqueued, so ticking off a plan session was the one study action that failed offline while card reviews worked. Both toggle call sites now queue like reviews do. FE-4 — the drag-and-drop boards registered only a PointerSensor, and moving a module to another semester exists only as a drag: unreachable without a mouse. The plan board even rendered a focusable handle announcing itself as "reorder" that did nothing. Adds KeyboardSensor. FE-5, FE-6 — the dashboard awaited five independent queries in sequence on the page that re-renders after every toggle; they now run together. The calendar loaded every event the user had ever created, with the full module relation, into the client payload — now bounded to the same window as the plan sessions, keeping live recurring series regardless of start. FE-7 — no page set a title, so the prepared "%s · appName" template was never used and every tab read "StudyHelper". Top-level pages and the module workspace now name themselves. FE-13 — requireSession redirected via next/navigation, dropping the locale prefix, so an expired session sent English users to the German login page (which has no language switcher). Now builds the path through the routing config. FE-8..FE-12, FE-14, FE-15 — accessibility and localization sweep: translated aria-labels in the mini calendar and module dialog, an accessible name for the chat send button and the session checkboxes, aria-current on every active nav item, ICU plurals for the offline counts, and three call sites moved off raw server messages onto the localized error-code path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round 5 of the audit fixes. QA-1 — the .db.test.ts suites are gated on RUN_DB_TESTS and CI never set it, so the folder and zip-unpack integration tests had never run anywhere. CI now starts a pgvector service, applies the migrations (which also proves every migration still applies to an empty database) and runs them. QA-4, QA-5 — tests for the two most consequential untested units: the generation deduper (key normalization, cosine threshold, and specifically that item/vector indices stay aligned after a rejection) and registerUploadedFile, which both upload routes share — quota rejection, zip hand-off, hash dedup, and the enqueue-failure path that has to mark the material failed without throwing. 34 new tests; no bugs found. QA-7 — coverage was not measurable at all: no provider installed, no configuration. Adds @vitest/coverage-v8 with thresholds calibrated just under the current ~34%, run in CI so coverage can no longer slide unnoticed — which is how 21 server-action files ended up untested. QA-8 — enables noImplicitOverride. noUncheckedIndexedAccess, which would have caught the crypto.ts crash at compile time, reports ~255 errors and needs a pass of its own; left off deliberately rather than silenced. OPS-5, OPS-6 — supply chain: every action is pinned to a commit SHA (a mutable tag can be repointed at attacker-controlled code), CI and the release-candidate workflow declare least-privilege permissions, dependabot keeps the pins and npm/docker dependencies current, dependency-review blocks new high-severity dependencies on PRs, and the published multi-arch image now carries an SBOM plus max provenance and is scanned with Trivy, with results going to code scanning. Base images are still on tags rather than digests — the registry was not reachable from the environment this was prepared in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Final round: the medium and low findings not covered by rounds 1-5. Backend / AI - BE-2: after an embedding-model switch, the re-embed DELETE removed the level-1 summary chunks while the follow-up summarize job short-circuited on the still-present material.summary, so those retrieval nodes were gone for good. summarizeMaterial now also runs when the summary exists but its chunks don't, rebuilding them without regenerating the doc text. - BE-7: a crash between submitBatch and persisting batchRef made the retry submit a second paid batch. A marker is now written before the submit and recognised on retry. - BE-8: applyBatchResults never touched updatedAt, so a slow apply tripped the 15-minute stale-claim reclaim and got ingested twice. Adds a heartbeat. - BE-9: hitting the token cap mid-generation still reported "completed" with no note, so a truncated deck looked finished. Records the reason. - BE-10: media files were read into memory whole (up to 200 MB) for STT endpoints that accept ~25 MB, and the provider rejection surfaced as the misleading "Transcription returned no text". Size is checked up front. - BE-13: the chat route returned raw error text — a rotated ENCRYPTION_KEY leaked as Node's "Unsupported state or unable to authenticate data". Now mapped onto the existing AI_ERROR codes, with its own case for an undecryptable BYOK key, since only re-entering it helps. Database - DB-7, DB-11, DB-13: conversation history, due-card counts and the outline material load all pulled full result sets into memory to then slice, count or filter in JS — including a column holding up to 200k characters per row. All three now bound the work in SQL. - DB-9: the better-auth tables were the last ones on timestamp without time zone, so moving a deployment between timezones would shift session expiry, ban windows and 2FA lockouts. Migration 0044 converts them, with an explicit UTC source zone rather than relying on the migrating session. Security - SEC-2: updateThesis spread a partial schema that still carried programId without an ownership check. Removed from the update schema — rehoming a thesis is not a supported operation and would undercut the one-active-thesis-per-program invariant. - SEC-3: the health token was compared with ===; now timingSafeEqual. Operations - OPS-7..OPS-12: log rotation limits, documented memory-limit guidance, a major-version image default instead of :latest (a pull could otherwise apply irreversible migrations unintentionally), a healthcheck for the app container, graceful pg-boss shutdown for the in-process worker, and loopback-only port publishing since Docker bypasses UFW. Tests - QA-6, QA-10, QA-11: cover the auth configuration (including the invite-only signup gate that a bypass fix depends on), the job wiring (queue-to-handler and cron expressions, derived from the exported constants so a rename fails the test rather than production), and the settings TTL cache that admin changes propagate through. - QA-12: removed a dead import that was hidden from lint with `void`. - worker.ts: shutdown gained a timeout fallback and exits non-zero on error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Index accesses (arr[i], obj[key], destructuring from .split()) were typed as always-defined. That is the class of defect behind the crypto.ts crash the audit found, where a truncated payload reached Buffer.from(undefined) and threw an opaque TypeError from node:crypto. Enabling the flag surfaced 275 errors across 49 files. Roughly three quarters are the Drizzle `const [row] = await db.insert(...).returning()` pattern, where exactly one row is guaranteed unless the insert throws — those are non-null assertions with the invariant written down once per file (once per section in seed.ts, which is 32 of them and only ever runs behind SEED_TEST_DATA). The rest became real guards. Behaviour is unchanged except where a guard closes an actual gap: - materials-browser: shift-click range selection indexed currentFiles with a remembered index that is never reset when the list shrinks (folder change, search filter, deletion). A shift-click after that read undefined.id and took down the file list. Now skips missing entries. - rate-limit clientIp: an "x-forwarded-for" of "," or " " keyed every such request under the empty string, bucketing unrelated clients together. Falls through to x-real-ip instead. - semester-modules-board, study-session, ai-settings-form: drag targets, touch events and provider lookups that could legitimately be absent now return early instead of dereferencing. Date and time parsing kept its previous NaN result for malformed input rather than defaulting to some epoch value that would render as "overdue" or silently reinterpret "10" as 10:00. exactOptionalPropertyTypes was evaluated and deliberately not enabled: it reports 56 errors, nearly all from Zod-inferred types (`prop?: T | undefined`) meeting hand-written `prop?: T` interfaces. Satisfying it means annotating optional props across the app for no defect-catching value. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
veniplex
added
the
release-candidate
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
1 participant
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.
Implements every finding from a full-app audit of v1.2.1 (69 findings: 9 high, 26 medium, 34 low), plus the deferred TypeScript strictness pass. Each of the 9 high findings was independently verified against the code before being fixed.
Seven commits, one per round — reviewable in order.
What this fixes
AI spending had no ceiling. The monthly token cap shipped unlimited and is only checked against already-recorded usage, so any signed-up user (registration defaults to open) could loop generation calls against the operator's shared provider key. Adds a per-user request rate limit across the AI actions and the chat route, and a 5M-token monthly default. Background summarization ignored the cap entirely — the most expensive automatic pass — and even overwrote the "limit reached" status with "ready", hiding the skip.
Offline work was silently destroyed. When a session expired, the redirect to
/loginrejects the action promise with aNEXT_REDIRECTdigest, which is not a network error — so every queued flashcard review was dropped as a permanent failure, with no message. Redirects now count as retryable and discarded entries are reported.No error boundaries existed. Not one
error.tsx,not-found.tsxorglobal-error.tsxin the entire app, while every route isforce-dynamicand database-backed. Any DB blip dropped the user on the unstyled framework error page.Self-hosting had three sharp edges. The documented backup was a file copy of the running PostgreSQL data directory (inconsistent, may not restore);
POSTGRES_PASSWORDsilently defaulted tostudyand was missing from.env.exampleentirely; and every container migrated on boot with no lock, so an app plus a worker starting together raced over the same DDL.Also in here
ON DELETE SET NULLran a full-table scan per deleted row (quadratic on every material delete), and a dead trigram index maintained on every write over a 200k-character column. Auth tables moved off timezone-naive timestamps.aria-currenton navigation, accessible names for icon buttons and checkboxes, page titles, a dashboard waterfall and an unbounded calendar query.noUncheckedIndexedAccessenabled — 275 errors resolved, which turned up a real crash in shift-click range selection.Deliberate decisions worth reviewing
0is left alone. This was a conscious choice to close the hole rather than grandfather it.ENCRYPTION_KEYshorter than 32 chars only warns, it does not refuse to boot — the key cannot be rotated without losing every stored secret, so a hard failure would strand a running instance. The publishedchange-meplaceholder is rejected outright.exactOptionalPropertyTypeswas evaluated and not enabled: 56 errors, nearly all Zod-inferredprop?: T | undefinedmeeting hand-writtenprop?: T. No defect-catching value for the churn.material_chunk.parent_chunk_idkept its column, against the audit's suggestion to drop it — level-1 chunks are genuinely written, so the tree is half-implemented rather than dead.Verification
npm run lint,npm run typecheck,npm test(297 passing, up from 221) andnpm run buildall pass locally.Two things could not be verified here and should be watched on the first CI run: migrations 0043 and 0044 never ran against a live database (no Docker daemon available), though CI now applies them; and base images are still on tags rather than digests because the container registry was unreachable from this environment — Dependabot is configured for the Docker ecosystem.
🤖 Generated with Claude Code