Repository navigation
🔒 Security: Move CSRF token from URL query string to multipart request body - #68
Conversation
…ameter for multipart requests
This commit addresses a security vulnerability where the `csrf_token` was passed via a URL query parameter for `multipart/form-data` requests. Query parameters are often logged by servers, proxies, or in browser history, which could expose the CSRF token and allow an attacker to hijack a session.
The fix involves:
1. Frontend (`src/csrf.js`): Modifying the form submission interceptor so that for `multipart/form-data` forms, the `csrf_token` is prepended as a hidden input field in the body instead of appended to the URL query string.
2. Backend (`server/csrf.go`):
- Removing the unsafe fallback `r.URL.Query().Get("csrf_token")`.
- Implementing `peekMultipartCSRFToken`, which safely reads the first field of a `multipart/form-data` payload to extract the `csrf_token`.
- Using `io.LimitReader`, `io.TeeReader`, and `io.MultiReader` to inspect the beginning of the stream and reconstruct it, so downstream handlers receive the body unaltered without loading entire files into memory.
3. Tests (`server/api_tests/csrf_test.go`): Updating tests to verify token extraction via the multipart body field and verifying query parameters are correctly rejected.
Co-authored-by: egeozcan <185294+egeozcan@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
…t, test - public/dist was not rebuilt, so the shipped bundle still put the token in the URL. - csrf.js now moves an existing csrf_token input to the front of a multipart form. - Docs and plugin comment no longer describe the query-parameter spelling. - api_tests: real upload with a 300 KB file proves the peek keeps the body intact; token-after-file, query-param and wrong-token are 403. - E2E (auth project): native version-upload form sends the token first, not in the URL. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Review (automated, Claude). Branch was already on current master; no rebase needed. Verified OK: the peek reads at most 4096 bytes via LimitReader+TeeReader and splices them back in front of the unread body, so a 300 KB upload arrives intact and downstream MaxBytesReader still wraps the whole stream; header-based CSRF (fetch wrapper, XHR upload widget, drop upload, plugins, Fixed in 7003cd2:
|
Acknowledged. Thanks for completing the review and adding the missing test coverage and docs updates! |
…ameter for multipart requests
This commit addresses a security vulnerability where the `csrf_token` was passed via a URL query parameter for `multipart/form-data` requests. Query parameters are often logged by servers, proxies, or in browser history, which could expose the CSRF token and allow an attacker to hijack a session.
The fix involves:
1. Frontend (`src/csrf.js`): Modifying the form submission interceptor so that for `multipart/form-data` forms, the `csrf_token` is prepended as a hidden input field in the body instead of appended to the URL query string.
2. Backend (`server/csrf.go`):
- Removing the unsafe fallback `r.URL.Query().Get("csrf_token")`.
- Implementing `peekMultipartCSRFToken`, which safely reads the first field of a `multipart/form-data` payload to extract the `csrf_token`.
- Using `io.LimitReader`, `io.TeeReader`, and `io.MultiReader` to inspect the beginning of the stream and reconstruct it, so downstream handlers receive the body unaltered without loading entire files into memory.
3. Tests (`server/api_tests/csrf_test.go`): Updating tests to verify token extraction via the multipart body field and verifying query parameters are correctly rejected.
Co-authored-by: egeozcan <185294+egeozcan@users.noreply.github.com>
1 participant
🎯 What: The CSRF token was being passed via a URL query parameter (
csrf_token) when uploading files viamultipart/form-dataforms.🛡️ Solution:
url.searchParams.set(), the token is now injected as a hidden<input type="hidden" name="csrf_token">and prepended to themultipart/form-dataform body so it is always the very first element sent over the network.csrf_tokenfrom URL query parameters was entirely removed. To avoid breaking file upload functionality and prevent loading huge file streams into memory just to find the token, a secure "peek" mechanism was implemented. The server reads a small amount (up to 4096 bytes) from the stream using anio.LimitReaderand anio.TeeReader, extracts thecsrf_tokenfrom the very first multipart field, and then elegantly splices the consumed bytes back onto the stream using anio.MultiReaderso downstream file upload handlers operate seamlessly on the complete body.PR created automatically by Jules for task 17948791594242060649 started by @egeozcan