diff --git a/admin.go b/admin.go index 10524ac..068d980 100644 --- a/admin.go +++ b/admin.go @@ -590,17 +590,35 @@ const adminPageHTML = ` s.className = isErr ? "err" : ""; } + // showAuthGate switches to the key-entry view. message is OPTIONAL + // (review sweep, 2026-08-20): passed with a string, it replaces the + // auth-error text; passed as undefined (refresh's own no-stored-key + // branch), it leaves whatever error text is already there untouched — + // otherwise a 5s poll running while no key is stored (the state right + // after a 401 already showed "invalid key, or not an admin") would call + // showAuthGate() on every tick and silently wipe that message back to + // empty before the user ever reads it. function showAuthGate(message) { document.getElementById("dashboard").classList.add("hidden"); document.getElementById("auth-gate").classList.remove("hidden"); - document.getElementById("auth-error").textContent = message || ""; + if (message !== undefined) { + document.getElementById("auth-error").textContent = message; + } } function showDashboard() { document.getElementById("auth-gate").classList.add("hidden"); document.getElementById("dashboard").classList.remove("hidden"); + document.getElementById("auth-error").textContent = ""; } + // fetchJSON captures the key it was actually sent with (key) onto a + // thrown auth error, rather than reading getStoredKey() again at the + // catch site: refresh's own Promise.all can still have this request's + // 401 in flight after the user has already submitted a newer key + // (setStoredKey then a fresh refresh() call) — capturing at throw time + // lets the catch handler tell that late, stale-key failure apart from a + // genuine rejection of the key currently stored. function fetchJSON(url) { var key = getStoredKey(); var headers = key ? { "x-api-key": key } : {}; @@ -608,6 +626,7 @@ const adminPageHTML = ` if (resp.status === 401 || resp.status === 403) { var err = new Error("admin key rejected: HTTP " + resp.status); err.authFailed = true; + err.key = key; throw err; } if (!resp.ok) throw new Error(url + ": HTTP " + resp.status); @@ -617,7 +636,7 @@ const adminPageHTML = ` function refresh() { if (!getStoredKey()) { - showAuthGate(""); + showAuthGate(); return; } Promise.all([fetchJSON(OVERVIEW_URL), fetchJSON(USAGE_URL)]).then(function (results) { @@ -627,8 +646,15 @@ const adminPageHTML = ` setStatus("last updated " + new Date().toLocaleTimeString(), false); }).catch(function (err) { if (err.authFailed) { - clearStoredKey(); - showAuthGate("invalid key, or not an admin"); + // Guard against the late-401 race (review sweep, 2026-08-20): only + // clear the stored key and show the rejection when the FAILING + // request's own key still matches what is currently stored. A + // stale in-flight request for a key the user has since replaced + // must not clobber the newer key that may well be valid. + if (err.key === getStoredKey()) { + clearStoredKey(); + showAuthGate("invalid key, or not an admin"); + } return; } setStatus("refresh failed: " + err.message, true); @@ -641,6 +667,7 @@ const adminPageHTML = ` var val = input.value; input.value = ""; if (!val) return; + document.getElementById("auth-error").textContent = ""; setStoredKey(val); refresh(); }); diff --git a/admin_test.go b/admin_test.go index eedc22a..482a73c 100644 --- a/admin_test.go +++ b/admin_test.go @@ -465,6 +465,39 @@ func TestAdminPage_CSPHeaderAndFetchURLs(t *testing.T) { } } +// --- JSON routes: nosniff/no-store/CSP headers (review sweep, 2026-08-20) --- + +// TestAdmin_JSONSecurityHeaders proves both /admin/api/* routes carry +// X-Content-Type-Options: nosniff and Cache-Control: no-store alongside +// the same Content-Security-Policy the HTML page sends +// (setAdminJSONHeaders, admin.go) — folded review item, 2026-08-20 +// review, moved from a bare doc-comment claim to an assertion here. +func TestAdmin_JSONSecurityHeaders(t *testing.T) { + t.Parallel() + cfg := newAdminTestConfig() + h, _ := newAdminGatewayHandle(t, cfg) + + wantCSP := "default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; connect-src 'self'" + for _, p := range []string{adminOverviewPath, adminUsagePath} { + t.Run(p, func(t *testing.T) { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, adminRequest(http.MethodGet, p, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + if got := rec.Header().Get("X-Content-Type-Options"); got != "nosniff" { + t.Errorf("X-Content-Type-Options = %q, want %q", got, "nosniff") + } + if got := rec.Header().Get("Cache-Control"); got != "no-store" { + t.Errorf("Cache-Control = %q, want %q", got, "no-store") + } + if got := rec.Header().Get("Content-Security-Policy"); got != wantCSP { + t.Errorf("Content-Security-Policy = %q, want %q", got, wantCSP) + } + }) + } +} + // --- redaction: no secret literal, "apiKey", or key digest ever appears --- func TestAdmin_SecretRedaction(t *testing.T) { diff --git a/limits.go b/limits.go index ca33c30..2571435 100644 --- a/limits.go +++ b/limits.go @@ -742,8 +742,15 @@ func (l *limiter) currentUsage(scopes []limitScope) []scopeUsage { now := l.now() out := make([]scopeUsage, len(scopes)) for i, sc := range scopes { - vals, ok := l.storeGetMulti(usageWindowKeys(sc, now)) - if !ok { + keys := usageWindowKeys(sc, now) + vals, ok := l.storeGetMulti(keys) + // len(vals) != len(keys) is reachable only from a non-conforming + // counterStore implementation (a real respClient.getMulti and + // memoryStore.getMulti both always return one value per key) — + // guarded defensively so a future or test-only store's short + // slice reports storeDown instead of panicking on an + // out-of-range index below (review sweep, 2026-08-20). + if !ok || len(vals) != len(keys) { out[i] = scopeUsage{kind: sc.kind, id: sc.id, storeDown: true} continue } diff --git a/provider_openai.go b/provider_openai.go index 3678529..a2d18c7 100644 --- a/provider_openai.go +++ b/provider_openai.go @@ -319,11 +319,14 @@ func (a *openaiAdapter) audioSpeech(ctx context.Context, w http.ResponseWriter, } // audioTranscription implements providerAdapter: a native forward to -// {base}/v1/audio/transcriptions. body is the client's original raw -// multipart request, replayed unchanged (routes_media.go never rewrites -// it), and contentType is the client's original Content-Type header, -// forwarded verbatim so the upstream sees the same multipart boundary the -// client used. The response (JSON or plain text, depending on the +// {base}/v1/audio/transcriptions. body is the multipart request +// routes_media.go hands in — the client's original bytes when its "model" +// field already named the resolved upstream model id, or a rewritten +// body (a fresh boundary, every other part copied verbatim) when it did +// not, per rewriteMultipartModel's controller ruling — and contentType +// matches whichever body this call received, so the upstream always sees +// a Content-Type whose boundary parameter is consistent with what +// actually follows it. The response (JSON or plain text, depending on the // client's requested response_format) is copied through unchanged, the // same non-streaming path images.generations uses. func (a *openaiAdapter) audioTranscription(ctx context.Context, w http.ResponseWriter, body []byte, contentType string) (usage, error) { diff --git a/routes_media.go b/routes_media.go index 449fb57..7cdd370 100644 --- a/routes_media.go +++ b/routes_media.go @@ -8,7 +8,6 @@ import ( "mime" "mime/multipart" "net/http" - "strings" ) // Route paths for the three unified media endpoints (spec §3, v0.2): @@ -182,7 +181,7 @@ func (g *Gateway) handleAudioTranscriptions(w http.ResponseWriter, r *http.Reque writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "invalid Content-Type") return } - if !strings.HasPrefix(mediaType, "multipart/form-data") { + if mediaType != "multipart/form-data" { writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "Content-Type must be multipart/form-data") return } @@ -192,12 +191,23 @@ func (g *Gateway) handleAudioTranscriptions(w http.ResponseWriter, r *http.Reque return } + // The three failure shapes below are kept distinct rather than folded + // into one "model is required" 400 (review sweep, 2026-08-20): a + // duplicate "model" part is ambiguous, a body extractMultipartModel + // cannot even parse (a malformed or truncated tail) never reached a + // point where "model" could be evaluated at all, and only a + // successfully parsed body with no "model" part is actually missing + // one — each names its own actual problem instead of collapsing every + // case into the same misleading message. requestedModel, err := extractMultipartModel(body, boundary) switch { case errors.Is(err, errDuplicateModelField): writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "duplicate model field") return - case err != nil || requestedModel == "": + case err != nil: + writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "malformed multipart body") + return + case requestedModel == "": writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "model is required") return } @@ -319,6 +329,13 @@ func extractMultipartModel(body []byte, boundary string) (string, error) { func rewriteMultipartModel(body []byte, boundary, upstreamModel string) ([]byte, string, error) { mr := multipart.NewReader(bytes.NewReader(body), boundary) var buf bytes.Buffer + // The rebuilt body is at least as large as the original (same parts, + // same content, only the "model" value and the boundary differ) plus + // the fresh Writer's own per-part header/boundary overhead — growing + // buf to body's length up front, with headroom, avoids the repeated + // reallocate-and-copy growth bytes.Buffer would otherwise do one + // io.Copy at a time. + buf.Grow(len(body) + 512) mw := multipart.NewWriter(&buf) for { diff --git a/routes_media_test.go b/routes_media_test.go index 2df29fb..0890e42 100644 --- a/routes_media_test.go +++ b/routes_media_test.go @@ -552,6 +552,54 @@ func TestHandleAudioTranscriptions_ContentTypeNotFormData_400(t *testing.T) { } } +// TestHandleAudioTranscriptions_MalformedTail_400 proves a syntactically +// valid Content-Type and boundary, but a body whose closing boundary +// marker is truncated, is reported as "malformed multipart body" — a +// review-sweep fix (2026-08-20) distinct from "model is required": before +// it, any error extractMultipartModel returned that was not +// errDuplicateModelField collapsed into the same misleading +// "model is required" 400, even though the body never got far enough to +// evaluate whether a "model" part was present at all. +func TestHandleAudioTranscriptions_MalformedTail_400(t *testing.T) { + gw := newMediaTestGateway(t, newMediaTestConfig("http://127.0.0.1:1", "whisper-test")) + + var buf bytes.Buffer + mw := multipart.NewWriter(&buf) + fw, err := mw.CreateFormField("model") + if err != nil { + t.Fatalf("CreateFormField(model): %v", err) + } + if _, err := fw.Write([]byte("whisper-test")); err != nil { + t.Fatalf("write model field: %v", err) + } + if err := mw.Close(); err != nil { + t.Fatalf("close multipart writer: %v", err) + } + contentType := mw.FormDataContentType() + + // Cut the last 20 bytes off — well within the closing "--boundary--" + // marker for Go's own (60-character) generated boundary — so reading + // the "model" part's content runs past the truncated tail without ever + // finding a delimiter, instead of cleanly reaching io.EOF. + truncated := buf.Bytes()[:buf.Len()-20] + + req := httptest.NewRequest(http.MethodPost, audioTranscriptionsPath, bytes.NewReader(truncated)) + req.Header.Set("Content-Type", contentType) + req.Header.Set("Authorization", "Bearer sk-alice") + rec := httptest.NewRecorder() + gw.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400, body=%s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "malformed multipart body") { + t.Errorf("body = %q, want it to mention %q", rec.Body.String(), "malformed multipart body") + } + if strings.Contains(rec.Body.String(), "model is required") { + t.Errorf("body = %q, must not report the misleading %q for a body that never parsed far enough to know", rec.Body.String(), "model is required") + } +} + // TestHandleAudioTranscriptions_DuplicateModelField_400 proves a // multipart body carrying two "model" parts is rejected outright (400) // rather than silently resolving to whichever one extractMultipartModel