The session cookie silently loses Secure in the default environment, and X-Forwarded-Proto parsing is exact-match #269
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
Secureand nothing says soSecureon 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.devis the default whenWEBHOOKER_ENVIRONMENTis unset.Verified behind a working TLS proxy with
WEBHOOKER_ENVIRONMENT=dev: the response carriesset-cookie: webhooker_session=...; Path=/; HttpOnly; SameSite=Laxwith noSecure, while the CSRF cookie on the very same response ISSecure.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-Protomatching is exact and brittleinternal/middleware/csrf.go:20compares== "https". Probed directly against a real proxy:https— strict path, correctHTTPS— relaxed pathhttps, http— relaxed pathhttps,https— relaxed pathhttps(trailing space) — strictUppercase 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: httpand 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
Securefollows the same per-request TLS decision the CSRF cookie makes, OR startup warns loudly when running in a non-devposture 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-Protoparsing is case-insensitive and takes the FIRST comma-separated element, trimmed.devworkflow over plain HTTP — confirmmake devand a plain-HTTP local run still work, and say so.Verification
make checkgreen.Securein the default environment, and a plain-HTTP local dev run still works.X-Forwarded-Protospelling above, asserting the strict path forHTTPS,https, httpandhttps,https, withhttpas the negative control.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/sessionsCookieStore.Newcopies*s.Optionsinto each*Session(store.go:92-93), andCookieStore.Savebuilds the cookie fromsession.Options, not from the store's (store.go:114). So the per-sessionSecureis writable at save time, and every session-cookie write in this app already goes through a method that has the*http.Request:Session.SaveandSession.Regenerate. No second store, no warning needed.internal/reqtls, one exportedIsTLS(r). It has to be a third package rather than a helper in either existing one:internal/middlewareimportsinternal/session, sosessioncannot importmiddlewareback.X-Forwarded-Proto, trimmed, lowercased.internal/session:SaveandRegeneratesetOptions.Securefromreqtls.IsTLS(r); the store default becomesSecure: trueand bothConfig.IsDev()call sites go away.internal/middleware/csrf.go:isClientTLSdeleted, callsreqtls.IsTLS.TLS-decision audit
Grepped
r.TLS,X-Forwarded-Proto,IsDev,Secureand 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-440builds the displayedBaseURLand assigns the raw header straight into the scheme (scheme = fwdProto), soHTTPSyieldsHTTPS://hostandhttps, httpyieldshttps, http://host.internal/handlersis 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 onIsDev(). 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.TLSand no header meansSecure: false, exactly as today. Will confirm against a running instance both ways regardless.Built in #276 (branch
issue-269-tls-detection, basenext). 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.Newgives each session its own copy of the store'sOptionsandCookieStore.Saverenders the cookie from that copy, and every session-cookie write here already goes throughSession.SaveorSession.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 becauseinternal/middlewareimportsinternal/session, sosessioncannot importmiddlewareback.Third site found, and left alone:
internal/handlers/source_management.goassigns the raw header straight into a URL scheme, soHTTPSrendersHTTPS://host.internal/handlersis a parallel unit's, so it is filed as #272 rather than fixed here.CORS()keying onIsDev()is a deployment posture rather than a transport fact and is correctly left at startup;internal/server/sentry.gomakes 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_ENVIRONMENTunset (log confirms"environment":"dev"— the default posture the defect lived in). Six TLS server blocks differing only in the spelling forwarded:The header itself, for
X-Forwarded-Proto: HTTPS(values elided):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 noSecure, and a following authenticatedGET /sources/returns200. The flag tracks the transport both ways rather than latching, which also matters for the deletion cookies inDestroyandRegenerate— aSecuredeletion cookie over plaintext is dropped, leaving a logged-out session live.make checkgreen withGOFLAGS=-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.