The session cookie silently loses Secure in the default environment, and X-Forwarded-Proto parsing is exact-match #269

New Issue

2026-08-24T02:11:31+02:00

clawbot commented

2026-08-24 02:11:31 +02:00

Two defects in the same mechanism — how the app decides a request is TLS — found by a TLS deployment audit running real nginx in front of the app.

1. The session cookie loses `Secure` and nothing says so

Secure on the session cookie is fixed at startup from !Config.IsDev() (internal/session/session.go:175), NOT from the per-request TLS detection the CSRF cookie already uses. dev is the default when WEBHOOKER_ENVIRONMENT is unset.

Verified behind a working TLS proxy with WEBHOOKER_ENVIRONMENT=dev: the response carries set-cookie: webhooker_session=...; Path=/; HttpOnly; SameSite=Lax with no Secure, while the CSRF cookie on the very same response IS Secure.

The problem is that it fails silently. Everything continues to work, so nothing prompts the operator to look. Combined with #268 — a cleartext port on the same host — a browser will send the authentication credential in the clear. HSTS mitigates substantially after a first visit, but the default being the unsafe one with no warning is the wrong polarity for a service built to face the internet.

Note the app ALREADY warns loudly at startup for an empty TRUSTED_PROXIES, so the precedent for warning exists.

2. `X-Forwarded-Proto` matching is exact and brittle

internal/middleware/csrf.go:20 compares == "https". Probed directly against a real proxy:

https — strict path, correct
HTTPS — relaxed path
https, http — relaxed path
https,https — relaxed path
https (trailing space) — strict

Uppercase and comma-appended chains are both things real infrastructure emits: proxies that append rather than replace produce the comma forms. Landing on the relaxed path means gorilla/csrf stops enforcing Referer on a genuine HTTPS site.

The audit separately confirmed a client CANNOT force this through a correctly configured proxy — nginx overrode both a forged X-Forwarded-Proto: http and duplicate headers. So this is reached by proxy configuration, not by attack.

Why one unit

Both are the same question — "is this request TLS?" — answered in two places with two different mechanisms, one of which the other already does correctly. Fixing them together is what prevents a third divergence.

Definition of done

The session cookie's Secure follows the same per-request TLS decision the CSRF cookie makes, OR startup warns loudly when running in a non-dev posture without it. State which you chose and why. If you make it per-request, verify the session package can actually see the request at cookie-write time before committing to that design.
X-Forwarded-Proto parsing is case-insensitive and takes the FIRST comma-separated element, trimmed.
Audit for any OTHER place the app decides whether a request is TLS, and make them all agree. Two have now been found; assume a third until proven otherwise, and state what you checked.
The behaviour change in the first bullet is potentially breaking for a local dev workflow over plain HTTP — confirm make dev and a plain-HTTP local run still work, and say so.

Verification

make check green.
Evidence from a RUNNING instance behind a TLS-terminating proxy: the session cookie carries Secure in the default environment, and a plain-HTTP local dev run still works.
A test for each X-Forwarded-Proto spelling above, asserting the strict path for HTTPS, https, http and https,https, with http as the negative control.

Two defects in the same mechanism — how the app decides a request is TLS — found by a TLS deployment audit running real nginx in front of the app. ## 1. The session cookie loses `Secure` and nothing says so `Secure` on the session cookie is fixed at startup from `!Config.IsDev()` (`internal/session/session.go:175`), NOT from the per-request TLS detection the CSRF cookie already uses. `dev` is the default when `WEBHOOKER_ENVIRONMENT` is unset. Verified behind a working TLS proxy with `WEBHOOKER_ENVIRONMENT=dev`: the response carries `set-cookie: webhooker_session=...; Path=/; HttpOnly; SameSite=Lax` with **no `Secure`**, while the CSRF cookie on the very same response IS `Secure`. The problem is that it fails silently. Everything continues to work, so nothing prompts the operator to look. Combined with https://git.eeqj.de/sneak/webhooker/issues/268 — a cleartext port on the same host — a browser will send the authentication credential in the clear. HSTS mitigates substantially after a first visit, but the default being the unsafe one with no warning is the wrong polarity for a service built to face the internet. Note the app ALREADY warns loudly at startup for an empty `TRUSTED_PROXIES`, so the precedent for warning exists. ## 2. `X-Forwarded-Proto` matching is exact and brittle `internal/middleware/csrf.go:20` compares `== "https"`. Probed directly against a real proxy: - `https` — strict path, correct - `HTTPS` — **relaxed path** - `https, http` — **relaxed path** - `https,https` — **relaxed path** - `https ` (trailing space) — strict Uppercase and comma-appended chains are both things real infrastructure emits: proxies that append rather than replace produce the comma forms. Landing on the relaxed path means gorilla/csrf stops enforcing Referer on a genuine HTTPS site. The audit separately confirmed a client CANNOT force this through a correctly configured proxy — nginx overrode both a forged `X-Forwarded-Proto: http` and duplicate headers. So this is reached by proxy configuration, not by attack. ## Why one unit Both are the same question — "is this request TLS?" — answered in two places with two different mechanisms, one of which the other already does correctly. Fixing them together is what prevents a third divergence. ## Definition of done - The session cookie's `Secure` follows the same per-request TLS decision the CSRF cookie makes, OR startup warns loudly when running in a non-`dev` posture without it. State which you chose and why. If you make it per-request, verify the session package can actually see the request at cookie-write time before committing to that design. - `X-Forwarded-Proto` parsing is case-insensitive and takes the FIRST comma-separated element, trimmed. - Audit for any OTHER place the app decides whether a request is TLS, and make them all agree. Two have now been found; assume a third until proven otherwise, and state what you checked. - The behaviour change in the first bullet is potentially breaking for a local `dev` workflow over plain HTTP — confirm `make dev` and a plain-HTTP local run still work, and say so. ## Verification - `make check` green. - Evidence from a RUNNING instance behind a TLS-terminating proxy: the session cookie carries `Secure` in the default environment, and a plain-HTTP local dev run still works. - A test for each `X-Forwarded-Proto` spelling above, asserting the strict path for `HTTPS`, `https, http` and `https,https`, with `http` as the negative control.

clawbot added this to the 1.0.0 milestone 2026-08-24 02:11:35 +02:00

clawbot referenced this issue

2026-08-24 02:15:44 +02:00

BaseURL on the source detail page uses the raw X-Forwarded-Proto value as the URL scheme #272

clawbot commented

2026-08-24 02:15:58 +02:00

Plan

Settled the per-request-vs-warning question first, as the issue asks. Per-request is possible with no store restructuring, so that is the path.

gorilla/sessions CookieStore.New copies *s.Options into each *Session (store.go:92-93), and CookieStore.Save builds the cookie from session.Options, not from the store's (store.go:114). So the per-session Secure is writable at save time, and every session-cookie write in this app already goes through a method that has the *http.Request: Session.Save and Session.Regenerate. No second store, no warning needed.

New internal/reqtls, one exported IsTLS(r). It has to be a third package rather than a helper in either existing one: internal/middleware imports internal/session, so session cannot import middleware back.
Parsing: first comma-separated element of X-Forwarded-Proto, trimmed, lowercased.
internal/session: Save and Regenerate set Options.Secure from reqtls.IsTLS(r); the store default becomes Secure: true and both Config.IsDev() call sites go away.
internal/middleware/csrf.go: isClientTLS deleted, calls reqtls.IsTLS.

TLS-decision audit

Grepped r.TLS, X-Forwarded-Proto, IsDev, Secure and scheme construction across the tree. Four sites, beyond the two in the issue:

A third real one, NOT fixed here — filed as #272. internal/handlers/source_management.go:432-440 builds the displayed BaseURL and assigns the raw header straight into the scheme (scheme = fwdProto), so HTTPS yields HTTPS://host and https, http yields https, http://host. internal/handlers is held by a parallel unit, so it stays out of this branch; #272 adopts the predicate this one lands.

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 decision. Correctly startup-fixed.
internal/server/sentry.go:149 — makes no decision of its own; the comment documents that the Sentry SDK's own predicate happens to be byte-for-byte the old CSRF one. Updating that comment to match is part of this branch, but no logic there changes.

Plain-HTTP local dev is unaffected by construction: no r.TLS and no header means Secure: false, exactly as today. Will confirm against a running instance both ways regardless.

## Plan Settled the per-request-vs-warning question first, as the issue asks. **Per-request is possible with no store restructuring**, so that is the path. `gorilla/sessions` `CookieStore.New` copies `*s.Options` into each `*Session` (`store.go:92-93`), and `CookieStore.Save` builds the cookie from `session.Options`, not from the store's (`store.go:114`). So the per-session `Secure` is writable at save time, and every session-cookie write in this app already goes through a method that has the `*http.Request`: `Session.Save` and `Session.Regenerate`. No second store, no warning needed. - New `internal/reqtls`, one exported `IsTLS(r)`. It has to be a third package rather than a helper in either existing one: `internal/middleware` imports `internal/session`, so `session` cannot import `middleware` back. - Parsing: first comma-separated element of `X-Forwarded-Proto`, trimmed, lowercased. - `internal/session`: `Save` and `Regenerate` set `Options.Secure` from `reqtls.IsTLS(r)`; the store default becomes `Secure: true` and both `Config.IsDev()` call sites go away. - `internal/middleware/csrf.go`: `isClientTLS` deleted, calls `reqtls.IsTLS`. ### TLS-decision audit Grepped `r.TLS`, `X-Forwarded-Proto`, `IsDev`, `Secure` and scheme construction across the tree. Four sites, beyond the two in the issue: **A third real one, NOT fixed here** — filed as https://git.eeqj.de/sneak/webhooker/issues/272. `internal/handlers/source_management.go:432-440` builds the displayed `BaseURL` and assigns the raw header straight into the scheme (`scheme = fwdProto`), so `HTTPS` yields `HTTPS://host` and `https, http` yields `https, http://host`. `internal/handlers` is held by a parallel unit, so it stays out of this branch; https://git.eeqj.de/sneak/webhooker/issues/272 adopts the predicate this one lands. **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 decision. Correctly startup-fixed. - `internal/server/sentry.go:149` — makes no decision of its own; the comment documents that the Sentry SDK's own predicate happens to be byte-for-byte the old CSRF one. Updating that comment to match is part of this branch, but no logic there changes. Plain-HTTP local dev is unaffected by construction: no `r.TLS` and no header means `Secure: false`, exactly as today. Will confirm against a running instance both ways regardless.

clawbot referenced this issue from a commit

2026-08-24 02:31:55 +02:00

Decide request TLS in one place, per request (closes #269)

clawbot referenced a pull request that will close this issue

2026-08-24 02:32:34 +02:00

Decide request TLS in one place, per request (closes #269) #276

clawbot commented

2026-08-24 02:32:59 +02:00

Built in #276 (branch issue-269-tls-detection, base next). Full rationale is in the PR body; this is what was decided and how it was checked.

Chose per-request, not the startup warning. It needed no restructuring of the store: CookieStore.New gives each session its own copy of the store's Options and CookieStore.Save renders the cookie from that copy, and every session-cookie write here already goes through Session.Save or Session.Regenerate, which hold the request. Both defects now go through one predicate, internal/reqtls.IsTLS — case-folded, leftmost comma-separated element, trimmed. A third package was needed because internal/middleware imports internal/session, so session cannot import middleware back.

Third site found, and left alone: internal/handlers/source_management.go assigns the raw header straight into a URL scheme, so HTTPS renders HTTPS://host. internal/handlers is a parallel unit's, so it is filed as #272 rather than fixed here. CORS() keying on IsDev() is a deployment posture rather than a transport fact and is correctly left at startup; internal/server/sentry.go makes no decision of its own.

Verified against a running instance, not simulated. Real nginx terminating TLS with a self-signed cert, app started with WEBHOOKER_ENVIRONMENT unset (log confirms "environment":"dev" — the default posture the defect lived in). Six TLS server blocks differing only in the spelling forwarded:

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 header itself, for X-Forwarded-Proto: HTTPS (values elided):

Set-Cookie: webhooker_session=...; Path=/; Max-Age=604800; HttpOnly; Secure; SameSite=Lax

Session cookie and CSRF cookie now agree on the same response.

Local dev still works, checked rather than assumed: a plain-HTTP login against the app directly returns 303, sets session cookies with no Secure, and a following authenticated GET /sources/ returns 200. The flag tracks the transport both ways rather than latching, which also matters for the deletion cookies in Destroy and Regenerate — a Secure deletion cookie over plaintext is dropped, leaving a logged-out session live.

make check green with GOFLAGS=-count=1; the lint layer genuinely ran (60.3s, 0 issues) rather than replaying cache. Every new test was confirmed to FAIL against the unfixed code by hand-reverting both defects and re-running — 24 subtests failed, all negative controls still passed.

Built in https://git.eeqj.de/sneak/webhooker/pulls/276 (branch `issue-269-tls-detection`, base `next`). Full rationale is in the PR body; this is what was decided and how it was checked. **Chose per-request, not the startup warning.** It needed no restructuring of the store: `CookieStore.New` gives each session its own copy of the store's `Options` and `CookieStore.Save` renders the cookie from that copy, and every session-cookie write here already goes through `Session.Save` or `Session.Regenerate`, which hold the request. Both defects now go through one predicate, `internal/reqtls.IsTLS` — case-folded, leftmost comma-separated element, trimmed. A third package was needed because `internal/middleware` imports `internal/session`, so `session` cannot import `middleware` back. **Third site found**, and left alone: `internal/handlers/source_management.go` assigns the raw header straight into a URL scheme, so `HTTPS` renders `HTTPS://host`. `internal/handlers` is a parallel unit's, so it is filed as https://git.eeqj.de/sneak/webhooker/issues/272 rather than fixed here. `CORS()` keying on `IsDev()` is a deployment posture rather than a transport fact and is correctly left at startup; `internal/server/sentry.go` makes no decision of its own. **Verified against a running instance**, not simulated. Real nginx terminating TLS with a self-signed cert, app started with `WEBHOOKER_ENVIRONMENT` unset (log confirms `"environment":"dev"` — the default posture the defect lived in). Six TLS server blocks differing only in the spelling forwarded: ``` 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 header itself, for `X-Forwarded-Proto: HTTPS` (values elided): ``` Set-Cookie: webhooker_session=...; Path=/; Max-Age=604800; HttpOnly; Secure; SameSite=Lax ``` Session cookie and CSRF cookie now agree on the same response. **Local dev still works**, checked rather than assumed: a plain-HTTP login against the app directly returns `303`, sets session cookies with no `Secure`, and a following authenticated `GET /sources/` returns `200`. The flag tracks the transport both ways rather than latching, which also matters for the deletion cookies in `Destroy` and `Regenerate` — a `Secure` deletion cookie over plaintext is dropped, leaving a logged-out session live. `make check` green with `GOFLAGS=-count=1`; the lint layer genuinely ran (`60.3s`, `0 issues`) rather than replaying cache. Every new test was confirmed to FAIL against the unfixed code by hand-reverting both defects and re-running — 24 subtests failed, all negative controls still passed.

clawbot referenced this issue

2026-08-24 02:45:09 +02:00

Bind the app port deliberately and document the proxy deployment (closes #268) #277

clawbot referenced this issue

2026-08-24 02:47:05 +02:00

Decide request TLS in one place, per request (closes #269) #276

clawbot referenced this issue from a commit

2026-08-24 02:51:33 +02:00

Decide request TLS in one place, per request (closes #269)

clawbot referenced this issue

2026-08-24 02:56:40 +02:00

Decide request TLS in one place, per request (closes #269) #276