sneak/pixa
1
0
Fork 0
Code Issues 11 Pull Requests 4 Actions 11 Packages Projects Releases Wiki Activity

Response header gaps: no Vary, X-Request-ID not returned or propagated, /v1/e/ lacks ETag/HEAD/304 #84

New Issue
Closed
opened 2026-08-09 03:49:48 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:49:48 +02:00
Collaborator
Copy Link
Copy Source

Verified against main at 61f42e6. Bundles three small, related response-header gaps from TODO.md plus one asymmetry found during the survey.

1. No Vary header anywhere. grep for Vary returns nothing. Harmless today, but it becomes a correctness bug the moment format=auto lands (filed separately): serving content negotiated on Accept without Vary: Accept means a shared cache will hand a WebP to a client that cannot display it. Add it as part of whichever change first makes a response depend on a request header — and add Vary: Accept defensively now if that is soon.

2. X-Request-ID is generated and logged but never returned or propagated. chi's RequestID middleware is installed (internal/server/routes.go:19) and the ID reaches the logs (internal/middleware/middleware.go:90,96), but it is never written to the response, so a client cannot quote an ID when reporting a problem — which is the main reason to have one. It is also not forwarded upstream (internal/httpfetcher/httpfetcher.go:206-207 sets only User-Agent and Accept), so a request cannot be traced across the proxy boundary. Additionally the per-image log lines (internal/handlers/image.go:161-168, internal/imgcache/service.go:270-280,338-351) do not carry the request ID, so they cannot be correlated with the access log line for the same request.

3. /v1/e/ is missing response features /v1/image/ has. internal/handlers/imageenc.go:69-85 sets Content-Type, Content-Length, Cache-Control, and X-Pixa-Cache — but no ETag, no If-None-Match/304 handling, and internal/server/routes.go:60 registers only Get, no Head. /v1/image/ has all three (image.go:129-147, routes.go:55-56). Encrypted URLs are the ones most likely to be embedded in pages and re-requested, so they benefit from revalidation at least as much.

Definition of done

  1. X-Request-ID is set on every response.
  2. The request ID is forwarded on upstream fetches, and included in the per-image log lines so they correlate with the access log.
  3. /v1/e/ sets ETag, handles If-None-Match with a 304, and the route is registered for HEAD as well as GET — reusing /v1/image/'s implementation rather than duplicating it.
  4. Vary is emitted wherever a response varies on a request header; if nothing varies yet, state that explicitly in the PR and leave it to the format=auto work.
  5. Failing tests first for each: response carries the ID; /v1/e/ returns 304 on a matching If-None-Match; HEAD /v1/e/... returns headers with no body.
  6. make check green.

Note

Split into separate commits (one per item) — they are independent, and item 3 is the only one touching handler logic.

Verified against `main` at `61f42e6`. Bundles three small, related response-header gaps from `TODO.md` plus one asymmetry found during the survey. **1. No `Vary` header anywhere.** `grep` for `Vary` returns nothing. Harmless today, but it becomes a correctness bug the moment `format=auto` lands (filed separately): serving content negotiated on `Accept` without `Vary: Accept` means a shared cache will hand a WebP to a client that cannot display it. Add it as part of whichever change first makes a response depend on a request header — and add `Vary: Accept` defensively now if that is soon. **2. `X-Request-ID` is generated and logged but never returned or propagated.** `chi`'s `RequestID` middleware is installed (`internal/server/routes.go:19`) and the ID reaches the logs (`internal/middleware/middleware.go:90,96`), but it is never written to the response, so a client cannot quote an ID when reporting a problem — which is the main reason to have one. It is also not forwarded upstream (`internal/httpfetcher/httpfetcher.go:206-207` sets only User-Agent and Accept), so a request cannot be traced across the proxy boundary. Additionally the per-image log lines (`internal/handlers/image.go:161-168`, `internal/imgcache/service.go:270-280,338-351`) do not carry the request ID, so they cannot be correlated with the access log line for the same request. **3. `/v1/e/` is missing response features `/v1/image/` has.** `internal/handlers/imageenc.go:69-85` sets Content-Type, Content-Length, Cache-Control, and X-Pixa-Cache — but no `ETag`, no `If-None-Match`/304 handling, and `internal/server/routes.go:60` registers only `Get`, no `Head`. `/v1/image/` has all three (`image.go:129-147`, `routes.go:55-56`). Encrypted URLs are the ones most likely to be embedded in pages and re-requested, so they benefit from revalidation at least as much. ## Definition of done 1. `X-Request-ID` is set on every response. 2. The request ID is forwarded on upstream fetches, and included in the per-image log lines so they correlate with the access log. 3. `/v1/e/` sets `ETag`, handles `If-None-Match` with a 304, and the route is registered for `HEAD` as well as `GET` — reusing `/v1/image/`'s implementation rather than duplicating it. 4. `Vary` is emitted wherever a response varies on a request header; if nothing varies yet, state that explicitly in the PR and leave it to the `format=auto` work. 5. Failing tests first for each: response carries the ID; `/v1/e/` returns 304 on a matching `If-None-Match`; `HEAD /v1/e/...` returns headers with no body. 6. `make check` green. ## Note Split into separate commits (one per item) — they are independent, and item 3 is the only one touching handler logic.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:49:48 +02:00
clawbot commented 2026-10-04 08:09:07 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan, checked against next at 5b17d1f. One commit per item, each with its failing test first.

  1. Every response carries X-Request-ID: a small middleware right after chi's RequestID in internal/server/routes.go sets the response header from the request's ID.
  2. The upstream fetch sends the same X-Request-ID (internal/httpfetcher/httpfetcher.go, next to User-Agent and Accept), and the per-image log lines in internal/handlers and internal/imgcache carry request_id like the access log line. A fetch shared by several requests (#65) carries the ID of the request that started it; the README says so where it describes the shared fetch.
  3. /v1/e/ sets ETag, answers a matching If-None-Match with 304, and is registered for HEAD as well as GET. The ETag/304 code in HandleImage becomes one small function both handlers call; nothing else in the two handlers is merged here (that is a recommendation on #168, which waits for the owner).
  4. Vary: no pixa response depends on a request header yet, so pixa adds none; Vary: Accept comes with #88. Confirm whether go-chi/cors already sends Vary: Origin when access_control_allow_origin is one origin, and say which in the PR.

Tests: a response has X-Request-ID; an upstream fetch receives it; /v1/e/ answers 304 to a matching If-None-Match; HEAD /v1/e/... returns the headers and no body. README.md "Routes" gains HEAD and 304 for /v1/e/ and mentions X-Request-ID.

Model: opus-5-5

Plan, checked against `next` at `5b17d1f`. One commit per item, each with its failing test first. 1. Every response carries `X-Request-ID`: a small middleware right after chi's `RequestID` in `internal/server/routes.go` sets the response header from the request's ID. 2. The upstream fetch sends the same `X-Request-ID` (`internal/httpfetcher/httpfetcher.go`, next to `User-Agent` and `Accept`), and the per-image log lines in `internal/handlers` and `internal/imgcache` carry `request_id` like the access log line. A fetch shared by several requests (https://git.eeqj.de/sneak/pixa/issues/65) carries the ID of the request that started it; the README says so where it describes the shared fetch. 3. `/v1/e/` sets `ETag`, answers a matching `If-None-Match` with 304, and is registered for `HEAD` as well as `GET`. The `ETag`/304 code in `HandleImage` becomes one small function both handlers call; nothing else in the two handlers is merged here (that is a recommendation on https://git.eeqj.de/sneak/pixa/issues/168, which waits for the owner). 4. `Vary`: no pixa response depends on a request header yet, so pixa adds none; `Vary: Accept` comes with https://git.eeqj.de/sneak/pixa/issues/88. Confirm whether `go-chi/cors` already sends `Vary: Origin` when `access_control_allow_origin` is one origin, and say which in the PR. Tests: a response has `X-Request-ID`; an upstream fetch receives it; `/v1/e/` answers 304 to a matching `If-None-Match`; `HEAD /v1/e/...` returns the headers and no body. `README.md` "Routes" gains `HEAD` and 304 for `/v1/e/` and mentions `X-Request-ID`. Model: opus-5-5
clawbot commented 2026-10-04 10:51:36 +02:00
Author
Collaborator
Copy Link
Copy Source

Built as planned above in #179. One deviation: the encrypted route handler was already at the function length limit, so its token checks moved unchanged into a function of their own; the PR lists it with the judgement calls.

Model: opus-5-5

Built as planned above in https://git.eeqj.de/sneak/pixa/pulls/179. One deviation: the encrypted route handler was already at the function length limit, so its token checks moved unchanged into a function of their own; the PR lists it with the judgement calls. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#84