Staging - #96
Merged
Merged
Conversation
Works through the audit report's 54 findings across security, data correctness, performance, accessibility, code quality and testing. Data correctness (the audit's top theme): - Capture the browser's IANA timezone in a cookie during the initial HTML parse and resolve "today" from it server-side, so SSR renders the user's calendar day instead of the Node process day. - Resolve the onboarding weigh-in date and the recipe planned-vs-eaten status in the browser; delete the server-clock copy of that predicate. - Validate entry_date on the internal RPC path, so Postgres specials like 'infinity' can no longer be stored and then break /progress permanently. - Window rollingAverages.days7/days30 separately instead of emitting the all-time average under both labels. - Pin peak-hour extraction to UTC rather than the session TimeZone GUC. Security: - Enforce admin role in the data layer: admin read RPCs now take an actor and check it in Rust, and the five unguarded admin pages call requireAdminUser(). Replace the arbitrary-role ensureUserRole RPC with a config-driven owner reconcile plus a test-only, feature-gated variant. - Bound outbound calls: client-wide reqwest timeouts, a capped provider body reader, a session gate and concurrency limit on barcode lookup, search length/term caps, and a timeout on every web to backend fetch. - Reject traversal segments before the internal secret is attached, and stop attaching it to Bearer-authenticated /api/v1 proxying at all. - Map OpenRouter statuses and text to server-owned responses, mask serde and reqwest details, add security headers, and require SESSION_SECRET and explicit test-route flags in every environment. Also: numeric bounds matching the column domains, batched meal-group reorder, concurrent admin detail queries, single-row weight lookup, lazy recipe-builder modals, dialog semantics with Escape and focus trapping on every overlay, restored pinch-zoom, 44px touch targets, confirmation on role change and token revoke, preferred weight unit threaded through /progress, idempotency keys on meal creates, dead validators and error mappers deleted, the experimental- prefix dropped, and new tests for timezones, dates, macro rendering, modal accessibility and the API v1 contract. Not addressed: Q-5 (benchmark route as a background job) and Q-6 (splitting db.rs by domain) are left as follow-ups.
- The stats test asserted a 7-day average that reached forward into a future-dated day; that was the TZ-5 bug, so it now asserts the window. - Role changes need a confirming second tap, so the admin e2e helper does. - Convert the modal-chunk-fallback test to JSX: the provider's props require children, which createElement's props argument cannot satisfy.
- The recipe editor mounted its lazy barcode overlay unconditionally, so the full-screen loading backdrop appeared on every editor load and dismissal could not unmount it. Gate it on active capture state, the way dashboard-shell already does. - Barcode-limiter saturation returned None, which the route reported as `found: false` — the same "outage looks like a missing product" problem fixed elsewhere. Model the outcome as Found/NotFound/Busy and answer Busy with a retryable 503, which the client already treats as unavailable rather than not-found. - Onboarding writes the goal columns and the first weight entry directly rather than through save_user_goals/the weight endpoint, so it bypassed the new domain validation. Route both through the shared validators so an oversized macro or a post-rounding weight overflow fails as a bad request instead of rolling back onboarding with a database error. - A tower TimeoutLayer over /api/v1 emitted a bare 504 with no body and no CORS headers, breaking the documented error envelope. Enforce the deadline inside the handler instead, and keep the layer on the internal and health routes only. Adds regression tests for each: envelope-preserving timeout, limiter saturation, macro-goal and weight bounds, and the lazy mount conditions.
Fix the full-codebase audit findings
The Content-Security-Policy added for SEC-8 broke both ends of the auth flow. Both are reproduced and fixed against a real browser. Sign-out did nothing. `form-action 'self'` is enforced across the whole redirect chain in Chromium, and every form here is POST-then-redirect: signing out (`/api/auth/logout` -> `/login`) and each admin server action that calls `redirect()`. A same-origin POST *without* a redirect was allowed, which is what made this look like it should work. The directive only guards against form-jacking to an external origin and no form here takes a caller-supplied action, so it is dropped rather than worked around. Sign-in failed with "Failed to fetch" on /auth/callback. `@shoojs/auth` runs the code-for-token exchange in the browser against `<SHOO_BASE_URL>/token`, which `connect-src 'self'` blocked. The identity provider origin is now in `connect-src`, derived from `SHOO_BASE_URL`. `headers()` is baked into the routes manifest at build time, so `SHOO_BASE_URL` has to be present in the build environment; the README now says so, and `buildContentSecurityPolicy` is exported and unit tested so neither directive can silently regress again.
Picking a day pushes the new URL inside a transition, so for a moment `window.location.search` still holds the previous query while `selectedDate` already holds the new day. The effect read that as "no date requested, and the selected day is not today", and replaced the URL with today — silently discarding the user's choice. It is a *startup* correction for a cold load whose server render predates the timezone cookie, so it belongs on mount rather than on every `selectedDate` change. Guarded with a ref, with the race captured as a unit test on `getStartupDateRedirect`. Surfaced as a flaky failure of the "tracks food items across days" browser test; the full Playwright suite and 8 repeats of that spec pass locally against a real backend.
Fix CSP breaking sign-out and sign-in
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.
No description provided.