Headers set before an uncommitted panic survive onto the recovered 500, including Set-Cookie #193

New Issue

clawbot · 2026-08-18T04:18:05+02:00

clawbot commented

2026-08-18 04:18:05 +02:00

Found by the independent review of #189 and measured there. NOT milestoned 1.0.0, and filed as a question rather than an assertion of a defect — see below.

When a handler sets response headers and then panics before committing the response, the recover middleware writes a 500 via http.Error, which clears only Content-Length and Content-Type. Everything else the handler already put in the header map survives onto the 500. That includes Set-Cookie and Location.

The concrete shape worth thinking about: a handler that establishes a session — writes Set-Cookie — and then panics before finishing. The client receives a 500 and a valid session cookie. The server-side effects of that half-finished request are whatever they were; the client is left holding credentials issued by a request that visibly failed. Location has a milder version of the same problem: a 500 carrying a redirect target.

Why this is a question and not a filed defect

This is exactly what chi v5's Recoverer does, and it is ordinary Go practice — http.Error has never cleared the header map, and almost nobody clears it manually. Changing it means deviating from the idiom, and there is a real argument for the current behaviour: a handler that sets Set-Cookie early and panics late may have committed the session server-side already, in which case dropping the cookie strands it.

So the options are genuinely open:

Leave it. Matches chi, matches the idiom, no surprise to anyone reading the middleware.
Clear the header map before writing the 500, except for headers the middleware itself owns. Safest, but deviates from the idiom and can strand a committed session.
Clear only Set-Cookie, on the argument that credentials specifically should never ride out on a failed response, and leave everything else alone.

Recommendation: option 3, if anything is done at all. It addresses the case with security consequences and leaves the idiomatic behaviour otherwise intact. But this is a judgement call about a deviation from standard Go practice, so it should be decided rather than assumed.

Note this is pre-existing in spirit: before #189, a panicking request had its connection dropped outright, so the question could not arise. That PR makes panics answerable, which is what surfaces it. It is not a regression that PR introduced — a dropped connection was strictly worse.

Definition of done

Depends entirely on which option is chosen. If 2 or 3:

The recover middleware clears the agreed headers before writing the 500, and a test drives a handler that sets Set-Cookie and Location and then panics uncommitted, asserting exactly which survive.
The committed case is asserted separately and unchanged: once the response is committed nothing can be cleared, and the middleware must still not attempt a second WriteHeader.
The mutation bites: removing the clearing must fail the test.
The reasoning is recorded in the middleware's doc comment in one sentence, since the behaviour deviates from http.Error's default and the next reader will otherwise assume it is a bug.

If option 1: close this issue with the reason, and say so in the middleware doc comment so it is not rediscovered.

Implementation requirements

Branch from next, PR based on next, single commit, title ending (closes #N).
Do not modify TODO.md (see #112).
Run make bootstrap in a fresh clone before gating. Gate on make check plus the cache-defeated Docker lint path; all linting in Docker, never the host. Clean up every container and image; never prune.

Found by the independent review of https://git.eeqj.de/sneak/webhooker/pulls/189 and measured there. NOT milestoned 1.0.0, and filed as a question rather than an assertion of a defect — see below. When a handler sets response headers and then panics **before committing** the response, the recover middleware writes a 500 via `http.Error`, which clears only `Content-Length` and `Content-Type`. Everything else the handler already put in the header map survives onto the 500. That includes `Set-Cookie` and `Location`. The concrete shape worth thinking about: a handler that establishes a session — writes `Set-Cookie` — and then panics before finishing. The client receives a 500 and a valid session cookie. The server-side effects of that half-finished request are whatever they were; the client is left holding credentials issued by a request that visibly failed. `Location` has a milder version of the same problem: a 500 carrying a redirect target. ## Why this is a question and not a filed defect This is **exactly what chi v5's `Recoverer` does**, and it is ordinary Go practice — `http.Error` has never cleared the header map, and almost nobody clears it manually. Changing it means deviating from the idiom, and there is a real argument for the current behaviour: a handler that sets `Set-Cookie` early and panics late may have committed the session server-side already, in which case dropping the cookie strands it. So the options are genuinely open: 1. **Leave it.** Matches chi, matches the idiom, no surprise to anyone reading the middleware. 2. **Clear the header map before writing the 500**, except for headers the middleware itself owns. Safest, but deviates from the idiom and can strand a committed session. 3. **Clear only `Set-Cookie`**, on the argument that credentials specifically should never ride out on a failed response, and leave everything else alone. Recommendation: **option 3**, if anything is done at all. It addresses the case with security consequences and leaves the idiomatic behaviour otherwise intact. But this is a judgement call about a deviation from standard Go practice, so it should be decided rather than assumed. Note this is **pre-existing in spirit**: before https://git.eeqj.de/sneak/webhooker/pulls/189, a panicking request had its connection dropped outright, so the question could not arise. That PR makes panics answerable, which is what surfaces it. It is not a regression that PR introduced — a dropped connection was strictly worse. ## Definition of done Depends entirely on which option is chosen. If 2 or 3: - The recover middleware clears the agreed headers before writing the 500, and a test drives a handler that sets `Set-Cookie` and `Location` and then panics uncommitted, asserting exactly which survive. - The **committed** case is asserted separately and unchanged: once the response is committed nothing can be cleared, and the middleware must still not attempt a second `WriteHeader`. - The mutation bites: removing the clearing must fail the test. - The reasoning is recorded in the middleware's doc comment in one sentence, since the behaviour deviates from `http.Error`'s default and the next reader will otherwise assume it is a bug. If option 1: close this issue with the reason, and say so in the middleware doc comment so it is not rediscovered. ## Implementation requirements - Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`. - Do not modify `TODO.md` (see https://git.eeqj.de/sneak/webhooker/issues/112). - Run `make bootstrap` in a fresh clone before gating. Gate on `make check` plus the cache-defeated Docker lint path; all linting in Docker, never the host. Clean up every container and image; never prune.

clawbot referenced this issue

2026-08-18 08:04:23 +02:00

Report handler panics through the logger and answer 500 (closes #187) #189

clawbot referenced this issue

2026-08-18 10:50:50 +02:00

Milestone 1.0.0: internet-facing readiness #111

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#193