From 31bccf6e195334e613e922fe5e34091effd157fd Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Thu, 16 Jul 2026 19:07:37 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20review=20findings=20=E2=80=94=20clone-ro?= =?UTF-8?q?ute=20audio=20sniffing,=20zip=20caps,=20filename=20sanitization?= =?UTF-8?q?,=20WAV=20response=20caps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - speakWithReference now validates the response IS audio via the same audioResultMIME sniff the sfx/enhance surfaces use — a 2xx JSON soft error or HTML proxy page was previously wrapped up as audio/wav bytes. - audioResultMIME moves to audio.go (next to speechMIME; it was defined in sfx.go but shared by enhance/clone) and learns the Ogg container normalization (application/ogg -> audio/ogg). - Stems zip unpack gains entry-count (16) and total-decompressed (1GB) caps on top of the existing per-entry cap — the per-entry bound alone still let a many-entry bomb multiply up. - sanitizeFilename drops NUL and both path separators too, so upload metadata can never smuggle directory structure to a file-writing shim. - New maxAudioResponseBytes (256MB) for bodies that ARE one audio clip (clone, enhance, sfx): a long WAV legitimately passes the 64MB JSON cap. - speakWithReference local renamed path -> upPath (naming parity with stems/enhance). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT --- provider/llamaswap/audio.go | 52 ++++++++++++++------ provider/llamaswap/clone_translate_test.go | 44 +++++++++++++++-- provider/llamaswap/enhance.go | 2 +- provider/llamaswap/llamaswap.go | 6 +++ provider/llamaswap/sfx.go | 20 +------- provider/llamaswap/stems.go | 19 +++++++ provider/llamaswap/stems_sfx_enhance_test.go | 23 +++++++++ 7 files changed, 128 insertions(+), 38 deletions(-) diff --git a/provider/llamaswap/audio.go b/provider/llamaswap/audio.go index 3372250..717a4ec 100644 --- a/provider/llamaswap/audio.go +++ b/provider/llamaswap/audio.go @@ -83,12 +83,12 @@ func (m *speechModel) Speak(ctx context.Context, req audio.SpeechRequest, opts . // speakWithReference performs zero-shot voice cloning via the upstream // passthrough clone route. The reference sample rides as the `voice_file` // part and the text as an `input` field; voice/format/speed still go on the -// wire when set (upstreams ignore fields they don't understand). The route's -// default container is WAV, so MIME resolution falls back to audio/wav (not -// the JSON route's mp3) when neither the response header nor the requested -// format decides. +// wire when set (upstreams ignore fields they don't understand). The result +// MIME comes from the response header or content sniffing (audioResultMIME, +// same validation as the sfx/enhance surfaces) — a JSON soft error or an +// HTML proxy page must never be wrapped up as audio bytes. func (m *speechModel) speakWithReference(ctx context.Context, req audio.SpeechRequest) (*audio.SpeechResult, error) { - path, err := upstreamPath(m.id, "/v1/audio/speech/upload") + upPath, err := upstreamPath(m.id, "/v1/audio/speech/upload") if err != nil { return nil, err } @@ -107,18 +107,17 @@ func (m *speechModel) speakWithReference(ctx context.Context, req audio.SpeechRe if err != nil { return nil, err } - audioBytes, contentType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, formType, body, maxResponseBytes) + audioBytes, contentType, err := m.p.doRaw(ctx, http.MethodPost, upPath, m.id, formType, body, maxAudioResponseBytes) if err != nil { return nil, err } if len(audioBytes) == 0 { return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: "speech clone response contained no audio"} } - mimeType := "audio/wav" - if mt := mimeFromContentType(contentType, "audio/"); mt != "" { - mimeType = mt - } else if req.Format != "" { - mimeType = speechMIME("", req.Format) + mimeType := audioResultMIME(contentType, audioBytes) + if mimeType == "" { + return nil, &llm.APIError{Provider: m.p.name, Model: m.id, + Message: fmt.Sprintf("speech clone response is not audio (Content-Type %q): %s", contentType, truncateForError(audioBytes))} } return &audio.SpeechResult{Audio: audioBytes, MIME: mimeType}, nil } @@ -144,6 +143,28 @@ func speechMIME(contentType, format string) string { } } +// audioResultMIME resolves an audio body's MIME type: the response +// Content-Type when it is a concrete audio type, else content sniffing +// (RIFF/WAVE, Ogg and friends), else "" — the caller treats undetectable as +// an upstream error, mirroring videoMIME. Sniffed "audio/wave" is normalized +// to the conventional "audio/wav", and Ogg's container type +// "application/ogg" to "audio/ogg". Shared by the clone, sfx, and enhance +// surfaces, which all answer raw audio bodies. +func audioResultMIME(contentType string, data []byte) string { + if mt := mimeFromContentType(contentType, "audio/"); mt != "" { + return mt + } + switch mt := http.DetectContentType(data); { + case mt == "audio/wave": + return "audio/wav" + case mt == "application/ogg": + return "audio/ogg" + case strings.HasPrefix(mt, "audio/"): + return mt + } + return "" +} + // TranscriptionModel implements audio.TranscriptionProvider, binding a // speech-to-text model served by llama-swap (routed to a whisper.cpp-style // upstream). @@ -241,10 +262,13 @@ func transcriptionFilename(filename, mimeType string) string { } // sanitizeFilename strips characters that would corrupt or inject into the -// multipart Content-Disposition header. Quotes and backslashes are escaped -// by mime/multipart itself; CR/LF are not — they must go. +// multipart Content-Disposition header, or smuggle directory structure to +// the receiving side. Quotes and backslashes are escaped by mime/multipart +// itself, but a file-writing shim decodes them right back — so CR/LF/NUL +// and both path separators are dropped: an upload-metadata filename must +// never traverse ("../x", "a/b", "C:\x"). func sanitizeFilename(name string) string { - name = strings.NewReplacer("\r", "", "\n", "").Replace(name) + name = strings.NewReplacer("\r", "", "\n", "", "\x00", "", "/", "", "\\", "").Replace(name) return strings.TrimSpace(name) } diff --git a/provider/llamaswap/clone_translate_test.go b/provider/llamaswap/clone_translate_test.go index 788e425..0917fdb 100644 --- a/provider/llamaswap/clone_translate_test.go +++ b/provider/llamaswap/clone_translate_test.go @@ -2,12 +2,14 @@ package llamaswap import ( "context" + "errors" "io" "net/http" "net/http/httptest" "testing" "gitea.stevedudenhoeffer.com/steve/majordomo/audio" + "gitea.stevedudenhoeffer.com/steve/majordomo/llm" ) func TestSpeakWithReferenceUsesCloneRoute(t *testing.T) { @@ -54,7 +56,7 @@ func TestSpeakWithReferenceUsesCloneRoute(t *testing.T) { } } -func TestSpeakWithReferenceDefaultsToWavMIME(t *testing.T) { +func TestSpeakWithReferenceSniffsHeaderlessWav(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if err := r.ParseMultipartForm(1 << 20); err != nil { t.Fatalf("parse multipart: %v", err) @@ -65,7 +67,7 @@ func TestSpeakWithReferenceDefaultsToWavMIME(t *testing.T) { } } w.Header()["Content-Type"] = nil // no declared type at all - _, _ = w.Write([]byte("RAWAUDIO")) + _, _ = w.Write(wavFixture()) })) defer srv.Close() @@ -77,9 +79,43 @@ func TestSpeakWithReferenceDefaultsToWavMIME(t *testing.T) { if err != nil { t.Fatalf("Speak: %v", err) } - // The clone route's default container is wav, not the JSON route's mp3. + // Headerless RIFF sniffs audio/wave, normalized to the conventional wav. if res.MIME != "audio/wav" { - t.Errorf("MIME = %q, want audio/wav fallback", res.MIME) + t.Errorf("MIME = %q, want sniffed audio/wav", res.MIME) + } +} + +func TestSpeakWithReferenceRejectsNonAudioResponse(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // A FastAPI-style 2xx soft error must not be wrapped up as audio. + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"detail":"reference audio too short"}`)) + })) + defer srv.Close() + + p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client())) + sm, _ := p.SpeechModel("chatterbox") + _, err := sm.Speak(context.Background(), + audio.SpeechRequest{Input: "hi"}, + audio.WithReferenceAudio([]byte("REF"), "audio/wav")) + var apiErr *llm.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("err = %v, want APIError for non-audio clone body", err) + } +} + +func TestSanitizeFilenameStripsPathAndControlBytes(t *testing.T) { + for in, want := range map[string]string{ + "song.mp3": "song.mp3", + "../../etc/passwd": "....etcpasswd", + "a\r\nContent-Type: evil": "aContent-Type: evil", + "..\\..\\boot.ini": "....boot.ini", + "nul\x00byte.wav": "nulbyte.wav", + " / ": "", + } { + if got := sanitizeFilename(in); got != want { + t.Errorf("sanitizeFilename(%q) = %q, want %q", in, got, want) + } } } diff --git a/provider/llamaswap/enhance.go b/provider/llamaswap/enhance.go index 9509430..69f8adf 100644 --- a/provider/llamaswap/enhance.go +++ b/provider/llamaswap/enhance.go @@ -45,7 +45,7 @@ func (m *speechEnhancerModel) Enhance(ctx context.Context, req audio.Enhancement if err != nil { return nil, err } - raw, respType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, contentType, body, maxResponseBytes) + raw, respType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, contentType, body, maxAudioResponseBytes) if err != nil { return nil, err } diff --git a/provider/llamaswap/llamaswap.go b/provider/llamaswap/llamaswap.go index 7713c02..1668117 100644 --- a/provider/llamaswap/llamaswap.go +++ b/provider/llamaswap/llamaswap.go @@ -53,6 +53,12 @@ const maxResponseBytes = 64 << 20 // can't allocate without limit. const maxVideoResponseBytes = 512 << 20 +// maxAudioResponseBytes caps bodies that ARE a single encoded audio clip +// (voice clone, speech enhancement, sfx): a long uncompressed WAV +// legitimately passes the 64MB JSON cap. Still bounded so a buggy upstream +// can't allocate without limit. +const maxAudioResponseBytes = 256 << 20 + // Provider is a llama-swap client. It satisfies llm.Provider (chat, delegated // to provider/openai) and imagegen.Provider (image generation), and exposes // llama-swap's management endpoints as concrete methods. diff --git a/provider/llamaswap/sfx.go b/provider/llamaswap/sfx.go index f0959e8..8c7b785 100644 --- a/provider/llamaswap/sfx.go +++ b/provider/llamaswap/sfx.go @@ -84,7 +84,7 @@ func (m *sfxModel) Generate(ctx context.Context, req musicgen.Request, opts ...m if err != nil { return nil, fmt.Errorf("llama-swap: encode sfx request: %w", err) } - raw, contentType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, "application/json", bytes.NewReader(encoded), maxResponseBytes) + raw, contentType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, "application/json", bytes.NewReader(encoded), maxAudioResponseBytes) if err != nil { return nil, err } @@ -98,21 +98,3 @@ func (m *sfxModel) Generate(ctx context.Context, req musicgen.Request, opts ...m } return &musicgen.Result{Audio: musicgen.Audio{Data: raw, MIME: mimeType}}, nil } - -// audioResultMIME resolves an audio body's MIME type: the response -// Content-Type when it is a concrete audio type, else content sniffing -// (RIFF/WAVE and friends), else "" — the caller treats undetectable as an -// upstream error, mirroring videoMIME. Sniffed "audio/wave" is normalized -// to the conventional "audio/wav". -func audioResultMIME(contentType string, data []byte) string { - if mt := mimeFromContentType(contentType, "audio/"); mt != "" { - return mt - } - if mt := http.DetectContentType(data); strings.HasPrefix(mt, "audio/") { - if mt == "audio/wave" { - return "audio/wav" - } - return mt - } - return "" -} diff --git a/provider/llamaswap/stems.go b/provider/llamaswap/stems.go index 3a7fbb3..d53fadf 100644 --- a/provider/llamaswap/stems.go +++ b/provider/llamaswap/stems.go @@ -31,6 +31,15 @@ const maxStemsResponseBytes = 512 << 20 // A single WAV stem of even a very long song stays far under this. const maxStemEntryBytes = 256 << 20 +// maxStemEntries caps how many stem entries are unpacked: Demucs emits at +// most six, so anything past a small multiple of that is a hostile or +// broken archive, not a result. +const maxStemEntries = 16 + +// maxStemsTotalBytes caps the AGGREGATE decompressed size across entries — +// the per-entry bound alone would still let a many-entry bomb multiply up. +const maxStemsTotalBytes = 1 << 30 + // StemSeparatorModel implements audio.StemSeparationProvider. The id selects // which upstream llama-swap loads. func (p *Provider) StemSeparatorModel(id string, opts ...audio.StemSeparatorModelOption) (audio.StemSeparator, error) { @@ -95,15 +104,25 @@ func (m *stemSeparatorModel) SeparateStems(ctx context.Context, req audio.StemSe Message: fmt.Sprintf("stems response is not a zip (Content-Type %q): %s", respType, truncateForError(raw))} } res := &audio.StemSeparationResult{} + var totalBytes int64 for _, f := range zr.File { if f.FileInfo().IsDir() { continue } + if len(res.Stems) >= maxStemEntries { + return nil, &llm.APIError{Provider: m.p.name, Model: m.id, + Message: fmt.Sprintf("stems zip holds more than %d entries", maxStemEntries)} + } data, err := readZipEntry(f) if err != nil { return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: fmt.Sprintf("stems zip entry %q: %v", f.Name, err)} } + totalBytes += int64(len(data)) + if totalBytes > maxStemsTotalBytes { + return nil, &llm.APIError{Provider: m.p.name, Model: m.id, + Message: fmt.Sprintf("stems zip decompresses past %d bytes", int64(maxStemsTotalBytes))} + } // Entry name → stem name; extension → MIME. Entries may sit in a // per-model directory ("htdemucs/vocals.mp3"), so use the base name. base := path.Base(f.Name) diff --git a/provider/llamaswap/stems_sfx_enhance_test.go b/provider/llamaswap/stems_sfx_enhance_test.go index 41e93bd..8e5a11a 100644 --- a/provider/llamaswap/stems_sfx_enhance_test.go +++ b/provider/llamaswap/stems_sfx_enhance_test.go @@ -6,8 +6,10 @@ import ( "context" "encoding/json" "errors" + "fmt" "net/http" "net/http/httptest" + "strings" "testing" "gitea.stevedudenhoeffer.com/steve/majordomo/audio" @@ -169,6 +171,27 @@ func TestSeparateStemsRejectsEmptyZip(t *testing.T) { } } +func TestSeparateStemsRejectsTooManyEntries(t *testing.T) { + entries := map[string]string{} + for i := 0; i <= maxStemEntries; i++ { + entries[fmt.Sprintf("stem%02d.wav", i)] = "X" + } + zipBody := stemsZipFixture(t, entries) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/zip") + _, _ = w.Write(zipBody) + })) + defer srv.Close() + + p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client())) + ss, _ := p.StemSeparatorModel("audioutils") + _, err := ss.SeparateStems(context.Background(), audio.StemSeparationRequest{Audio: []byte("SONG")}) + var apiErr *llm.APIError + if !errors.As(err, &apiErr) || !strings.Contains(apiErr.Message, "entries") { + t.Fatalf("err = %v, want APIError for over-long stems zip", err) + } +} + // wavFixture is a minimal RIFF/WAVE header so http.DetectContentType sniffs // audio/wave. func wavFixture() []byte {