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/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.