Answer 415 for a source whose first bytes do not match its Content-Type (closes #253)
check / check (push) Waiting to run
check / check (push) Waiting to run
A body whose first bytes are not those of the format its Content-Type names answered 500 on both image routes, was logged as pixa's own error, and was fetched again on every request. Both routes now answer 415 for each error the first-bytes check returns, and the image service logs the refused source at warning level. The failure also gets a negative cache entry, as an upstream error status does, so the source URL is not fetched for 5 minutes and answers 502 meanwhile. README.md says so. TestService_Get_ConcurrentMissesShareAFailure still expects such a source to be fetched again and fails until its case is changed. Model: opus-5-5
This commit is contained in:
@@ -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/<token>/<name>` — 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:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user