fix: address gadfly review — filename sanitization, truncation guard, shared plumbing

- Transcribe: sanitize the caller-supplied multipart filename (CR/LF would inject Content-Disposition headers; upload metadata is untrusted), always send the required model/response_format fields, parse MIME parameters before extension matching, and give audio/opus its own .opus extension. - doRaw: a response larger than maxResponseBytes is now an error, not a silent truncation. - Shared plumbing: requireBaseURL() + newRequest() helpers replace the 7x-duplicated guard/error string and the triplicated request building across doJSON/doRaw/Health. - Health: non-2xx now returns *llm.APIError (package convention, programmatically distinguishable from transport failure) instead of a one-off unexported error type. - Speak: reject negative Speed; speechMIME no longer accepts video/* Content-Types. - image.go: Generate/Edit share one sdWire validate+map helper. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
2026-07-11 23:46:23 -04:00
parent 434d721b99
commit 9c0ac1d60b
4 changed files with 155 additions and 128 deletions
@@ -12,7 +12,7 @@ import (
 	"net/url"
 	"strings"

-	majaudio "gitea.stevedudenhoeffer.com/steve/majordomo/audio"
+	"gitea.stevedudenhoeffer.com/steve/majordomo/audio"
 	"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
 )

@@ -20,11 +20,11 @@ import (
 // served by llama-swap (routed to a kokoro/chatterbox-style OpenAI-compatible
 // upstream). The id is passed through verbatim and selects which upstream
 // llama-swap loads.
-func (p *Provider) SpeechModel(id string, opts ...majaudio.SpeechModelOption) (majaudio.SpeechModel, error) {
-	if p.baseURL == "" {
-		return nil, fmt.Errorf("llama-swap provider %q: no base URL configured (set one via WithBaseURL or an LLM_* env DSN)", p.name)
+func (p *Provider) SpeechModel(id string, opts ...audio.SpeechModelOption) (audio.SpeechModel, error) {
+	if err := p.requireBaseURL(); err != nil {
+		return nil, err
 	}
-	_ = majaudio.ApplySpeechModelOptions(opts)
+	_ = audio.ApplySpeechModelOptions(opts)
 	return &speechModel{p: p, id: id}, nil
 }

@@ -44,11 +44,14 @@ type speechRequest struct {
 }

 // Speak implements audio.SpeechModel via POST {base}/v1/audio/speech.
-func (m *speechModel) Speak(ctx context.Context, req majaudio.SpeechRequest, opts ...majaudio.SpeechOption) (*majaudio.SpeechResult, error) {
+func (m *speechModel) Speak(ctx context.Context, req audio.SpeechRequest, opts ...audio.SpeechOption) (*audio.SpeechResult, error) {
 	req = req.Apply(opts...)
 	if strings.TrimSpace(req.Input) == "" {
 		return nil, fmt.Errorf("%w: speech synthesis requires input text", llm.ErrUnsupported)
 	}
+	if req.Speed < 0 {
+		return nil, fmt.Errorf("%w: speech speed must be >= 0, got %g", llm.ErrUnsupported, req.Speed)
+	}
 	wire := speechRequest{
 		Model:          m.id,
 		Input:          req.Input,
@@ -67,21 +70,17 @@ func (m *speechModel) Speak(ctx context.Context, req majaudio.SpeechRequest, opt
 	if len(audioBytes) == 0 {
 		return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: "speech response contained no audio"}
 	}
-	return &majaudio.SpeechResult{Audio: audioBytes, MIME: speechMIME(contentType, req.Format)}, nil
+	return &audio.SpeechResult{Audio: audioBytes, MIME: speechMIME(contentType, req.Format)}, nil
 }

 // speechMIME resolves the result MIME type: the response Content-Type when it
 // is a concrete audio type, else a mapping from the requested format, else
 // audio/mpeg (the OpenAI endpoint's default container is mp3).
 func speechMIME(contentType, format string) string {
-	if mt, _, err := mime.ParseMediaType(contentType); err == nil {
-		if strings.HasPrefix(mt, "audio/") || strings.HasPrefix(mt, "video/") {
-			return mt
-		}
+	if mt, _, err := mime.ParseMediaType(contentType); err == nil && strings.HasPrefix(mt, "audio/") {
+		return mt
 	}
 	switch strings.ToLower(strings.TrimSpace(format)) {
-	case "", "mp3":
-		return "audio/mpeg"
 	case "wav":
 		return "audio/wav"
 	case "opus":
@@ -90,9 +89,7 @@ func speechMIME(contentType, format string) string {
 		return "audio/aac"
 	case "flac":
 		return "audio/flac"
-	case "pcm":
-		return "audio/pcm"
-	default:
+	default: // "", "mp3", and anything unrecognized
 		return "audio/mpeg"
 	}
 }
@@ -100,11 +97,11 @@ func speechMIME(contentType, format string) string {
 // TranscriptionModel implements audio.TranscriptionProvider, binding a
 // speech-to-text model served by llama-swap (routed to a whisper.cpp-style
 // upstream).
-func (p *Provider) TranscriptionModel(id string, opts ...majaudio.TranscriptionModelOption) (majaudio.TranscriptionModel, error) {
-	if p.baseURL == "" {
-		return nil, fmt.Errorf("llama-swap provider %q: no base URL configured (set one via WithBaseURL or an LLM_* env DSN)", p.name)
+func (p *Provider) TranscriptionModel(id string, opts ...audio.TranscriptionModelOption) (audio.TranscriptionModel, error) {
+	if err := p.requireBaseURL(); err != nil {
+		return nil, err
 	}
-	_ = majaudio.ApplyTranscriptionModelOptions(opts)
+	_ = audio.ApplyTranscriptionModelOptions(opts)
 	return &transcriptionModel{p: p, id: id}, nil
 }

@@ -116,7 +113,7 @@ type transcriptionModel struct {
 // Transcribe implements audio.TranscriptionModel via POST
 // {base}/v1/audio/transcriptions (multipart/form-data — llama-swap routes by
 // the `model` form field).
-func (m *transcriptionModel) Transcribe(ctx context.Context, req majaudio.TranscriptionRequest, opts ...majaudio.TranscriptionOption) (*majaudio.TranscriptionResult, error) {
+func (m *transcriptionModel) Transcribe(ctx context.Context, req audio.TranscriptionRequest, opts ...audio.TranscriptionOption) (*audio.TranscriptionResult, error) {
 	req = req.Apply(opts...)
 	if len(req.Audio) == 0 {
 		return nil, fmt.Errorf("%w: transcription requires audio bytes", llm.ErrUnsupported)
@@ -131,17 +128,22 @@ func (m *transcriptionModel) Transcribe(ctx context.Context, req majaudio.Transc
 	if _, err := fw.Write(req.Audio); err != nil {
 		return nil, fmt.Errorf("llama-swap: build transcription form: %w", err)
 	}
-	fields := map[string]string{
-		"model":           m.id,
-		"language":        req.Language,
-		"prompt":          req.Prompt,
-		"response_format": "json",
+	// Required fields always go on the wire (an empty model id should fail
+	// loudly upstream, not silently vanish); optional ones only when set.
+	fields := []struct {
+		key, value string
+		required   bool
+	}{
+		{"model", m.id, true},
+		{"response_format", "json", true},
+		{"language", req.Language, false},
+		{"prompt", req.Prompt, false},
 	}
-	for k, v := range fields {
-		if v == "" {
+	for _, f := range fields {
+		if !f.required && f.value == "" {
 			continue
 		}
-		if err := w.WriteField(k, v); err != nil {
+		if err := w.WriteField(f.key, f.value); err != nil {
 			return nil, fmt.Errorf("llama-swap: build transcription form: %w", err)
 		}
 	}
@@ -159,22 +161,31 @@ func (m *transcriptionModel) Transcribe(ctx context.Context, req majaudio.Transc
 	if err := json.Unmarshal(raw, &out); err != nil {
 		return nil, fmt.Errorf("llama-swap: decode transcription response: %w", err)
 	}
-	return &majaudio.TranscriptionResult{Text: out.Text, Raw: json.RawMessage(raw)}, nil
+	return &audio.TranscriptionResult{Text: out.Text, Raw: json.RawMessage(raw)}, nil
 }

-// transcriptionFilename picks the multipart filename hint: the caller's, else
-// one derived from the MIME subtype ("audio.mp3"), else "audio".
-func transcriptionFilename(req majaudio.TranscriptionRequest) string {
-	if req.Filename != "" {
-		return req.Filename
+// transcriptionFilename picks the multipart filename hint: the caller's
+// (sanitized — upload metadata is untrusted and CR/LF would inject multipart
+// headers), else one derived from the MIME subtype ("audio.mp3"), else
+// "audio". MIME parameters ("audio/ogg; codecs=opus") are stripped before
+// matching.
+func transcriptionFilename(req audio.TranscriptionRequest) string {
+	if name := sanitizeFilename(req.Filename); name != "" {
+		return name
 	}
-	switch strings.ToLower(req.MIME) {
+	mt := strings.ToLower(strings.TrimSpace(req.MIME))
+	if parsed, _, err := mime.ParseMediaType(mt); err == nil {
+		mt = parsed
+	}
+	switch mt {
 	case "audio/mpeg", "audio/mp3":
 		return "audio.mp3"
 	case "audio/wav", "audio/x-wav", "audio/wave":
 		return "audio.wav"
-	case "audio/ogg", "audio/opus":
+	case "audio/ogg":
 		return "audio.ogg"
+	case "audio/opus":
+		return "audio.opus"
 	case "audio/flac", "audio/x-flac":
 		return "audio.flac"
 	case "audio/mp4", "audio/m4a", "audio/x-m4a":
@@ -186,6 +197,14 @@ func transcriptionFilename(req majaudio.TranscriptionRequest) 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.
+func sanitizeFilename(name string) string {
+	name = strings.NewReplacer("\r", "", "\n", "").Replace(name)
+	return strings.TrimSpace(name)
+}
+
 // ListVoices returns the voices a TTS model offers (GET
 // /v1/audio/voices?model=...). The decode is tolerant: upstreams answer with
 // either a bare string list or a list of {id|name} objects.
@@ -221,7 +240,9 @@ func parseVoices(raw []byte) ([]string, error) {
 	if err := json.Unmarshal(list, &names); err == nil {
 		return names, nil
 	}
-	// A failed decode above may have partially populated names — start fresh.
+	// Not a plain string list. The failed decode above may have partially
+	// populated names (Unmarshal appends zero values before erroring on an
+	// element type mismatch) — start fresh for the object shape.
 	names = nil
 	var objs []struct {
 		ID      string `json:"id"`
@@ -247,20 +268,15 @@ func parseVoices(raw []byte) ([]string, error) {
 // doRaw performs a request to a llama-swap endpoint and returns the raw
 // response body and its Content-Type — the sibling of doJSON for endpoints
 // whose success payload is not JSON (audio bytes) or whose shape varies.
-// contentType sets the request Content-Type when body is non-nil.
+// contentType sets the request Content-Type when body is non-nil. A response
+// larger than maxResponseBytes is an error, never a silent truncation.
 func (p *Provider) doRaw(ctx context.Context, method, path, model, contentType string, body io.Reader) ([]byte, string, error) {
-	if p.baseURL == "" {
-		return nil, "", fmt.Errorf("llama-swap provider %q: no base URL configured (set one via WithBaseURL or an LLM_* env DSN)", p.name)
+	if err := p.requireBaseURL(); err != nil {
+		return nil, "", err
 	}
-	req, err := http.NewRequestWithContext(ctx, method, p.baseURL+path, body)
+	req, err := p.newRequest(ctx, method, path, contentType, body)
 	if err != nil {
-		return nil, "", fmt.Errorf("llama-swap: build request: %w", err)
-	}
-	if body != nil && contentType != "" {
-		req.Header.Set("Content-Type", contentType)
-	}
-	if p.token != "" {
-		req.Header.Set("Authorization", "Bearer "+p.token)
+		return nil, "", err
 	}
 	resp, err := p.client.Do(req)
 	if err != nil {
@@ -270,9 +286,12 @@ func (p *Provider) doRaw(ctx context.Context, method, path, model, contentType s
 	if resp.StatusCode/100 != 2 {
 		return nil, "", p.apiError(resp, model)
 	}
-	data, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))
+	data, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1))
 	if err != nil {
 		return nil, "", fmt.Errorf("llama-swap: read response: %w", err)
 	}
+	if len(data) > maxResponseBytes {
+		return nil, "", fmt.Errorf("llama-swap: response exceeds %d bytes", maxResponseBytes)
+	}
 	return data, resp.Header.Get("Content-Type"), nil
 }