diff --git a/README.md b/README.md index 5fb6a49..f1bed6e 100644 --- a/README.md +++ b/README.md @@ -198,9 +198,11 @@ path under `/v1/` answers 200, in maintenance mode too. 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); 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 + `Content-Type` that is not one of the source formats (below), with a body + whose first bytes are not those of the format its `Content-Type` names, 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, or after + a body whose first bytes are not those of its `Content-Type`, 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 @@ -279,8 +281,12 @@ above 8192. 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 415. So is a body whose first bytes are not those of the format its +`Content-Type` names, such as a JPEG sent as `image/png`, text sent as an image, +or an empty body. pixa then does not fetch that source URL again for 5 minutes, +and answers 502 for it meanwhile, as after an upstream error status. An image +whose first bytes libvips reads as another format, such as an SVG whose first +bytes pass the check for AVIF, is refused with 415 too. 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 7793ef4..496df89 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,15 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-15 a source whose first bytes do not match its `Content-Type` answers + 415 (closes #253): a body whose first bytes are not those of the format its + `Content-Type` names, such as a JPEG sent as `image/png`, text, or a body + under 12 bytes, answered 500 on both image routes and was logged as an error + of pixa's, and each request for it fetched the whole body again. Both routes + now answer 415, and the image service logs it at warning level as a refused + source. It also gets a negative cache entry, as an upstream error status does, + so the source URL is not fetched again for 5 minutes and answers 502 + meanwhile. - 2026-10-15 a `:` in the source path or query no longer moves where the path ends and the query begins (closes #247): the signed data and the cache key joined host, path and query with `:` and no escaping, so a URL signed for diff --git a/internal/handlers/first_bytes_internal_test.go b/internal/handlers/first_bytes_internal_test.go new file mode 100644 index 0000000..fb3ba30 --- /dev/null +++ b/internal/handlers/first_bytes_internal_test.go @@ -0,0 +1,180 @@ +package handlers + +import ( + "bytes" + "encoding/json" + "image/color" + "log/slog" + "net/http" + "slices" + "testing" + "testing/fstest" + "time" + + "github.com/go-chi/chi/v5" + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/httpfetcher" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// mismatchedSourcePath is the path of the source that +// TestFirstBytesNotMatchingContentType serves on allowlistedHost. The mock +// fetcher names a file's Content-Type from its extension, so the source is +// sent as image/png. +const mismatchedSourcePath = "/images/source.png" + +// newRoutesFetchingThrough returns both image routes, and the Handlers behind +// them, whose image service fetches through fetcher and has allowlistedHost on +// its allowlist. The handlers and the image service log to log. +func newRoutesFetchingThrough( + t *testing.T, fetcher httpfetcher.Fetcher, log *slog.Logger, +) (*Handlers, http.Handler) { + t.Helper() + + cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{ + StateDir: t.TempDir(), + NegativeTTL: 5 * time.Minute, + }) + if err != nil { + t.Fatalf("imgcache.NewCache() error = %v", err) + } + + svc, err := imgcache.NewService(&imgcache.ServiceConfig{ + Cache: cache, + Fetcher: fetcher, + SigningKey: testSigningKey, + Allowlist: []string{allowlistedHost}, + Logger: log, + }) + if err != nil { + t.Fatalf("imgcache.NewService() error = %v", err) + } + + encGen, err := encurl.NewGenerator(testSigningKey) + if err != nil { + t.Fatalf("encurl.NewGenerator() error = %v", err) + } + + h := &Handlers{log: log, imgSvc: svc, encGen: encGen} + + r := chi.NewRouter() + r.Get("/v1/image/*", h.HandleImage()) + r.Get("/v1/e/{token}/*", h.HandleImageEnc()) + + return h, r +} + +// mismatchedSourcePlainURL returns the /v1/image/ URL of mismatchedSourcePath +// on allowlistedHost at 50x50 as a JPEG. +func mismatchedSourcePlainURL(*testing.T, *Handlers) string { + return "/v1/image/" + allowlistedHost + mismatchedSourcePath + "/50x50.jpeg" +} + +// mismatchedSourceEncURL returns an encrypted /v1/e/ URL, made with h's +// generator, of mismatchedSourcePath on allowlistedHost at 50x50 as a JPEG. +func mismatchedSourceEncURL(t *testing.T, h *Handlers) string { + t.Helper() + + token, err := h.encGen.Generate(&encurl.Payload{ + SourceHost: allowlistedHost, + SourcePath: mismatchedSourcePath, + Width: 50, + Height: 50, + Format: imgcache.FormatJPEG, + }) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + + return "/v1/e/" + token + "/img.jpg" +} + +// loggedLevels reads the JSON log lines in logs and returns the messages +// logged at each level. +func loggedLevels(t *testing.T, logs *bytes.Buffer) map[string][]string { + t.Helper() + + t.Logf("logged:\n%s", logs.String()) + + messages := make(map[string][]string) + + dec := json.NewDecoder(logs) + for dec.More() { + var line struct { + Level string `json:"level"` + Msg string `json:"msg"` + } + + err := dec.Decode(&line) + if err != nil { + t.Fatalf("decoding log line: %v", err) + } + + messages[line.Level] = append(messages[line.Level], line.Msg) + } + + return messages +} + +// TestFirstBytesNotMatchingContentType checks that both image routes answer +// 415 for an upstream body whose first bytes are not those of the format its +// Content-Type names, and log it as a refused source, not as an error of +// pixa's. The next request for that source must answer 502 without fetching +// it again, as after an upstream error status. +func TestFirstBytesNotMatchingContentType(t *testing.T) { + t.Parallel() + + bodies := map[string][]byte{ + "JPEG sent as image/png": generateTestJPEG(t, 10, 10, color.Black), + "text sent as image/png": []byte("This is just text, not a PNG"), + "body too short to name a format": []byte("short"), + } + + // The source's URL on each image route. + sourceURLs := map[string]func(*testing.T, *Handlers) string{ + "plain URL": mismatchedSourcePlainURL, + "encrypted URL": mismatchedSourceEncURL, + } + + for bodyName, body := range bodies { + for urlName, sourceURL := range sourceURLs { + t.Run(urlName+", "+bodyName, func(t *testing.T) { + t.Parallel() + + fetcher := &countingFetcher{ + Fetcher: httpfetcher.NewMock(fstest.MapFS{ + allowlistedHost + mismatchedSourcePath: &fstest.MapFile{ + Data: body, + }, + }), + } + + var logs bytes.Buffer + + h, routes := newRoutesFetchingThrough(t, fetcher, + slog.New(slog.NewJSONHandler(&logs, nil))) + target := sourceURL(t, h) + + checkErrorBody(t, sendGet(t, routes, target), + http.StatusUnsupportedMediaType, "unsupported source format") + + levels := loggedLevels(t, &logs) + if len(levels[slog.LevelError.String()]) != 0 { + t.Errorf("logged at error level: %q", + levels[slog.LevelError.String()]) + } + + if !slices.Contains(levels[slog.LevelWarn.String()], "source refused") { + t.Error(`no "source refused" line logged at warning level`) + } + + checkErrorBody(t, sendGet(t, routes, target), + http.StatusBadGateway, "upstream error") + + if n := fetcher.fetches.Load(); n != 1 { + t.Errorf("upstream fetched %d times, want 1", n) + } + }) + } + } +} diff --git a/internal/handlers/image.go b/internal/handlers/image.go index aefde8e..5cbda60 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -15,6 +15,7 @@ import ( "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/magic" ) // HandleImage handles the main image proxy route: @@ -220,6 +221,18 @@ func (s *Handlers) HandleTooManyRequests() http.HandlerFunc { func (s *Handlers) respondImageError( w http.ResponseWriter, req *imgcache.ImageRequest, err error, ) { + // An upstream body whose first bytes are not those of the format its + // Content-Type names is the upstream's error, not pixa's: the image + // service has logged it as a refused source. + if errors.Is(err, magic.ErrMagicByteMismatch) || + errors.Is(err, magic.ErrUnknownFormat) || + errors.Is(err, magic.ErrNotEnoughData) { + s.respondError(w, "unsupported source format", + http.StatusUnsupportedMediaType) + + return + } + s.log.Error("failed to get image", "host", req.SourceHost, "path", req.SourcePath, diff --git a/internal/handlers/imageenc.go b/internal/handlers/imageenc.go index 8da6d47..8925e93 100644 --- a/internal/handlers/imageenc.go +++ b/internal/handlers/imageenc.go @@ -15,6 +15,7 @@ import ( "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/magic" ) // HandleImageEnc handles requests to /v1/e/{token}/* for encrypted @@ -162,7 +163,10 @@ func (s *Handlers) handleImageError(w http.ResponseWriter, err error) { s.respondError(w, "server busy, try again later", http.StatusServiceUnavailable) case errors.Is(err, httpfetcher.ErrInvalidContentType), - errors.Is(err, imageprocessor.ErrUnsupportedInputFormat): + errors.Is(err, imageprocessor.ErrUnsupportedInputFormat), + errors.Is(err, magic.ErrMagicByteMismatch), + errors.Is(err, magic.ErrUnknownFormat), + errors.Is(err, magic.ErrNotEnoughData): s.respondError(w, "unsupported source format", http.StatusUnsupportedMediaType) case errors.Is(err, imageprocessor.ErrOutputTooLarge): diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index 52684a9..26a012f 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -438,13 +438,7 @@ func (s *Service) fetchAndProcess( if err != nil { // Store negative cache for certain errors if isNegativeCacheable(err) { - statusCode := extractStatusCode(err) - - storeErr := s.cache.StoreNegative(ctx, req, statusCode, err.Error()) - if storeErr != nil { - s.log.Warn("failed to store negative cache entry", - "host", req.SourceHost, "path", req.SourcePath, "error", storeErr) - } + s.storeNegative(ctx, req, extractStatusCode(err), err) } return nil, fmt.Errorf("upstream fetch failed: %w", err) @@ -490,9 +484,16 @@ func (s *Service) fetchAndProcess( "cipher", fetchResult.TLSCipherSuite, ) - // Validate magic bytes match content type + // Validate magic bytes match content type. A body that does not is the + // upstream's error, so it gets a negative cache entry, as an upstream + // error status does. err = magic.ValidateMagicBytes(sourceData, fetchResult.ContentType) if err != nil { + s.log.Warn("source refused", "request_id", middleware.GetReqID(ctx), + "host", req.SourceHost, "path", req.SourcePath, + "content_type", fetchResult.ContentType, "error", err) + s.storeNegative(ctx, req, httpStatusUnsupportedMediaType, err) + return nil, fmt.Errorf("content validation failed: %w", err) } @@ -508,6 +509,19 @@ func (s *Service) fetchAndProcess( ) } +// storeNegative stores a negative cache entry for req's source, whose fetch +// failed with err, so the source is not fetched again until the entry +// expires. A failure to store the entry is only logged. +func (s *Service) storeNegative( + ctx context.Context, req *ImageRequest, statusCode int, err error, +) { + storeErr := s.cache.StoreNegative(ctx, req, statusCode, err.Error()) + if storeErr != nil { + s.log.Warn("failed to store negative cache entry", + "host", req.SourceHost, "path", req.SourcePath, "error", storeErr) + } +} + // processedVariant is a variant as processAndStore made it. Each request that // shared its processing serves it through a reader of its own. type processedVariant struct { @@ -616,8 +630,9 @@ func signatureRequest(req *ImageRequest) *signature.Request { // HTTP status codes for error responses. const ( - httpStatusBadGateway = 502 - httpStatusInternalError = 500 + httpStatusBadGateway = 502 + httpStatusInternalError = 500 + httpStatusUnsupportedMediaType = 415 ) // isNegativeCacheable returns true if the error should be cached.