fix(chat): send the double-submit CSRF token on attachment uploads - #3640
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChat attachment upload and removal requests now use ChangesChat attachment CSRF protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant UploadHook
participant csrfMutationHeaders
participant BrowserDocument
participant CsrfHandler
UploadHook->>csrfMutationHeaders: provide upload or removal URL and existing headers
csrfMutationHeaders->>BrowserDocument: read CSRF cookie
BrowserDocument-->>csrfMutationHeaders: return cookie value
csrfMutationHeaders-->>UploadHook: return mutation headers
UploadHook->>CsrfHandler: send attachment mutation request
CsrfHandler-->>UploadHook: validate request and return response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c02e73c54b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/agent/react/use-chat/use-chat.csrf.test.tsx (1)
110-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative control so the suite cannot pass vacuously.
Every edge assertion expects 200. If
CsrfHandlerstopped enforcing, orcsrfCtx()stopped enablingcsrf, all three protected-request tests would still pass. Add one case that sends a mutation without the token through the same edge and expect 403. This pins the enforcement that the positive cases rely on.🧪 Proposed control test
+ it("rejects the same mutation when the client omits the token", async () => { + const restoreDom = installDom(); + loadDocumentCookie(); + const edge = installCsrfEdge(() => agUiResponse()); + try { + await fetch("/api/ag-ui", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: "{}", + }); + + assertEquals(edge.statuses.get("POST /api/ag-ui"), 403); + } finally { + edge.restore(); + restoreDom(); + } + });Also applies to: 144-167
🤖 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/agent/react/use-chat/use-chat.csrf.test.tsx` around lines 110 - 134, Add a negative-control test using installCsrfEdge that sends a protected mutation without the CSRF token and asserts a 403 response. Keep it alongside the existing protected-request cases so the suite verifies CsrfHandler enforcement and csrfCtx() configuration rather than passing vacuously.
🤖 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.
Nitpick comments:
In `@src/agent/react/use-chat/use-chat.csrf.test.tsx`:
- Around line 110-134: Add a negative-control test using installCsrfEdge that
sends a protected mutation without the CSRF token and asserts a 403 response.
Keep it alongside the existing protected-request cases so the suite verifies
CsrfHandler enforcement and csrfCtx() configuration rather than passing
vacuously.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: baa495b0-7fb7-48a7-89ff-b0fc01ccc1bb
⛔ Files ignored due to path filters (1)
src/server/handlers/dev/framework-candidates.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (8)
src/agent/react/use-chat/use-chat.csrf.test.tsxsrc/agent/react/use-chat/use-chat.tssrc/react/components/chat/chat/hooks/use-uploads-registry.tssrc/security/csrf/mutation-headers.tssrc/workflow/react/mutation-headers.tssrc/workflow/react/use-approval.tssrc/workflow/react/use-workflow-start.tssrc/workflow/react/use-workflow.ts
💤 Files with no reviewable changes (1)
- src/workflow/react/mutation-headers.ts
c02e73c to
0bf8946
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
kojiwakayama
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsx (3)
143-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove test setup inside
tryso patched globals always restore.Each test patches
globalThisbefore thetryblock.loadDocumentCookiethrows by design when the CSRF cookie isHttpOnly.installCsrfEdgeandrenderAttachmentscan also throw. In those casesrestoreDomnever runs, so jsdomwindow,document, and the patchedfetchleak into the following tests in the same process. The failure then appears in an unrelated test.Acquire the restore function first, then perform the remaining setup inside
try.♻️ Proposed structure for each test
const restoreDom = installDom(); - loadDocumentCookie(); - const edge = installCsrfEdge((req) => - req.method === "GET" - ? Response.json({ items: [] }) - : Response.json({ id: "up-1", name: "a.txt", url: "/files/a.txt", size: 1 }) - ); - const view = renderAttachments("/api/uploads"); + let edge: ReturnType<typeof installCsrfEdge> | undefined; + let view: ReturnType<typeof renderAttachments> | undefined; try { + loadDocumentCookie(); + edge = installCsrfEdge((req) => + req.method === "GET" + ? Response.json({ items: [] }) + : Response.json({ id: "up-1", name: "a.txt", url: "/files/a.txt", size: 1 }) + ); + view = renderAttachments("/api/uploads"); view.attachments().upload([new File(["a"], "a.txt", { type: "text/plain" })]); await waitFor(() => edge.statuses.has("POST /api/uploads")); assertEquals(edge.statuses.get("POST /api/uploads"), 200); } finally { - view.unmount(); - edge.restore(); + view?.unmount(); + edge?.restore(); restoreDom(); }The same pattern applies to the removal test at lines 164-166 and the cross-origin test at lines 187-188.
Also applies to: 164-166, 187-188
🤖 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/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsx` around lines 143 - 150, Move loadDocumentCookie, installCsrfEdge, and renderAttachments setup inside the try block for each affected test, including the removal and cross-origin tests, while acquiring restoreDom first. Keep cleanup in finally so restoreDom always runs when any setup step throws.
106-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe simulated edge does not reproduce the multipart body.
installCsrfEdgebuilds a nativeRequestfrominit.body, which is a jsdomFormDatainstance. A nativeRequestdoes not recognize aFormDatafrom another realm, so the body is coerced instead of encoded as multipart. The CSRF assertions do not read the body, so the current tests still pass. If a later test asserts on upload content, the body will be wrong.
String(input)also fails if a caller passes aRequestorURL. Normalize the input instead.♻️ Proposed hardening of the fetch stub
globalThis.fetch = async (input, init) => { - const url = new URL(String(input), document.baseURI); + const raw = input instanceof Request ? input.url : String(input); + const url = new URL(raw, document.baseURI); const headers = new Headers(init?.headers); if (document.cookie) headers.set("cookie", document.cookie); const req = new Request(url, { method: init?.method ?? "GET", headers, body: init?.body as BodyInit | undefined, });🤖 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/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsx` around lines 106 - 114, Update installCsrfEdge’s globalThis.fetch stub to normalize input via the existing Request/URL-aware URL handling instead of String(input). Preserve jsdom FormData when constructing the native Request by converting or re-encoding the cross-realm form data as multipart, including the correct boundary-bearing Content-Type, so upload bodies remain valid for future assertions.
186-206: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider covering the cross-origin removal request too.
This test proves that the upload
POSTomits the token for a cross-origin endpoint. TheDELETEremoval path callscsrfMutationHeaders(target, ...)with a different URL built bysetQueryParameter, so it has its own origin computation. A cross-originDELETEcase would guard that second call site.🤖 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/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsx` around lines 186 - 206, Add a separate test alongside the existing cross-origin upload test that exercises attachment removal and verifies the DELETE request sends no page CSRF token. Use a cross-origin upload endpoint, capture headers for DELETE, remove the uploaded attachment, and assert the captured x-csrf-token is null, covering the URL produced through setQueryParameter and its csrfMutationHeaders call.
🤖 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.
Nitpick comments:
In `@src/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsx`:
- Around line 143-150: Move loadDocumentCookie, installCsrfEdge, and
renderAttachments setup inside the try block for each affected test, including
the removal and cross-origin tests, while acquiring restoreDom first. Keep
cleanup in finally so restoreDom always runs when any setup step throws.
- Around line 106-114: Update installCsrfEdge’s globalThis.fetch stub to
normalize input via the existing Request/URL-aware URL handling instead of
String(input). Preserve jsdom FormData when constructing the native Request by
converting or re-encoding the cross-realm form data as multipart, including the
correct boundary-bearing Content-Type, so upload bodies remain valid for future
assertions.
- Around line 186-206: Add a separate test alongside the existing cross-origin
upload test that exercises attachment removal and verifies the DELETE request
sends no page CSRF token. Use a cross-origin upload endpoint, capture headers
for DELETE, remove the uploaded attachment, and assert the captured x-csrf-token
is null, covering the URL produced through setQueryParameter and its
csrfMutationHeaders call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b840b2d-627d-4549-8e04-23aa7779292c
⛔ Files ignored due to path filters (1)
src/server/handlers/dev/framework-candidates.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (2)
src/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsxsrc/react/components/chat/chat/hooks/use-uploads-registry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/components/chat/chat/hooks/use-uploads-registry.ts
0bf8946 to
b6820dd
Compare
kojiwakayama
changed the title
#3611 fixed the AG-UI chat turn, but attachments travel over two *other* transports and neither sent the token: - `useUpload` — the one `<Chat uploadApi>` actually wires up (`app-mode-chat.tsx:113`, `controlled-chat.tsx:99`). It uploads over `XMLHttpRequest`, since fetch has no upload-progress event, and applied only caller-supplied headers. - `useAttachments` — the durable uploads registry exported from `veryfront/chat`. `POST {url}` to upload, `DELETE {url}?id=` to remove. A deployed chat *with attachments* therefore still answered 403 after #3611. `veryfront dev` does not enable CSRF, so this only ever showed up in production. All three mutations now route their headers through `csrfMutationHeaders` — the helper #3611 already extracted to `security/csrf/browser-mutation-headers.ts`. No new CSRF implementation is added here. The registry's list `GET` is a safe method and is left alone. The registry `DELETE` passes its real `?id=` target so the helper's same-origin guard evaluates the URL actually being hit. The tests drive the real hooks — including a fake `XMLHttpRequest` that replays whatever `useUpload` sets into a real `Request` — and pipe the result through the real `CsrfHandler` with `securityConfig: { csrf: true }`, so they fail on an actual 403 rather than on a header assertion.
b6820dd to
e9f5d6b
Compare
2 participants
The bug
Chat attachments still 403 in production.
#3611 fixed the AG-UI
chat turn —
useChatnow sends thex-csrf-tokendouble-submit header. Butattachments travel over two separate transports, and neither was covered:
useUpload— what<Chat uploadApi>actually wiresPOST {api}viaXMLHttpRequestx-csrf-token→ 403useAttachments— the durable uploads registryPOST {url}(upload)x-csrf-token→ 403useAttachmentsDELETE {url}?id=(remove)x-csrf-token→ 403useAttachmentsGET {url}(list)useUploadis the important one:app-mode-chat.tsx:113andcontrolled-chat.tsx:99both send files through it, so it is the transport theadvertised chat-with-attachments flow actually 403s on. It uploads over
XMLHttpRequest(fetch has no upload-progress event) and applied onlycaller-supplied headers.
A production build defaults
security.csrfto on, so a deployed chat withattachments still answers
403 Forbidden – invalid or missing CSRF tokenafter #3611.
veryfront devdoes not enable CSRF, so it works locally andfails only in production.
The fix
All three mutations route their headers through
csrfMutationHeaders— thehelper #3611 already extracted to
src/security/csrf/browser-mutation-headers.ts. No new CSRF implementationis added here, and none of #3611's design is revisited. The registry
DELETEpasses the actual
?id=target rather than the bare endpoint, so the helper'ssame-origin guard evaluates the URL that is really being hit.
The list
GETis untouched.Red → green
The tests drive the real hooks and pipe whatever they emit through the
real
CsrfHandlerwithsecurityConfig: { csrf: true }, so they fail onan actual 403 rather than on a header assertion. For
useUploadthat means afake
XMLHttpRequestthat replays whatever the hook sets into a realRequest— the assertion is still the handler's verdict, not a header check.Red, with both source files reverted to current
origin/main:Green:
The two cross-origin tests pass in both columns by design — they guard against
the fix over-reaching and leaking the page token off-origin.
loadDocumentCookie()calls the realapplyCsrfCookieand throws if theSet-Cookieit produces is HttpOnly, so if anyone ever flips that default thetest fails loudly instead of the fix silently becoming a no-op.
Wider gates:
deno testoverreact/components/chat,security,agent/react→ 255 passed, 0 failed.deno task lintanddeno task typecheckclean. The full pre-push suite passed on push.(A broader run including
src/workflow/also tripssrc/workflow/blob/veryfront-cloud-storage.test.ts, which fails identicallyon a clean
origin/mainworktree — environmental, the cloud blob tests timeout at 10s without credentials. Nothing here touches it.)
Scope note — the AG-UI half was already fixed in #3611
This PR originally also carried the
useChatfix and a move ofworkflow/react/mutation-headers.tsintosecurity/. That work branched froma stale
origin/mainand duplicated #3611, which had already landed thesame design under the same helper name. That duplication has been dropped and
the branch rebuilt on current
main; what remains is only the attachment fix,which #3611 does not cover (
use-chat.csrf.test.tsxonmainhas noattachment coverage).
Thanks to @chatgpt-codex-connector for catching that the first version of this
reduced PR patched only
useAttachments— the registry hook<Chat>nevercalls — and left the actual
<Chat uploadApi>upload path still broken.Follow-ups, not done here
headers/fetchescape hatch onChat.useChatalready acceptsheaders, but app-mode<Chat>never forwards them, so there was noclient-side workaround at all. That is a public API-surface change and
deserves its own PR.
csrfrelease manifest bug that blocked the correctcsrf: { excludePaths: [...] }fix. It is the reason a customer demo iscurrently running with
security: { csrf: false }.use-completion,use-streaming,use-agents) have the same gap and can adoptcsrfMutationHeadersin a follow-up.