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

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.

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,