fix: work through the security and correctness audit queue - #108
Merged
Conversation
The AI gateway is now the only food-photo provider; the free-model allowlist, deprecated-model handling, OpenRouter request shape and headers, and the OPENROUTER_* configuration are gone. The per-attempt timeout override moves to AI_GATEWAY_MODEL_TIMEOUT_MS (default 20s, clamped 3-30s). Without AI_GATEWAY_URL the feature reports itself unavailable instead of falling back. OpenRouter still works as a stand-in gateway by pointing AI_GATEWAY_URL at its chat-completions endpoint, which is why the error reader keeps parsing openrouter_metadata details.
…od-photo refactor(ai): drop the free-OpenRouter food-photo path
Resolves the 22 next advisories reported by pnpm audit, including the beforeInteractive script XSS that affects the mt_tz bootstrap in app/layout.tsx. Patch-level bump within 16.x, no migration required.
OPS-02: audit:code now runs as a CI job inside the checks gate; nothing invoked the audit scripts before, which is why the broken knip config went unnoticed. OPS-03: SHA-pin every action and pin the Rust toolchain to 1.89 to match railpack.json. dtolnay/rust-toolchain@stable was a moving branch, so CI's toolchain could change between runs. Adds Dependabot to keep the pins fresh. OPS-04: add Swatinem/rust-cache to the five Rust jobs. OPS-05: drop the duplicate apps/web/railway.toml in favour of the root one. DOC-01: fix the two broken lnieuwenhuis/marco-tracker URLs, stop hardcoding the app version, and drop the db:generate reference. SEC-08: stop handing operators a publicly known SESSION_SECRET placeholder. CLEAN-04: gitignore .codex/ and the remediation queue; delete audit-report.html.
…skipped tests visible SEC-03: match logins on shoo_pairwise_sub only. The previous query also matched by email and then unconditionally rebound that row's subject, so an ID token carrying a victim's email under a different subject took over the account. An email already held under another subject is now a conflict. DATA-01/DATA-03: enforce MAX_QUANTITY and MAX_MACRO_GRAMS in validate_meal_components so the product-linked path cannot bypass them, bound calories, and aggregate as bigint so existing rows cannot overflow the ::int cast and 500 the summary, dashboard, stats and leaderboard queries. BUG-01: stats_page_data_json hardcoded a zero streak, so every user's Summary page read 0. It now shares the dated_islands/streaks CTE with leaderboard_json. DATA-05..09, PERF-01..04, LOW-A3, LOW-B1, API-07, API-10, CLEAN-03, CLEAN-11. TEST-01: 21 integration tests returned early when no database was configured, reporting as passed with zero assertions. A build script now sets a cfg so they report as ignored instead, while still running in CI.
…est guard SEC-04: sslmode=require yielded rejectUnauthorized:false, the same TLS posture no-verify is rejected for a few lines earlier, and it is the value Railway, Neon and Supabase hand out. It now verifies the chain; ALLOW_UNVERIFIED_DB_TLS=true is the only way back, and it warns loudly. DB-01: drizzle-kit diffs against meta/0004, the last snapshot present, while the journal has 15 entries, so the next db:generate would have emitted a destructive migration. Migrations are now declared hand-authored: db:generate is gone, MIGRATIONS.md documents the procedure, and a test locks the invariant in. DB-02/DB-07: set lock_timeout and statement_timeout on the migration connection and bound the advisory lock, so a blocked ALTER during preDeployCommand aborts instead of stalling every query queued behind it. DB-03..DB-06, DB-08..DB-10, LOW-F1, LOW-F2. OPS-01: knip rejected a '//' comment key, so audit:unused had been exiting 2 before analysing anything. CLEAN-01/CLEAN-02/CLEAN-09: drop the dead wrappers, types, and the unreachable date helpers whose logic now lives in SQL, along with their tests.
…en headers SEC-01: /api/auth/shoo/verify was unauthenticated, parsed with request.json() (which ignores Content-Type) and set the session cookie, with no origin check. A cross-site text/plain form post needed no preflight and no JS, and SameSite=Lax governs sending a cookie, not setting one, so a victim's browser silently stored the attacker's session. It now requires same-origin and fails closed when neither Sec-Fetch-Site nor Origin is present. SEC-07: sessions were re-minted for 7 days from the old token's claims on every request, so a captured token never expired. Renewal is now capped against the original iat. SEC-08: mirror the backend's SESSION_SECRET validation, measured on the trimmed value, and reject the known placeholder and dev literals. SEC-12..SEC-16, SEC-21, SEC-22, API-16, LOW-D1, LOW-D2. DATA-04: thread a clientMutationId through recipe portion logging; it was the one create path without one, so a double-tap wrote two entries. SEC-10 (CSP nonce) and DATA-10 (parseDecimalInput) are NOT in this commit and land later in the branch.
OPS-02 added audit:code to the CI gate, but knip still exited 1: types that are exported and used only within their own file were reported as unused, and postgres-config.d.ts's exports are a known false positive (they are consumed by client.ts, start-with-migrations.mjs and the tests across a boundary knip's resolution misses). ignoreExportsUsedInFile plus an ignore entry settles both. CLEAN-06: drop the unused @testing-library/user-event devDependency. Removes resolveBrowserTimeZone, which had no callers as far back as the audited commit.
SEC-02: Validation::new seeds required_spec_claims with only "exp", and
neither set_issuer nor set_audience adds to it. In validate(), the iss and aud
match arms both end in `_ => {}`, so an absent claim fell through and passed -
issuer and audience were enforced only for tokens that happened to carry them.
A token signed by any key in Shoo's JWKS but omitting aud was accepted as a
login. Requires both explicitly now.
Also stops deriving the algorithm from the token's own header, which let the
caller choose it. Shoo's live JWKS serves a single ES256 key over P-256, so the
algorithm is pinned to that.
SEC-19: DecodingKey::from_jwk builds an HMAC key from a symmetric oct JWK, so a
symmetric key in the JWKS would have let anyone who can read the public JWKS
mint logins for any email. The key type is now pinned to EC P-256. The check is
on kty/crv rather than the optional alg member, so a JWKS that omits alg keeps
working instead of taking every login down.
Tests were confirmed to fail against the unfixed code: dropping the required
claims fails the two absent-claim tests, and dropping the key-type guard lets a
symmetric key verify a login. The test fixtures move from HS256 to a throwaway
P-256 keypair, since a symmetric key is no longer accepted.
The data-layer changes in c3adfde7 were not rustfmt-clean, and CI runs cargo fmt --all --check as a hard gate.
…CHEMA_SQL to the migrations API-07: completeOnboardingSetup was the third write path into meal_templates.type and the only one still calling required_string instead of normalize_template_type. Migration 0016 adds a CHECK on that column, so an out-of-union value would have surfaced as a raw 23514 -> 500 rather than a 400. CLEAN-03: SCHEMA_SQL carried #[allow(dead_code)] and a comment calling it a temporary parity reference, but test_db() runs it to build the schema for every backend integration test - so drift from packages/db/drizzle silently stops the suite testing the schema production runs. The comment is corrected and a new test applies the migrations and SCHEMA_SQL into two scratch schemas and diffs the resulting catalogs. That test immediately caught real drift: migration 0015 made admin_audit_events.actor_user_id nullable with ON DELETE SET NULL, and SCHEMA_SQL still declared it NOT NULL. Aligned.
SEC-10: script-src carried 'unsafe-inline', so any injected script would have run, with img-src https: as an open exfiltration channel. next.config.ts headers() is baked into the routes manifest at build time and cannot carry a per-request value, so the policy moves to proxy.ts, which is also where Next reads the nonce back out. 'unsafe-inline' is gone from script-src in both dev and production; style-src deliberately keeps it and gets no nonce, since a style nonce makes browsers ignore 'unsafe-inline' and would break every inline style Next and next/font emit. Verified in a real browser: zero CSP violations across sign-in, sign-out and soft navigation, both bootstraps still run, mt_tz still tracks the browser zone, nonce differs per request, 23/23 e2e green. UI-02: todayStr was computed with getLocalDateString() during render in app-shell and dashboard-shell, resolving to the Node process zone on the server and the browser zone on hydration. loadOnboardedDateParam now also returns getRequestToday(), and all eight pages pass it down like selectedDate. recipe-card.tsx and onboarding-shell.tsx call the same helper inside click handlers, which is correct, and were left alone. UI-01: revert the optimistic meal-group change when the save fails. UI-03: dismiss the MealCard overflow menu on Escape and outside pointerdown. UI-04: clear the pending 'Copied' timers on unmount. UI-05: stop BarcodeResult's manual-entry form being dismissable mid-save. DATA-10: parseDecimalInput turned "1,234" into 1.234 silently. A comma is now only a decimal separator when unambiguous; grouped forms are rejected rather than mangled. CLEAN-05: drop the dead slideIn/fadeIn keyframes. apps/web/AGENTS.md is regenerated by next dev; committed so the tree stays clean.
…cated, rate limit SEC-05: BACKEND_ENABLE_TEST_ROUTES was accepted for any APP_URL, unlike its sibling BACKEND_ALLOW_INSECURE_LOCAL, while playwright.config.ts ships it in a copy-pasteable env block. Copied into a staging service, anyone reaching /internal/rpc with the shared secret could call ensureUserRoleForTesting and become owner, bypassing the actor check, the FOR UPDATE, the last-owner refusal and the audit event. Now refused unless APP_URL is loopback. SEC-06: insecure-local mode drops internal auth entirely while the listener bound 0.0.0.0, exposing the whole internal RPC surface to the local network. The bind address is now a pure, tested function that returns 127.0.0.1 in that mode. SEC-08: validate_secret measured the untrimmed value, so 32 spaces passed. It now measures the trimmed one, and the committed development secrets are blocklisted. SEC-09: no rate limiting existed and the pool defaulted to 3, while both /health and /api/v1 hit the database before any credential check - a few hundred garbage-bearer requests could starve the pool and 503 /health, which trips Railway's restart policy. Adds a per-IP limiter on /api/v1, raises the pool to 10, and caches the /health probe for a second. SEC-11, SEC-17 (subtle::ConstantTimeEq), SEC-18 (warned variant - see below), SEC-20, CLEAN-A1, CLEAN-A2 (JWKS single-flight + negative caching). SEC-18 is deliberately partial: AppUser has no column recording whether ownership came from ADMIN_OWNER_EMAILS or the admin UI, so demoting every owner absent from the list would strip admin-granted owners and could leave zero owners. Ships as a tested policy function that warns once per account instead. Scopes SCHEMA_SQL to cfg(test), which is what it is - removing its #[allow(dead_code)] in 4152b6ef left a clippy warning in non-test builds.
…ill the API spec API-01: a method listed in Endpoint::methods but absent from Endpoint::scopes silently required no scopes. The 130-line match is now an ordered table keyed on the OpenAPI path template, required_scopes returns Option, and a missing tuple is a logged 500 rather than an empty set. The hand-kept test path list that omitted four endpoint shapes is now derived from the table and cross-checked against the published contract in both directions. DATA-02: the patch-merge loop copied every client key, so a caller could set __recalculateProductMacros and have its own macro numbers stored verbatim against a product snapshot. Reserved keys are stripped and the flag is re-derived from the stored row. API-02/API-12: the two aiResponse branches and the reqwest transport error went straight to the caller, contradicting the invariant the same module documents. Both now go through upstream_photo_failure. API-03, API-04, API-05, API-06, API-08, API-09, API-11, API-13, API-14, API-15, CLEAN-C1, CLEAN-C2. CONCERN-C3: every benchmark fixture pointed at a Commons article URL serving text/html, so the benchmark scored models against a web page. Swapped for direct file URLs, with image_url split from image_source_url so the licence link survives. Also fixes two problems the rate limiter shipped with: - the 429 carried no CORS headers, so a browser client saw a CORS failure instead of the throttle - the same defect class API-06 just fixed. Verified on the wire. - the limiter throttled the request-level web suite, which drives hundreds of sequential calls from one address. The burst widens behind enable_test_routes, which SEC-05 restricts to a loopback APP_URL.
…ailpack split
CLEAN-07: round1 was defined identically in api.rs, legacy_api.rs and db.rs, and
round2 in two of them. Both move to a shared module.
api.rs's require_object/require_string_field/require_date are deliberately NOT
merged with db.rs's object_arg/required_string/date_arg. They return different
error types, take different shapes (owned body vs nested arg), and db.rs's
required_string trims and enforces MAX_TEXT_FIELD_LENGTH while api.rs's does
not - merging would silently move validation across a trust boundary. This
repo has already been bitten twice by drifted 'duplicates' (DATA-05, DATA-08).
CLEAN-10: 18 clones -> 14, 2.17% -> 1.62%, all in test scaffolding. The
remaining clones are jsonb_build_object projections and positional bind chains,
left as plain readable SQL on purpose - hiding a column list behind a macro is
how DATA-08 happened.
API-15 follow-up: PATCH /meal-entries/{id} and POST /templates/{id}/apply can
both return 409 now that unique violations map to Conflict, so the published
spec and the web contract say so. openapi-parity passes both directions again.
CLEAN-08: drop the parameters resolvePresetModalActiveKind never read, keeping
the behaviour its test pins, with a note on why the counts are ignored.
OPS-06: document why railpack.json carries a Rust toolchain for both services
and what splitting it would actually require. Not expressed as a comment key in
the JSON itself - the schema sets additionalProperties:false, which is exactly
how the knip config in OPS-01 came to be broken.
TEST-02: /settings/api had no coverage at any level - api-token-actions.test.ts mocked ApiSettingsClient out entirely. The surface where a raw secret is shown exactly once is now rendered for real: create -> reveal -> next action clears it, and create -> revoke never leaks it into the revoke form or the token list. TEST-03: the recipe journey had no e2e at all, which is why DATA-04 shipped. Builds a recipe, checks per-portion macro scaling, logs a portion, and asserts the dashboard entry. A second spec dispatches two clicks in one page task to reproduce the double-tap race and asserts exactly one entry is written. TEST-04: lazy-modal-mounting.test.tsx asserted on component source text, so renaming a guard variable or breaking its boolean logic still passed. Replaced with RTL renders that check the modal subtree is really absent or present. TEST-05: canAccessAdmin/isOwnerRole are the sole admin authorization primitive and admin-auth.test.ts reimplemented them inline rather than importing them, so a bug in the real code could not fail that suite. Tested directly. TEST-06: calculateMacroTargets only had loss scenarios; adds a surplus case. DATA-02: request-level coverage for the reserved-key strip, which could not live in api.rs because it has no database harness. Every one of these was verified by breaking the behaviour it covers and confirming the test failed - including removing the __-prefix strip from apply_client_patch, which let the mass assignment through as expected.
DEP-02. Every remaining advisory after the next bump was transitive through dev tooling, so pnpm overrides pull in the patched versions without chasing the eslint 9->10 and typescript 5->7 majors the audit called out as not worth doing opportunistically. esbuild, postcss, nanoid, js-yaml, @babel/core and brace-expansion (across all three majors in the tree) are now on patched releases, and vitest moves 4.1.2 -> 4.1.11. 28 advisories -> 5. The five that remain are all vite, reached only as vitest's bundler. An override for it is genuinely inert - pnpm keeps resolving 8.0.3 through vitest's dependency range whatever is pinned, verified with 'pnpm install --force' and 'pnpm update vite --latest' - so no dead override is left behind implying a protection that is not there. vite never reaches a production artifact; next build uses its own bundler.
This remediation pass changes behaviour rather than only fixing bugs: sessions now have an absolute lifetime, the CSP serves a nonce instead of 'unsafe-inline', COOP/CORP are enforced, /api/v1 is rate limited, and database TLS verifies by default. Per AGENTS.md that is a feature-level change, not a bug fix.
…-path read Reverses the OPS-05 consolidation. The audit proposed collapsing the frontend's two railway.toml copies into the root one, and that is wrong: Railway applies the root config to any service without a config-as-code path of its own, which is exactly what commit 8cac5be had to fix when this file's 'pnpm start' replaced the cliproxyapi container entrypoint and failed that deploy. Restores apps/web/railway.toml and documents which file belongs to which service. Also caps read_upstream_error, which still buffered an unbounded error body via response.json() while the success path went through read_capped_json. Same exhaustion CLEAN-C1 closed, on the branch a hostile gateway actually controls.
getByText("Operations Panel") also matched Next's route announcer, the
#__next-route-announcer__ live region that mirrors the page heading after a
client-side navigation, so the locator resolved to two elements and tripped
strict mode. The page itself renders once - one h1, one main - and the announcer
is correct accessibility behaviour, so the selector was the problem. Asserting
on the heading role is both unambiguous and closer to what the test means.
Surfaced by rebasing onto the CSP-nonce work, which makes that navigation soft
and therefore populates the announcer where a full load left it empty.
Pinning dtolnay/rust-toolchain to an explicit 1.89 in OPS-03 dropped the components the 'stable' alias installs by default, so the Rust job failed with "'cargo-fmt' is not installed for the toolchain '1.89-x86_64-unknown-linux-gnu'" before running a single check.
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.
Works through the audit queue in
AUDIT-REMEDIATION.md. Every P0 and P1 is closed, along with the P2/P3/P4 items. One item is deliberately partial and two audit recommendations turned out to be wrong and were reversed — details below.P0
/api/auth/shoo/verifywas unauthenticated, parsed withrequest.json()(which ignoresContent-Type) and set the session cookie with no origin check. A cross-sitetext/plainform post needed no preflight and no JS, andSameSite=Laxgoverns sending a cookie, not setting one — so a victim's browser silently stored the attacker's session. Now requires same-origin and fails closed when neitherSec-Fetch-SitenorOriginis present.Validationseedsrequired_spec_claimswith onlyexp, and itsiss/audmatch arms fall through on an absent claim — so both were enforced only for tokens that happened to carry them. A token omittingaudauthenticated. Also stopped deriving the algorithm from the token's own header.emailand then unconditionally rebound that row'sshoo_pairwise_sub, so an ID token carrying a victim's email under a different subject took over the account. Now matches on subject only; an email held under another subject is a conflict.meta/had snapshots0000–0004against a 15-entry journal, so the nextdb:generatewould have diffed against the 0004 state and emitted a destructive migration. Migrations are now declared hand-authored, withMIGRATIONS.mdand a test locking it in.next16.2.3 → 16.3.1, clearing 22 advisories including abeforeInteractiveXSS affecting themt_tzbootstrap this app actually uses.Behavioural changes worth reviewing closely
next.config.ts(baked into the routes manifest at build time) intoproxy.ts.'unsafe-inline'is gone fromscript-src.style-srcdeliberately keeps it and gets no nonce, since a style nonce makes browsers ignore'unsafe-inline'and would break every inline style Next andnext/fontemit.iat. Previously every request re-minted a 7-day token, so a captured one never expired. No schema change; no "sign out everywhere".sslmode=requirenow verifies the chain. It previously yieldedrejectUnauthorized: false, the same postureno-verifyis rejected for, and it is the value Railway/Neon/Supabase hand out.ALLOW_UNVERIFIED_DB_TLS=trueis the only way back and warns loudly./api/v1, pool default 3 → 10,/healthprobe cached. Note it keys on the peer IP, so behind Railway's edge this degrades to a global cap; switching toX-Forwarded-Foris only safe if Railway strips inbound XFF.admin_audit_events.actor_user_idbecomes nullable withON DELETE SET NULL, and enum-like columns gain CHECK constraints addedNOT VALIDso a blockingpreDeployCommandcannot fail on legacy rows.Deliberately not done
SEC-18 (owner demotion) is partial.
AppUserhas no column recording whether ownership came fromADMIN_OWNER_EMAILSor the admin UI, so demoting every owner absent from the list would strip admin-granted owners and could leave zero owners. Ships as a tested policy function that warns instead. Real revocation needs a schema change.Two audit recommendations that were wrong
railway.tomlcopies into the root one. Reversed: Railway applies the root config to any service without its own config-as-code path — which is exactly what8cac5beahad to fix when it hijacked the cliproxyapi service.apps/web/railway.tomlis restored and the ownership of each file documented.unsupported_modelshould be a 500/503. After the AI-gateway refactor its premise (free-model filtering) no longer exists, and the branch's only producer is the gateway reporting a server-configured model can't accept images — already a 502, which is correct. Dropped and pinned with a test rather than reinstating the removed allowlist.Verification
cargo test -p macro-tracker-backendcargo clippy --all-targetscargo fmt --all --checkvitest)packages/dbtypecheck/lint/audit:codeWeb and db suites were run against a live Rust backend and a real Postgres, matching how CI runs them — several are integration tests that pass vacuously without one.
Test-coverage items were each verified by breaking the behaviour and confirming the test failed, including removing the
__-prefix strip to confirm the mass assignment reappears.Notes
devmid-flight; the provider-specific fixes were re-targeted rather than redone.vite, reachable only as vitest's bundler and never in a production artifact. An override for it is inert (pnpm resolves it through vitest's range regardless), so none was left behind implying protection that isn't there.loremflickr.com, which 302s to a different random photo per request, so those benchmarks can't be comparable between runs. Left unchanged but pinned by a test; fixing needs licence-checked replacements.v3.03), since sessions, CSP, headers, rate limiting and DB TLS all change behaviour.