diff --git a/README.md b/README.md index e8e8df4..49c74bb 100644 --- a/README.md +++ b/README.md @@ -190,9 +190,12 @@ path under `/v1/` answers 200, in maintenance mode too. the cache and the upstream fetch; 403 when the upstream host, or a host it redirects to, is `localhost`, ends in `.localhost` or `.local`, or has an address in a blocked network (see `blocked_networks`); 429 over - `image_requests_per_minute` (below); 502 when the upstream answered with an - error status, and for 5 minutes after that for the same source URL; 503 when - pixa is busy or in maintenance mode; 500 for any other failure. + `image_requests_per_minute` (below); 415 when the upstream answers with a + `Content-Type` that is not one of the source formats (below), or with an image + whose first bytes libvips reads as another format; 502 when the upstream + answered with an error status, and for 5 minutes after that for the same + source URL; 503 when pixa is busy or in maintenance mode; 500 for any other + failure. - `GET` or `HEAD` `/v1/e//` — an image through an encrypted URL (see Encrypted URLs). Needs: nothing but the URL. Answers: 200; 304 when `If-None-Match` matches the image's `ETag`; 400 for a token that does not @@ -200,7 +203,7 @@ path under `/v1/` answers 200, in maintenance mode too. expired; 504 when the upstream has not sent its response headers within `upstream_fetch_timeout`, but 500 when that time runs out while the image itself is still arriving; 400 for an `Accept` header that is not valid, and - 406, 403, 429, 502, 503 and 500, as for `/v1/image/`. + 406, 403, 415, 429, 502, 503 and 500, as for `/v1/image/`. - `GET /robots.txt` — asks every crawler to stay away (`Disallow: /`). Needs: nothing. Answers: 200. - `GET /.well-known/healthcheck.json` — JSON with `status` (`ok`), `now`, @@ -260,9 +263,11 @@ once, is refused with 400. both optional (values under Signature Specification). Both are part of what is cached, so each value of either is a separate cached image. -The source image may be JPEG, PNG, GIF, WebP, AVIF, JPEG XL or SVG. The -upstream's `Content-Type` must name its format, and the image's first bytes must -match it. +The source image may be JPEG, PNG, GIF, WebP, AVIF or JPEG XL. The upstream's +`Content-Type` must name its format, and the image's first bytes must match it. +An upstream answering with any other `Content-Type`, SVG included, is refused +with 415, and so is an image whose first bytes libvips reads as another format, +such as an SVG whose first bytes pass the check for AVIF. With the format `auto`, pixa chooses the format for each request from its `Accept` header, in this order: diff --git a/TODO.md b/TODO.md index 66067f4..f239092 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,22 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-13 SVG sources are refused, and `orig` is never saved as JPEG in place + of the source's own format (closes #68): the upstream fetch no longer accepts + `image/svg+xml`, which libvips would render. Both image routes answer 415 for + an upstream `Content-Type` that is not one of the source formats, SVG + included, where they answered 500. Before decoding, the image processor + refuses with `ErrUnsupportedInputFormat`, also answered 415, any input that + govips' `DetermineImageType`, which picks the libvips loader from the first + bytes, does not name as a source format. That catches an SVG whose first bytes + pass the first-bytes check for another type, as an XML comment can make them + do for AVIF; an SVG whose first bytes do not pass that check answers 500, as + before, and so does a damaged image in a source format, whose first bytes + `DetermineImageType` names. `orig` is always the source's own format, where a + source the processor could not name was saved as JPEG. `README.md` no longer + lists SVG. The SVG detection in `internal/magic` stays: the request path does + not call it, and it goes with the unused code of + https://git.eeqj.de/sneak/pixa/issues/168. - 2026-10-12 per-client limit on the image routes (closes #236): the new setting `image_requests_per_minute` (`PIXA_IMAGE_REQUESTS_PER_MINUTE`), default 600, is how many `GET` and `HEAD` requests one client may send to `/v1/image/` and diff --git a/internal/handlers/image.go b/internal/handlers/image.go index 848a252..9978565 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -247,6 +247,14 @@ func (s *Handlers) respondImageError( return } + if errors.Is(err, httpfetcher.ErrInvalidContentType) || + errors.Is(err, imageprocessor.ErrUnsupportedInputFormat) { + s.respondError(w, "unsupported source format", + http.StatusUnsupportedMediaType) + + return + } + s.respondError(w, "internal error", http.StatusInternalServerError) } diff --git a/internal/handlers/imageenc.go b/internal/handlers/imageenc.go index bf79054..816e83f 100644 --- a/internal/handlers/imageenc.go +++ b/internal/handlers/imageenc.go @@ -161,6 +161,10 @@ func (s *Handlers) handleImageError(w http.ResponseWriter, err error) { errors.Is(err, imageprocessor.ErrTooManyImages): s.respondError(w, "server busy, try again later", http.StatusServiceUnavailable) + case errors.Is(err, httpfetcher.ErrInvalidContentType), + errors.Is(err, imageprocessor.ErrUnsupportedInputFormat): + s.respondError(w, "unsupported source format", + http.StatusUnsupportedMediaType) default: s.log.Error("image request failed", "error", err) s.respondError(w, "internal error", http.StatusInternalServerError) diff --git a/internal/handlers/unsupported_source_internal_test.go b/internal/handlers/unsupported_source_internal_test.go new file mode 100644 index 0000000..a10e37e --- /dev/null +++ b/internal/handlers/unsupported_source_internal_test.go @@ -0,0 +1,54 @@ +package handlers + +import ( + "fmt" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "sneak.berlin/go/pixa/internal/httpfetcher" + "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// TestUnsupportedSourceFormatAnswers415 checks that both image routes answer +// 415 with a clear error when the upstream answers with a content type pixa +// does not accept, such as SVG, and when orig is asked of a source in a +// format the processor cannot name, wrapped as the service wraps them. +func TestUnsupportedSourceFormatAnswers415(t *testing.T) { + t.Parallel() + + h := &Handlers{log: slog.New(slog.DiscardHandler)} + req := &imgcache.ImageRequest{SourceHost: "img.example.com", SourcePath: "/a.svg"} + + for _, err := range []error{ + fmt.Errorf("upstream fetch failed: %w: image/svg+xml", + httpfetcher.ErrInvalidContentType), + fmt.Errorf("image processing failed: %w: unknown", + imageprocessor.ErrUnsupportedInputFormat), + } { + plain := httptest.NewRecorder() + h.respondImageError(plain, req, err) + + encrypted := httptest.NewRecorder() + h.handleImageError(encrypted, err) + + for route, rec := range map[string]*httptest.ResponseRecorder{ + "HandleImage": plain, "HandleImageEnc": encrypted, + } { + t.Logf("%s for %v: %d %s", route, err, rec.Code, rec.Body.String()) + + if rec.Code != http.StatusUnsupportedMediaType { + t.Errorf("%s for %v: status = %d, want %d", + route, err, rec.Code, http.StatusUnsupportedMediaType) + } + + if !strings.Contains(rec.Body.String(), "unsupported source format") { + t.Errorf("%s for %v: body = %q, want the unsupported source format error", + route, err, rec.Body.String()) + } + } + } +} diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index dd37489..9d09295 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/internal/httpfetcher/httpfetcher.go @@ -158,7 +158,6 @@ func DefaultConfig() *Config { contentTypeWebP, contentTypeAVIF, contentTypeJXL, - contentTypeSVG, }, AllowHTTP: false, MaxConnectionsPerHost: DefaultMaxConnectionsPerHost, diff --git a/internal/imageprocessor/imageprocessor.go b/internal/imageprocessor/imageprocessor.go index 6d59b34..9f08387 100644 --- a/internal/imageprocessor/imageprocessor.go +++ b/internal/imageprocessor/imageprocessor.go @@ -137,6 +137,12 @@ var ErrInputDataTooLarge = errors.New("input data exceeds maximum allowed size") // not supported. var ErrUnsupportedOutputFormat = errors.New("unsupported output format") +// ErrUnsupportedInputFormat is returned, before the input is decoded, when +// libvips reads the input's first bytes as another format than the source +// formats, or as none: for example an SVG whose first bytes pass the +// first-bytes check for AVIF. +var ErrUnsupportedInputFormat = errors.New("unsupported input format") + // ErrTooManyImages is returned when MaxConcurrentProcessing images are being // processed and none finishes within ProcessingWaitTimeout. var ErrTooManyImages = errors.New("too many images being processed at once") @@ -219,6 +225,17 @@ func (p *ImageProcessor) Process( return nil, ErrInputDataTooLarge } + // govips picks the libvips loader from the input's first bytes with + // DetermineImageType. Input whose first bytes it reads as no source + // format, such as SVG, which librsvg would render, is refused before + // libvips reads it. + inputFormat := p.detectFormat(vips.DetermineImageType(data)) + + sourceFormat, err := p.formatFromString(inputFormat) + if err != nil { + return nil, err + } + // Decode image img, err := vips.NewImageFromBuffer(data) if err != nil { @@ -237,9 +254,6 @@ func (p *ImageProcessor) Process( origWidth := img.Width() origHeight := img.Height() - // Detect input format - inputFormat := p.detectFormat(img) - // Validate input dimensions to prevent DoS via decompression bombs if origWidth > MaxInputDimension || origHeight > MaxInputDimension { return nil, ErrInputTooLarge @@ -259,7 +273,7 @@ func (p *ImageProcessor) Process( // orig is the source's own format; encode refuses an empty format outputFormat := req.Format if outputFormat == FormatOriginal { - outputFormat = p.formatFromString(inputFormat) + outputFormat = sourceFormat } // Encode to target format @@ -403,10 +417,9 @@ func (p *ImageProcessor) acquireSlot(ctx context.Context) (func(), error) { } } -// detectFormat returns the format string from a vips image. -func (p *ImageProcessor) detectFormat(img *vips.ImageRef) string { - format := img.Format() - switch format { +// detectFormat returns the format string for a govips image type. +func (p *ImageProcessor) detectFormat(imageType vips.ImageType) string { + switch imageType { case vips.ImageTypeJPEG: return "jpeg" case vips.ImageTypePNG: @@ -687,22 +700,23 @@ func jxlDistance(quality int) float64 { return 53.0/3000.0*q*q - 23.0/20.0*q + 25.0 } -// formatFromString converts a format string to Format. -func (p *ImageProcessor) formatFromString(format string) Format { +// formatFromString converts a format string from detectFormat to Format. A +// format detectFormat could not name is ErrUnsupportedInputFormat. +func (p *ImageProcessor) formatFromString(format string) (Format, error) { switch format { case "jpeg": - return FormatJPEG + return FormatJPEG, nil case "png": - return FormatPNG + return FormatPNG, nil case "gif": - return FormatGIF + return FormatGIF, nil case "webp": - return FormatWebP + return FormatWebP, nil case string(FormatAVIF): - return FormatAVIF + return FormatAVIF, nil case string(FormatJXL): - return FormatJXL + return FormatJXL, nil default: - return FormatJPEG + return "", fmt.Errorf("%w: %s", ErrUnsupportedInputFormat, format) } } diff --git a/internal/imageprocessor/orig_format_internal_test.go b/internal/imageprocessor/orig_format_internal_test.go new file mode 100644 index 0000000..453d176 --- /dev/null +++ b/internal/imageprocessor/orig_format_internal_test.go @@ -0,0 +1,39 @@ +package imageprocessor + +import ( + "bytes" + "errors" + "testing" + + "github.com/davidbyttow/govips/v2/vips" +) + +// TestImageProcessor_OrigOfUnnamedFormatRefused verifies that orig of a TIFF +// input, which is not a source format, is refused with +// ErrUnsupportedInputFormat before it is decoded, and not saved as JPEG. +func TestImageProcessor_OrigOfUnnamedFormatRefused(t *testing.T) { + t.Parallel() + + img, err := vips.NewImageFromBuffer(createTestPNG(t, 20, 20)) + if err != nil { + t.Fatalf("failed to decode test PNG: %v", err) + } + + t.Cleanup(img.Close) + + tiff, _, err := img.ExportTiff(vips.NewTiffExportParams()) + if err != nil { + t.Fatalf("failed to save test TIFF: %v", err) + } + + result, err := New(Params{}).Process( + t.Context(), bytes.NewReader(tiff), &Request{Format: FormatOriginal}, + ) + if result != nil { + t.Logf("Process() returned %s", result.ContentType) + } + + if !errors.Is(err, ErrUnsupportedInputFormat) { + t.Errorf("Process() error = %v, want %v", err, ErrUnsupportedInputFormat) + } +} diff --git a/internal/imageprocessor/source_formats_internal_test.go b/internal/imageprocessor/source_formats_internal_test.go new file mode 100644 index 0000000..5f1dbbd --- /dev/null +++ b/internal/imageprocessor/source_formats_internal_test.go @@ -0,0 +1,46 @@ +package imageprocessor + +import ( + "bytes" + "io" + "testing" +) + +// TestImageProcessor_LoadsEachSourceFormat verifies that an image in each of +// the six source formats, saved by the processor itself, is loaded and saved +// again in its own format (orig). +func TestImageProcessor_LoadsEachSourceFormat(t *testing.T) { + t.Parallel() + + proc := New(Params{}) + + for _, format := range []Format{ + FormatJPEG, FormatPNG, FormatGIF, FormatWebP, FormatAVIF, FormatJXL, + } { + saved, err := proc.Process(t.Context(), + bytes.NewReader(createTestPNG(t, 20, 20)), &Request{Format: format}) + if err != nil { + t.Fatalf("saving the test image as %s: %v", format, err) + } + + source, err := io.ReadAll(saved.Content) + if err != nil { + t.Fatalf("reading the %s image: %v", format, err) + } + + result, err := proc.Process(t.Context(), bytes.NewReader(source), + &Request{Format: FormatOriginal}) + if err != nil { + t.Errorf("orig of the %s image: %v", format, err) + + continue + } + + t.Logf("orig of the %s image: %s", format, result.ContentType) + + if result.ContentType != FormatToMIME(format) { + t.Errorf("orig of the %s image is %s, want %s", + format, result.ContentType, FormatToMIME(format)) + } + } +} diff --git a/internal/server/svg_as_avif_internal_test.go b/internal/server/svg_as_avif_internal_test.go new file mode 100644 index 0000000..cd05947 --- /dev/null +++ b/internal/server/svg_as_avif_internal_test.go @@ -0,0 +1,71 @@ +package server + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// svgServedAsAVIF is an SVG whose first bytes pass the first-bytes check for +// AVIF: its XML comment puts "ftypavis" at bytes 4 to 11, where an AVIF file +// has its ftyp box and brand. libvips would load it as SVG. +const svgServedAsAVIF = `` + +// TestImageProxyRefusesSVGServedAsAVIF requests svgServedAsAVIF, which the +// upstream answers as image/avif, through both image routes, resized as PNG +// and in its own format (orig). Each request is answered 415, as for an SVG +// source, never with an image. +func TestImageProxyRefusesSVGServedAsAVIF(t *testing.T) { + t.Parallel() + + upstream := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "image/avif") + _, _ = io.WriteString(w, svgServedAsAVIF) + })) + t.Cleanup(upstream.Close) + + s, _, _ := startImageProxy(t, upstream) + + generator, err := encurl.NewGenerator(testSigningKey) + if err != nil { + t.Fatalf("encurl.NewGenerator: %v", err) + } + + encryptedURL := func(format imgcache.ImageFormat) string { + token, err := generator.Generate(&encurl.Payload{ + SourceHost: upstreamHost, SourcePath: "/drawing.avif", + Width: 32, Height: 24, Format: format, + }) + if err != nil { + t.Fatalf("encurl Generate: %v", err) + } + + return "/v1/e/" + token + "/drawing." + string(format) + } + + for _, target := range []string{ + "/v1/image/" + upstreamHost + "/drawing.avif/32x24.png", + "/v1/image/" + upstreamHost + "/drawing.avif/64x48.orig", + encryptedURL(imgcache.FormatPNG), + encryptedURL(imgcache.FormatOriginal), + } { + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, target, nil)) + t.Logf("GET %s: %d, Content-Type %s, body %q", + target, rec.Code, rec.Header().Get("Content-Type"), rec.Body.String()) + + if rec.Code != http.StatusUnsupportedMediaType || + !strings.Contains(rec.Body.String(), "unsupported source format") { + t.Errorf("GET %s: %d %q, want %d unsupported source format", + target, rec.Code, rec.Body.String(), http.StatusUnsupportedMediaType) + } + } +} diff --git a/internal/server/svg_internal_test.go b/internal/server/svg_internal_test.go new file mode 100644 index 0000000..95b46fa --- /dev/null +++ b/internal/server/svg_internal_test.go @@ -0,0 +1,41 @@ +package server + +import ( + "io" + "net/http" + "net/http/httptest" + "testing" +) + +// TestImageProxyRefusesSVG requests an image the upstream answers as SVG, +// in its own format (orig) and resized as PNG, through pixa's router, +// upstream fetcher and image processor. pixa does not accept SVG as a +// source, so each request is answered 415, never with an image. +func TestImageProxyRefusesSVG(t *testing.T) { + t.Parallel() + + upstream := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "image/svg+xml") + _, _ = io.WriteString(w, ``) + })) + t.Cleanup(upstream.Close) + + s, _, _ := startImageProxy(t, upstream) + + for _, sizeFormat := range []string{"64x48.orig", "32x24.png"} { + target := "/v1/image/" + upstreamHost + "/drawing.svg/" + sizeFormat + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, target, nil)) + t.Logf("GET %s: %d, Content-Type %s, body %s", + target, rec.Code, rec.Header().Get("Content-Type"), rec.Body.String()) + + if rec.Code != http.StatusUnsupportedMediaType { + t.Errorf("GET %s status = %d, want %d", + target, rec.Code, http.StatusUnsupportedMediaType) + } + } +}