mirror of
https://github.com/lukaszraczylo/traefik-llmgateway.git
synced 2026-10-12 04:54:45 +00:00
fix: v0.2 review sweep
Controller-ruled small fixes accumulated from T3/T4 reviews: 1. routes_media.go: multipart Content-Type check is now an exact mediaType != "multipart/form-data" comparison (HasPrefix admitted "multipart/form-datax"); a malformed-tail multipart body (fails to parse at all) now reports "malformed multipart body", distinct from "model is required" (a body that parses fine but carries no model part) and "duplicate model field". 2. provider_openai.go: audioTranscription's doc comment no longer claims the body is always "replayed unchanged"/the boundary always "verbatim" — it now reflects the rewrite path routes_media.go already takes for an aliased model id. 3. rewriteMultipartModel: buf.Grow(len(body)+512) up front, avoiding repeated reallocate-and-copy growth while rebuilding an aliased multipart body. 4. limits.go currentUsage: guards len(vals) == len(keys) before indexing storeGetMulti's result, so a non-conforming counterStore's short slice reports storeDown instead of panicking. 5. admin.go dashboard JS: the 5s poll no longer wipes an existing auth-error message when no key is stored (only a successful load or a new submission clears it now); clearStoredKey on a 401/403 now only fires when the failing request's own key still matches what's currently stored, guarding against a late response for a key the user has since replaced. 6. Spec §4's duplicated /admin/api/overview bullet and the nosniff/no-store sentence's placement were already fixed upstream (docs/superpowers/specs/2026-08-20-llmgateway-v02-design.md, commit c02010d) — verified identical, nothing further to commit here. 7. Added TestAdmin_JSONSecurityHeaders (nosniff/no-store/CSP on both admin JSON APIs) and TestHandleAudioTranscriptions_MalformedTail_400 (covers fix 1's new malformed-body branch). go test ./... -count=1 -race, gofmt, vet, make yaegi-check, golangci-lint (incl. gosec) all pass.
This commit is contained in:
@@ -590,17 +590,35 @@ const adminPageHTML = `<!doctype html>
|
||||
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 = `<!doctype html>
|
||||
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 = `<!doctype html>
|
||||
|
||||
function refresh() {
|
||||
if (!getStoredKey()) {
|
||||
showAuthGate("");
|
||||
showAuthGate();
|
||||
return;
|
||||
}
|
||||
Promise.all([fetchJSON(OVERVIEW_URL), fetchJSON(USAGE_URL)]).then(function (results) {
|
||||
@@ -627,8 +646,15 @@ const adminPageHTML = `<!doctype html>
|
||||
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 = `<!doctype html>
|
||||
var val = input.value;
|
||||
input.value = "";
|
||||
if (!val) return;
|
||||
document.getElementById("auth-error").textContent = "";
|
||||
setStoredKey(val);
|
||||
refresh();
|
||||
});
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
+8
-5
@@ -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) {
|
||||
|
||||
+20
-3
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user