diff --git a/internal/middleware/csrf.go b/internal/middleware/csrf.go index 70b86ad..03a27a4 100644 --- a/internal/middleware/csrf.go +++ b/internal/middleware/csrf.go @@ -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) diff --git a/internal/middleware/csrf_test.go b/internal/middleware/csrf_test.go index d93161f..4273964 100644 --- a/internal/middleware/csrf_test.go +++ b/internal/middleware/csrf_test.go @@ -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) { +// 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(), 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() - r := httptest.NewRequestWithContext( - context.Background(), http.MethodGet, "/", nil) - r.TLS = &tls.ConnectionState{} + 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", ) } diff --git a/internal/middleware/export_test.go b/internal/middleware/export_test.go index b75e467..76ed406 100644 --- a/internal/middleware/export_test.go +++ b/internal/middleware/export_test.go @@ -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. diff --git a/internal/reqtls/reqtls.go b/internal/reqtls/reqtls.go new file mode 100644 index 0000000..f1704a4 --- /dev/null +++ b/internal/reqtls/reqtls.go @@ -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)) +} diff --git a/internal/reqtls/reqtls_test.go b/internal/reqtls/reqtls_test.go new file mode 100644 index 0000000..4a2c6f0 --- /dev/null +++ b/internal/reqtls/reqtls_test.go @@ -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", + ) +} diff --git a/internal/server/sentry.go b/internal/server/sentry.go index 5ad9314..a88e7d1 100644 --- a/internal/server/sentry.go +++ b/internal/server/sentry.go @@ -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 diff --git a/internal/session/export_test.go b/internal/session/export_test.go index 626a6fd..0b2ebc7 100644 --- a/internal/session/export_test.go +++ b/internal/session/export_test.go @@ -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) } diff --git a/internal/session/session.go b/internal/session/session.go index 4269f10..901568e 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -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 ( @@ -84,10 +85,9 @@ type Params struct { // Session manages encrypted session storage. 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 + store *sessions.CookieStore + key []byte // raw 32-byte auth key, also used for CSRF cookie signing + log *slog.Logger // 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 diff --git a/internal/session/session_test.go b/internal/session/session_test.go index 31bb472..962df24 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -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, + ) + } + }) + } +} diff --git a/internal/session/testing.go b/internal/session/testing.go index 0080a62..37666f1 100644 --- a/internal/session/testing.go +++ b/internal/session/testing.go @@ -32,7 +32,6 @@ func NewForTest( return &Session{ store: store, key: key, - config: cfg, log: log, idleTimeout: cfg.SessionIdleTimeout, now: now,