Decide request TLS in one place, per request (closes #269)
All checks were successful
check / check (push) Successful in 3m46s
All checks were successful
check / check (push) Successful in 3m46s
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.
This commit is contained in:
74
README.md
74
README.md
@@ -74,27 +74,41 @@ you can place variables in a `.env` file in the project root (loaded
|
||||
automatically via `godotenv/autoload`).
|
||||
|
||||
The environment is selected by setting `WEBHOOKER_ENVIRONMENT` to `dev`
|
||||
or `prod` (default: `dev`). The setting controls several behaviors:
|
||||
or `prod` (default: `dev`). The setting controls exactly one behavior:
|
||||
|
||||
| Behavior | `dev` | `prod` |
|
||||
| --------------------- | -------------------------------- | ------------------------------- |
|
||||
| CORS | Allows any origin (`*`) | Disabled (no-op) |
|
||||
| Session cookie Secure | `false` (works over plain HTTP) | `true` (requires HTTPS) |
|
||||
| Behavior | `dev` | `prod` |
|
||||
| -------- | ----------------------- | ---------------- |
|
||||
| CORS | Allows any origin (`*`) | Disabled (no-op) |
|
||||
|
||||
The CSRF cookie's `Secure` flag and Origin/Referer validation mode are
|
||||
determined per-request based on the actual transport protocol, not the
|
||||
environment setting. The middleware checks `r.TLS` (direct TLS) and the
|
||||
`X-Forwarded-Proto` header (TLS-terminating reverse proxy) to decide:
|
||||
The environment setting does **not** control cookie security. Both the
|
||||
session cookie and the CSRF cookie get their `Secure` flag, and the
|
||||
CSRF middleware its Origin/Referer validation mode, from the transport
|
||||
of each individual request, decided by one predicate —
|
||||
`internal/reqtls.IsTLS`. It reports TLS for a direct TLS connection
|
||||
(`r.TLS`) or for a TLS-terminating reverse proxy that reports one in
|
||||
`X-Forwarded-Proto`:
|
||||
|
||||
- **Direct TLS or `X-Forwarded-Proto: https`**: Secure cookies, strict
|
||||
Origin/Referer validation.
|
||||
- **Plaintext HTTP**: Non-Secure cookies, relaxed Origin/Referer
|
||||
checks (token validation still enforced).
|
||||
|
||||
This means CSRF protection works correctly in all deployment scenarios:
|
||||
behind a TLS-terminating reverse proxy, with direct TLS, or over plain
|
||||
HTTP during development. When running behind a reverse proxy, ensure it
|
||||
sets the `X-Forwarded-Proto: https` header.
|
||||
The `X-Forwarded-Proto` value is matched case-insensitively on its
|
||||
first comma-separated element, trimmed, so `HTTPS` and the appended
|
||||
chains a proxy behind another proxy emits (`https, http`) are all read
|
||||
as TLS.
|
||||
|
||||
This means both cookie security and CSRF protection work correctly in
|
||||
all deployment scenarios: behind a TLS-terminating reverse proxy, with
|
||||
direct TLS, or over plain HTTP during development — a plain-HTTP local
|
||||
run gets non-`Secure` cookies and remains usable, and a proxied
|
||||
deployment gets `Secure` ones without the operator setting anything.
|
||||
When running behind a reverse proxy, ensure it sets the
|
||||
`X-Forwarded-Proto: https` header. Unlike `X-Forwarded-For`, this
|
||||
header is read from any peer and is **not** gated by
|
||||
`TRUSTED_PROXIES`; a correctly configured proxy overwrites whatever a
|
||||
client sent. On a listener exposed directly to clients, any client can
|
||||
assert it, so do not run one without a proxy in front.
|
||||
|
||||
All other differences (log format, security headers, etc.) are
|
||||
independent of the environment setting — log format is determined by
|
||||
@@ -1853,13 +1867,16 @@ the rest. Nothing dropped is needed for the likeliest use, debugging a
|
||||
CSRF rejection. Its three inputs are the TLS decision, `Origin` and
|
||||
`Referer`; the latter two are kept, and the first is the scheme of the
|
||||
retained URL, because the SDK derives that scheme from
|
||||
`r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"` — byte
|
||||
for byte the predicate `internal/middleware/csrf.go` uses to choose
|
||||
between the `csrf.Secure(true)` and `csrf.Secure(false)` handlers.
|
||||
That is what the rewrite above preserves it for, and it is why
|
||||
dropping `X-Forwarded-Proto` costs nothing. The dropped provider
|
||||
headers (`X-GitHub-Event`, `X-Gitlab-Event` and the like) are real
|
||||
signal but are recorded locally on the event, and
|
||||
`r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"`. That
|
||||
predicate is the SDK's own and is stricter than `internal/reqtls.IsTLS`,
|
||||
which this service now uses everywhere it decides transport: the SDK
|
||||
reports `http` for the `HTTPS` and `https, http` spellings `reqtls`
|
||||
accepts. Only a reported scheme is affected, no decision is, so it is
|
||||
left to the SDK rather than reimplemented. That is what the rewrite
|
||||
above preserves it for, and it is why dropping `X-Forwarded-Proto`
|
||||
costs nothing. The dropped provider headers (`X-GitHub-Event`,
|
||||
`X-Gitlab-Event` and the like) are real signal but are recorded
|
||||
locally on the event, and
|
||||
`Sentry-Trace`/`Baggage` are already reflected in the event's trace
|
||||
context.
|
||||
|
||||
@@ -2515,8 +2532,9 @@ check, see [The login endpoint](#the-login-endpoint).
|
||||
|
||||
- **Web UI:** Cookie-based sessions using gorilla/sessions with
|
||||
encrypted cookies. Sessions are configured with HttpOnly, SameSite
|
||||
Lax, and Secure (in production). Absolute session lifetime is 7 days,
|
||||
with a sliding idle timeout on top of it (see
|
||||
Lax, and Secure whenever the request is on TLS — the flag follows the
|
||||
request's transport, not the environment. Absolute session lifetime
|
||||
is 7 days, with a sliding idle timeout on top of it (see
|
||||
[Sessions](#sessions)).
|
||||
- **API (planned):** API key authentication via `Authorization: Bearer`
|
||||
header. API keys are stored per-user with usage tracking
|
||||
@@ -2529,7 +2547,10 @@ check, see [The login endpoint](#the-login-endpoint).
|
||||
### Security
|
||||
|
||||
- Passwords hashed with Argon2id (64 MB memory cost)
|
||||
- Session cookies are HttpOnly, SameSite Lax, Secure (prod only)
|
||||
- Session cookies are HttpOnly, SameSite Lax, and Secure on any request
|
||||
that arrived over TLS (directly or through a reverse proxy reporting
|
||||
it), decided per-request by `internal/reqtls.IsTLS` rather than by the
|
||||
configured environment
|
||||
- Session regeneration on login to prevent session fixation attacks
|
||||
- Session key is a 32-byte value auto-generated on first startup and
|
||||
stored in the database
|
||||
@@ -2542,9 +2563,10 @@ check, see [The login endpoint](#the-login-endpoint).
|
||||
on all state-changing forms (cookie-based double-submit tokens with
|
||||
HMAC authentication). Applied to `/pages`, `/sources`, `/source`, and
|
||||
`/user` routes. Excluded from `/webhook` (inbound webhook POSTs) and
|
||||
`/api` (stateless API). The middleware auto-detects TLS status
|
||||
per-request (via `r.TLS` and `X-Forwarded-Proto`) to set appropriate
|
||||
cookie security flags and Origin/Referer validation mode
|
||||
`/api` (stateless API). The middleware detects TLS per-request through
|
||||
`internal/reqtls.IsTLS` — the same predicate the session cookie uses —
|
||||
to set appropriate cookie security flags and Origin/Referer validation
|
||||
mode
|
||||
- **Optional inbound signature verification** per entrypoint (GitHub
|
||||
`X-Hub-Signature-256`, GitLab `X-Gitlab-Token`). Off by default and
|
||||
off after an upgrade, so behaviour is unchanged until an operator
|
||||
|
||||
Reference in New Issue
Block a user