From 2c8a32c77a42a45803b51ad82c011236e0184ce1 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 20 Aug 2026 19:03:54 +0100 Subject: [PATCH] fix(media): rewrite prefixed alias in multipart replay, strip alias key, multipart validation --- README.md | 14 +++- provider_openai.go | 10 +++ routes_media.go | 128 +++++++++++++++++++++++++---- routes_media_test.go | 192 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 328 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index d39da71..35d17a2 100644 --- a/README.md +++ b/README.md @@ -444,12 +444,22 @@ adapter call — minus response caching and token/cost accounting (below). |---|---|---|---| | `images/generations` | Native forward. | Translated to Imagen's `:predict` (below). | **501**. | | `audio/speech` | Native forward; the binary response is streamed to the client as it arrives. | **501** — no OpenAI-compatible text-to-speech endpoint. | **501**. | -| `audio/transcriptions` | Native forward; the client's original multipart body is replayed upstream unchanged. | **501** — no OpenAI-compatible speech-to-text endpoint. | **501**. | +| `audio/transcriptions` | Native forward; see below for how the multipart body is forwarded. | **501** — no OpenAI-compatible speech-to-text endpoint. | **501**. | - **Model routing**: `images/generations` and `audio/speech` read `model` from the JSON request body. `audio/transcriptions` reads it from the multipart request's `model` form field instead — the field can appear - in any position among the request's parts. + in any position among the request's parts, and a body with more than + one `model` field is a **400**. +- **Multipart forwarding**: when the client's `model` field already + equals the resolved upstream model id (a bare id, the common case), the + original multipart body is replayed byte-for-byte, unchanged. When it + does not — a provider-prefixed id (`openai/whisper-1`) or another alias + resolves to a different upstream model string — the body is rebuilt + with every other part copied verbatim and only the `model` part's value + rewritten, so the alias never reaches the real provider. The rebuilt + body uses a new multipart boundary; the client's exact boundary is not + preserved, only an equivalent body. - **Gemini image translation**: `{model, prompt, n, size, response_format}` maps to Imagen's `{instances:[{prompt}], parameters: {sampleCount, aspectRatio}}`. `size` maps to `aspectRatio`: diff --git a/provider_openai.go b/provider_openai.go index e5eb7e3..3678529 100644 --- a/provider_openai.go +++ b/provider_openai.go @@ -261,6 +261,16 @@ func (a *openaiAdapter) forwardStream(w http.ResponseWriter, body io.Reader, cli // cost-accounted (spec §3, v0.2), so this always returns a zero usage, // regardless of what the upstream response reports. func (a *openaiAdapter) imagesGeneration(ctx context.Context, w http.ResponseWriter, req map[string]any) (usage, error) { + // gatewayAliasKey must never reach the real provider, mirroring + // chatCompletion's identical delete above: routes_media.go never sets + // this key itself for a media route (its doc comment), but this + // adapter forwards req verbatim as the upstream wire body, so a + // client independently sending a literal "__alias" field of its own + // would otherwise leak straight through as an unrecognized request + // field. Unconditional, not conditional on whether the gateway set + // it — the same defensive posture chatCompletion/embeddings already + // take. + delete(req, gatewayAliasKey) resp, err := upstreamJSON(ctx, a.client, http.MethodPost, a.baseURL+"/v1/images/generations", a.requestHeaders(true), req, a.retry) if err != nil { return usage{}, err diff --git a/routes_media.go b/routes_media.go index ba0e7a6..aca661d 100644 --- a/routes_media.go +++ b/routes_media.go @@ -8,6 +8,7 @@ import ( "mime" "mime/multipart" "net/http" + "strings" ) // Route paths for the three unified media endpoints (spec §3, v0.2): @@ -142,13 +143,16 @@ func (g *Gateway) handleAudioSpeech(w http.ResponseWriter, r *http.Request, u *u // handleAudioTranscriptions implements POST /v1/audio/transcriptions // (spec §3): an openai-type provider forwards natively; gemini and -// anthropic both always answer 501. Unlike images/speech, the multipart -// request body is replayed upstream byte-for-byte, unchanged — spec §3's -// model routing only reads the "model" form field to pick a provider and -// check authorization; it never rewrites the field a client's own model -// alias occupies inside the multipart body, since editing one part of a -// multipart payload in place while preserving its exact boundary framing -// is unnecessary complexity this endpoint's contract does not require. +// anthropic both always answer 501. The multipart request body is +// replayed upstream unchanged only when the client's own "model" form +// field already equals the resolved upstream model id (the common case — +// a bare id); when it does not (a provider-prefixed or otherwise aliased +// id resolves to a different upstream model string), the body is +// rebuilt via rewriteMultipartModel so the alias never reaches the real +// provider, the same invariant every other unified route enforces +// (controller ruling, 2026-08-20, amends spec §3's original "REPLAY the +// full body upstream unchanged" — see rewriteMultipartModel's doc +// comment). func (g *Gateway) handleAudioTranscriptions(w http.ResponseWriter, r *http.Request, u *user, grp *group) { sw := &statusTrackingWriter{ResponseWriter: w} @@ -163,11 +167,15 @@ func (g *Gateway) handleAudioTranscriptions(w http.ResponseWriter, r *http.Reque } contentType := r.Header.Get("Content-Type") - _, params, err := mime.ParseMediaType(contentType) + mediaType, params, err := mime.ParseMediaType(contentType) if err != nil { writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "invalid Content-Type") return } + if !strings.HasPrefix(mediaType, "multipart/form-data") { + writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "Content-Type must be multipart/form-data") + return + } boundary := params["boundary"] if boundary == "" { writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "missing multipart boundary") @@ -175,17 +183,34 @@ func (g *Gateway) handleAudioTranscriptions(w http.ResponseWriter, r *http.Reque } requestedModel, err := extractMultipartModel(body, boundary) - if err != nil || requestedModel == "" { + switch { + case errors.Is(err, errDuplicateModelField): + writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "duplicate model field") + return + case err != nil || requestedModel == "": writeOAIError(sw, http.StatusBadRequest, "invalid_request_error", "model is required") return } - adapter, _, ok := g.resolveMediaRequest(sw, u, grp, requestedModel) + adapter, upstreamModel, ok := g.resolveMediaRequest(sw, u, grp, requestedModel) if !ok { return } - if _, err := adapter.audioTranscription(r.Context(), sw, body, contentType); err != nil { + uploadBody := body + uploadContentType := contentType + if upstreamModel != requestedModel { + rebuilt, rebuiltContentType, rerr := rewriteMultipartModel(body, boundary, upstreamModel) + if rerr != nil { + g.errorf("audio transcriptions: rewrite multipart model field: %v", rerr) + writeOAIError(sw, http.StatusInternalServerError, "server_error", "internal error") + return + } + uploadBody = rebuilt + uploadContentType = rebuiltContentType + } + + if _, err := adapter.audioTranscription(r.Context(), sw, uploadBody, uploadContentType); err != nil { g.handleAdapterError(sw, err, adapter.name()) } } @@ -210,22 +235,34 @@ func readCapped(r io.Reader, limit int64) (body []byte, oversize bool, err error return body, false, nil } +// errDuplicateModelField is extractMultipartModel's sentinel for a +// multipart body carrying more than one part named "model" — an +// ambiguous request a first-found parser and a last-found parser would +// resolve differently. handleAudioTranscriptions rejects it outright +// (400) rather than silently picking one. +var errDuplicateModelField = errors.New("multipart body has more than one model field") + // extractMultipartModel scans body — a full multipart/form-data payload // already read into memory (bounded by handleAudioTranscriptions' own // readCapped call before this is ever reached) — for its "model" form // field, returning the field's value. It reads over a fresh // bytes.NewReader(body), never consuming or mutating body itself, so the -// caller can replay the exact same bytes upstream afterward (spec §3). +// caller can replay or rewrite the exact same bytes afterward (spec §3). +// It always scans every part, never returning as soon as the first +// "model" part is found, so a second "model" part later in the body is +// caught as errDuplicateModelField instead of being silently ignored. // Parts are read in whatever order the client sent them — a "model" // field following a large audio-file part is found the same as one sent // first. Returns "" with a nil error when no part named "model" is // present; the caller treats an empty model the same as a missing one. func extractMultipartModel(body []byte, boundary string) (string, error) { mr := multipart.NewReader(bytes.NewReader(body), boundary) + found := false + var model string for { part, err := mr.NextPart() if errors.Is(err, io.EOF) { - return "", nil + return model, nil } if err != nil { return "", err @@ -234,11 +271,74 @@ func extractMultipartModel(body []byte, boundary string) (string, error) { _ = part.Close() //nolint:errcheck // read-side close; nothing actionable on failure continue } + if found { + _ = part.Close() //nolint:errcheck // read-side close; nothing actionable on failure + return "", errDuplicateModelField + } val, err := io.ReadAll(io.LimitReader(part, maxMultipartModelFieldBytes)) _ = part.Close() //nolint:errcheck // read-side close; nothing actionable on failure if err != nil { return "", err } - return string(val), nil + model = string(val) + found = true } } + +// rewriteMultipartModel rebuilds body — a multipart/form-data payload +// already validated to parse and to carry exactly one "model" field +// (extractMultipartModel) — with a fresh mime/multipart.Writer, copying +// every part verbatim (its original headers and content, in the +// client's original order) except the "model" part's value, which +// becomes upstreamModel. +// +// Controller ruling (2026-08-20, amends spec §3's original "REPLAY the +// full body upstream unchanged"): a client addressing a +// provider-prefixed or otherwise aliased model id must never have that +// alias reach the real upstream provider — the same invariant every +// other unified route already enforces by rewriting "model" before +// forwarding. handleAudioTranscriptions calls this only when the +// client's own model string differs from the resolved upstream model +// id; when they already match (the bare-id case), it replays the +// original bytes untouched instead (the fast path, no rebuild cost). +// +// The rebuilt body uses the Writer's own new boundary, not body's +// original one, and contentType is that Writer's own +// FormDataContentType() — byte-identical boundary preservation is not +// required, only a valid, equivalent multipart body. +func rewriteMultipartModel(body []byte, boundary, upstreamModel string) ([]byte, string, error) { + mr := multipart.NewReader(bytes.NewReader(body), boundary) + var buf bytes.Buffer + mw := multipart.NewWriter(&buf) + + for { + part, err := mr.NextPart() + if errors.Is(err, io.EOF) { + break + } + if err != nil { + return nil, "", err + } + + pw, err := mw.CreatePart(part.Header) + if err != nil { + _ = part.Close() //nolint:errcheck // read-side close; nothing actionable on failure + return nil, "", err + } + + if part.FormName() == "model" { + _, err = pw.Write([]byte(upstreamModel)) + } else { + _, err = io.Copy(pw, part) + } + _ = part.Close() //nolint:errcheck // read-side close; nothing actionable on failure + if err != nil { + return nil, "", err + } + } + + if err := mw.Close(); err != nil { + return nil, "", err + } + return buf.Bytes(), mw.FormDataContentType(), nil +} diff --git a/routes_media_test.go b/routes_media_test.go index e5a363f..dc1b7cb 100644 --- a/routes_media_test.go +++ b/routes_media_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "io" + "mime" "mime/multipart" "net/http" "net/http/httptest" @@ -165,6 +166,37 @@ func TestHandleImagesGenerations_OpenAI_HappyPath_NativeForward(t *testing.T) { } } +// TestHandleImagesGenerations_StripsClientSuppliedAliasKey proves a +// client independently including a literal "__alias" field in its own +// JSON body (never one the gateway itself injects for a media route — +// routes_media.go never sets gatewayAliasKey here) never reaches the +// upstream provider: openaiAdapter.imagesGeneration deletes it +// unconditionally, mirroring chatCompletion's identical delete. +func TestHandleImagesGenerations_StripsClientSuppliedAliasKey(t *testing.T) { + var gotBody map[string]any + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewDecoder(r.Body).Decode(&gotBody) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"created":1,"data":[]}`)) + })) + defer srv.Close() + + gw := newMediaTestGateway(t, newMediaTestConfig(srv.URL, "img-test")) + + body := map[string]any{"model": "img-test", "prompt": "a cat", gatewayAliasKey: "sneaky-client-value"} + req := newUnifiedRequest(t, http.MethodPost, imagesGenerationsPath, "sk-alice", body) + rec := httptest.NewRecorder() + gw.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + if _, leaked := gotBody[gatewayAliasKey]; leaked { + t.Errorf("upstream request body leaked client-supplied %q: %v", gatewayAliasKey, gotBody) + } +} + func TestHandleImagesGenerations_MissingModel_400(t *testing.T) { gw := newMediaTestGateway(t, newMediaTestConfig("http://127.0.0.1:1", "img-test")) @@ -383,6 +415,13 @@ func buildMultipartTranscription(t *testing.T, model string) (body []byte, conte return buf.Bytes(), mw.FormDataContentType() } +// TestHandleAudioTranscriptions_OpenAI_HappyPath_MultipartReplayed +// exercises the bare-id fast path: the client's own "model" field +// ("whisper-test") already equals the resolved upstream model id, so +// handleAudioTranscriptions replays the original multipart bytes +// byte-for-byte unchanged rather than calling rewriteMultipartModel — see +// TestHandleAudioTranscriptions_PrefixedAlias_RewritesModelFieldOnly for +// the alias case that does rewrite. func TestHandleAudioTranscriptions_OpenAI_HappyPath_MultipartReplayed(t *testing.T) { const respBody = `{"text":"hello world"}` var gotContentType string @@ -450,6 +489,159 @@ func TestHandleAudioTranscriptions_InvalidContentType_400(t *testing.T) { if rec.Code != http.StatusBadRequest { t.Fatalf("status = %d, want 400, body=%s", rec.Code, rec.Body.String()) } + if !strings.Contains(rec.Body.String(), "multipart/form-data") { + t.Errorf("body = %q, want it to mention %q", rec.Body.String(), "multipart/form-data") + } +} + +// TestHandleAudioTranscriptions_ContentTypeNotFormData_400 covers the +// case a plain "not multipart at all" Content-Type does not: a +// syntactically valid multipart Content-Type, with a real boundary +// parameter, whose subtype is not "form-data". Before the folded review +// fix, this fell through the boundary check (a boundary was present) and +// produced a misleading "model is required" 400 instead of naming the +// actual problem. +func TestHandleAudioTranscriptions_ContentTypeNotFormData_400(t *testing.T) { + gw := newMediaTestGateway(t, newMediaTestConfig("http://127.0.0.1:1", "whisper-test")) + + req := httptest.NewRequest(http.MethodPost, audioTranscriptionsPath, strings.NewReader("--xyz--\r\n")) + req.Header.Set("Content-Type", "multipart/mixed; boundary=xyz") + 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(), "multipart/form-data") { + t.Errorf("body = %q, want it to mention %q", rec.Body.String(), "multipart/form-data") + } +} + +// TestHandleAudioTranscriptions_DuplicateModelField_400 proves a +// multipart body carrying two "model" parts is rejected outright (400) +// rather than silently resolving to whichever one extractMultipartModel +// happened to see first. +func TestHandleAudioTranscriptions_DuplicateModelField_400(t *testing.T) { + gw := newMediaTestGateway(t, newMediaTestConfig("http://127.0.0.1:1", "whisper-test")) + + var buf bytes.Buffer + mw := multipart.NewWriter(&buf) + for _, v := range []string{"whisper-test", "whisper-other"} { + fw, err := mw.CreateFormField("model") + if err != nil { + t.Fatalf("CreateFormField: %v", err) + } + if _, err := fw.Write([]byte(v)); err != nil { + t.Fatalf("write model field: %v", err) + } + } + if err := mw.Close(); err != nil { + t.Fatalf("close multipart writer: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, audioTranscriptionsPath, &buf) + req.Header.Set("Content-Type", mw.FormDataContentType()) + 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(), "duplicate model field") { + t.Errorf("body = %q, want it to mention %q", rec.Body.String(), "duplicate model field") + } +} + +// TestHandleAudioTranscriptions_PrefixedAlias_RewritesModelFieldOnly +// proves the controller ruling (2026-08-20, amends spec §3): a +// provider-prefixed model id ("openai/whisper-test") must never reach +// the real upstream verbatim. rewriteMultipartModel rebuilds the +// multipart body with the bare upstream id in the "model" field, leaving +// every other part (here, the audio file) byte-for-byte unchanged. +func TestHandleAudioTranscriptions_PrefixedAlias_RewritesModelFieldOnly(t *testing.T) { + const fileContent = "FAKEAUDIOBYTES-DISTINCTIVE-PAYLOAD" + var gotContentType string + var gotBody []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotContentType = r.Header.Get("Content-Type") + gotBody, _ = io.ReadAll(r.Body) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"text":"ok"}`)) + })) + defer srv.Close() + + gw := newMediaTestGateway(t, newMediaTestConfig(srv.URL, "whisper-test")) + + var buf bytes.Buffer + mw := multipart.NewWriter(&buf) + ffw, err := mw.CreateFormFile("file", "audio.wav") + if err != nil { + t.Fatalf("CreateFormFile: %v", err) + } + if _, err = ffw.Write([]byte(fileContent)); err != nil { + t.Fatalf("write file field: %v", err) + } + mfw, err := mw.CreateFormField("model") + if err != nil { + t.Fatalf("CreateFormField(model): %v", err) + } + // "openai/whisper-test" is a provider-prefixed alias resolving to the + // same upstream model the bare form registers under. + if _, err = mfw.Write([]byte("openai/whisper-test")); err != nil { + t.Fatalf("write model field: %v", err) + } + if err = mw.Close(); err != nil { + t.Fatalf("close multipart writer: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, audioTranscriptionsPath, &buf) + req.Header.Set("Content-Type", mw.FormDataContentType()) + req.Header.Set("Authorization", "Bearer sk-alice") + rec := httptest.NewRecorder() + gw.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + _, params, err := mime.ParseMediaType(gotContentType) + if err != nil { + t.Fatalf("upstream Content-Type %q did not parse: %v", gotContentType, err) + } + mr := multipart.NewReader(bytes.NewReader(gotBody), params["boundary"]) + var gotModel string + var gotFile []byte + var sawModel, sawFile bool + for { + part, perr := mr.NextPart() + if perr == io.EOF { //nolint:errorlint // multipart.Reader.NextPart's own contract returns io.EOF bare + break + } + if perr != nil { + t.Fatalf("parse upstream multipart body: %v", perr) + } + switch part.FormName() { + case "model": + b, _ := io.ReadAll(part) + gotModel = string(b) + sawModel = true + case "file": + b, _ := io.ReadAll(part) + gotFile = b + sawFile = true + } + _ = part.Close() + } + + if !sawModel || gotModel != "whisper-test" { + t.Errorf("upstream model field = %q (present=%v), want the bare upstream id %q", gotModel, sawModel, "whisper-test") + } + if !sawFile || string(gotFile) != fileContent { + t.Errorf("upstream file part = %q (present=%v), want unchanged %q", gotFile, sawFile, fileContent) + } } // TestHandleAudioTranscriptions_OversizeBody_413 proves a multipart body