diff --git a/server/api_tests/csrf_test.go b/server/api_tests/csrf_test.go index 5ef47d24..bd6eccce 100644 --- a/server/api_tests/csrf_test.go +++ b/server/api_tests/csrf_test.go @@ -62,9 +62,9 @@ func TestCSRF_CookiePostWithoutTokenIsRejected(t *testing.T) { } } -// The token is accepted via the X-CSRF-Token header, the csrf_token query -// parameter, and (for urlencoded bodies) the csrf_token form field. -func TestCSRF_TokenAcceptedViaHeaderQueryAndField(t *testing.T) { +// The token is accepted via the X-CSRF-Token header, and (for urlencoded bodies) +// the csrf_token form field. +func TestCSRF_TokenAcceptedViaHeaderAndField(t *testing.T) { tc := setupAuthEnv(t) cookie, token := loginCookieAndCSRF(t, tc) @@ -76,14 +76,6 @@ func TestCSRF_TokenAcceptedViaHeaderQueryAndField(t *testing.T) { t.Fatalf("POST with X-CSRF-Token header should not be 403, got %d (%s)", rr.Code, rr.Body.String()) } - // Query parameter - rr = doReq(tc, http.MethodPost, "/v1/tag?csrf_token="+token, - map[string]string{"Content-Type": urlEncoded}, []*http.Cookie{cookie}, - strings.NewReader("name=csrf-query")) - if rr.Code == http.StatusForbidden { - t.Fatalf("POST with csrf_token query param should not be 403, got %d (%s)", rr.Code, rr.Body.String()) - } - // Urlencoded body field rr = doReq(tc, http.MethodPost, "/v1/tag", map[string]string{"Content-Type": urlEncoded}, []*http.Cookie{cookie}, @@ -93,6 +85,23 @@ func TestCSRF_TokenAcceptedViaHeaderQueryAndField(t *testing.T) { } } +// The token is accepted via the csrf_token form field in a multipart/form-data body +// when it is the first part. +func TestCSRF_TokenAcceptedViaMultipartField(t *testing.T) { + tc := setupAuthEnv(t) + cookie, token := loginCookieAndCSRF(t, tc) + + boundary := "test-boundary" + bodyStr := "--" + boundary + "\r\nContent-Disposition: form-data; name=\"csrf_token\"\r\n\r\n" + token + "\r\n--" + boundary + "\r\nContent-Disposition: form-data; name=\"name\"\r\n\r\ncsrf-multipart\r\n--" + boundary + "--\r\n" + + rr := doReq(tc, http.MethodPost, "/v1/tag", + map[string]string{"Content-Type": "multipart/form-data; boundary=" + boundary}, + []*http.Cookie{cookie}, strings.NewReader(bodyStr)) + if rr.Code == http.StatusForbidden { + t.Fatalf("POST with csrf_token multipart field should not be 403, got %d (%s)", rr.Code, rr.Body.String()) + } +} + // A wrong token is rejected. func TestCSRF_WrongTokenRejected(t *testing.T) { tc := setupAuthEnv(t) diff --git a/server/csrf.go b/server/csrf.go index 59cd3eb7..e5a678cb 100644 --- a/server/csrf.go +++ b/server/csrf.go @@ -1,8 +1,12 @@ package server import ( + "bytes" "crypto/subtle" "encoding/json" + "io" + "mime" + "mime/multipart" "net/http" "strings" @@ -66,27 +70,80 @@ func csrfExempt(r *http.Request) bool { } // csrfTokenFromRequest extracts the submitted CSRF token without ever reading a -// multipart or JSON body (which would defeat the per-upload size limits applied +// full multipart or JSON body (which would defeat the per-upload size limits applied // downstream via http.MaxBytesReader). Resolution order: // 1. X-CSRF-Token header — sent by the JS fetch layer for all AJAX requests. -// 2. csrf_token query parameter — used by native multipart upload forms, whose -// body cannot be read here. -// 3. csrf_token field of an application/x-www-form-urlencoded body — native +// 2. csrf_token field of an application/x-www-form-urlencoded body — native // non-upload forms. ParseForm caches the parse, so the handler re-reads it // for free. +// 3. csrf_token field in a multipart/form-data body. This requires the frontend +// to send it as the first part. We peek at the body stream to read it safely. func csrfTokenFromRequest(r *http.Request) string { if v := r.Header.Get("X-CSRF-Token"); v != "" { return v } - if v := r.URL.Query().Get("csrf_token"); v != "" { - return v - } - if strings.HasPrefix(r.Header.Get("Content-Type"), "application/x-www-form-urlencoded") { + + ct := r.Header.Get("Content-Type") + if strings.HasPrefix(ct, "application/x-www-form-urlencoded") { return r.PostFormValue("csrf_token") } + + if strings.HasPrefix(ct, "multipart/form-data") { + return peekMultipartCSRFToken(r) + } return "" } +// peekMultipartCSRFToken reads the first part of a multipart/form-data request +// body, expecting it to be the csrf_token. It reconstructs the request body +// so downstream handlers can parse the full multipart data. +func peekMultipartCSRFToken(r *http.Request) string { + if r.Body == nil { + return "" + } + + _, params, err := mime.ParseMediaType(r.Header.Get("Content-Type")) + if err != nil { + return "" + } + boundary := params["boundary"] + if boundary == "" { + return "" + } + + // We only read a small amount of the body to prevent loading a huge file + // into memory just to find the token. The frontend prepends the token, + // so it will be within the first few kilobytes. + var buf bytes.Buffer + const maxPeekSize = 4096 + limitedBody := io.LimitReader(r.Body, maxPeekSize) + tee := io.TeeReader(limitedBody, &buf) + + mr := multipart.NewReader(tee, boundary) + + var token string + part, err := mr.NextPart() + if err == nil { + if part.FormName() == "csrf_token" { + if tokenBytes, err := io.ReadAll(part); err == nil { + token = string(tokenBytes) + } + } + } + + // Reconstruct the body so the downstream handler can read the whole thing. + // We combine the bytes we buffered with the unread remainder of the body. + r.Body = struct { + io.Reader + io.Closer + }{ + Reader: io.MultiReader(&buf, r.Body), + Closer: r.Body, + } + + return token +} + // denyCSRF rejects a request that failed the CSRF check. API/JSON callers get a // machine-readable 403; browser navigations get the styled forbidden page. func denyCSRF(appCtx *application_context.MahresourcesContext, w http.ResponseWriter, r *http.Request) { diff --git a/src/csrf.js b/src/csrf.js index 0ab12da9..5147d33b 100644 --- a/src/csrf.js +++ b/src/csrf.js @@ -4,10 +4,9 @@ * tag, to same-origin state-changing requests so the server can verify them: * * - fetch() requests get an `X-CSRF-Token` header. - * - native form submits get a hidden `csrf_token` field (urlencoded forms, - * where the token travels in the body) or a `csrf_token` query parameter - * (multipart upload forms, whose body the server cannot read without - * defeating the upload size limit). + * - native form submits get a hidden `csrf_token` field in the body. + * For multipart forms, this field is prepended to ensure the server can + * read it before processing potentially large file uploads. * * Entirely a no-op when auth is disabled: the meta tag renders empty, so nothing * is attached and behaviour matches the historical no-auth deployment. @@ -77,22 +76,22 @@ document.addEventListener( if (!isSameOrigin(action)) return; const enctype = (form.enctype || form.getAttribute('enctype') || '').toLowerCase(); - if (enctype === 'multipart/form-data') { - // The server does not read multipart bodies for the token (it would defeat - // the streaming upload size limit), so pass it as a query parameter. - const url = new URL(action, window.location.origin); - url.searchParams.set('csrf_token', token); - form.setAttribute('action', url.pathname + url.search + url.hash); - } else { - let input = form.querySelector('input[name="csrf_token"]'); - if (!input) { - input = document.createElement('input'); - input.type = 'hidden'; - input.name = 'csrf_token'; + + let input = form.querySelector('input[name="csrf_token"]'); + if (!input) { + input = document.createElement('input'); + input.type = 'hidden'; + input.name = 'csrf_token'; + + if (enctype === 'multipart/form-data') { + // For multipart, it must be the very first part so the server can read it + // quickly without parsing the entire file. + form.prepend(input); + } else { form.appendChild(input); } - input.value = token; } + input.value = token; }, true );