Derive cookie Secure and CSRF strictness from the request transport (closes #269)

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

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

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,
+				)
+			}
+		})
+	}
+}

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,