From 3bcd032d37985c57e9dbce22e57ef8a77e718583 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 08:30:58 +0000 Subject: [PATCH 1/8] Test that every response carries X-Request-Id A response of any route and status must carry the request's ID: one pixa made up when the request brought none, otherwise the request's own X-Request-Id. No response carries it yet. Model: opus-5-5 --- internal/server/request_id_internal_test.go | 58 +++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 internal/server/request_id_internal_test.go diff --git a/internal/server/request_id_internal_test.go b/internal/server/request_id_internal_test.go new file mode 100644 index 0000000..d81a05b --- /dev/null +++ b/internal/server/request_id_internal_test.go @@ -0,0 +1,58 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// requestIDHeader is the header that carries a request's ID. +const requestIDHeader = "X-Request-Id" + +// TestResponsesCarryRequestID verifies that every response, whatever its route +// and status, carries the request's ID as X-Request-Id, so a client can quote +// it when reporting a problem: one pixa made up when the request brought none, +// and the request's own X-Request-Id when it brought one. +func TestResponsesCarryRequestID(t *testing.T) { + t.Parallel() + + const clientRequestID = "client-request-id" + + s := newTestServer(t) + + paths := []string{ + "/robots.txt", + "/no-such-path", + unsignedImagePath, + encryptedImagePath, + } + + for _, path := range paths { + t.Run(path, func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, path, nil)) + t.Logf("status %d, %s %q", + rec.Code, requestIDHeader, rec.Header().Get(requestIDHeader)) + + if rec.Header().Get(requestIDHeader) == "" { + t.Errorf("response has no %s", requestIDHeader) + } + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, path, nil) + req.Header.Set(requestIDHeader, clientRequestID) + + rec = httptest.NewRecorder() + s.ServeHTTP(rec, req) + + got := rec.Header().Get(requestIDHeader) + if got != clientRequestID { + t.Errorf("%s = %q, want the request's own %q", + requestIDHeader, got, clientRequestID) + } + }) + } +} -- 2.54.0 From afe4b3bb70da9538c7582ae40fc84f0924cc8d6e Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 08:31:17 +0000 Subject: [PATCH 2/8] Send the request's ID back as X-Request-Id A middleware right after chi's RequestID sets the X-Request-Id response header from the ID RequestID stored in the request context, so every response carries it and a client can quote it when reporting a problem. README "Routes" says where the ID comes from. Model: opus-5-5 --- README.md | 6 ++++++ internal/middleware/middleware.go | 14 ++++++++++++++ internal/server/routes.go | 1 + 3 files changed, 21 insertions(+) diff --git a/README.md b/README.md index 588e2db..fece930 100644 --- a/README.md +++ b/README.md @@ -137,6 +137,12 @@ path under `/v1/` answers 200, in maintenance mode too. authentication with `metrics.username` and `metrics.password`. Answers: 200; 401 without them; 404 when they are not set, as the route then does not exist. +Every response carries an `X-Request-ID` header holding the request's ID, which +a client can quote when reporting a problem: the request's own `X-Request-ID` +when it sent one, as a reverse proxy in front of pixa may, otherwise one pixa +makes up from its host name, a random string chosen at startup and a counter. +pixa's log line for the request carries the same ID as `request_id`. + Both `POST` routes accept only a form that pixa's own page served: the page puts a token in the form and sets a cookie to match, and a request without both is refused with 403, so another site cannot submit the form from a visitor's diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 492ba87..758f774 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -115,6 +115,20 @@ func (s *Middleware) RateLimit( }) } +// RequestIDResponseHeader returns a middleware that sends the request's ID as +// the X-Request-Id response header, so a client can quote it when reporting a +// problem. The ID is the one chi's RequestID middleware stored in the request +// context, so RequestID must run first. +func (s *Middleware) RequestIDResponseHeader() func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set(middleware.RequestIDHeader, + middleware.GetReqID(r.Context())) + next.ServeHTTP(w, r) + }) + } +} + type loggingResponseWriter struct { http.ResponseWriter diff --git a/internal/server/routes.go b/internal/server/routes.go index f341e8a..f92b472 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -29,6 +29,7 @@ func (s *Server) SetupRoutes() { s.router.Use(middleware.Recoverer) s.router.Use(middleware.RequestID) + s.router.Use(s.mw.RequestIDResponseHeader()) s.router.Use(s.mw.ClientIP()) s.router.Use(s.mw.SecurityHeaders()) s.router.Use(s.mw.Logging()) -- 2.54.0 From e00bf374cb259746cc29aa4ca5e923cbc9b21e12 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 08:34:04 +0000 Subject: [PATCH 3/8] Test that the upstream fetch and image log lines carry the request ID A fetch must send the ID of the request it serves as X-Request-Id, and the "upstream fetched", "image converted" and "image served" lines must carry it as request_id, through either image route. newSignedHostServer takes the logger its handlers and image service write to; its existing callers pass a discarding one. Model: opus-5-5 --- .../image_cache_control_internal_test.go | 13 +- internal/handlers/request_id_internal_test.go | 131 ++++++++++++++++++ .../httpfetcher/request_id_internal_test.go | 50 +++++++ 3 files changed, 189 insertions(+), 5 deletions(-) create mode 100644 internal/handlers/request_id_internal_test.go create mode 100644 internal/httpfetcher/request_id_internal_test.go diff --git a/internal/handlers/image_cache_control_internal_test.go b/internal/handlers/image_cache_control_internal_test.go index bb6a6b9..7845df5 100644 --- a/internal/handlers/image_cache_control_internal_test.go +++ b/internal/handlers/image_cache_control_internal_test.go @@ -23,8 +23,10 @@ const photoPath = "/images/photo.jpg" // newSignedHostServer returns a router for both image routes, and the Handlers // behind it, whose fetcher serves a JPEG at photoPath on signedHost. signedHost // is not on the allowlist, so a /v1/image/ URL for it is served only with a -// valid signature. -func newSignedHostServer(t *testing.T) (*Handlers, http.Handler) { +// valid signature. The handlers and the image service log to log. +func newSignedHostServer( + t *testing.T, log *slog.Logger, +) (*Handlers, http.Handler) { t.Helper() cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{ @@ -44,6 +46,7 @@ func newSignedHostServer(t *testing.T) (*Handlers, http.Handler) { signedHost + photoPath: &fstest.MapFile{Data: jpegData}, }), SigningKey: testSigningKey, + Logger: log, }) if err != nil { t.Fatalf("imgcache.NewService() error = %v", err) @@ -55,7 +58,7 @@ func newSignedHostServer(t *testing.T) (*Handlers, http.Handler) { } h := &Handlers{ - log: slog.New(slog.DiscardHandler), + log: log, imgSvc: svc, encGen: encGen, } @@ -103,7 +106,7 @@ func getMaxAge(t *testing.T, srv http.Handler, target string) int { func TestHandleImage_SignedURL_MaxAgeEndsAtExp(t *testing.T) { t.Parallel() - h, srv := newSignedHostServer(t) + h, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler)) signedURL, err := h.imgSvc.GenerateSignedURL("", &imgcache.ImageRequest{ SourceHost: signedHost, @@ -179,7 +182,7 @@ func TestHandleImageEnc_MaxAge(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - h, srv := newSignedHostServer(t) + h, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler)) token, err := h.encGen.Generate(&encurl.Payload{ SourceHost: signedHost, diff --git a/internal/handlers/request_id_internal_test.go b/internal/handlers/request_id_internal_test.go new file mode 100644 index 0000000..d26a2f6 --- /dev/null +++ b/internal/handlers/request_id_internal_test.go @@ -0,0 +1,131 @@ +package handlers + +import ( + "bytes" + "context" + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/go-chi/chi/v5/middleware" + + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// signedPhotoURL returns a signed /v1/image/ URL, valid for a minute, for the +// JPEG at photoPath on signedHost at 50x50, made with h's image service. +func signedPhotoURL(t *testing.T, h *Handlers) string { + t.Helper() + + signedURL, err := h.imgSvc.GenerateSignedURL("", &imgcache.ImageRequest{ + SourceHost: signedHost, + SourcePath: photoPath, + Size: imgcache.Size{Width: 50, Height: 50}, + Format: imgcache.FormatJPEG, + }, time.Minute) + if err != nil { + t.Fatalf("GenerateSignedURL() error = %v", err) + } + + return signedURL +} + +// encPhotoURL returns an encrypted /v1/e/ URL, which never expires, for the +// JPEG at photoPath on signedHost at 50x50, made with h's generator. +func encPhotoURL(t *testing.T, h *Handlers) string { + t.Helper() + + token, err := h.encGen.Generate(&encurl.Payload{ + SourceHost: signedHost, + SourcePath: photoPath, + Width: 50, + Height: 50, + Format: imgcache.FormatJPEG, + }) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + + return "/v1/e/" + token + "/img.jpg" +} + +// requestIDByMessage reads the JSON log lines in logs and returns the +// request_id of each line, by its message. +func requestIDByMessage(t *testing.T, logs io.Reader) map[string]string { + t.Helper() + + logged := make(map[string]string) + + dec := json.NewDecoder(logs) + for dec.More() { + var line map[string]any + + err := dec.Decode(&line) + if err != nil { + t.Fatalf("decoding log line: %v", err) + } + + msg, _ := line["msg"].(string) + requestID, _ := line["request_id"].(string) + logged[msg] = requestID + } + + return logged +} + +// TestImageLogLinesCarryRequestID verifies that the lines logged when an image +// is fetched, converted and served through either image route carry the +// request's ID as request_id, as the request log line does, so they can be +// found from it. +func TestImageLogLinesCarryRequestID(t *testing.T) { + t.Parallel() + + const requestID = "test-request-id" + + imageURLs := map[string]func(*testing.T, *Handlers) string{ + "/v1/image/": signedPhotoURL, + "/v1/e/": encPhotoURL, + } + + for route, imageURL := range imageURLs { + t.Run(route, func(t *testing.T) { + t.Parallel() + + var logs bytes.Buffer + + h, srv := newSignedHostServer(t, + slog.New(slog.NewJSONHandler(&logs, nil))) + + ctx := context.WithValue(t.Context(), + middleware.RequestIDKey, requestID) + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, httptest.NewRequestWithContext( + ctx, http.MethodGet, imageURL(t, h), nil)) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK) + } + + t.Logf("logged:\n%s", logs.String()) + + logged := requestIDByMessage(t, &logs) + + for _, msg := range []string{ + "upstream fetched", "image converted", "image served", + } { + got, ok := logged[msg] + if !ok { + t.Errorf("no %q line logged", msg) + } else if got != requestID { + t.Errorf("%q line has request_id %q, want %q", + msg, got, requestID) + } + } + }) + } +} diff --git a/internal/httpfetcher/request_id_internal_test.go b/internal/httpfetcher/request_id_internal_test.go new file mode 100644 index 0000000..374bdbb --- /dev/null +++ b/internal/httpfetcher/request_id_internal_test.go @@ -0,0 +1,50 @@ +package httpfetcher + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5/middleware" +) + +// TestFetchSendsRequestID verifies that a fetch sends the ID of the request +// it serves, which chi's RequestID middleware stores in the request context, +// to the upstream host as X-Request-Id, so the fetch can be found in that +// host's logs. +func TestFetchSendsRequestID(t *testing.T) { + t.Parallel() + + const requestID = "test-request-id" + + received := make(chan string, 1) + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + received <- r.Header.Get("X-Request-Id") + + w.Header().Set("Content-Type", contentTypeJPEG) + _, _ = io.WriteString(w, imagePayload) + })) + t.Cleanup(srv.Close) + + f, _ := newServerFetcher(t, srv, nil) + + ctx := context.WithValue(testContext(t), middleware.RequestIDKey, requestID) + + res, err := f.Fetch(ctx, upstreamURL("/image")) + if err != nil { + t.Fatalf("Fetch() error = %v", err) + } + + _ = res.Content.Close() + + got := <-received + t.Logf("upstream received X-Request-Id %q", got) + + if got != requestID { + t.Errorf("upstream X-Request-Id = %q, want %q", got, requestID) + } +} -- 2.54.0 From 8d96bd710b547482e2576cc0d837309cc9ca0ee4 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 08:34:50 +0000 Subject: [PATCH 4/8] Send the request ID upstream and log it with each image The fetcher sends the request ID chi's RequestID middleware put in the context as X-Request-Id, next to User-Agent and Accept, and the "upstream fetched", "image converted" and "image served" lines carry it as request_id, as the request log line does. A fetch shared by several requests runs with the first request's context values, so it carries that request's ID; README says so where it describes the shared fetch. Model: opus-5-5 --- README.md | 8 ++++++-- internal/handlers/image.go | 2 ++ internal/handlers/imageenc.go | 2 ++ internal/httpfetcher/httpfetcher.go | 9 +++++++++ internal/imgcache/service.go | 3 +++ 5 files changed, 22 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index fece930..b0287be 100644 --- a/README.md +++ b/README.md @@ -141,7 +141,9 @@ Every response carries an `X-Request-ID` header holding the request's ID, which a client can quote when reporting a problem: the request's own `X-Request-ID` when it sent one, as a reverse proxy in front of pixa may, otherwise one pixa makes up from its host name, a random string chosen at startup and a counter. -pixa's log line for the request carries the same ID as `request_id`. +pixa's log line for the request carries the same ID as `request_id`, and so do +the lines it logs when it fetches, converts and serves an image; the fetch sends +it to the upstream host as `X-Request-ID`. Both `POST` routes accept only a form that pixa's own page served: the page puts a token in the form and sets a cookie to match, and a request without both is @@ -192,7 +194,9 @@ source) and one transcode: the first request does the work, and the others wait for its image or its error, holding no upstream connection or processing slot of their own. A waiting request stops waiting when its own client goes away. The work goes on for the others even if the first request's client goes away, -until that request's `downstream_timeout` ends. +until that request's `downstream_timeout` ends. The shared fetch sends the first +request's ID upstream, and the lines logged for the fetch and the transcode +carry that ID. The login form (`POST /`) is limited to 5 attempts per minute per client address, counting an IPv6 client by its /64; an attempt over the limit is diff --git a/internal/handlers/image.go b/internal/handlers/image.go index 3c94bc8..a239ebf 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -10,6 +10,7 @@ import ( "time" "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" @@ -298,6 +299,7 @@ func (s *Handlers) writeImageResponse( // Log cache status and timing after serving duration := time.Since(startTime) s.log.Info("image served", + "request_id", middleware.GetReqID(r.Context()), "cache_key", cacheKey, "cache_status", resp.CacheStatus, "duration_ms", duration.Milliseconds(), diff --git a/internal/handlers/imageenc.go b/internal/handlers/imageenc.go index 56af31b..8e5e564 100644 --- a/internal/handlers/imageenc.go +++ b/internal/handlers/imageenc.go @@ -9,6 +9,7 @@ import ( "time" "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" @@ -105,6 +106,7 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { // Log completion duration := time.Since(start) s.log.Info("image served", + "request_id", middleware.GetReqID(ctx), "cache_key", imgcache.CacheKey(req), "host", req.SourceHost, "path", req.SourcePath, diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index 3cfee1c..f76e8b1 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/internal/httpfetcher/httpfetcher.go @@ -18,6 +18,8 @@ import ( "strings" "sync" "time" + + "github.com/go-chi/chi/v5/middleware" ) // Fetcher configuration constants. @@ -267,6 +269,13 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*FetchResult, erro req.Header.Set("User-Agent", f.config.UserAgent) req.Header.Set("Accept", strings.Join(f.config.AllowedContentTypes, ", ")) + // The ID of the request this fetch serves, so the fetch can be found in + // the upstream host's logs + requestID := middleware.GetReqID(ctx) + if requestID != "" { + req.Header.Set(middleware.RequestIDHeader, requestID) + } + // Use httptrace to capture connection details var remoteAddr string diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index 968dd95..562595c 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -13,6 +13,7 @@ import ( "github.com/dustin/go-humanize" "github.com/getsentry/sentry-go" + "github.com/go-chi/chi/v5/middleware" "golang.org/x/sync/singleflight" "sneak.berlin/go/pixa/internal/allowlist" "sneak.berlin/go/pixa/internal/httpfetcher" @@ -461,6 +462,7 @@ func (s *Service) fetchAndProcess( // Log upstream fetch details s.log.Info("upstream fetched", + "request_id", middleware.GetReqID(ctx), "host", req.SourceHost, "path", req.SourcePath, "bytes", fetchBytes, @@ -545,6 +547,7 @@ func (s *Service) processAndStore( } s.log.Info("image converted", + "request_id", middleware.GetReqID(ctx), "host", req.SourceHost, "path", req.SourcePath, "src_format", processResult.InputFormat, -- 2.54.0 From 20a46e96a28ba36161f65edd44fde1880518334d Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 08:35:53 +0000 Subject: [PATCH 5/8] Test ETag, 304 and HEAD on /v1/e/ An image served through an encrypted URL must carry an ETag, a request whose If-None-Match is that ETag must get 304 with no body, HEAD must get 200 with the headers and no body, and the server must route HEAD on /v1/e/ to its handler instead of answering 405. Model: opus-5-5 --- internal/handlers/imageenc_internal_test.go | 67 +++++++++++++++++++++ internal/server/routes_internal_test.go | 27 +++++++++ 2 files changed, 94 insertions(+) create mode 100644 internal/server/routes_internal_test.go diff --git a/internal/handlers/imageenc_internal_test.go b/internal/handlers/imageenc_internal_test.go index be2db22..d4f8c6b 100644 --- a/internal/handlers/imageenc_internal_test.go +++ b/internal/handlers/imageenc_internal_test.go @@ -96,3 +96,70 @@ func TestHandleImageEnc_InvalidFitMode_Returns400(t *testing.T) { t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) } } + +// TestHandleImageEnc_IfNoneMatch_Returns304 verifies that an image served +// through an encrypted URL carries an ETag, and that a request whose +// If-None-Match is that ETag is answered 304 Not Modified with no body. +func TestHandleImageEnc_IfNoneMatch_Returns304(t *testing.T) { + t.Parallel() + + h, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler)) + target := encPhotoURL(t, h) + + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodGet, target, nil)) + + etag := rec.Header().Get("ETag") + t.Logf("GET: %d, ETag %q", rec.Code, etag) + + if rec.Code != http.StatusOK || etag == "" { + t.Fatalf("GET: status = %d, ETag = %q, want %d and an ETag", + rec.Code, etag, http.StatusOK) + } + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, target, nil) + req.Header.Set("If-None-Match", etag) + + rec = httptest.NewRecorder() + srv.ServeHTTP(rec, req) + t.Logf("GET with If-None-Match: %d, %d body bytes", rec.Code, rec.Body.Len()) + + if rec.Code != http.StatusNotModified || rec.Body.Len() != 0 { + t.Errorf("status = %d with %d body bytes, want %d with none", + rec.Code, rec.Body.Len(), http.StatusNotModified) + } +} + +// TestHandleImageEnc_HEAD_ReturnsHeadersOnly verifies that HEAD on an +// encrypted URL is answered 200 with the headers GET sends and no body. +func TestHandleImageEnc_HEAD_ReturnsHeadersOnly(t *testing.T) { + t.Parallel() + + h, _ := newSignedHostServer(t, slog.New(slog.DiscardHandler)) + + r := chi.NewRouter() + r.Head("/v1/e/{token}/*", h.HandleImageEnc()) + + rec := httptest.NewRecorder() + r.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodHead, encPhotoURL(t, h), nil)) + t.Logf("HEAD: %d, headers %v, %d body bytes", + rec.Code, rec.Header(), rec.Body.Len()) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK) + } + + for _, name := range []string{ + "Content-Type", "Content-Length", "Cache-Control", "ETag", + } { + if rec.Header().Get(name) == "" { + t.Errorf("HEAD response has no %s", name) + } + } + + if rec.Body.Len() != 0 { + t.Errorf("HEAD response body has %d bytes, want none", rec.Body.Len()) + } +} diff --git a/internal/server/routes_internal_test.go b/internal/server/routes_internal_test.go new file mode 100644 index 0000000..f072a3d --- /dev/null +++ b/internal/server/routes_internal_test.go @@ -0,0 +1,27 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// TestEncryptedImageRouteAnswersHEAD verifies that HEAD on the encrypted image +// route reaches its handler, as GET does, instead of being answered 405 Method +// Not Allowed. The handler refuses a token it cannot decrypt with 400, so that +// status shows the request got through. +func TestEncryptedImageRouteAnswersHEAD(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), http.MethodHead, encryptedImagePath, nil)) + t.Logf("status %d", rec.Code) + + if rec.Code != http.StatusBadRequest { + t.Errorf("status = %d, want %d from the encrypted image handler", + rec.Code, http.StatusBadRequest) + } +} -- 2.54.0 From f507b6076d6ba0e6c5ae3cdc022cc554bb365950 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 08:39:36 +0000 Subject: [PATCH 6/8] Give /v1/e/ ETag, 304 and HEAD as /v1/image/ has (closes #84) The ETag and If-None-Match code of the /v1/image/ handler becomes notModified, which both image handlers call; /v1/e/ answers HEAD with the headers only and is routed for HEAD. HandleImageEnc was at the 80-line function limit, so its token checks move unchanged into parseImageEncRequest, as parseImageRequest does for /v1/image/. No Vary is added: only the image routes' CORS headers depend on a request header, and go-chi/cors already sends Vary: Origin with them. Model: opus-5-5 --- README.md | 5 +- TODO.md | 14 ++++- internal/handlers/image.go | 32 +++++++---- internal/handlers/imageenc.go | 100 +++++++++++++++++++++------------- internal/server/routes.go | 1 + 5 files changed, 101 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index b0287be..e847aaf 100644 --- a/README.md +++ b/README.md @@ -120,8 +120,9 @@ path under `/v1/` answers 200, in maintenance mode too. `blocked_networks`); 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 /v1/e//` — an image through an encrypted URL (see Encrypted - URLs). Needs: nothing but the URL. Answers: 200; 400 for a token that does not +- `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 decrypt, or that asks for a size or fit that is not valid; 410 once it has 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 diff --git a/TODO.md b/TODO.md index 0c4ac74..2188291 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,19 @@ P2: security: referer blacklist # Completed Steps +- 2026-10-04 request IDs returned and passed on, and `/v1/e/` revalidates + (closes #84): a middleware right after chi's `RequestID` sets `X-Request-ID` + on every response from the ID `RequestID` stores in the request context, which + is the request's own `X-Request-ID` when it sent one; the upstream fetch sends + that ID, and the "upstream fetched", "image converted" and "image served" log + lines carry it as `request_id`, a fetch shared by several requests carrying + the first request's; `/v1/e/` sets `ETag`, answers a matching `If-None-Match` + with 304 and is routed for `HEAD`, the `ETag` and 304 code being + `notModified`, which `/v1/image/` calls too; its token checks moved unchanged + into `parseImageEncRequest` to keep `HandleImageEnc` within the line limit; no + `Vary` is added, as no response depends on a request header except the image + routes' CORS headers, for which `go-chi/cors` already sends `Vary: Origin`; + `Vary: Accept` is left to #88. - 2026-10-04 routes, encrypted URLs and config file documented (closes #75): "Routes" in `README.md` lists every route with its method, purpose, what it needs and the status codes it answers with, and says `q` and `fit` are part @@ -438,7 +451,6 @@ P2: security: referer blacklist - P2: HTTP response handling - Last-Modified headers - Vary header for content negotiation - - X-Request-ID propagation - P2: auto format selection (format=auto based on Accept header) - P2: configuration - YAML config file support diff --git a/internal/handlers/image.go b/internal/handlers/image.go index a239ebf..1a46a58 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -248,6 +248,25 @@ func cacheControl(expires time.Time) string { return fmt.Sprintf("public, max-age=%d, immutable", int64(maxAge/time.Second)) } +// notModified sets the ETag header to etag and, when the request's +// If-None-Match is that ETag, answers 304 Not Modified. It reports whether it +// answered. An empty etag sets no header and never answers. +func notModified(w http.ResponseWriter, r *http.Request, etag string) bool { + if etag == "" { + return false + } + + w.Header().Set("ETag", etag) + + if r.Header.Get("If-None-Match") != etag { + return false + } + + w.WriteHeader(http.StatusNotModified) + + return true +} + // writeImageResponse writes headers and streams the image content, // handling conditional and HEAD requests. func (s *Handlers) writeImageResponse( @@ -266,17 +285,8 @@ func (s *Handlers) writeImageResponse( w.Header().Set("Cache-Control", cacheControl(req.Expires)) w.Header().Set("X-Pixa-Cache", string(resp.CacheStatus)) - if resp.ETag != "" { - w.Header().Set("ETag", resp.ETag) - - // Check for conditional request (If-None-Match) - if ifNoneMatch := r.Header.Get("If-None-Match"); ifNoneMatch != "" { - if ifNoneMatch == resp.ETag { - w.WriteHeader(http.StatusNotModified) - - return - } - } + if notModified(w, r, resp.ETag) { + return } // Handle HEAD request - return headers only diff --git a/internal/handlers/imageenc.go b/internal/handlers/imageenc.go index 8e5e564..dadb6bd 100644 --- a/internal/handlers/imageenc.go +++ b/internal/handlers/imageenc.go @@ -25,43 +25,8 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { ctx := r.Context() start := time.Now() - // Extract token from URL - token := chi.URLParam(r, "token") - if token == "" { - s.respondError(w, "missing token", http.StatusBadRequest) - - return - } - - // Decrypt and validate the payload - payload, err := s.encGen.Parse(token) - if err != nil { - if errors.Is(err, encurl.ErrExpired) { - s.log.Debug("encrypted URL expired", "error", err) - s.respondError(w, "URL has expired", http.StatusGone) - - return - } - - s.log.Debug("failed to decrypt URL", "error", err) - s.respondError(w, "invalid encrypted URL", http.StatusBadRequest) - - return - } - - // 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) - + req, ok := s.parseImageEncRequest(w, r) + if !ok { return } @@ -95,6 +60,17 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { w.Header().Set("Cache-Control", cacheControl(req.Expires)) w.Header().Set("X-Pixa-Cache", string(resp.CacheStatus)) + if notModified(w, r, resp.ETag) { + return + } + + // A HEAD request gets the headers only + if r.Method == http.MethodHead { + w.WriteHeader(http.StatusOK) + + return + } + // Stream the response written, err := io.Copy(w, resp.Content) if err != nil { @@ -118,6 +94,56 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { } } +// parseImageEncRequest decrypts the token of an encrypted image URL into an +// ImageRequest and checks it. On a token that is missing, does not decrypt, +// has expired or asks for something not valid, it writes an error response +// and returns false. +func (s *Handlers) parseImageEncRequest( + w http.ResponseWriter, r *http.Request, +) (*imgcache.ImageRequest, bool) { + // Extract token from URL + token := chi.URLParam(r, "token") + if token == "" { + s.respondError(w, "missing token", http.StatusBadRequest) + + return nil, false + } + + // Decrypt and validate the payload + payload, err := s.encGen.Parse(token) + if err != nil { + if errors.Is(err, encurl.ErrExpired) { + s.log.Debug("encrypted URL expired", "error", err) + s.respondError(w, "URL has expired", http.StatusGone) + + return nil, false + } + + s.log.Debug("failed to decrypt URL", "error", err) + s.respondError(w, "invalid encrypted URL", http.StatusBadRequest) + + return nil, false + } + + // 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 nil, false + } + + return req, true +} + // handleImageError converts image service errors to HTTP responses. func (s *Handlers) handleImageError(w http.ResponseWriter, err error) { switch { diff --git a/internal/server/routes.go b/internal/server/routes.go index f92b472..d7d515b 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -98,6 +98,7 @@ func (s *Server) SetupRoutes() { // The trailing filename (e.g., /img.jpg) is ignored but helps // browsers with content type r.Get("/e/{token}/*", s.h.HandleImageEnc()) + r.Head("/e/{token}/*", s.h.HandleImageEnc()) }) }) -- 2.54.0 From 6f7007a0eab8833aa08a3f93db49443525934128 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 09:43:47 +0000 Subject: [PATCH 7/8] Test that only a short, plain request ID is kept or passed on A request's own X-Request-Id must be kept only when it is at most 64 letters, digits, '-', '_' or '.'; any other must be neither sent back nor sent upstream, and the request gets a fresh ID instead. An ID made up for a request must not hold the host name and must differ for every request. pixa has no middleware of its own for this yet. Model: opus-5-5 --- .../middleware/request_id_internal_test.go | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 internal/middleware/request_id_internal_test.go diff --git a/internal/middleware/request_id_internal_test.go b/internal/middleware/request_id_internal_test.go new file mode 100644 index 0000000..5b381d9 --- /dev/null +++ b/internal/middleware/request_id_internal_test.go @@ -0,0 +1,118 @@ +package middleware + +import ( + "log/slog" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + + "github.com/go-chi/chi/v5/middleware" + + "sneak.berlin/go/pixa/internal/config" +) + +// sendRequestID sends a request through the RequestID middleware, carrying +// incoming as its X-Request-Id unless that is empty. It returns the ID the +// next handler found in the request context, which the upstream fetch sends +// and the log lines carry, and the X-Request-Id of the response. +func sendRequestID(t *testing.T, incoming string) (string, string) { + t.Helper() + + mw := &Middleware{log: slog.Default(), config: &config.Config{}} + + var inContext string + + handler := mw.RequestID()(http.HandlerFunc( + func(_ http.ResponseWriter, r *http.Request) { + inContext = middleware.GetReqID(r.Context()) + })) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) + if incoming != "" { + req.Header.Set("X-Request-Id", incoming) + } + + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + return inContext, rec.Header().Get("X-Request-Id") +} + +// TestRequestIDKeepsShortPlainID verifies that a request's own X-Request-Id of +// at most 64 letters, digits, '-', '_' or '.' is kept as its ID. +func TestRequestIDKeepsShortPlainID(t *testing.T) { + t.Parallel() + + for _, incoming := range []string{ + "client-request-id", + "A1_b2.c3-d4", + strings.Repeat("a", 64), + } { + inContext, inResponse := sendRequestID(t, incoming) + + if inContext != incoming || inResponse != incoming { + t.Errorf("incoming %q: context has %q, response %q, want both %q", + incoming, inContext, inResponse, incoming) + } + } +} + +// TestRequestIDReplacesLongOrUnusualID verifies that a request's own +// X-Request-Id that is over 64 characters or holds anything but letters, +// digits, '-', '_' or '.' is neither sent back nor sent upstream: the request +// gets a fresh ID instead. +func TestRequestIDReplacesLongOrUnusualID(t *testing.T) { + t.Parallel() + + for _, incoming := range []string{ + strings.Repeat("a", 65), + strings.Repeat("a", 9000), + "has space", + "a/b", + "a,b", + "