From 9c0ac1d60bc0c3cf7d51c7d058185112a1a9bb61 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 11 Jul 2026 23:46:23 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20gadfly=20review=20=E2=80=94=20?= =?UTF-8?q?filename=20sanitization,=20truncation=20guard,=20shared=20plumb?= =?UTF-8?q?ing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj --- provider/llamaswap/audio.go | 119 ++++++++++++++++++-------------- provider/llamaswap/health.go | 31 ++++----- provider/llamaswap/image.go | 80 +++++++++------------ provider/llamaswap/llamaswap.go | 53 +++++++++----- 4 files changed, 155 insertions(+), 128 deletions(-) diff --git a/provider/llamaswap/audio.go b/provider/llamaswap/audio.go index 2308680..77ffc6a 100644 --- a/provider/llamaswap/audio.go +++ b/provider/llamaswap/audio.go @@ -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 } diff --git a/provider/llamaswap/health.go b/provider/llamaswap/health.go index 50a679a..7c25253 100644 --- a/provider/llamaswap/health.go +++ b/provider/llamaswap/health.go @@ -5,6 +5,8 @@ import ( "fmt" "io" "net/http" + + "gitea.stevedudenhoeffer.com/steve/majordomo/llm" ) // Health reports whether the llama-swap instance is reachable (GET @@ -13,16 +15,17 @@ import ( // request will take (a cold model swap can still block for minutes). Bound // the probe with a short context deadline; the client has no timeout by // design. +// +// Failure taxonomy matches the rest of the package: transport failures wrap +// the raw net error; a reachable-but-unhealthy status comes back as +// *llm.APIError, so callers can errors.As-distinguish the two. func (p *Provider) Health(ctx context.Context) error { - if p.baseURL == "" { - return 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 err } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, p.baseURL+"/health", nil) + req, err := p.newRequest(ctx, http.MethodGet, "/health", "", nil) if err != nil { - return fmt.Errorf("llama-swap: build request: %w", err) - } - if p.token != "" { - req.Header.Set("Authorization", "Bearer "+p.token) + return err } resp, err := p.client.Do(req) if err != nil { @@ -31,15 +34,11 @@ func (p *Provider) Health(ctx context.Context) error { defer resp.Body.Close() _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 1<<10)) if resp.StatusCode/100 != 2 { - return &apiHealthError{status: resp.StatusCode} + return &llm.APIError{ + Provider: p.name, + Status: resp.StatusCode, + Message: "health endpoint returned a non-2xx status", + } } return nil } - -// apiHealthError distinguishes "reachable but unhealthy" from transport -// failure without pulling in the llm error taxonomy for a probe. -type apiHealthError struct{ status int } - -func (e *apiHealthError) Error() string { - return fmt.Sprintf("llama-swap: health endpoint returned status %d", e.status) -} diff --git a/provider/llamaswap/image.go b/provider/llamaswap/image.go index c5edd19..e10af4c 100644 --- a/provider/llamaswap/image.go +++ b/provider/llamaswap/image.go @@ -16,8 +16,8 @@ import ( // served by llama-swap (routed to a stable-diffusion.cpp upstream). The id is // passed through verbatim and selects which upstream llama-swap loads. func (p *Provider) ImageModel(id string, opts ...imagegen.ModelOption) (imagegen.Model, 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 } _ = imagegen.ApplyModelOptions(opts) return &imageModel{p: p, id: id}, nil @@ -53,32 +53,39 @@ type txt2imgResponse struct { Images []string `json:"images"` } +// sdWire validates the generation knobs shared by Generate and Edit and +// builds the common txt2img wire fields. verb labels validation errors. +func (m *imageModel) sdWire(verb, prompt, negativePrompt, sampler, size string, seed *int64, steps *int, cfgScale *float64, n int) (txt2imgRequest, error) { + if strings.TrimSpace(prompt) == "" { + return txt2imgRequest{}, fmt.Errorf("%w: image %s requires a prompt", llm.ErrUnsupported, verb) + } + if n < 0 { + return txt2imgRequest{}, fmt.Errorf("%w: image count N must be >= 0, got %d", llm.ErrUnsupported, n) + } + width, height, err := parseSize(size) + if err != nil { + return txt2imgRequest{}, fmt.Errorf("%w: %v", llm.ErrUnsupported, err) + } + return txt2imgRequest{ + Model: m.id, + Prompt: prompt, + NegativePrompt: negativePrompt, + Seed: seed, + Steps: steps, + CFGScale: cfgScale, + Width: width, + Height: height, + SampleMethod: sampler, + BatchCount: n, + }, nil +} + // Generate implements imagegen.Model via POST {base}/sdapi/v1/txt2img. func (m *imageModel) Generate(ctx context.Context, req imagegen.Request, opts ...imagegen.Option) (*imagegen.Result, error) { req = req.Apply(opts...) - if strings.TrimSpace(req.Prompt) == "" { - return nil, fmt.Errorf("%w: image generation requires a prompt", llm.ErrUnsupported) - } - if req.N < 0 { - return nil, fmt.Errorf("%w: image count N must be >= 0, got %d", llm.ErrUnsupported, req.N) - } - - width, height, err := parseSize(req.Size) + wire, err := m.sdWire("generation", req.Prompt, req.NegativePrompt, req.Sampler, req.Size, req.Seed, req.Steps, req.CFGScale, req.N) if err != nil { - return nil, fmt.Errorf("%w: %v", llm.ErrUnsupported, err) - } - - wire := txt2imgRequest{ - Model: m.id, - Prompt: req.Prompt, - NegativePrompt: req.NegativePrompt, - Seed: req.Seed, - Steps: req.Steps, - CFGScale: req.CFGScale, - Width: width, - Height: height, - SampleMethod: req.Sampler, - BatchCount: req.N, + return nil, err } var resp txt2imgResponse @@ -126,37 +133,18 @@ type img2imgRequest struct { // Edit implements imagegen.Editor via POST {base}/sdapi/v1/img2img. func (m *imageModel) Edit(ctx context.Context, req imagegen.EditRequest, opts ...imagegen.EditOption) (*imagegen.Result, error) { req = req.Apply(opts...) - if strings.TrimSpace(req.Prompt) == "" { - return nil, fmt.Errorf("%w: image edit requires a prompt", llm.ErrUnsupported) - } if len(req.Init.Data) == 0 { return nil, fmt.Errorf("%w: image edit requires an init image", llm.ErrUnsupported) } - if req.N < 0 { - return nil, fmt.Errorf("%w: image count N must be >= 0, got %d", llm.ErrUnsupported, req.N) - } if req.Strength != nil && (*req.Strength < 0 || *req.Strength > 1) { return nil, fmt.Errorf("%w: edit strength must be in [0,1], got %g", llm.ErrUnsupported, *req.Strength) } - - width, height, err := parseSize(req.Size) + base, err := m.sdWire("edit", req.Prompt, req.NegativePrompt, req.Sampler, req.Size, req.Seed, req.Steps, req.CFGScale, req.N) if err != nil { - return nil, fmt.Errorf("%w: %v", llm.ErrUnsupported, err) + return nil, err } - wire := img2imgRequest{ - txt2imgRequest: txt2imgRequest{ - Model: m.id, - Prompt: req.Prompt, - NegativePrompt: req.NegativePrompt, - Seed: req.Seed, - Steps: req.Steps, - CFGScale: req.CFGScale, - Width: width, - Height: height, - SampleMethod: req.Sampler, - BatchCount: req.N, - }, + txt2imgRequest: base, InitImages: []string{base64.StdEncoding.EncodeToString(req.Init.Data)}, DenoisingStrength: req.Strength, } diff --git a/provider/llamaswap/llamaswap.go b/provider/llamaswap/llamaswap.go index f973cf1..c175a44 100644 --- a/provider/llamaswap/llamaswap.go +++ b/provider/llamaswap/llamaswap.go @@ -9,9 +9,11 @@ // package adds beyond a bare OpenAI-compat endpoint is the "tailored" surface: // // - llama-swap management endpoints exposed as concrete methods — ListModels -// (GET /v1/models), Running (GET /running), Unload (POST /api/models/unload) -// — which have no place on the canonical llm.Provider interface; -// - image generation via the imagegen interface (see image.go); and +// (GET /v1/models), Running (GET /running), Unload (POST /api/models/unload), +// ListVoices (GET /v1/audio/voices), Health (GET /health) — which have no +// place on the canonical llm.Provider interface; +// - image generation + editing via the imagegen interfaces (see image.go); +// - speech synthesis + transcription via the audio interfaces (see audio.go); and // - swap-aware defaults: the HTTP client carries NO timeout, because the // first request to an unloaded model blocks while llama-swap spawns the // upstream (its healthCheckTimeout is at least 15s). Bound a call with a @@ -100,8 +102,8 @@ func (p *Provider) BaseURL() string { return p.baseURL } // endpoint, delegating to provider/openai. The id is passed through verbatim // and selects which upstream llama-swap loads. func (p *Provider) Model(id string, opts ...llm.ModelOption) (llm.Model, 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 } return p.chatProvider().Model(id, opts...) } @@ -178,7 +180,32 @@ func (p *Provider) Unload(ctx context.Context, model string) error { return p.doJSON(ctx, http.MethodPost, path, "", nil, nil) } -// --- shared HTTP helper for management + image endpoints --- +// --- shared HTTP helpers for management + image + audio endpoints --- + +// requireBaseURL is the shared guard for every entry point: construction +// never fails (see New), so a missing base URL surfaces here, at use time. +func (p *Provider) requireBaseURL() error { + if p.baseURL == "" { + return fmt.Errorf("llama-swap provider %q: no base URL configured (set one via WithBaseURL or an LLM_* env DSN)", p.name) + } + return nil +} + +// newRequest builds an authenticated request relative to baseURL. +// contentType is applied only when a body is present. +func (p *Provider) newRequest(ctx context.Context, method, path, contentType string, body io.Reader) (*http.Request, error) { + req, err := http.NewRequestWithContext(ctx, method, p.baseURL+path, 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 req, nil +} // doJSON performs a request to a llama-swap endpoint relative to baseURL, // optionally encoding body and decoding into out (either may be nil). model @@ -186,8 +213,8 @@ func (p *Provider) Unload(ctx context.Context, model string) error { // model-specific). Transport failures are wrapped raw so llm.Classify still // sees the underlying net error; non-2xx responses become *llm.APIError. func (p *Provider) doJSON(ctx context.Context, method, path, model string, body, out any) error { - if p.baseURL == "" { - return 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 err } var rdr io.Reader if body != nil { @@ -197,15 +224,9 @@ func (p *Provider) doJSON(ctx context.Context, method, path, model string, body, } rdr = bytes.NewReader(b) } - req, err := http.NewRequestWithContext(ctx, method, p.baseURL+path, rdr) + req, err := p.newRequest(ctx, method, path, "application/json", rdr) if err != nil { - return fmt.Errorf("llama-swap: build request: %w", err) - } - if body != nil { - req.Header.Set("Content-Type", "application/json") - } - if p.token != "" { - req.Header.Set("Authorization", "Bearer "+p.token) + return err } resp, err := p.client.Do(req) if err != nil {