From 6d380fbf9829a55fece1ab4b522ad5ec9be219b4 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 20:17:08 +0000 Subject: [PATCH 1/2] test: cover encrypted-URL and generator validation gaps (#62) Failing tests for the missing validation on the encrypted-URL path and the token generator: an encrypted token carrying an over-limit dimension or an unrecognized fit mode must be rejected with 400, and POST /generate with a non-numeric or over-limit width must return 400 rather than minting a token. Also covers the shared imgcache validator directly. Model: opus-4-8 --- .../handlers/auth_generate_internal_test.go | 66 +++++++++++++ internal/handlers/imageenc_internal_test.go | 98 +++++++++++++++++++ internal/imgcache/validate_internal_test.go | 64 ++++++++++++ 3 files changed, 228 insertions(+) create mode 100644 internal/handlers/auth_generate_internal_test.go create mode 100644 internal/handlers/imageenc_internal_test.go create mode 100644 internal/imgcache/validate_internal_test.go diff --git a/internal/handlers/auth_generate_internal_test.go b/internal/handlers/auth_generate_internal_test.go new file mode 100644 index 0000000..63d3c59 --- /dev/null +++ b/internal/handlers/auth_generate_internal_test.go @@ -0,0 +1,66 @@ +package handlers + +import ( + "maps" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" +) + +// generatePost submits the /generate form with a valid session and CSRF token +// plus the caller's extra fields, returning the recorder. +func generatePost( + t *testing.T, extra url.Values, +) *httptest.ResponseRecorder { + t.Helper() + + h, srv := newCSRFTestRouter(t) + + sessionCookie := newSessionCookie(t, h) + cookies, token := csrfCredentials(t, srv, []*http.Cookie{sessionCookie}) + cookies = append(cookies, sessionCookie) + + form := url.Values{ + sourceURLField: {testSourceURL}, + csrfTokenField: {token}, + } + maps.Copy(form, extra) + + return postForm(srv, "/generate", cookies, form) +} + +// TestGeneratePostRejectsNonNumericWidth verifies that a non-numeric width is +// rejected with 400 naming the field rather than being coerced to 0 and +// minting a 0-width token. +func TestGeneratePostRejectsNonNumericWidth(t *testing.T) { + t.Parallel() + + rec := generatePost(t, url.Values{"width": {"abc"}}) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + if strings.Contains(rec.Body.String(), "/v1/e/") { + t.Error("a token was generated for non-numeric width") + } +} + +// TestGeneratePostRejectsOverLimitWidth verifies that a width beyond +// MaxDimension is rejected at generation time so an unusable token cannot be +// minted. +func TestGeneratePostRejectsOverLimitWidth(t *testing.T) { + t.Parallel() + + rec := generatePost(t, url.Values{"width": {"100000"}}) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + if strings.Contains(rec.Body.String(), "/v1/e/") { + t.Error("a token was generated for an over-limit width") + } +} diff --git a/internal/handlers/imageenc_internal_test.go b/internal/handlers/imageenc_internal_test.go new file mode 100644 index 0000000..be2db22 --- /dev/null +++ b/internal/handlers/imageenc_internal_test.go @@ -0,0 +1,98 @@ +package handlers + +import ( + "context" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5" + + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// newEncTestServer builds a router serving the encrypted-URL route with a +// generator seeded by the shared test signing key. The image service is left +// nil: these tests exercise validation that rejects a token before any image +// is fetched, so the handler must never reach the service. +func newEncTestServer(t *testing.T) (*encurl.Generator, http.Handler) { + t.Helper() + + encGen, err := encurl.NewGenerator(testSigningKey) + if err != nil { + t.Fatalf("encurl.NewGenerator() error = %v", err) + } + + h := &Handlers{ + log: slog.New(slog.DiscardHandler), + encGen: encGen, + } + + r := chi.NewRouter() + r.Get("/v1/e/{token}/*", h.HandleImageEnc()) + + return encGen, r +} + +// getEncToken issues a GET for the given token and returns the recorder. +func getEncToken(srv http.Handler, token string) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/v1/e/"+token+"/img.jpg", nil) + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, req) + + return rec +} + +// TestHandleImageEnc_OverLimitDimension_Returns400 verifies that a decrypted +// token requesting a dimension beyond MaxDimension is rejected with 400 +// instead of reaching the image processor and libvips. +func TestHandleImageEnc_OverLimitDimension_Returns400(t *testing.T) { + t.Parallel() + + encGen, srv := newEncTestServer(t) + + token, err := encGen.Generate(&encurl.Payload{ + SourceHost: "cdn.example.com", + SourcePath: "/photo.jpg", + Width: 100000, + Height: 100000, + }) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + + rec := getEncToken(srv, token) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } +} + +// TestHandleImageEnc_InvalidFitMode_Returns400 verifies that a decrypted token +// carrying an unrecognized fit mode is rejected with 400 rather than surfacing +// as a 500 from the image processor's default branch. +func TestHandleImageEnc_InvalidFitMode_Returns400(t *testing.T) { + t.Parallel() + + encGen, srv := newEncTestServer(t) + + token, err := encGen.Generate(&encurl.Payload{ + SourceHost: "cdn.example.com", + SourcePath: "/photo.jpg", + Width: 800, + Height: 600, + FitMode: imgcache.FitMode("bogus"), + }) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + + rec := getEncToken(srv, token) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } +} diff --git a/internal/imgcache/validate_internal_test.go b/internal/imgcache/validate_internal_test.go new file mode 100644 index 0000000..99ecf36 --- /dev/null +++ b/internal/imgcache/validate_internal_test.go @@ -0,0 +1,64 @@ +package imgcache + +import ( + "errors" + "testing" +) + +func TestValidateImageRequest(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + req ImageRequest + wantErr error + }{ + { + name: "within bounds", + req: ImageRequest{Size: Size{Width: 800, Height: 600}, FitMode: FitCover}, + }, + { + name: "original size and empty fit", + req: ImageRequest{Size: Size{Width: 0, Height: 0}}, + }, + { + name: "width over limit", + req: ImageRequest{Size: Size{Width: MaxDimension + 1, Height: 600}}, + wantErr: ErrDimensionTooLarge, + }, + { + name: "height over limit", + req: ImageRequest{Size: Size{Width: 800, Height: MaxDimension + 1}}, + wantErr: ErrDimensionTooLarge, + }, + { + name: "negative width", + req: ImageRequest{Size: Size{Width: -1, Height: 600}}, + wantErr: ErrInvalidSize, + }, + { + name: "invalid fit mode", + req: ImageRequest{Size: Size{Width: 800, Height: 600}, FitMode: "bogus"}, + wantErr: ErrInvalidFitMode, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := ValidateImageRequest(&tt.req) + if tt.wantErr == nil { + if err != nil { + t.Fatalf("ValidateImageRequest() error = %v, want nil", err) + } + + return + } + + if !errors.Is(err, tt.wantErr) { + t.Fatalf("ValidateImageRequest() error = %v, want %v", err, tt.wantErr) + } + }) + } +} -- 2.54.0 From a663669286c1eaf98a6d207a75c95996902be566 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 20:17:08 +0000 Subject: [PATCH 2/2] fix: validate dimensions and fit mode on encrypted URLs (closes #62) The encrypted /v1/e/ route built its image request straight from the decrypted payload, so a token could request an over-limit dimension (reaching libvips and exhausting memory) or an unknown fit mode (surfacing as a 500 from the processor). The token generator discarded every strconv.Atoi error, silently turning non-numeric width, height, quality, or ttl into 0 and applying no upper bound on dimensions. Add a shared ValidateImageRequest in internal/imgcache enforcing the MaxDimension bound and ValidateFitMode, and apply it on both the plain image route and the encrypted route so both reject an over-limit size or an unrecognized fit mode with 400. The generator now parses each numeric field explicitly and returns 400 naming the field for non-numeric or out-of-range input, with width and height bounds-checked so an unusable token cannot be minted. Model: opus-4-8 --- TODO.md | 11 ++++ internal/handlers/auth.go | 115 +++++++++++++++++++++++++++++----- internal/handlers/image.go | 19 +++--- internal/handlers/imageenc.go | 13 ++++ internal/imgcache/imgcache.go | 17 +++++ 5 files changed, 151 insertions(+), 24 deletions(-) diff --git a/TODO.md b/TODO.md index dab3310..4caa287 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,17 @@ P1: implement blocked networks configuration to extend SSRF protection # Completed Steps +- 2026-09-21 validate dimensions and fit mode on the encrypted-URL + route and the token generator (closes #62): added a shared + `ValidateImageRequest` in `internal/imgcache` enforcing the + `MaxDimension` bound and `ValidateFitMode`, applied by both the + `/v1/image/` and `/v1/e/` routes, so an over-limit size or an unknown + fit mode is a 400 rather than an out-of-memory or a 500 from the + processor; the URL generator now checks every numeric form field and + rejects a non-numeric or out-of-range `width`, `height`, `quality`, + or `ttl` with a 400 naming the field instead of coercing it to `0`, + and `width`/`height` are bounds-checked so an unusable token cannot be + minted - 2026-09-21 http.Server hardening (closes #92): added `HTTPReadHeaderTimeout` (10s, bounds the slowloris header dribble) and `HTTPIdleTimeout` (120s, bounds keep-alive reuse) alongside the diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 5e74e1f..6e3cdd0 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -2,6 +2,8 @@ package handlers import ( "crypto/subtle" + "errors" + "fmt" "html/template" "net/http" "net/url" @@ -13,6 +15,11 @@ import ( "sneak.berlin/go/pixa/internal/templates" ) +// errInvalidFormField reports a generator form field whose value is +// non-numeric or out of range. The offending field name is wrapped in so the +// response can name it. +var errInvalidFormField = errors.New("invalid") + // HandleRoot serves the login page or generator page based on authentication state. func (s *Handlers) HandleRoot() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -98,18 +105,26 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { // Validate source URL parsed, err := url.Parse(sourceURL) if err != nil || parsed.Host == "" { - s.renderGeneratorWithForm(w, r, "Invalid source URL", r.Form) + s.renderGeneratorWithForm(w, r, "Invalid source URL", r.Form, + http.StatusBadRequest) return } - payload, expiresAt, ttl := buildGeneratePayload(parsed, r.Form) + payload, expiresAt, ttl, err := buildGeneratePayload(parsed, r.Form) + if err != nil { + s.renderGeneratorWithForm(w, r, err.Error(), r.Form, + http.StatusBadRequest) + + return + } // Generate encrypted token token, err := s.encGen.Generate(payload) if err != nil { s.log.Error("failed to generate encrypted URL", "error", err) - s.renderGeneratorWithForm(w, r, "Failed to generate URL", r.Form) + s.renderGeneratorWithForm(w, r, "Failed to generate URL", r.Form, + http.StatusInternalServerError) return } @@ -137,17 +152,38 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { } // buildGeneratePayload parses the numeric form fields and assembles the -// encrypted URL payload. ttl=0 means never expires (ExpiresAt stays 0). +// encrypted URL payload. ttl=0 means never expires (ExpiresAt stays 0). A +// non-numeric or out-of-range field, or an unrecognized fit mode, is a client +// error naming the offending field, so an unusable token is never minted. func buildGeneratePayload( parsed *url.URL, form url.Values, -) (*encurl.Payload, time.Time, int) { - width, _ := strconv.Atoi(form.Get("width")) - height, _ := strconv.Atoi(form.Get("height")) - quality, _ := strconv.Atoi(form.Get("quality")) - ttl, _ := strconv.Atoi(form.Get("ttl")) +) (*encurl.Payload, time.Time, int, error) { + width, err := parseFormDimension(form, "width") + if err != nil { + return nil, time.Time{}, 0, err + } - if quality <= 0 { - quality = 85 + height, err := parseFormDimension(form, "height") + if err != nil { + return nil, time.Time{}, 0, err + } + + quality, err := parseFormCount(form, "quality", encurl.DefaultQuality) + if err != nil { + return nil, time.Time{}, 0, err + } + + ttl, err := parseFormCount(form, "ttl", 0) + if err != nil { + return nil, time.Time{}, 0, err + } + + fitMode := imgcache.FitMode(form.Get("fit")) + + err = imgcache.ValidateFitMode(fitMode) + if err != nil { + return nil, time.Time{}, 0, + fmt.Errorf("%w: %s", imgcache.ErrInvalidFitMode, form.Get("fit")) } var ( @@ -168,11 +204,45 @@ func buildGeneratePayload( Height: height, Format: imgcache.ImageFormat(form.Get("format")), Quality: quality, - FitMode: imgcache.FitMode(form.Get("fit")), + FitMode: fitMode, ExpiresAt: expiresAtUnix, } - return payload, expiresAt, ttl + return payload, expiresAt, ttl, nil +} + +// parseFormDimension reads an optional width or height form field. An empty +// value means "original size" (0). A non-numeric, negative, or over-limit +// value is rejected with an error naming the field. +func parseFormDimension(form url.Values, field string) (int, error) { + raw := form.Get(field) + if raw == "" { + return 0, nil + } + + value, err := strconv.Atoi(raw) + if err != nil || value < 0 || value > imgcache.MaxDimension { + return 0, fmt.Errorf("%w %s", errInvalidFormField, field) + } + + return value, nil +} + +// parseFormCount reads an optional non-negative integer form field, returning +// def when the field is empty and an error naming the field when the value is +// non-numeric or negative. +func parseFormCount(form url.Values, field string, def int) (int, error) { + raw := form.Get(field) + if raw == "" { + return def, nil + } + + value, err := strconv.Atoi(raw) + if err != nil || value < 0 { + return 0, fmt.Errorf("%w %s", errInvalidFormField, field) + } + + return value, nil } // generatorData holds template data for the generator page. @@ -212,6 +282,15 @@ func (s *Handlers) renderLogin( func (s *Handlers) renderGenerator( w http.ResponseWriter, r *http.Request, data *generatorData, +) { + s.renderGeneratorStatus(w, r, data, http.StatusOK) +} + +// renderGeneratorStatus renders the generator page with an explicit HTTP +// status. The status is written before the body so both it and the +// Content-Type header take effect; a rejected form uses 400. +func (s *Handlers) renderGeneratorStatus( + w http.ResponseWriter, r *http.Request, data *generatorData, status int, ) { w.Header().Set("Content-Type", "text/html; charset=utf-8") @@ -221,17 +300,19 @@ func (s *Handlers) renderGenerator( data.CSRFField = csrfField(r) + w.WriteHeader(status) + err := templates.Render(w, "generator.html", data) if err != nil { s.log.Error("failed to render generator template", "error", err) - http.Error(w, "Internal server error", http.StatusInternalServerError) } } func (s *Handlers) renderGeneratorWithForm( - w http.ResponseWriter, r *http.Request, errorMsg string, form url.Values, + w http.ResponseWriter, r *http.Request, errorMsg string, + form url.Values, status int, ) { - s.renderGenerator(w, r, &generatorData{ + s.renderGeneratorStatus(w, r, &generatorData{ Error: errorMsg, FormURL: form.Get("url"), FormWidth: form.Get("width"), @@ -240,7 +321,7 @@ func (s *Handlers) renderGeneratorWithForm( FormQuality: form.Get("quality"), FormFit: form.Get("fit"), FormTTL: form.Get("ttl"), - }) + }, status) } func (s *Handlers) buildGeneratedURL(r *http.Request, token, format string) string { diff --git a/internal/handlers/image.go b/internal/handlers/image.go index c3de6b5..7411e56 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -110,13 +110,6 @@ func (s *Handlers) parseImageRequest( if fit := query.Get("fit"); fit != "" { req.FitMode = imgcache.FitMode(fit) - - fitErr := imgcache.ValidateFitMode(req.FitMode) - if fitErr != nil { - s.respondError(w, "invalid fit mode: "+fit, http.StatusBadRequest) - - return nil, false - } } // Default quality if not set @@ -129,6 +122,18 @@ func (s *Handlers) parseImageRequest( req.FitMode = imgcache.FitCover } + // Enforce dimension and fit-mode bounds, shared with the encrypted-URL + // route. Dimensions are already bounded by the path parser above; this + // also rejects an unrecognized fit mode with 400 instead of letting it + // reach the processor as a 500. + err = imgcache.ValidateImageRequest(req) + if err != nil { + s.respondError(w, "invalid image request: "+err.Error(), + http.StatusBadRequest) + + return nil, false + } + return req, true } diff --git a/internal/handlers/imageenc.go b/internal/handlers/imageenc.go index e01b2cd..8089f64 100644 --- a/internal/handlers/imageenc.go +++ b/internal/handlers/imageenc.go @@ -50,6 +50,19 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { // Convert payload to ImageRequest req := payload.ToImageRequest() + // Apply the same dimension and fit-mode bounds as the plain image + // route: a sealed payload is trusted for its origin, not for staying + // within limits, so an over-limit size or unknown fit mode is a 400 + // here rather than an out-of-memory or a 500 from the processor. + err = imgcache.ValidateImageRequest(req) + if err != nil { + s.log.Debug("encrypted URL failed validation", "error", err) + s.respondError(w, "invalid encrypted URL: "+err.Error(), + http.StatusBadRequest) + + return + } + // Log the request s.log.Debug("encrypted image request", "host", req.SourceHost, diff --git a/internal/imgcache/imgcache.go b/internal/imgcache/imgcache.go index e3dd61e..5d4f27e 100644 --- a/internal/imgcache/imgcache.go +++ b/internal/imgcache/imgcache.go @@ -59,6 +59,23 @@ func ValidateFitMode(fit FitMode) error { } } +// ValidateImageRequest checks that a request's dimensions are within +// MaxDimension and its fit mode is recognized. Both the plain /v1/image/ +// route and the encrypted /v1/e/ route validate through this function so a +// request from either source enforces identical bounds, regardless of how it +// was constructed. A width or height of 0 means "original size" and is valid. +func ValidateImageRequest(req *ImageRequest) error { + if req.Size.Width < 0 || req.Size.Height < 0 { + return ErrInvalidSize + } + + if req.Size.Width > MaxDimension || req.Size.Height > MaxDimension { + return ErrDimensionTooLarge + } + + return ValidateFitMode(req.FitMode) +} + // ImageRequest represents a request for a processed image type ImageRequest struct { // SourceHost is the origin host (e.g., "cdn.example.com") -- 2.54.0