sneak/webhooker
1
0
Fork 0
Code Issues 62 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

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

Merged
clawbot merged 1 commits from issue-269-tls-detection into next 2026-08-24 03:01:38 +02:00
Conversation 4 Commits 1 Files Changed 11 +809 -98
11 changed files with 809 additions and 98 deletions
Download Patch File Download Diff File Expand all files Collapse all files

70
README.md
View File

@@ -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) |
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

17
internal/middleware/csrf.go
View File

@@ -5,6 +5,7 @@ import (
"github.com/gorilla/csrf"
"sneak.berlin/go/webhooker/internal/logfield"
"sneak.berlin/go/webhooker/internal/reqtls"
)
// CSRFToken retrieves the CSRF token from the request context.
@@ -13,13 +14,6 @@ func CSRFToken(r *http.Request) string {
return csrf.Token(r)
}
// isClientTLS reports whether the client-facing connection uses TLS.
// It checks for a direct TLS connection (r.TLS) or a TLS-terminating
// reverse proxy that sets the standard X-Forwarded-Proto header.
func isClientTLS(r *http.Request) bool {
return r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"
}
// CSRF returns middleware that provides CSRF protection using the
// gorilla/csrf library. The middleware uses the session authentication
// key to sign a CSRF cookie and validates a masked token submitted via
@@ -27,9 +21,10 @@ func isClientTLS(r *http.Request) bool {
// POST/PUT/PATCH/DELETE requests. Requests with an invalid or missing
// token receive a 403 Forbidden response.
//
// The middleware detects the client-facing transport protocol per-request
// using r.TLS and the X-Forwarded-Proto header. This allows correct
// behavior in all deployment scenarios:
// The middleware detects the client-facing transport protocol
// per-request via reqtls.IsTLS, the single TLS predicate the session
// cookie also uses. This allows correct behavior in all deployment
// scenarios:
//
// - Direct HTTPS: strict Referer/Origin checks, Secure cookies.
// - Behind a TLS-terminating reverse proxy: strict checks (the
@@ -83,7 +78,7 @@ func (m *Middleware) CSRF() func(http.Handler) http.Handler {
httpCSRF := httpProtect(next)
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if isClientTLS(r) {
if reqtls.IsTLS(r) {
// Client is on TLS (directly or via reverse proxy).
// Use Secure cookies and strict Origin/Referer checks.
tlsCSRF.ServeHTTP(w, r)

195
internal/middleware/csrf_test.go
View File

@@ -297,55 +297,176 @@ func TestCSRFToken_NoMiddleware(t *testing.T) {
}
// --- TLS Detection Tests ---
//
// The predicate itself is tested in internal/reqtls. What is tested
// here is the consequence that actually matters: which of the two
// gorilla/csrf instances a request is routed to.
//
// The two are told apart behaviourally rather than by inspection. On
// the STRICT (TLS) instance, a state-changing request carrying no
// Origin header must supply a Referer -- gorilla/csrf rejects it with
// ErrNoReferer before it ever looks at the token, to defend a
// TLS site against an HTTP machine-in-the-middle injecting a form. On
// the RELAXED (plaintext) instance that check is skipped and a valid
// token is enough. So: valid token, no Origin, no Referer, and the
// outcome names the instance.
//
// Landing on the relaxed instance for a genuinely-HTTPS deployment is
// the defect: an exact == "https" comparison did exactly that for the
// uppercase and comma-appended spellings below.
func TestIsClientTLS_DirectTLS(t *testing.T) {
t.Parallel()
// csrfTookStrictPath reports whether the CSRF middleware routed a
// request with the given transport to the strict instance. It also
// asserts the CSRF cookie's Secure attribute agrees, since the two are
// set by the same choice and must never disagree.
func csrfTookStrictPath(
t *testing.T,
env string,
directTLS bool,
fwdProto string,
) bool {
t.Helper()
m, _ := testMiddleware(t, env)
csrfMW := m.CSRF()
newReq := func(method string) *http.Request {
r := httptest.NewRequestWithContext(
context.Background(), http.MethodGet, "/", nil)
context.Background(), method,
"http://example.com/form", nil,
)
if directTLS {
r.TLS = &tls.ConnectionState{}
}
if fwdProto != "" {
r.Header.Set("X-Forwarded-Proto", fwdProto)
}
return r
}
token, cookies := csrfGetToken(t, csrfMW, newReq(http.MethodGet))
// Deliberately no Origin and no Referer: that is what makes the
// two instances distinguishable.
called, code := csrfPostWithToken(
t, csrfMW, newReq(http.MethodPost), token, cookies,
)
strict := !called
if strict {
assert.Equal(
t, http.StatusForbidden, code,
"the strict instance rejects a Referer-less POST",
)
}
for _, c := range cookies {
if c.Name == csrfCookieName {
assert.Equal(
t, strict, c.Secure,
"the CSRF cookie's Secure attribute and the "+
"chosen instance come from one decision "+
"and must agree",
)
}
}
return strict
}
// TestCSRF_ForwardedProtoSpellingsTakeStrictPath runs the header
// spellings a real proxy emits through the middleware. The environment
// is dev -- the DEFAULT when WEBHOOKER_ENVIRONMENT is unset -- to pin
// that the routing is a per-request transport decision and owes
// nothing to configuration.
func TestCSRF_ForwardedProtoSpellingsTakeStrictPath(t *testing.T) {
t.Parallel()
cases := []struct {
name string
header string
strict bool
why string
}{
{
name: "lowercase",
header: "https",
strict: true,
why: "the ordinary spelling",
},
{
name: "uppercase",
header: "HTTPS",
strict: true,
why: "the header value is a case-insensitive token",
},
{
name: "chain with plaintext inner hop",
header: "https, http",
strict: true,
why: "a chained proxy appends its hop; the leftmost " +
"element is the browser's connection",
},
{
name: "chain of two TLS hops",
header: "https,https",
strict: true,
why: "appended chain with no space after the comma",
},
{
name: "trailing space",
header: "https ",
strict: true,
why: "whitespace is not part of the token",
},
{
name: "plaintext",
header: "http",
strict: false,
why: "the negative control: the proxy reports a " +
"plaintext client connection",
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
assert.Equal(
t, tc.strict,
csrfTookStrictPath(
t, config.EnvironmentDev, false, tc.header,
),
"X-Forwarded-Proto %q: %s", tc.header, tc.why,
)
})
}
}
// TestCSRF_DirectTLSTakesStrictPath covers the no-proxy TLS
// deployment, and TestCSRF_PlaintextTakesRelaxedPath the no-proxy
// plaintext one -- the local development case that must keep working.
func TestCSRF_DirectTLSTakesStrictPath(t *testing.T) {
t.Parallel()
assert.True(
t, middleware.IsClientTLS(r),
"should detect direct TLS connection",
t,
csrfTookStrictPath(t, config.EnvironmentDev, true, ""),
"a request that arrived over TLS takes the strict path",
)
}
func TestIsClientTLS_XForwardedProto(t *testing.T) {
func TestCSRF_PlaintextTakesRelaxedPath(t *testing.T) {
t.Parallel()
r := httptest.NewRequestWithContext(
context.Background(), http.MethodGet, "/", nil)
r.Header.Set("X-Forwarded-Proto", "https")
assert.True(
t, middleware.IsClientTLS(r),
"should detect TLS via X-Forwarded-Proto",
)
}
func TestIsClientTLS_PlaintextHTTP(t *testing.T) {
t.Parallel()
r := httptest.NewRequestWithContext(
context.Background(), http.MethodGet, "/", nil)
assert.False(
t, middleware.IsClientTLS(r),
"should detect plaintext HTTP",
)
}
func TestIsClientTLS_XForwardedProtoHTTP(t *testing.T) {
t.Parallel()
r := httptest.NewRequestWithContext(
context.Background(), http.MethodGet, "/", nil)
r.Header.Set("X-Forwarded-Proto", "http")
assert.False(
t, middleware.IsClientTLS(r),
"should detect plaintext when X-Forwarded-Proto is http",
t,
csrfTookStrictPath(t, config.EnvironmentProd, false, ""),
"no TLS and no proxy header is plaintext, in any environment",
)
}

5
internal/middleware/export_test.go
View File

@@ -56,11 +56,6 @@ func ClientKeyForTest(m *Middleware, r *http.Request) string {
return m.clientKey(r)
}
// IsClientTLS exposes isClientTLS for testing.
func IsClientTLS(r *http.Request) bool {
return isClientTLS(r)
}
// LoginRateLimitConst exposes the loginRateLimit constant: the
// number of FAILED login attempts one client may make against one
// submitted username per interval.

59
internal/reqtls/reqtls.go Normal file
View File

@@ -0,0 +1,59 @@
// Package reqtls answers one question, in one place, for the whole
// application: did this request reach the service over TLS?
//
// It exists because that question used to be answered independently in
// several packages, by hand, and the answers disagreed. The session
// cookie's Secure attribute was decided at startup from the configured
// environment while the CSRF cookie's was decided per-request, so a
// deployment behind a TLS proxy in the default environment emitted one
// Secure cookie and one non-Secure cookie on the same response.
// Everything kept working, which is exactly why nobody noticed.
//
// Any code that needs a scheme or a Secure flag must call IsTLS rather
// than reading the request itself.
package reqtls
import (
"net/http"
"strings"
)
// forwardedProtoHeader is the de-facto standard header by which a
// TLS-terminating reverse proxy reports the protocol the CLIENT used.
const forwardedProtoHeader = "X-Forwarded-Proto"
// IsTLS reports whether the client-facing connection uses TLS: either
// the request arrived over TLS directly, or a reverse proxy terminated
// TLS and said so in X-Forwarded-Proto.
//
// The header is only as trustworthy as whatever sits in front of the
// listener. A proxy that overwrites it -- which is what the deployment
// documentation requires -- makes it authoritative; a listener exposed
// directly to clients lets any client assert it. That is the same
// exposure every X-Forwarded-* consumer carries.
func IsTLS(r *http.Request) bool {
return r.TLS != nil || forwardedProto(r) == "https"
}
// forwardedProto reduces X-Forwarded-Proto to a bare, comparable
// protocol token, or "" when the header is absent or blank.
//
// Two shapes that real infrastructure emits do not survive an exact
// comparison against "https", and both name a TLS client connection:
//
// - "HTTPS", because the header value is a case-insensitive token and
// nothing obliges a proxy to emit it lowercased.
// - "https, http", because a proxy chained behind another proxy
// APPENDS its own hop instead of replacing the value. As with
// X-Forwarded-For, the leftmost element is the one nearest the
// client, so it is the element that describes the browser's
// connection -- the only hop a cookie's Secure attribute is about.
//
// Landing on the plaintext path for either of those spellings is not a
// cosmetic error: it stops gorilla/csrf enforcing the strict Referer
// check on a site that genuinely is HTTPS.
func forwardedProto(r *http.Request) string {
first, _, _ := strings.Cut(r.Header.Get(forwardedProtoHeader), ",")
return strings.ToLower(strings.TrimSpace(first))
}

209
internal/reqtls/reqtls_test.go Normal file
View File

@@ -0,0 +1,209 @@
package reqtls_test
import (
"context"
"crypto/tls"
"net/http"
"net/http/httptest"
"testing"
"github.com/stretchr/testify/assert"
"sneak.berlin/go/webhooker/internal/reqtls"
)
// newReq builds a plaintext request with no forwarding headers.
func newReq(t *testing.T) *http.Request {
t.Helper()
return httptest.NewRequestWithContext(
context.Background(), http.MethodGet, "/", nil,
)
}
func TestIsTLS_DirectTLS(t *testing.T) {
t.Parallel()
r := newReq(t)
r.TLS = &tls.ConnectionState{}
assert.True(
t, reqtls.IsTLS(r),
"a request that arrived over TLS is TLS",
)
}
func TestIsTLS_PlaintextNoHeader(t *testing.T) {
t.Parallel()
assert.False(
t, reqtls.IsTLS(newReq(t)),
"no TLS connection and no header means plaintext",
)
}
// protoCase is one X-Forwarded-Proto spelling and the answer IsTLS
// owes it.
type protoCase struct {
name string
header string
want bool
why string
}
// protoCases enumerates the header values real infrastructure emits.
func protoCases() []protoCase {
return append(protoTLSCases(), protoPlaintextCases()...)
}
// protoTLSCases are the spellings that name a TLS client connection.
// Every one but the first is a spelling an exact == "https"
// comparison used to miss, silently downgrading a genuinely-HTTPS
// deployment to the plaintext path.
func protoTLSCases() []protoCase {
return []protoCase{
{
name: "lowercase",
header: "https",
want: true,
why: "the ordinary spelling",
},
{
name: "uppercase",
header: "HTTPS",
want: true,
why: "the value is a case-insensitive token; " +
"nothing obliges a proxy to lowercase it",
},
{
name: "mixed case",
header: "HttpS",
want: true,
why: "case folding must be total, not just the two extremes",
},
{
name: "chain with plaintext inner hop",
header: "https, http",
want: true,
why: "a chained proxy appends its hop; the leftmost " +
"element is the client-facing one",
},
{
name: "chain of two TLS hops",
header: "https,https",
want: true,
why: "appended chain with no space after the comma",
},
{
name: "trailing space",
header: "https ",
want: true,
why: "surrounding whitespace is not part of the token",
},
{
name: "leading space",
header: " https",
want: true,
why: "surrounding whitespace is not part of the token",
},
{
name: "uppercase chain",
header: "HTTPS, HTTP",
want: true,
why: "case folding and chain splitting must compose",
},
}
}
// protoPlaintextCases are the values that must NOT be read as TLS.
func protoPlaintextCases() []protoCase {
return []protoCase{
{
name: "plaintext",
header: "http",
want: false,
why: "the negative control: the proxy reports plaintext",
},
{
name: "plaintext chain with TLS inner hop",
header: "http, https",
want: false,
why: "the client-facing hop is plaintext even though " +
"an inner hop used TLS",
},
{
name: "empty",
header: "",
want: false,
why: "an empty header asserts nothing",
},
{
name: "whitespace only",
header: " ",
want: false,
why: "a blank header asserts nothing",
},
{
name: "unrelated token",
header: "ftp",
want: false,
why: "only https means TLS",
},
{
name: "https as a substring",
header: "nothttps",
want: false,
why: "matching must be on the whole token, not a substring",
},
}
}
func TestIsTLS_ForwardedProtoSpellings(t *testing.T) {
t.Parallel()
for _, tc := range protoCases() {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
r := newReq(t)
r.Header.Set("X-Forwarded-Proto", tc.header)
assert.Equal(
t, tc.want, reqtls.IsTLS(r),
"X-Forwarded-Proto %q: %s", tc.header, tc.why,
)
})
}
}
// TestIsTLS_DirectTLSBeatsPlaintextHeader pins the precedence: a
// connection this process itself terminated with TLS is a fact, and a
// header claiming otherwise does not override it.
func TestIsTLS_DirectTLSBeatsPlaintextHeader(t *testing.T) {
t.Parallel()
r := newReq(t)
r.TLS = &tls.ConnectionState{}
r.Header.Set("X-Forwarded-Proto", "http")
assert.True(
t, reqtls.IsTLS(r),
"an actual TLS connection outranks a header claiming plaintext",
)
}
// TestIsTLS_FirstHeaderValueWins covers a proxy that adds a second
// header line rather than appending to the existing one. net/http
// keeps them as separate values; the first is the client-facing hop,
// matching how the comma-separated form is read.
func TestIsTLS_FirstHeaderValueWins(t *testing.T) {
t.Parallel()
r := newReq(t)
r.Header.Add("X-Forwarded-Proto", "https")
r.Header.Add("X-Forwarded-Proto", "http")
assert.True(
t, reqtls.IsTLS(r),
"the first header line is the client-facing hop",
)
}

11
internal/server/sentry.go
View File

@@ -147,10 +147,13 @@ func sentryRoutePattern(hint *sentry.EventHint) string {
//
// The scheme is load-bearing and is kept: the SDK derives it from
// r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https"
// (interfaces.go:180), byte for byte the predicate
// internal/middleware/csrf.go uses, so it is the CSRF TLS decision and
// the reason dropping X-Forwarded-Proto from the header allowlist
// costs nothing. The host is parsed.Host of the SDK's
// (interfaces.go:180), which is the reason dropping X-Forwarded-Proto
// from the header allowlist costs nothing. That predicate is the SDK's
// own and is stricter than 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. The host is parsed.Host of the SDK's
// scheme://r.Host/path, so it is whatever the client's Host header
// carried: this service validates no hostname. It is kept because that
// same header is on the allowlist, so scrubbing it here would withhold

4
internal/session/export_test.go
View File

@@ -5,6 +5,6 @@ import "github.com/gorilla/sessions"
// NewStore exposes the production cookie-store constructor so tests
// exercise the store the application actually runs with, rather than a
// lookalike assembled in the test.
func NewStore(key []byte, secure bool) *sessions.CookieStore {
return newStore(key, secure)
func NewStore(key []byte) *sessions.CookieStore {
return newStore(key)
}

60
internal/session/session.go
View File

@@ -17,6 +17,7 @@ import (
"sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/logger"
"sneak.berlin/go/webhooker/internal/reqtls"
)
const (
@@ -87,7 +88,6 @@ type Session struct {
store *sessions.CookieStore
key []byte // raw 32-byte auth key, also used for CSRF cookie signing
log *slog.Logger
config *config.Config
// idleTimeout is the sliding inactivity window. A session that
// sees no authenticated request within this window expires,
@@ -104,6 +104,10 @@ type Session struct {
// cookie. MaxAge is deliberately left at its zero value: for a store
// it is set through CookieStore.MaxAge (see newStore), and for a
// single session it is copied from the store's options.
//
// Secure is a parameter rather than a constant because it is the one
// attribute here that is not a policy -- it is a fact about the
// connection carrying this particular response. See applyTransport.
func cookieOptions(secure bool) *sessions.Options {
return &sessions.Options{
Path: "/",
@@ -121,14 +125,52 @@ func cookieOptions(secure bool) *sessions.Options {
// Options never touches Codecs -- so a store configured that way still
// decodes a 30-day-old cookie, leaving the cookie attribute and the
// codec disagreeing about the same policy. store.MaxAge sets both.
func newStore(key []byte, secure bool) *sessions.CookieStore {
//
// The store's Secure is fixed at true, and is only a template: every
// write path overwrites it for the request in hand (applyTransport).
// It is true rather than false so that a write path added later which
// forgets to call applyTransport fails loudly -- the browser drops the
// cookie over plaintext HTTP and the developer sees it immediately --
// instead of silently shipping the authentication credential without
// Secure, which is the exact failure this store already had once.
func newStore(key []byte) *sessions.CookieStore {
store := sessions.NewCookieStore(key)
store.Options = cookieOptions(secure)
store.Options = cookieOptions(true)
store.MaxAge(secondsPerDay * sessionMaxAgeDays)
return store
}
// applyTransport sets the session cookie's Secure attribute from the
// transport of the request being answered.
//
// This is decided per-request, not once at startup. Deciding it at
// startup from the configured environment is what this replaces, and
// it got the DEFAULT posture wrong: "dev" is the environment when
// WEBHOOKER_ENVIRONMENT is unset, so a deployment terminating TLS at a
// proxy without also setting the environment emitted the
// authentication cookie with no Secure attribute -- silently, and on
// the same response as a CSRF cookie that did have one.
//
// gorilla/sessions makes this cheap and local: CookieStore.New gives
// every session its own copy of the store's Options, and
// CookieStore.Save renders the cookie from that copy rather than from
// the store. So the flag is set on the one session being saved,
// without a second store and without reaching across concurrent
// requests.
//
// The flag tracks the transport in BOTH directions rather than being
// latched on once seen. Secure on a plaintext response is worse than
// useless: the browser discards such a cookie without any error, so a
// latched flag would make a plain-HTTP local run impossible to log
// into. It is also why every write path must call this, including the
// deletion cookies in Destroy and Regenerate -- a Secure deletion
// cookie sent over plaintext is dropped too, leaving the session the
// caller believed it had just revoked.
func applyTransport(r *http.Request, sess *sessions.Session) {
sess.Options.Secure = reqtls.IsTLS(r)
}
// New creates a new session manager. The cookie store is
// initialized during the fx OnStart phase after the database is
// connected, using a session key that is auto-generated and stored
@@ -139,7 +181,6 @@ func New(
) (*Session, error) {
s := &Session{
log: params.Logger.Get(),
config: params.Config,
idleTimeout: params.Config.SessionIdleTimeout,
now: time.Now,
}
@@ -172,7 +213,7 @@ func New(
}
s.key = keyBytes
s.store = newStore(keyBytes, !params.Config.IsDev())
s.store = newStore(keyBytes)
s.log.Info("session manager initialized")
return nil
@@ -196,12 +237,16 @@ func (s *Session) GetKey() []byte {
return s.key
}
// Save saves the session.
// Save saves the session. Every session-cookie write in the
// application goes through here or through Regenerate, which is what
// makes applyTransport a complete answer rather than a best effort.
func (s *Session) Save(
r *http.Request,
w http.ResponseWriter,
sess *sessions.Session,
) error {
applyTransport(r, sess)
return sess.Save(r, w)
}
@@ -340,6 +385,7 @@ func (s *Session) Regenerate(
// Destroy the old session
oldSess.Options.MaxAge = -1
s.ClearUser(oldSess)
applyTransport(r, oldSess)
err := oldSess.Save(r, w)
if err != nil {
@@ -368,7 +414,7 @@ func (s *Session) Regenerate(
// Apply the standard session options (the destroyed old
// session had MaxAge = -1, which store.New might inherit
// from the cookie).
newSess.Options = cookieOptions(!s.config.IsDev())
newSess.Options = cookieOptions(reqtls.IsTLS(r))
newSess.Options.MaxAge = secondsPerDay * sessionMaxAgeDays
return newSess, nil

264
internal/session/session_test.go
View File

@@ -2,6 +2,7 @@ package session_test
import (
"context"
"crypto/tls"
"log/slog"
"net/http"
"net/http/httptest"
@@ -73,7 +74,7 @@ func testSessionWithClock(
t.Helper()
key := testKey()
store := session.NewStore(key, false)
store := session.NewStore(key)
cfg := &config.Config{
Environment: config.EnvironmentDev,
@@ -880,3 +881,264 @@ func TestDestroy_ThenSave_DeletesCookie(t *testing.T) {
"destroyed session cookie should have negative MaxAge",
)
}
// --- Secure Attribute / Transport Tests ---
// transportCase describes one client-facing transport and the Secure
// attribute the session cookie must carry for it.
type transportCase struct {
name string
tls bool
header string
want bool
why string
}
// transportCases enumerates the transports the session cookie has to
// get right. Every https spelling here is one a real proxy emits.
func transportCases() []transportCase {
return []transportCase{
{
name: "direct TLS",
tls: true,
want: true,
why: "this process terminated TLS itself",
},
{
name: "proxy reports https",
header: "https",
want: true,
why: "the ordinary reverse-proxy deployment",
},
{
name: "proxy reports HTTPS",
header: "HTTPS",
want: true,
why: "the header value is a case-insensitive token",
},
{
name: "appended chain https, http",
header: "https, http",
want: true,
why: "the leftmost hop is the browser's connection",
},
{
name: "appended chain https,https",
header: "https,https",
want: true,
why: "two TLS hops, no space after the comma",
},
{
name: "trailing space",
header: "https ",
want: true,
why: "whitespace is not part of the token",
},
{
name: "proxy reports http",
header: "http",
want: false,
why: "the negative control: Secure over plaintext is " +
"dropped by the browser without a word",
},
{
name: "plaintext, no proxy",
want: false,
why: "a plain local run must stay loggable-in",
},
}
}
// transportRequest builds a request carrying the case's transport.
func (tc transportCase) request(t *testing.T) *http.Request {
t.Helper()
r := httptest.NewRequestWithContext(
context.Background(), http.MethodGet,
"http://example.com/", nil,
)
if tc.tls {
r.TLS = &tls.ConnectionState{}
}
if tc.header != "" {
r.Header.Set("X-Forwarded-Proto", tc.header)
}
return r
}
// sessionCookieFrom returns the session cookie from a response, or
// fails the test if there is none.
func sessionCookieFrom(
t *testing.T,
w *httptest.ResponseRecorder,
) *http.Cookie {
t.Helper()
for _, c := range w.Result().Cookies() {
if c.Name == session.SessionName {
return c
}
}
require.FailNow(t, "no session cookie in response")
return nil
}
// TestSave_SecureFollowsRequestTransport is the regression test for
// the defect this replaces: Secure was fixed at startup from the
// configured environment, and "dev" is the environment when
// WEBHOOKER_ENVIRONMENT is unset. A deployment behind a TLS proxy in
// that DEFAULT posture shipped the authentication cookie with no
// Secure attribute and said nothing about it.
//
// testSession builds its config with EnvironmentDev precisely so that
// the https cases below fail against the old startup-fixed behaviour.
func TestSave_SecureFollowsRequestTransport(t *testing.T) {
t.Parallel()
for _, tc := range transportCases() {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
s := testSession(t)
r := tc.request(t)
w := httptest.NewRecorder()
sess, err := s.Get(r)
require.NoError(t, err)
s.SetUser(sess, "user-1", "alice")
require.NoError(t, s.Save(r, w, sess))
assert.Equal(
t, tc.want, sessionCookieFrom(t, w).Secure,
"session cookie Secure for %q: %s",
tc.name, tc.why,
)
})
}
}
// TestSave_SecureTracksTransportBothWays pins that the flag is not
// latched. One store serves every request, so a Secure cookie set for
// a proxied request must not leak into a later plaintext response --
// the browser would silently discard that one, and a local run would
// become impossible to log into.
func TestSave_SecureTracksTransportBothWays(t *testing.T) {
t.Parallel()
s := testSession(t)
secureReq := httptest.NewRequestWithContext(
context.Background(), http.MethodGet,
"http://example.com/", nil,
)
secureReq.Header.Set("X-Forwarded-Proto", "https")
secureW := httptest.NewRecorder()
secureSess, err := s.Get(secureReq)
require.NoError(t, err)
require.NoError(t, s.Save(secureReq, secureW, secureSess))
require.True(
t, sessionCookieFrom(t, secureW).Secure,
"proxied request should produce a Secure cookie",
)
plainReq := httptest.NewRequestWithContext(
context.Background(), http.MethodGet,
"http://example.com/", nil,
)
plainW := httptest.NewRecorder()
plainSess, err := s.Get(plainReq)
require.NoError(t, err)
require.NoError(t, s.Save(plainReq, plainW, plainSess))
assert.False(
t, sessionCookieFrom(t, plainW).Secure,
"a later plaintext request must not inherit Secure from "+
"the earlier proxied one",
)
}
// TestDestroy_DeletionCookieFollowsTransport covers the trap in the
// deletion path. The store's template Secure is true, so a logout over
// plaintext that failed to track the transport would emit a Secure
// deletion cookie -- which the browser drops, leaving the session the
// user just tried to end still sitting in the jar.
func TestDestroy_DeletionCookieFollowsTransport(t *testing.T) {
t.Parallel()
s := testSession(t)
r := httptest.NewRequestWithContext(
context.Background(), http.MethodGet,
"http://example.com/", nil,
)
w := httptest.NewRecorder()
sess, err := s.Get(r)
require.NoError(t, err)
s.Destroy(sess)
require.NoError(t, s.Save(r, w, sess))
cookie := sessionCookieFrom(t, w)
require.Negative(
t, cookie.MaxAge,
"Destroy then Save should emit a deletion cookie",
)
assert.False(
t, cookie.Secure,
"a deletion cookie sent over plaintext must not be Secure, "+
"or the browser discards it and the session survives",
)
}
// TestRegenerate_BothCookiesFollowTransport covers the login path.
// Regenerate writes two cookies -- a deletion for the pre-login
// session and the new authenticated one -- and both have to match the
// transport or one of them is silently dropped.
func TestRegenerate_BothCookiesFollowTransport(t *testing.T) {
t.Parallel()
for _, tc := range transportCases() {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
s := testSession(t)
r := tc.request(t)
w := httptest.NewRecorder()
oldSess, err := s.Get(r)
require.NoError(t, err)
newSess, err := s.Regenerate(r, w, oldSess)
require.NoError(t, err)
s.SetUser(newSess, "user-1", "alice")
require.NoError(t, s.Save(r, w, newSess))
cookies := w.Result().Cookies()
require.Len(
t, cookies, 2,
"Regenerate then Save writes a deletion cookie "+
"and a replacement",
)
for _, c := range cookies {
assert.Equal(
t, tc.want, c.Secure,
"cookie %d Secure for %q: %s",
c.MaxAge, tc.name, tc.why,
)
}
})
}
}

1
internal/session/testing.go
View File

@@ -32,7 +32,6 @@ func NewForTest(
return &Session{
store: store,
key: key,
config: cfg,
log: log,
idleTimeout: cfg.SessionIdleTimeout,
now: now,