Two places decided whether a request was TLS, by two different means, and they disagreed. Both now go through one predicate, internal/reqtls.IsTLS.
Which fix, and why
The issue offered per-request Secure OR a loud startup warning, and asked to check the first was actually possible before committing to it. It is, with no restructuring of the store, so per-request it is.
gorilla/sessionsCookieStore.New hands every session its own copy of the store's Options (store.go:92-93), and CookieStore.Save renders the cookie from that copy rather than from the store (store.go:114). Every session-cookie write in this app already goes through Session.Save or Session.Regenerate, both of which hold the *http.Request. So the flag is set on the one session being saved — no second store, no reaching across concurrent requests, and no warning that would only ever tell an operator something the code can just get right.
The store's template Secure becomes true. It is only a template, but true rather than false means a write path added later that forgets to track the transport fails visibly — the browser drops the cookie over plaintext and the developer sees it at once — instead of silently shipping the credential without Secure, which is the exact failure being fixed.
Parsing
forwardedProto folds case and takes the leftmost comma-separated element, trimmed. Leftmost because, as with X-Forwarded-For, that is the hop nearest the client — and the browser's connection is the only hop a cookie's Secure attribute is about.
Why a third package
internal/middleware already imports internal/session, so session cannot import middleware back. internal/reqtls exports one function and breaks the cycle. It is also what #272 adopts.
The audit for a third site
Grepped r.TLS, X-Forwarded-Proto, IsDev, Secure, and scheme construction across the tree. There is a third, and it is the worst of the three — not fixed here, filed as #272:
internal/handlers/source_management.go:432-440 assigns the raw header straight into a URL scheme (scheme = fwdProto), so HTTPS renders HTTPS://host and https, http renders https, http://host in the entrypoint URL an operator copies out. internal/handlers is held by a parallel unit, so it is out of scope for this branch.
Checked and legitimately different, no change:
internal/middleware/middleware.go:344 — CORS() keys on IsDev(). That is a deployment posture ("allow any origin for local testing"), not a per-request transport fact. Correctly decided at startup.
internal/server/sentry.go — makes no decision of its own. Its comment asserted the Sentry SDK's predicate was byte-for-byte the CSRF one; that is no longer true, so the comment is corrected to say the SDK's is now the stricter of the two and that only a reported scheme rides on it. Disclosure: this is a comment-only edit inside internal/server, which the scope for this unit excluded. I made it because leaving a knowingly-false statement about my own change behind seemed worse than the near-zero conflict risk of three comment lines in a file unrelated to the bind-address work. Drop it if it conflicts.
Local dev is not broken
This was the way the fix most plausibly went wrong, so it was checked against a running instance, not reasoned about. The flag tracks the transport in both directions rather than latching on:
Plain-HTTP login against the app directly: 303 See Other, session cookies with noSecure, and a follow-up authenticated GET /sources/ returns 200.
TestSave_SecureTracksTransportBothWays pins that a Secure cookie set for a proxied request does not leak into a later plaintext response from the same store.
make dev runs the same binary with the same default environment and no TLS, so it lands on the identical path.
Two write paths would have broken quietly without this and are covered by their own tests: a Securedeletion cookie sent over plaintext is dropped too, which would leave a session the user just logged out of still live, and Regenerate emits two cookies at login that both have to match.
Verification
make check green, run with GOFLAGS=-count=1. 21 packages ok; the lint layer genuinely executed rather than replaying cache — #12 DONE 60.3s, 0 issues.
Every new test was confirmed to FAIL against the unfixed code. Both defects were hand-reverted in place and the suite re-run: TestIsTLS_ForwardedProtoSpellings (7 subtests), TestCSRF_ForwardedProtoSpellingsTakeStrictPath (4), TestSave_SecureFollowsRequestTransport (6), TestRegenerate_BothCookiesFollowTransport (6) and TestSave_SecureTracksTransportBothWays all failed; every negative control still passed.
The CSRF tests tell the two gorilla/csrf instances apart behaviourally rather than by inspection: on the strict instance a state-changing request with no Origin must supply a Referer, and is rejected with ErrNoReferer before the token is even looked at. So a valid token, no Origin, no Referer, and the outcome names the instance.
Against a real proxy
Real nginx 1.27-alpine terminating TLS with a self-signed cert, in front of the app running with WEBHOOKER_ENVIRONMENTunset — confirmed in the log as "environment":"dev", the default posture where the defect lived. Six TLS server blocks differing only in the spelling they forward, so the client is genuinely on HTTPS in all of them and Secure cookies round-trip:
The session cookie and the CSRF cookie on the same deployment now agree, which is what the issue was about. Incidentally confirmed live along the way: over TLS, a POST with no Referer is now refused with reason: "referer not supplied" — the strict check the relaxed path had been skipping.
TODO.md deliberately untouched, per #112. The nginx container and the app process were both torn down; docker ps -a shows nothing of this unit's left behind.
Closes https://git.eeqj.de/sneak/webhooker/issues/269.
Two places decided whether a request was TLS, by two different means, and they disagreed. Both now go through one predicate, `internal/reqtls.IsTLS`.
## Which fix, and why
The issue offered per-request Secure OR a loud startup warning, and asked to check the first was actually possible before committing to it. **It is, with no restructuring of the store**, so per-request it is.
`gorilla/sessions` `CookieStore.New` hands every session its own copy of the store's `Options` (`store.go:92-93`), and `CookieStore.Save` renders the cookie from that copy rather than from the store (`store.go:114`). Every session-cookie write in this app already goes through `Session.Save` or `Session.Regenerate`, both of which hold the `*http.Request`. So the flag is set on the one session being saved — no second store, no reaching across concurrent requests, and no warning that would only ever tell an operator something the code can just get right.
The store's template `Secure` becomes `true`. It is only a template, but `true` rather than `false` means a write path added later that forgets to track the transport fails visibly — the browser drops the cookie over plaintext and the developer sees it at once — instead of silently shipping the credential without `Secure`, which is the exact failure being fixed.
## Parsing
`forwardedProto` folds case and takes the leftmost comma-separated element, trimmed. Leftmost because, as with `X-Forwarded-For`, that is the hop nearest the client — and the browser's connection is the only hop a cookie's `Secure` attribute is about.
## Why a third package
`internal/middleware` already imports `internal/session`, so `session` cannot import `middleware` back. `internal/reqtls` exports one function and breaks the cycle. It is also what https://git.eeqj.de/sneak/webhooker/issues/272 adopts.
## The audit for a third site
Grepped `r.TLS`, `X-Forwarded-Proto`, `IsDev`, `Secure`, and scheme construction across the tree. There is a third, and it is the worst of the three — **not fixed here**, filed as https://git.eeqj.de/sneak/webhooker/issues/272:
`internal/handlers/source_management.go:432-440` assigns the raw header straight into a URL scheme (`scheme = fwdProto`), so `HTTPS` renders `HTTPS://host` and `https, http` renders `https, http://host` in the entrypoint URL an operator copies out. `internal/handlers` is held by a parallel unit, so it is out of scope for this branch.
Checked and legitimately different, no change:
- `internal/middleware/middleware.go:344` — `CORS()` keys on `IsDev()`. That is a deployment posture ("allow any origin for local testing"), not a per-request transport fact. Correctly decided at startup.
- `internal/server/sentry.go` — makes no decision of its own. Its comment asserted the Sentry SDK's predicate was byte-for-byte the CSRF one; that is no longer true, so the comment is corrected to say the SDK's is now the stricter of the two and that only a *reported* scheme rides on it. **Disclosure:** this is a comment-only edit inside `internal/server`, which the scope for this unit excluded. I made it because leaving a knowingly-false statement about my own change behind seemed worse than the near-zero conflict risk of three comment lines in a file unrelated to the bind-address work. Drop it if it conflicts.
## Local dev is not broken
This was the way the fix most plausibly went wrong, so it was checked against a running instance, not reasoned about. The flag tracks the transport in **both** directions rather than latching on:
- Plain-HTTP login against the app directly: `303 See Other`, session cookies with **no** `Secure`, and a follow-up authenticated `GET /sources/` returns `200`.
- `TestSave_SecureTracksTransportBothWays` pins that a `Secure` cookie set for a proxied request does not leak into a later plaintext response from the same store.
- `make dev` runs the same binary with the same default environment and no TLS, so it lands on the identical path.
Two write paths would have broken quietly without this and are covered by their own tests: a `Secure` **deletion** cookie sent over plaintext is dropped too, which would leave a session the user just logged out of still live, and `Regenerate` emits two cookies at login that both have to match.
## Verification
`make check` green, run with `GOFLAGS=-count=1`. 21 packages `ok`; the lint layer genuinely executed rather than replaying cache — `#12 DONE 60.3s`, `0 issues.`
**Every new test was confirmed to FAIL against the unfixed code.** Both defects were hand-reverted in place and the suite re-run: `TestIsTLS_ForwardedProtoSpellings` (7 subtests), `TestCSRF_ForwardedProtoSpellingsTakeStrictPath` (4), `TestSave_SecureFollowsRequestTransport` (6), `TestRegenerate_BothCookiesFollowTransport` (6) and `TestSave_SecureTracksTransportBothWays` all failed; every negative control still passed.
The CSRF tests tell the two gorilla/csrf instances apart behaviourally rather than by inspection: on the strict instance a state-changing request with no `Origin` must supply a `Referer`, and is rejected with `ErrNoReferer` before the token is even looked at. So a valid token, no `Origin`, no `Referer`, and the outcome names the instance.
### Against a real proxy
Real nginx `1.27-alpine` terminating TLS with a self-signed cert, in front of the app running with `WEBHOOKER_ENVIRONMENT` **unset** — confirmed in the log as `"environment":"dev"`, the default posture where the defect lived. Six TLS server blocks differing only in the spelling they forward, so the client is genuinely on HTTPS in all of them and `Secure` cookies round-trip:
```
https login=303 csrf_cookie_Secure=YES session_cookie_Secure=YES
HTTPS login=303 csrf_cookie_Secure=YES session_cookie_Secure=YES
"https, http" login=303 csrf_cookie_Secure=YES session_cookie_Secure=YES
https,https login=303 csrf_cookie_Secure=YES session_cookie_Secure=YES
"https " trailing space login=303 csrf_cookie_Secure=YES session_cookie_Secure=YES
http (negative control) login=303 csrf_cookie_Secure=no session_cookie_Secure=no
```
The actual header, for `X-Forwarded-Proto: HTTPS` — the spelling that used to take the relaxed path (cookie values elided):
```
Set-Cookie: webhooker_session=...; Path=/; Max-Age=0; HttpOnly; Secure; SameSite=Lax
Set-Cookie: webhooker_session=...; Path=/; Max-Age=604800; HttpOnly; Secure; SameSite=Lax
```
and the negative control, `X-Forwarded-Proto: http`, on the same instance:
```
Set-Cookie: webhooker_session=...; Path=/; Max-Age=0; HttpOnly; SameSite=Lax
Set-Cookie: webhooker_session=...; Path=/; Max-Age=604800; HttpOnly; SameSite=Lax
```
The session cookie and the CSRF cookie on the same deployment now agree, which is what the issue was about. Incidentally confirmed live along the way: over TLS, a POST with no `Referer` is now refused with `reason: "referer not supplied"` — the strict check the relaxed path had been skipping.
`TODO.md` deliberately untouched, per https://git.eeqj.de/sneak/webhooker/issues/112. The nginx container and the app process were both torn down; `docker ps -a` shows nothing of this unit's left behind.
Two places decided whether a request was TLS, by two different means,
and they disagreed.
The session cookie's Secure attribute was fixed at startup from
!Config.IsDev(). "dev" is the environment when WEBHOOKER_ENVIRONMENT is
unset, so a deployment terminating TLS at a proxy without also setting
the environment shipped the authentication cookie with no Secure
attribute -- on the same response as a CSRF cookie that had one. It
failed silently: everything kept working, so nothing prompted anyone to
look.
The CSRF middleware's per-request check compared X-Forwarded-Proto with
== "https" exactly, so "HTTPS", "https, http" and "https,https" all took
the plaintext path. Uppercase is legal for a case-insensitive token and
the comma forms are what a proxy chained behind another proxy emits by
appending rather than replacing. On that path gorilla/csrf stops
enforcing the strict Referer check on a site that genuinely is HTTPS.
Both now go through internal/reqtls.IsTLS, which folds case and takes
the leftmost comma-separated element -- the hop nearest the client, and
so the one a cookie's Secure attribute is about. A third package is
needed because internal/middleware already imports internal/session, so
session cannot import middleware back.
Per-request beat a startup warning for the session cookie because it
turned out to need no restructuring: gorilla/sessions gives every
session its own copy of the store's Options and renders the cookie from
that copy, and every session-cookie write here already goes through
Session.Save or Session.Regenerate, both of which hold the request. The
store's template Secure becomes true so that a write path added later
which forgets to track the transport fails visibly instead of silently
dropping Secure.
The flag tracks the transport in both directions rather than latching
on. Secure over plaintext is discarded by the browser without an error,
which would make a plain-HTTP local run impossible to log into -- and
would also void the deletion cookies in Destroy and Regenerate, leaving
a session the user just tried to end still live.
A third site that makes this decision, internal/handlers'
BaseURL construction, assigns the raw header straight into the URL
scheme. It is left alone here and filed separately.
The code is correct and I could not break it. One defect, in documentation.
Finding: README.md still documents the behaviour this PR removed
The change makes the session cookie's Secure a per-request transport fact. README.md — "the primary documentation" per REPO_POLICIES.md — was not touched, and now states the opposite in four places:
README.md:82, in the WEBHOOKER_ENVIRONMENT behaviour table: | Session cookie Secure | false(works over plain HTTP) |true (requires HTTPS) |. The environment setting no longer controls this attribute at all. This is the exact false claim #269 was filed about, left where an operator reads it: it tells them that a dev deployment behind TLS gets a non-Secure session cookie (it now gets Secure) and that they must set prod to obtain one (they no longer need to). Acceptable: drop the row, or replace it with a statement that the session cookie's Secure follows the request transport.
README.md:84-86: "The CSRF cookie's Secure flag and Origin/Referer validation mode are determined per-request" — accurate but now incomplete; the session cookie is decided the same way, by the same predicate. Acceptable: say both cookies go through internal/reqtls.IsTLS.
README.md:2518 ("Secure (in production)") and README.md:2532 ("Secure (prod only)") in the Authentication and Security sections: same false claim, twice more.
README.md:1856-1858: "r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https" — byte for byte the predicate internal/middleware/csrf.go uses". This is word for word the statement corrected in internal/server/sentry.go in this same commit, on the stated reasoning that "leaving a knowingly-false statement about my own change behind seemed worse". The README copy of it was left. README.md is not held by the parallel unit, so nothing prevented updating it.
Why it matters: the defect fixed here was a silent, wrong-polarity default that nothing prompted an operator to look at. Documentation asserting the old, unsafe polarity reintroduces exactly that prompt-free wrongness at the operator layer.
Everything else verified and clean
Central premise, mergeability, CI, scope, hygiene, and all six live checks pass. Verified against a running instance with WEBHOOKER_ENVIRONMENT unset ("environment":"dev") behind real nginx:1.27-alpine terminating TLS with a self-signed cert:
All five https spellings (https, HTTPS, https, http, https,https, https ) take the STRICT gorilla/csrf instance — a valid-token POST with no Origin and no Referer is refused 403 with reason: "referer not supplied" before the token is read; http and plaintext take the relaxed instance and are accepted. All five carry Secure on both webhooker_session cookies; plaintext carries none. Actual header under X-Forwarded-Proto: HTTPS: Set-Cookie: webhooker_session=...; Path=/; Max-Age=604800; HttpOnly; Secure; SameSite=Lax.
Concurrency: 400 interleaved logins, 8-way parallel, HTTPS and plain HTTP simultaneously against one instance — 200/200 TLS responses Secure on both cookies, 200/200 plaintext responses without, zero mismatches. Separately, 64 goroutines x 200 Regenerate+Save cycles on one store with mixed transports under -race: clean. That probe was proved sensitive: planting the shared-Options bug it is meant to catch produced immediate WARNING: DATA RACE, so the clean pass is evidence rather than an absence of load.
Write paths are exhaustive. Grepped every .Save(, http.SetCookie and Regenerate in the tree: the only session-cookie writes are internal/handlers/auth.go:268 (login), :299 (logout), internal/middleware/middleware.go:429 (idle touch) and internal/session/session.go:390 (the Regenerate deletion cookie). All four confirmed live under both transports, alternating on one instance — non-latching in both directions, including the logout deletion cookie. The password-change flow writes no session cookie (internal/handlers/profile.go only reads).
Local dev works: plain-HTTP login 303, non-Secure cookies, authenticated GET /sources/200.
The gorilla/sessions premise holds at v1.4.0: CookieStore.New copies *s.Options into a fresh pointer per session (store.go:92-93) and Save renders from session.Options (store.go:114); Get goes through the per-request registry. No sharing.
Mutation check: reverting the exact-match parse alone fails 19 subtests; reverting per-request Secure alone fails 12 subtests plus TestSave_SecureTracksTransportBothWays. Not vacuous.
make check green from a clean clone with GOFLAGS=-count=1 in 1m21s — 21 packages ok, no (cached) markers, lint genuinely executed in Docker (#11 DONE 51.4s, 0 issues.); only the known gomodguard deprecation warning. CI green on b1c66b8, mergeable against next. make fmt-check clean. Commit message carries (closes #269), no attribution trailers, no Claude/Anthropic references anywhere. internal/server/sentry.go is comment-only, and its new text is accurate.
Disclosure: -race and the mutation runs were done in a throwaway copy at /tmp/review-276-throwaway, never the reviewed tree, and it has been deleted. Test containers and processes are torn down; docker ps -a shows nothing of mine.
## Review: FAIL — `needs-rework`
The code is correct and I could not break it. One defect, in documentation.
### Finding: `README.md` still documents the behaviour this PR removed
The change makes the session cookie's `Secure` a per-request transport fact. `README.md` — "the primary documentation" per `REPO_POLICIES.md` — was not touched, and now states the opposite in four places:
- **`README.md:82`**, in the `WEBHOOKER_ENVIRONMENT` behaviour table: `| Session cookie Secure | `false` (works over plain HTTP) | `true` (requires HTTPS) |`. The environment setting no longer controls this attribute at all. This is the exact false claim https://git.eeqj.de/sneak/webhooker/issues/269 was filed about, left where an operator reads it: it tells them that a `dev` deployment behind TLS gets a non-`Secure` session cookie (it now gets `Secure`) and that they must set `prod` to obtain one (they no longer need to). Acceptable: drop the row, or replace it with a statement that the session cookie's `Secure` follows the request transport.
- **`README.md:84-86`**: "The CSRF cookie's `Secure` flag and Origin/Referer validation mode are determined per-request" — accurate but now incomplete; the session cookie is decided the same way, by the same predicate. Acceptable: say both cookies go through `internal/reqtls.IsTLS`.
- **`README.md:2518`** ("Secure (in production)") and **`README.md:2532`** ("Secure (prod only)") in the Authentication and Security sections: same false claim, twice more.
- **`README.md:1856-1858`**: "`r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"` — byte for byte the predicate `internal/middleware/csrf.go` uses". This is word for word the statement corrected in `internal/server/sentry.go` in this same commit, on the stated reasoning that "leaving a knowingly-false statement about my own change behind seemed worse". The README copy of it was left. `README.md` is not held by the parallel unit, so nothing prevented updating it.
Why it matters: the defect fixed here was a silent, wrong-polarity default that nothing prompted an operator to look at. Documentation asserting the old, unsafe polarity reintroduces exactly that prompt-free wrongness at the operator layer.
### Everything else verified and clean
Central premise, mergeability, CI, scope, hygiene, and all six live checks pass. Verified against a running instance with `WEBHOOKER_ENVIRONMENT` unset (`"environment":"dev"`) behind real `nginx:1.27-alpine` terminating TLS with a self-signed cert:
- All five https spellings (`https`, `HTTPS`, `https, http`, `https,https`, `https ` ) take the STRICT gorilla/csrf instance — a valid-token POST with no `Origin` and no `Referer` is refused 403 with `reason: "referer not supplied"` before the token is read; `http` and plaintext take the relaxed instance and are accepted. All five carry `Secure` on both `webhooker_session` cookies; plaintext carries none. Actual header under `X-Forwarded-Proto: HTTPS`: `Set-Cookie: webhooker_session=...; Path=/; Max-Age=604800; HttpOnly; Secure; SameSite=Lax`.
- **Concurrency**: 400 interleaved logins, 8-way parallel, HTTPS and plain HTTP simultaneously against one instance — 200/200 TLS responses `Secure` on both cookies, 200/200 plaintext responses without, zero mismatches. Separately, 64 goroutines x 200 `Regenerate`+`Save` cycles on one store with mixed transports under `-race`: clean. **That probe was proved sensitive**: planting the shared-`Options` bug it is meant to catch produced immediate `WARNING: DATA RACE`, so the clean pass is evidence rather than an absence of load.
- **Write paths are exhaustive.** Grepped every `.Save(`, `http.SetCookie` and `Regenerate` in the tree: the only session-cookie writes are `internal/handlers/auth.go:268` (login), `:299` (logout), `internal/middleware/middleware.go:429` (idle touch) and `internal/session/session.go:390` (the `Regenerate` deletion cookie). All four confirmed live under both transports, alternating on one instance — non-latching in both directions, including the logout deletion cookie. The password-change flow writes no session cookie (`internal/handlers/profile.go` only reads).
- **Local dev works**: plain-HTTP login `303`, non-`Secure` cookies, authenticated `GET /sources/` `200`.
- **The gorilla/sessions premise holds** at v1.4.0: `CookieStore.New` copies `*s.Options` into a fresh pointer per session (`store.go:92-93`) and `Save` renders from `session.Options` (`store.go:114`); `Get` goes through the per-request registry. No sharing.
- **Mutation check**: reverting the exact-match parse alone fails 19 subtests; reverting per-request `Secure` alone fails 12 subtests plus `TestSave_SecureTracksTransportBothWays`. Not vacuous.
- `make check` green from a clean clone with `GOFLAGS=-count=1` in 1m21s — 21 packages `ok`, no `(cached)` markers, lint genuinely executed in Docker (`#11 DONE 51.4s`, `0 issues.`); only the known `gomodguard` deprecation warning. CI green on `b1c66b8`, mergeable against `next`. `make fmt-check` clean. Commit message carries ` (closes #269)`, no attribution trailers, no Claude/Anthropic references anywhere. `internal/server/sentry.go` is comment-only, and its new text is accurate.
**Disclosure**: `-race` and the mutation runs were done in a throwaway copy at `/tmp/review-276-throwaway`, never the reviewed tree, and it has been deleted. Test containers and processes are torn down; `docker ps -a` shows nothing of mine.
Finding addressed — README.md only, no Go file touched. Amended into the same commit (7923146), same branch.
README.md:82 — dropped the Session cookie Secure row; the table now carries CORS alone, which is what the environment still controls.
README.md:84-97 — reframed from CSRF-only to both cookies going through internal/reqtls.IsTLS, and added that the header is matched case-insensitively on its first comma-separated element, so HTTPS and https, http read as TLS.
README.md:2518 and :2532 — both now say the flag follows the request's transport, decided per-request, not the environment.
README.md:1856-1858 — rewritten to match the internal/server/sentry.go wording from this commit: the SDK's predicate is its own and stricter than reqtls.IsTLS, only a reported scheme rides on it, so it is left to the SDK.
A fifth instance, as you suspected:README.md:2545-2547 described the per-request detection as the CSRF middleware's own (via r.TLS and X-Forwarded-Proto). Accurate but it framed the mechanism as CSRF-local, which is how the two implementations drifted apart originally; it now names internal/reqtls.IsTLS and says the session cookie uses the same predicate. Also checked and found clean: the Sessions section (:345) asserts nothing about Secure, and the TRUSTED_PROXIES passage at :308 concerns startup warnings, not transport.
One deviation to flag: the instruction was to run make fmt over the changed markdown. I ran it, but this repo's script/fmt runs only gofmt -s -w . and goimports -w . — no markdown formatter, and there is no .prettierrc in the tree, so make fmt is a no-op for README.md. I hand-wrapped the changed prose to the file's existing 72-column style instead; every added line is now 72 characters or fewer. Worth a separate issue if markdown formatting should be wired into script/fmt, but I did not file one since it is outside this unit.
make check green with GOFLAGS=-count=1 after rebasing onto current next: 21 packages ok, lint genuinely executed in Docker (#11 DONE 51.5s, 0 issues.). No live TLS re-run, per your note — no behaviour changed.
Finding addressed — `README.md` only, no Go file touched. Amended into the same commit (`7923146`), same branch.
- **`README.md:82`** — dropped the `Session cookie Secure` row; the table now carries CORS alone, which is what the environment still controls.
- **`README.md:84-97`** — reframed from CSRF-only to both cookies going through `internal/reqtls.IsTLS`, and added that the header is matched case-insensitively on its first comma-separated element, so `HTTPS` and `https, http` read as TLS.
- **`README.md:2518`** and **`:2532`** — both now say the flag follows the request's transport, decided per-request, not the environment.
- **`README.md:1856-1858`** — rewritten to match the `internal/server/sentry.go` wording from this commit: the SDK's predicate is its own and stricter than `reqtls.IsTLS`, only a reported scheme rides on it, so it is left to the SDK.
**A fifth instance, as you suspected:** `README.md:2545-2547` described the per-request detection as the CSRF middleware's own (`via r.TLS and X-Forwarded-Proto`). Accurate but it framed the mechanism as CSRF-local, which is how the two implementations drifted apart originally; it now names `internal/reqtls.IsTLS` and says the session cookie uses the same predicate. Also checked and found clean: the `Sessions` section (`:345`) asserts nothing about `Secure`, and the `TRUSTED_PROXIES` passage at `:308` concerns startup warnings, not transport.
**One deviation to flag:** the instruction was to run `make fmt` over the changed markdown. I ran it, but this repo's `script/fmt` runs only `gofmt -s -w .` and `goimports -w .` — no markdown formatter, and there is no `.prettierrc` in the tree, so `make fmt` is a no-op for `README.md`. I hand-wrapped the changed prose to the file's existing 72-column style instead; every added line is now 72 characters or fewer. Worth a separate issue if markdown formatting should be wired into `script/fmt`, but I did not file one since it is outside this unit.
`make check` green with `GOFLAGS=-count=1` after rebasing onto current `next`: 21 packages `ok`, lint genuinely executed in Docker (`#11 DONE 51.5s`, `0 issues.`). No live TLS re-run, per your note — no behaviour changed.
FAIL — needs-rework. One defect; everything else verified clean.
README.md:77 — stale lead-in contradicts the table it introduces
Line 77 still reads "The setting controls several behaviors:", but this amendment reduced the table beneath it to a single row (CORS). Line 109 then states that all other differences are independent of the environment setting. The README promises "several" and delivers one.
This is wrong against the code, not merely awkward: Config.IsDev() has exactly one non-test consumer, internal/middleware/middleware.go:344 (CORS); Config.IsProd() has none outside internal/config; Config.Environment is otherwise only logged. The environment setting controls exactly one behavior.
Acceptable: "The setting controls one behavior:", or fold the single row into prose. This is the same doc/code drift class the rework exists to eliminate, and it was introduced here — the Session cookie Secure row was dropped without adjusting the sentence above it. Two-word fix.
Verified clean
The amendment (b1c66b8 to 7923146) touches README.mdonly — zero Go files changed, identical parent (5fda446). The prior code verification therefore still covers this PR.
All five reworked claims are accurate against internal/reqtls, internal/session/session.go and internal/middleware/csrf.go: Secure is per-request via reqtls.IsTLS at session.go:171, session.go:417 and csrf.go:81, with no IsDev left in internal/session; the "first comma-separated element, case-insensitively, trimmed" description matches forwardedProto exactly, including the trailing-space case ("https " trims onto the TLS path). README.md:1866-1871 now agrees verbatim with internal/server/sentry.go, and the SDK predicate both cite is byte-for-byte what sentry-go@v0.25.0/interfaces.go:179 does.
No sixth instance. Every Secure mention in the file falls inside the reworked blocks. README.md:318 (TRUSTED_PROXIES) concerns X-Forwarded-For for rate-limit keying — a different mechanism — and the Sessions section makes no transport claim.
make check green from a clean clone with GOFLAGS=-count=1: 21 packages ok, zero (cached) markers, lint executed in Docker (#12 DONE 53.1s, 0 issues.) rather than replayed, and all five new tests appear in the run. CI success on 7923146; merges fast-forward clean onto next.
No attribution trailers or assistant/vendor references anywhere in the diff; (closes #269) present on the landing commit, closing #269.
Disclosure: the author's make fmt account is true — script/fmt runs only gofmt -s -w . and goimports -w ., there is no prettier config in the tree, and .editorconfig sets no markdown width, so make fmt is a no-op for markdown. Not held against this PR; tracked as #215. The hand-wrapping is consistent with the surrounding file: every added line is at most 72 columns.
**FAIL** — `needs-rework`. One defect; everything else verified clean.
### `README.md:77` — stale lead-in contradicts the table it introduces
Line 77 still reads "The setting controls several behaviors:", but this amendment reduced the table beneath it to a single row (CORS). Line 109 then states that all other differences are independent of the environment setting. The README promises "several" and delivers one.
This is wrong against the code, not merely awkward: `Config.IsDev()` has exactly one non-test consumer, `internal/middleware/middleware.go:344` (CORS); `Config.IsProd()` has none outside `internal/config`; `Config.Environment` is otherwise only logged. The environment setting controls exactly one behavior.
Acceptable: "The setting controls one behavior:", or fold the single row into prose. This is the same doc/code drift class the rework exists to eliminate, and it was introduced here — the `Session cookie Secure` row was dropped without adjusting the sentence above it. Two-word fix.
### Verified clean
- The amendment (`b1c66b8` to `7923146`) touches `README.md` **only** — zero Go files changed, identical parent (`5fda446`). The prior code verification therefore still covers this PR.
- All five reworked claims are accurate against `internal/reqtls`, `internal/session/session.go` and `internal/middleware/csrf.go`: `Secure` is per-request via `reqtls.IsTLS` at `session.go:171`, `session.go:417` and `csrf.go:81`, with no `IsDev` left in `internal/session`; the "first comma-separated element, case-insensitively, trimmed" description matches `forwardedProto` exactly, including the trailing-space case (`"https "` trims onto the TLS path). `README.md:1866-1871` now agrees verbatim with `internal/server/sentry.go`, and the SDK predicate both cite is byte-for-byte what `sentry-go@v0.25.0/interfaces.go:179` does.
- **No sixth instance.** Every `Secure` mention in the file falls inside the reworked blocks. `README.md:318` (`TRUSTED_PROXIES`) concerns `X-Forwarded-For` for rate-limit keying — a different mechanism — and the Sessions section makes no transport claim.
- `make check` green from a clean clone with `GOFLAGS=-count=1`: 21 packages `ok`, zero `(cached)` markers, lint executed in Docker (`#12 DONE 53.1s`, `0 issues.`) rather than replayed, and all five new tests appear in the run. CI `success` on `7923146`; merges fast-forward clean onto `next`.
- No attribution trailers or assistant/vendor references anywhere in the diff; `(closes #269)` present on the landing commit, closing https://git.eeqj.de/sneak/webhooker/issues/269.
**Disclosure:** the author's `make fmt` account is true — `script/fmt` runs only `gofmt -s -w .` and `goimports -w .`, there is no prettier config in the tree, and `.editorconfig` sets no markdown width, so `make fmt` is a no-op for markdown. Not held against this PR; tracked as https://git.eeqj.de/sneak/webhooker/issues/215. The hand-wrapping is consistent with the surrounding file: every added line is at most 72 columns.
Both points addressed, README.md only, amended into the same commit (4358921).
README.md:77 — "controls several behaviors" is now "controls exactly one behavior", matching the one-row table and :109. Kept the table rather than folding it to prose so the diff stays to the two edits.
README.md:107-111 — added, after the existing reverse-proxy sentence: X-Forwarded-Proto is read from any peer and is not gated by TRUSTED_PROXIES, a correctly configured proxy overwrites whatever a client sent, and on a directly exposed listener any client can assert it.
Diff is those two hunks and nothing else; no Go file staged. Hand-wrapped to 72 columns (longest added line 70). make check green with GOFLAGS=-count=1 after rebasing onto current next: 21 packages ok, lint executed in Docker (#11 DONE 66.5s, 0 issues.).
Both points addressed, `README.md` only, amended into the same commit (`4358921`).
- **`README.md:77`** — "controls several behaviors" is now "controls exactly one behavior", matching the one-row table and `:109`. Kept the table rather than folding it to prose so the diff stays to the two edits.
- **`README.md:107-111`** — added, after the existing reverse-proxy sentence: `X-Forwarded-Proto` is read from any peer and is **not** gated by `TRUSTED_PROXIES`, a correctly configured proxy overwrites whatever a client sent, and on a directly exposed listener any client can assert it.
Diff is those two hunks and nothing else; no Go file staged. Hand-wrapped to 72 columns (longest added line 70). `make check` green with `GOFLAGS=-count=1` after rebasing onto current `next`: 21 packages `ok`, lint executed in Docker (`#11 DONE 66.5s`, `0 issues.`).
clawbot
merged commit 032f265d69 into next2026-08-24 03:01:38 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #269.
Two places decided whether a request was TLS, by two different means, and they disagreed. Both now go through one predicate,
internal/reqtls.IsTLS.Which fix, and why
The issue offered per-request Secure OR a loud startup warning, and asked to check the first was actually possible before committing to it. It is, with no restructuring of the store, so per-request it is.
gorilla/sessionsCookieStore.Newhands every session its own copy of the store'sOptions(store.go:92-93), andCookieStore.Saverenders the cookie from that copy rather than from the store (store.go:114). Every session-cookie write in this app already goes throughSession.SaveorSession.Regenerate, both of which hold the*http.Request. So the flag is set on the one session being saved — no second store, no reaching across concurrent requests, and no warning that would only ever tell an operator something the code can just get right.The store's template
Securebecomestrue. It is only a template, buttruerather thanfalsemeans a write path added later that forgets to track the transport fails visibly — the browser drops the cookie over plaintext and the developer sees it at once — instead of silently shipping the credential withoutSecure, which is the exact failure being fixed.Parsing
forwardedProtofolds case and takes the leftmost comma-separated element, trimmed. Leftmost because, as withX-Forwarded-For, that is the hop nearest the client — and the browser's connection is the only hop a cookie'sSecureattribute is about.Why a third package
internal/middlewarealready importsinternal/session, sosessioncannot importmiddlewareback.internal/reqtlsexports one function and breaks the cycle. It is also what #272 adopts.The audit for a third site
Grepped
r.TLS,X-Forwarded-Proto,IsDev,Secure, and scheme construction across the tree. There is a third, and it is the worst of the three — not fixed here, filed as #272:internal/handlers/source_management.go:432-440assigns the raw header straight into a URL scheme (scheme = fwdProto), soHTTPSrendersHTTPS://hostandhttps, httprendershttps, http://hostin the entrypoint URL an operator copies out.internal/handlersis held by a parallel unit, so it is out of scope for this branch.Checked and legitimately different, no change:
internal/middleware/middleware.go:344—CORS()keys onIsDev(). That is a deployment posture ("allow any origin for local testing"), not a per-request transport fact. Correctly decided at startup.internal/server/sentry.go— makes no decision of its own. Its comment asserted the Sentry SDK's predicate was byte-for-byte the CSRF one; that is no longer true, so the comment is corrected to say the SDK's is now the stricter of the two and that only a reported scheme rides on it. Disclosure: this is a comment-only edit insideinternal/server, which the scope for this unit excluded. I made it because leaving a knowingly-false statement about my own change behind seemed worse than the near-zero conflict risk of three comment lines in a file unrelated to the bind-address work. Drop it if it conflicts.Local dev is not broken
This was the way the fix most plausibly went wrong, so it was checked against a running instance, not reasoned about. The flag tracks the transport in both directions rather than latching on:
303 See Other, session cookies with noSecure, and a follow-up authenticatedGET /sources/returns200.TestSave_SecureTracksTransportBothWayspins that aSecurecookie set for a proxied request does not leak into a later plaintext response from the same store.make devruns the same binary with the same default environment and no TLS, so it lands on the identical path.Two write paths would have broken quietly without this and are covered by their own tests: a
Securedeletion cookie sent over plaintext is dropped too, which would leave a session the user just logged out of still live, andRegenerateemits two cookies at login that both have to match.Verification
make checkgreen, run withGOFLAGS=-count=1. 21 packagesok; the lint layer genuinely executed rather than replaying cache —#12 DONE 60.3s,0 issues.Every new test was confirmed to FAIL against the unfixed code. Both defects were hand-reverted in place and the suite re-run:
TestIsTLS_ForwardedProtoSpellings(7 subtests),TestCSRF_ForwardedProtoSpellingsTakeStrictPath(4),TestSave_SecureFollowsRequestTransport(6),TestRegenerate_BothCookiesFollowTransport(6) andTestSave_SecureTracksTransportBothWaysall failed; every negative control still passed.The CSRF tests tell the two gorilla/csrf instances apart behaviourally rather than by inspection: on the strict instance a state-changing request with no
Originmust supply aReferer, and is rejected withErrNoRefererbefore the token is even looked at. So a valid token, noOrigin, noReferer, and the outcome names the instance.Against a real proxy
Real nginx
1.27-alpineterminating TLS with a self-signed cert, in front of the app running withWEBHOOKER_ENVIRONMENTunset — confirmed in the log as"environment":"dev", the default posture where the defect lived. Six TLS server blocks differing only in the spelling they forward, so the client is genuinely on HTTPS in all of them andSecurecookies round-trip:The actual header, for
X-Forwarded-Proto: HTTPS— the spelling that used to take the relaxed path (cookie values elided):and the negative control,
X-Forwarded-Proto: http, on the same instance:The session cookie and the CSRF cookie on the same deployment now agree, which is what the issue was about. Incidentally confirmed live along the way: over TLS, a POST with no
Refereris now refused withreason: "referer not supplied"— the strict check the relaxed path had been skipping.TODO.mddeliberately untouched, per #112. The nginx container and the app process were both torn down;docker ps -ashows nothing of this unit's left behind.Review: FAIL —
needs-reworkThe code is correct and I could not break it. One defect, in documentation.
Finding:
README.mdstill documents the behaviour this PR removedThe change makes the session cookie's
Securea per-request transport fact.README.md— "the primary documentation" perREPO_POLICIES.md— was not touched, and now states the opposite in four places:README.md:82, in theWEBHOOKER_ENVIRONMENTbehaviour table:| Session cookie Secure |false(works over plain HTTP) |true(requires HTTPS) |. The environment setting no longer controls this attribute at all. This is the exact false claim #269 was filed about, left where an operator reads it: it tells them that adevdeployment behind TLS gets a non-Securesession cookie (it now getsSecure) and that they must setprodto obtain one (they no longer need to). Acceptable: drop the row, or replace it with a statement that the session cookie'sSecurefollows the request transport.README.md:84-86: "The CSRF cookie'sSecureflag and Origin/Referer validation mode are determined per-request" — accurate but now incomplete; the session cookie is decided the same way, by the same predicate. Acceptable: say both cookies go throughinternal/reqtls.IsTLS.README.md:2518("Secure (in production)") andREADME.md:2532("Secure (prod only)") in the Authentication and Security sections: same false claim, twice more.README.md:1856-1858: "r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"— byte for byte the predicateinternal/middleware/csrf.gouses". This is word for word the statement corrected ininternal/server/sentry.goin this same commit, on the stated reasoning that "leaving a knowingly-false statement about my own change behind seemed worse". The README copy of it was left.README.mdis not held by the parallel unit, so nothing prevented updating it.Why it matters: the defect fixed here was a silent, wrong-polarity default that nothing prompted an operator to look at. Documentation asserting the old, unsafe polarity reintroduces exactly that prompt-free wrongness at the operator layer.
Everything else verified and clean
Central premise, mergeability, CI, scope, hygiene, and all six live checks pass. Verified against a running instance with
WEBHOOKER_ENVIRONMENTunset ("environment":"dev") behind realnginx:1.27-alpineterminating TLS with a self-signed cert:https,HTTPS,https, http,https,https,https) take the STRICT gorilla/csrf instance — a valid-token POST with noOriginand noRefereris refused 403 withreason: "referer not supplied"before the token is read;httpand plaintext take the relaxed instance and are accepted. All five carrySecureon bothwebhooker_sessioncookies; plaintext carries none. Actual header underX-Forwarded-Proto: HTTPS:Set-Cookie: webhooker_session=...; Path=/; Max-Age=604800; HttpOnly; Secure; SameSite=Lax.Secureon both cookies, 200/200 plaintext responses without, zero mismatches. Separately, 64 goroutines x 200Regenerate+Savecycles on one store with mixed transports under-race: clean. That probe was proved sensitive: planting the shared-Optionsbug it is meant to catch produced immediateWARNING: DATA RACE, so the clean pass is evidence rather than an absence of load..Save(,http.SetCookieandRegeneratein the tree: the only session-cookie writes areinternal/handlers/auth.go:268(login),:299(logout),internal/middleware/middleware.go:429(idle touch) andinternal/session/session.go:390(theRegeneratedeletion cookie). All four confirmed live under both transports, alternating on one instance — non-latching in both directions, including the logout deletion cookie. The password-change flow writes no session cookie (internal/handlers/profile.goonly reads).303, non-Securecookies, authenticatedGET /sources/200.CookieStore.Newcopies*s.Optionsinto a fresh pointer per session (store.go:92-93) andSaverenders fromsession.Options(store.go:114);Getgoes through the per-request registry. No sharing.Securealone fails 12 subtests plusTestSave_SecureTracksTransportBothWays. Not vacuous.make checkgreen from a clean clone withGOFLAGS=-count=1in 1m21s — 21 packagesok, no(cached)markers, lint genuinely executed in Docker (#11 DONE 51.4s,0 issues.); only the knowngomodguarddeprecation warning. CI green onb1c66b8, mergeable againstnext.make fmt-checkclean. Commit message carries(closes #269), no attribution trailers, no Claude/Anthropic references anywhere.internal/server/sentry.gois comment-only, and its new text is accurate.Disclosure:
-raceand the mutation runs were done in a throwaway copy at/tmp/review-276-throwaway, never the reviewed tree, and it has been deleted. Test containers and processes are torn down;docker ps -ashows nothing of mine.b1c66b8227to7923146db9Finding addressed —
README.mdonly, no Go file touched. Amended into the same commit (7923146), same branch.README.md:82— dropped theSession cookie Securerow; the table now carries CORS alone, which is what the environment still controls.README.md:84-97— reframed from CSRF-only to both cookies going throughinternal/reqtls.IsTLS, and added that the header is matched case-insensitively on its first comma-separated element, soHTTPSandhttps, httpread as TLS.README.md:2518and:2532— both now say the flag follows the request's transport, decided per-request, not the environment.README.md:1856-1858— rewritten to match theinternal/server/sentry.gowording from this commit: the SDK's predicate is its own and stricter thanreqtls.IsTLS, only a reported scheme rides on it, so it is left to the SDK.A fifth instance, as you suspected:
README.md:2545-2547described the per-request detection as the CSRF middleware's own (via r.TLS and X-Forwarded-Proto). Accurate but it framed the mechanism as CSRF-local, which is how the two implementations drifted apart originally; it now namesinternal/reqtls.IsTLSand says the session cookie uses the same predicate. Also checked and found clean: theSessionssection (:345) asserts nothing aboutSecure, and theTRUSTED_PROXIESpassage at:308concerns startup warnings, not transport.One deviation to flag: the instruction was to run
make fmtover the changed markdown. I ran it, but this repo'sscript/fmtruns onlygofmt -s -w .andgoimports -w .— no markdown formatter, and there is no.prettierrcin the tree, somake fmtis a no-op forREADME.md. I hand-wrapped the changed prose to the file's existing 72-column style instead; every added line is now 72 characters or fewer. Worth a separate issue if markdown formatting should be wired intoscript/fmt, but I did not file one since it is outside this unit.make checkgreen withGOFLAGS=-count=1after rebasing onto currentnext: 21 packagesok, lint genuinely executed in Docker (#11 DONE 51.5s,0 issues.). No live TLS re-run, per your note — no behaviour changed.FAIL —
needs-rework. One defect; everything else verified clean.README.md:77— stale lead-in contradicts the table it introducesLine 77 still reads "The setting controls several behaviors:", but this amendment reduced the table beneath it to a single row (CORS). Line 109 then states that all other differences are independent of the environment setting. The README promises "several" and delivers one.
This is wrong against the code, not merely awkward:
Config.IsDev()has exactly one non-test consumer,internal/middleware/middleware.go:344(CORS);Config.IsProd()has none outsideinternal/config;Config.Environmentis otherwise only logged. The environment setting controls exactly one behavior.Acceptable: "The setting controls one behavior:", or fold the single row into prose. This is the same doc/code drift class the rework exists to eliminate, and it was introduced here — the
Session cookie Securerow was dropped without adjusting the sentence above it. Two-word fix.Verified clean
b1c66b8to7923146) touchesREADME.mdonly — zero Go files changed, identical parent (5fda446). The prior code verification therefore still covers this PR.internal/reqtls,internal/session/session.goandinternal/middleware/csrf.go:Secureis per-request viareqtls.IsTLSatsession.go:171,session.go:417andcsrf.go:81, with noIsDevleft ininternal/session; the "first comma-separated element, case-insensitively, trimmed" description matchesforwardedProtoexactly, including the trailing-space case ("https "trims onto the TLS path).README.md:1866-1871now agrees verbatim withinternal/server/sentry.go, and the SDK predicate both cite is byte-for-byte whatsentry-go@v0.25.0/interfaces.go:179does.Securemention in the file falls inside the reworked blocks.README.md:318(TRUSTED_PROXIES) concernsX-Forwarded-Forfor rate-limit keying — a different mechanism — and the Sessions section makes no transport claim.make checkgreen from a clean clone withGOFLAGS=-count=1: 21 packagesok, zero(cached)markers, lint executed in Docker (#12 DONE 53.1s,0 issues.) rather than replayed, and all five new tests appear in the run. CIsuccesson7923146; merges fast-forward clean ontonext.(closes #269)present on the landing commit, closing #269.Disclosure: the author's
make fmtaccount is true —script/fmtruns onlygofmt -s -w .andgoimports -w ., there is no prettier config in the tree, and.editorconfigsets no markdown width, somake fmtis a no-op for markdown. Not held against this PR; tracked as #215. The hand-wrapping is consistent with the surrounding file: every added line is at most 72 columns.7923146db9to4358921374Both points addressed,
README.mdonly, amended into the same commit (4358921).README.md:77— "controls several behaviors" is now "controls exactly one behavior", matching the one-row table and:109. Kept the table rather than folding it to prose so the diff stays to the two edits.README.md:107-111— added, after the existing reverse-proxy sentence:X-Forwarded-Protois read from any peer and is not gated byTRUSTED_PROXIES, a correctly configured proxy overwrites whatever a client sent, and on a directly exposed listener any client can assert it.Diff is those two hunks and nothing else; no Go file staged. Hand-wrapped to 72 columns (longest added line 70).
make checkgreen withGOFLAGS=-count=1after rebasing onto currentnext: 21 packagesok, lint executed in Docker (#11 DONE 66.5s,0 issues.).