fix: integrate PR #481 auth and payload patch into main#490
Conversation
… phase 3 — auth) (#481) * perf: skip the proxy session-refresh getUser for API routes Round-2 phase 3, auth part (isolated for security review). Every authenticated request previously paid two serial auth-server round trips: src/proxy.ts ran supabase.auth.getUser() for every matched path including /api/*, and the route handler then independently validated the caller via getOptionalAuthenticatedUser. Nothing consumed the proxy's result on API routes — the call existed only to keep session cookies fresh. The proxy now skips only the session-refresh getUser for /api/* paths: - API handlers keep validating the caller themselves (bearer token and/or @supabase/ssr cookie) — no authorization decision moves or weakens, and the handlers remain fail-closed exactly as before. - The CSP nonce header still applies to every matched response including /api/* (matcher unchanged). - Session cookies still refresh on every page navigation, and the browser Supabase client refreshes its own token (autoRefreshToken) for bearer-based API calls, so long-lived sessions behave as before. Deliberately NOT done: the trusted-internal-header design (proxy forwards a validated user id for handlers to trust) was rejected — handlers validating tokens independently is the stronger defense-in-depth posture. The legacy cookie-token extraction in supabase/auth.ts was left untouched: tests define legacy sb-access-token / plain-JSON cookie support as intended behavior. New tests pin the contract: getUser is skipped for /api/* (CSP still stamped), runs on page navigations with an sb- cookie, and never runs without one. Existing CSP nonce tests unchanged and green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf: trim the answer payload to what the client renders (#482) * fix: preserve cookie refresh and safety evidence --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe change trims server-only answer-source fields from JSON and SSE responses, adds transformation and proxy session-refresh tests, clarifies proxy comments, and updates branch review ledger records. ChangesAnswer payload boundary
Proxy session verification
Review ledger records
Estimated code review effort: 3 (Moderate) | ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/answer-client-payload.ts (1)
17-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDenylist trimming risks silently leaking future server-only fields.
trimSourceForClientdeletes today's known server-only fields. IfSearchResultlater gains another server-only field (similar in spirit toadjacent_context/memory_cards), it will pass through to the client by default unless someone remembers to add adeletehere — the opposite of what a trust-boundary trim function should guarantee.Consider inverting to an allowlist (or adding a compile-time exhaustiveness check keyed off
keyof SearchResult) so newly added fields must be explicitly classified as client-safe rather than defaulting to "leaked."♻️ Example exhaustiveness guard
+// Any new key added to SearchResult must be classified here as either +// client-visible or trimmed — this fails to compile otherwise. +type _SourceFieldAudit = { [K in keyof SearchResult]: true }; +const _sourceFieldAudit: _SourceFieldAudit = { + id: true, document_id: true, title: true, file_name: true, page_number: true, + chunk_index: true, section_heading: true, section_path: true, heading_level: true, + parent_heading: true, anchor_id: true, content: true, retrieval_synopsis: true, + image_ids: true, similarity: true, similarity_origin: true, text_rank: true, + hybrid_score: true, lexical_score: true, rrf_score: true, score_explanation: true, + source_strength: true, source_metadata: true, document_labels: true, + document_summary: true, adjacent_context: true, memory_cards: true, + memory_score: true, relevance: true, match_explanation: true, table_facts: true, + index_unit: true, indexing_quality: true, images: true, +};🤖 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/lib/answer-client-payload.ts` around lines 17 - 33, Update trimSourceForClient to use an explicit allowlist of client-safe SearchResult fields instead of spreading the full source and deleting known server-only fields. Ensure newly added SearchResult properties cannot pass through by default, while preserving content’s empty-string fallback and the existing toClientAnswerPayload mapping 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 `@docs/branch-review-ledger.md`:
- Line 49: Update the verification column of the ledger row for PR `#481` to
explicitly include the completed npm run verify:cheap check and replace the
ambiguous “production-readiness CI-safe” wording with the exact
production-readiness verification command or checks that were actually executed.
Preserve the other listed verification results and ensure the row accurately
reflects only completed checks.
In `@tests/proxy-session-refresh.test.ts`:
- Around line 64-70: Add the response-cookie assertion from the API test to the
page-navigation test for proxy(requestWithSessionCookie("/documents/some-id")),
verifying that the rotated session cookie is present on the response while
preserving the existing getUser and CSP assertions.
---
Nitpick comments:
In `@src/lib/answer-client-payload.ts`:
- Around line 17-33: Update trimSourceForClient to use an explicit allowlist of
client-safe SearchResult fields instead of spreading the full source and
deleting known server-only fields. Ensure newly added SearchResult properties
cannot pass through by default, while preserving content’s empty-string fallback
and the existing toClientAnswerPayload mapping behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 948c6ad6-5af9-445d-a001-6801e3d44c42
📒 Files selected for processing (7)
docs/branch-review-ledger.mdsrc/app/api/answer/route.tssrc/app/api/answer/stream/route.tssrc/lib/answer-client-payload.tssrc/proxy.tstests/answer-client-payload.test.tstests/proxy-session-refresh.test.ts
All actionable and nitpick findings fixed in a6a9e02: auditable exact verification commands, page-response cookie assertion, and an exhaustive explicit client-field policy that blocks unclassified runtime fields.
|
Resolved the full CodeRabbit set in |
Summary
Why
PR #481 merged into its already-merged feature base, so its patch was not reachable from main.
Verification