From 372bf826aa23c52a0db0da0a9d7fa32ff6ae2ced Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Fri, 31 Jul 2026 12:34:58 -0400 Subject: [PATCH] fix(llamaswap): a headerless non-image response was returned as a PNG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gadfly on #23, blocking, 2/2 agreement — and it is the exact defect this whole line of work has been about: a call that succeeds while handing back the wrong bytes. sniffImageMIME falls back to image/png when detection is inconclusive, and the guard only consulted Content-Type. A response with NO Content-Type therefore skipped the check entirely and was labelled a PNG. The shim answers JSON on a semantic miss (no face found in the source or target), which is precisely the body that would have sailed through as a successful image. The check now validates the BYTES — http.DetectContentType must say image/ — and the reported MIME prefers the server's own label only when that label is itself an image type. Break-checked by restoring the header-only condition, which fails the new test. Also from that review: - index is documented as ignored under all=true, so a negative one is no longer rejected there; it is still rejected when it would actually be sent, and both halves are tested. - initImageFilename (video.go) was imageFilename with the base fixed to "frame" and now delegates to it — two copies of one extension table is how they drift. - DetectedFace carried Width/Height alongside Box, two sources of truth for one fact that can disagree after any transform. Now a Size() method derived from Box. - a dead `apiErr` in the test (declared, then `_ = apiErr`) was an abandoned errors.As check; it is wired up and now asserts callers can classify the error. - swapImg duplicated editInit verbatim; removed. Not taken: adding a FaceSwapProvider/ModelOption surface to match the other optional imagegen capabilities (single-model finding). There are no options to carry yet, and inventing an empty option type to look symmetrical would be API surface with nothing behind it. Worth revisiting when a real knob exists. --- imagegen/faceswap.go | 10 ++-- provider/llamaswap/faceswap.go | 40 +++++++++------ provider/llamaswap/faceswap_test.go | 79 +++++++++++++++++++++++------ provider/llamaswap/video.go | 14 +---- 4 files changed, 96 insertions(+), 47 deletions(-) diff --git a/imagegen/faceswap.go b/imagegen/faceswap.go index 635863f..d7b09e8 100644 --- a/imagegen/faceswap.go +++ b/imagegen/faceswap.go @@ -55,9 +55,13 @@ type DetectedFace struct { Box [4]int // Score is the detector's confidence, 0-1. Score float64 - // Width and Height are the box dimensions, carried so a caller can pick - // "the big face" without recomputing them. - Width, Height int +} + +// Size returns the box dimensions. Derived rather than stored: carrying +// width/height alongside Box is two sources of truth for one fact, and the +// pair can disagree after any transform. +func (f DetectedFace) Size() (w, h int) { + return f.Box[2] - f.Box[0], f.Box[3] - f.Box[1] } // FaceSwapper is the optional face-transfer surface. Separate interface so diff --git a/provider/llamaswap/faceswap.go b/provider/llamaswap/faceswap.go index 787d609..a69fc9a 100644 --- a/provider/llamaswap/faceswap.go +++ b/provider/llamaswap/faceswap.go @@ -74,7 +74,7 @@ func (m *faceSwapModel) ListFaces(ctx context.Context, img imagegen.Image) ([]im } out := make([]imagegen.DetectedFace, 0, len(parsed.Faces)) for _, f := range parsed.Faces { - df := imagegen.DetectedFace{Index: f.Index, Score: f.Score, Width: f.Width, Height: f.Height} + df := imagegen.DetectedFace{Index: f.Index, Score: f.Score} // A short box would silently index out of range below; treat a // malformed entry as a protocol error rather than zero-filling it, // because a wrong box sends the caller at the wrong face. @@ -97,7 +97,10 @@ func (m *faceSwapModel) FaceSwap(ctx context.Context, req imagegen.FaceSwapReque if len(req.Source.Data) == 0 { return nil, fmt.Errorf("%w: face swap requires a source image", llm.ErrUnsupported) } - if req.Index != nil && *req.Index < 0 { + // Only when it will actually be sent: under All the index is documented + // as ignored, so rejecting a negative one there would fail a request that + // is perfectly well formed. + if !req.All && req.Index != nil && *req.Index < 0 { return nil, fmt.Errorf("%w: face index must be >= 0, got %d", llm.ErrUnsupported, *req.Index) } path, err := upstreamPath(m.id, "/v1/faceswap") @@ -130,20 +133,34 @@ func (m *faceSwapModel) FaceSwap(ctx context.Context, req imagegen.FaceSwapReque if len(raw) == 0 { return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: "face swap response contained no image"} } - mime := sniffImageMIME(raw) - if respType != "" && !isImageContentType(respType) { - // A JSON error body sniffs as text, not image — say what came back - // rather than handing the caller bytes that are not a picture. + // Validate the BYTES, not the header. sniffImageMIME falls back to + // image/png when detection is inconclusive, so trusting it here would + // label a JSON error body as a PNG and return it as a successful image — + // and a header check alone misses the case where the response carries no + // Content-Type at all. The shim answers JSON on a semantic miss (no face + // found), which is exactly the body that would sail through. + detected := http.DetectContentType(raw) + if !strings.HasPrefix(detected, "image/") { return nil, &llm.APIError{Provider: m.p.name, Model: m.id, - Message: fmt.Sprintf("face swap response is not an image (Content-Type %q): %s", respType, truncateForError(raw))} + Message: fmt.Sprintf("face swap response is not an image (sniffed %q, Content-Type %q): %s", + detected, respType, truncateForError(raw))} } - return &imagegen.Result{Images: []llm.ImagePart{{MIME: mime, Data: raw}}}, nil + // Prefer the server's own label when it is an image type (it knows + // subtypes the sniffer does not), else what the bytes actually are. + mimeType := detected + if hdr := mimeFromContentType(respType, "image/"); hdr != "" { + mimeType = hdr + } + return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil } // imageFilename picks a multipart filename for an image part. The shim reads // bytes, not names, but a plausible extension keeps server-side sniffing and // request logs honest. base distinguishes the parts of a multi-file form // ("target"/"source") so a log line says which one was malformed. +// +// initImageFilename (video.go) is this function with base fixed to "frame" +// and delegates here — two copies of one extension table is how they drift. func imageFilename(mimeType, base string) string { if base == "" { base = "image" @@ -167,10 +184,3 @@ func imageFilename(mimeType, base string) string { return base + ".png" } } - -// isImageContentType reports whether a response Content-Type is an image. -// Used to tell a returned picture apart from a JSON error body, which would -// otherwise be handed back as "image" bytes. -func isImageContentType(contentType string) bool { - return mimeFromContentType(contentType, "image/") != "" -} diff --git a/provider/llamaswap/faceswap_test.go b/provider/llamaswap/faceswap_test.go index 9f3d227..ca41de9 100644 --- a/provider/llamaswap/faceswap_test.go +++ b/provider/llamaswap/faceswap_test.go @@ -3,6 +3,7 @@ package llamaswap import ( "context" "encoding/base64" + "errors" "io" "mime" "mime/multipart" @@ -15,15 +16,6 @@ import ( "gitea.stevedudenhoeffer.com/steve/majordomo/llm" ) -func swapImg(t *testing.T) imagegen.Image { - t.Helper() - raw, err := base64.StdEncoding.DecodeString(onePixelPNG) - if err != nil { - t.Fatalf("decode fixture: %v", err) - } - return imagegen.Image{MIME: "image/png", Data: raw} -} - // parseParts pulls the multipart form a handler received. func parseParts(t *testing.T, r *http.Request) (files map[string][]byte, fields map[string]string) { t.Helper() @@ -73,7 +65,7 @@ func TestFaceSwapSendsBothFiles(t *testing.T) { if err != nil { t.Fatalf("model: %v", err) } - img := swapImg(t) + img := editInit(t) res, err := m.FaceSwap(context.Background(), imagegen.FaceSwapRequest{Target: img, Source: img}, imagegen.WithFaceIndex(2)) if err != nil { @@ -111,7 +103,7 @@ func TestFaceSwapAllSuppressesIndex(t *testing.T) { p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client())) m, _ := p.FaceSwapModel("faceswap") - img := swapImg(t) + img := editInit(t) if _, err := m.FaceSwap(context.Background(), imagegen.FaceSwapRequest{Target: img, Source: img, Index: new(int), All: true}); err != nil { t.Fatalf("faceswap: %v", err) @@ -129,7 +121,7 @@ func TestFaceSwapAllSuppressesIndex(t *testing.T) { func TestFaceSwapRejectsMissingImages(t *testing.T) { p := New(WithBaseURL("http://example.invalid")) m, _ := p.FaceSwapModel("faceswap") - img := swapImg(t) + img := editInit(t) for _, tc := range []struct { name string req imagegen.FaceSwapRequest @@ -157,16 +149,18 @@ func TestFaceSwapRejectsNonImageResponse(t *testing.T) { p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client())) m, _ := p.FaceSwapModel("faceswap") - img := swapImg(t) + img := editInit(t) _, err := m.FaceSwap(context.Background(), imagegen.FaceSwapRequest{Target: img, Source: img}) if err == nil { t.Fatal("a JSON body was accepted as an image") } var apiErr *llm.APIError + if !errors.As(err, &apiErr) { + t.Errorf("err = %T, want *llm.APIError so callers can classify it", err) + } if !strings.Contains(err.Error(), "no_face_in_source") { t.Errorf("err = %v, want it to relay the shim's reason", err) } - _ = apiErr } // TestListFacesParsesOrdering: the shim's left-to-right index is the contract @@ -185,7 +179,7 @@ func TestListFacesParsesOrdering(t *testing.T) { p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client())) m, _ := p.FaceSwapModel("faceswap") - faces, err := m.ListFaces(context.Background(), swapImg(t)) + faces, err := m.ListFaces(context.Background(), editInit(t)) if err != nil { t.Fatalf("list: %v", err) } @@ -208,7 +202,60 @@ func TestListFacesRejectsShortBox(t *testing.T) { p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client())) m, _ := p.FaceSwapModel("faceswap") - if _, err := m.ListFaces(context.Background(), swapImg(t)); err == nil { + if _, err := m.ListFaces(context.Background(), editInit(t)); err == nil { t.Fatal("a 2-element box was accepted") } } + +// TestFaceSwapRejectsHeaderlessNonImage is the regression for gadfly's +// blocking finding on #23, agreed by both models. sniffImageMIME falls back +// to image/png when detection is inconclusive, and the original guard only +// looked at Content-Type — so a JSON error body sent WITHOUT a Content-Type +// header was labelled a PNG and returned as a successful image. The shim +// answers JSON on a semantic miss, which is precisely the body that would +// have sailed through. +func TestFaceSwapRejectsHeaderlessNonImage(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // Explicitly no Content-Type — Go only sets one if we write before + // deleting it, so clear it to model a bare upstream response. + w.Header()["Content-Type"] = nil + _, _ = w.Write([]byte(`{"detail":{"error":"no_face_in_target"}}`)) + })) + defer srv.Close() + + p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client())) + m, _ := p.FaceSwapModel("faceswap") + img := editInit(t) + _, err := m.FaceSwap(context.Background(), imagegen.FaceSwapRequest{Target: img, Source: img}) + if err == nil { + t.Fatal("a headerless JSON body was accepted and would have been returned as image/png") + } + if !strings.Contains(err.Error(), "no_face_in_target") { + t.Errorf("err = %v, want it to relay what actually came back", err) + } +} + +// TestFaceSwapAllowsNegativeIndexUnderAll: index is documented as ignored +// when all=true, so validating it there would reject a well-formed request. +func TestFaceSwapAllowsNegativeIndexUnderAll(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "image/png") + raw, _ := base64.StdEncoding.DecodeString(onePixelPNG) + _, _ = w.Write(raw) + })) + defer srv.Close() + + p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client())) + m, _ := p.FaceSwapModel("faceswap") + img := editInit(t) + neg := -1 + if _, err := m.FaceSwap(context.Background(), + imagegen.FaceSwapRequest{Target: img, Source: img, Index: &neg, All: true}); err != nil { + t.Fatalf("negative index rejected under all=true, where it is ignored: %v", err) + } + // ...but still rejected when it WOULD be sent. + if _, err := m.FaceSwap(context.Background(), + imagegen.FaceSwapRequest{Target: img, Source: img, Index: &neg}); err == nil { + t.Error("negative index accepted when it would actually be sent") + } +} diff --git a/provider/llamaswap/video.go b/provider/llamaswap/video.go index 1586a6e..23f21ef 100644 --- a/provider/llamaswap/video.go +++ b/provider/llamaswap/video.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "fmt" - "mime" "mime/multipart" "net/http" "strconv" @@ -139,18 +138,7 @@ func singleVideoResult(provider, model, verb string, raw []byte, contentType str // frame from its MIME subtype. The name is provider-chosen (never // caller-supplied), so no sanitization is needed. func initImageFilename(mimeType string) string { - mt := strings.ToLower(strings.TrimSpace(mimeType)) - if parsed, _, err := mime.ParseMediaType(mt); err == nil { - mt = parsed - } - switch mt { - case "image/jpeg", "image/jpg": - return "frame.jpg" - case "image/webp": - return "frame.webp" - default: // unknown MIME — PNG is the safe hint - return "frame.png" - } + return imageFilename(mimeType, "frame") } // formatInt renders an optional int pointer for a form field; nil = "" (omit).