Stop refusing sign-in over a scheme other than the public URL's

The auth routes no longer return 400 when the resolved request scheme differs from MEALQUEST_PUBLIC_URL's. If Docker's published-port path presents its bridge gateway instead of Traefik as the direct peer, the forwarded headers are rightly ignored, but sign-in must still work: the OIDC redirect URI and the Secure cookies are fixed by MEALQUEST_PUBLIC_URL, not by the request. internal/auth is back to its state before the trusted-proxy change; client IP and scheme resolution stays in the router. Add TestLoginThroughRouter: it wires the real auth flow and session manager through server.NewRouter, trusting Traefik only, and checks that a login from Traefik and from an untrusted peer both redirect with the redirect URI from MEALQUEST_PUBLIC_URL and set a Secure login cookie. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-03 11:01:25 -04:00
parent 7e49b72ad4
commit 17d5152247
5 changed files with 152 additions and 185 deletions
@@ -34,9 +34,9 @@ Traefik terminates TLS. The service honours `X-Forwarded-For` and
 (comma-separated IPs/CIDRs; production lists `10.60.1.33`); from any other
 peer they are ignored. The client is the rightmost `X-Forwarded-For` address
 that is not a trusted proxy. The resolved client IP is added as `client_ip`
-to every log line written while handling a request, and the auth routes
-refuse requests whose resolved scheme differs from `MEALQUEST_PUBLIC_URL`'s. The OIDC redirect URI is always built
-from `MEALQUEST_PUBLIC_URL`, never from request headers.
+to every log line written while handling a request. The OIDC redirect URI
+and the Secure cookies are fixed by `MEALQUEST_PUBLIC_URL`, never by request
+headers, so sign-in works whichever peer the request arrives from.

 ## Repository layout

@@ -19,7 +19,6 @@ import (
 	"github.com/go-chi/chi/v5"
 	"golang.org/x/oauth2"

-	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/secureurl"
 )

@@ -84,10 +83,7 @@ type Auth struct {
 	// endSessionURL is the provider's end_session_endpoint; logout ends
 	// there so the provider's own session ends too.
 	endSessionURL string
-	// publicScheme is the public URL's scheme; requests to the auth routes
-	// must have arrived over it.
-	publicScheme string
-	logger       *slog.Logger
+	logger        *slog.Logger
 }

 // New discovers the provider at cfg.Issuer and returns the login flow. It
@@ -159,7 +155,6 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger
 		cookie:        newLoginCookie(cfg.SecretKey),
 		sessions:      sessions,
 		endSessionURL: discovered.EndSessionURI,
-		publicScheme:  cfg.PublicURL.Scheme,
 		logger:        logger,
 	}, nil
 }
@@ -198,39 +193,12 @@ func checkRedirect(req *http.Request, via []*http.Request) error {
 }

 // Mount registers GET LoginPath, GET CallbackPath and POST LogoutPath on r.
-// The routes need the request's resolved scheme in the context (see package
-// forwarded) and refuse requests that did not arrive over the public URL's
-// scheme.
 func (a *Auth) Mount(r chi.Router) {
-	r = r.With(a.requirePublicScheme)
 	r.Get(LoginPath, a.login)
 	r.Get(CallbackPath, a.callback)
 	r.Post(LogoutPath, a.logout)
 }

-// requirePublicScheme refuses a request whose resolved scheme differs from
-// the public URL's. The login and session cookies are Secure, and the
-// redirect URI is always built from the public URL, so a sign-in reaching the
-// service over another scheme cannot work: behind a TLS-terminating proxy
-// this means the proxy's X-Forwarded-Proto was not honoured, because the
-// proxy is not a trusted peer.
-func (a *Auth) requirePublicScheme(next http.Handler) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		scheme, ok := forwarded.Scheme(r.Context())
-		switch {
-		case !ok:
-			a.logger.ErrorContext(r.Context(), "auth route reached without a resolved request scheme")
-			http.Error(w, "internal error", http.StatusInternalServerError)
-		case scheme != a.publicScheme:
-			a.logger.WarnContext(r.Context(), "auth request scheme differs from the public URL; is the proxy a trusted peer?",
-				"scheme", scheme, "public_scheme", a.publicScheme)
-			http.Error(w, "sign-in must use the service's public URL", http.StatusBadRequest)
-		default:
-			next.ServeHTTP(w, r)
-		}
-	})
-}
-
 // EndSessionURL is the provider's end_session_endpoint.
 func (a *Auth) EndSessionURL() string { return a.endSessionURL }

@@ -17,7 +17,6 @@ import (
 	"math/big"
 	"net/http"
 	"net/http/httptest"
-	"net/netip"
 	"net/url"
 	"slices"
 	"strings"
@@ -27,8 +26,6 @@ import (
 	"time"

 	"github.com/go-chi/chi/v5"
-
-	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded"
 )

 const (
@@ -350,32 +347,11 @@ func newHarness(t *testing.T) *harness {
 	if err != nil {
 		t.Fatal(err)
 	}
-	r := publicRouter(a, testPublicURL.Scheme)
+	r := chi.NewRouter()
+	a.Mount(r)
 	return &harness{provider: p, sessions: sessions, router: r, logs: logs}
 }

-// publicRouter mounts a's routes behind the request-origin resolution the
-// server installs, with every request taken to have arrived over scheme.
-// Requests with noOrigin set skip the resolution.
-func publicRouter(a *Auth, scheme string) http.Handler {
-	r := chi.NewRouter()
-	r.Use(func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-			if req.Header.Get(noOrigin) == "" {
-				o := forwarded.Origin{ClientIP: netip.MustParseAddr("192.0.2.1"), Scheme: scheme}
-				req = req.WithContext(forwarded.NewContext(req.Context(), o))
-			}
-			next.ServeHTTP(w, req)
-		})
-	})
-	a.Mount(r)
-	return r
-}
-
-// noOrigin is a test-only request header telling publicRouter to leave the
-// request's origin unresolved.
-const noOrigin = "X-Test-No-Origin"
-
 func (h *harness) get(target string, cookies ...*http.Cookie) *http.Response {
 	req := httptest.NewRequest(http.MethodGet, target, nil)
 	for _, c := range cookies {
@@ -457,127 +433,6 @@ func TestLoginRedirect(t *testing.T) {
 	}
 }

-func TestAuthRoutesRequirePublicScheme(t *testing.T) {
-	t.Parallel()
-	p := newFakeProvider(t)
-	logs := &syncBuffer{}
-	a, err := New(context.Background(), Config{
-		Issuer:        p.issuer(),
-		ClientID:      testClientID,
-		ClientSecret:  testClientSecret,
-		PublicURL:     testPublicURL,
-		SecretKey:     []byte("server-secret-key"),
-		AllowedGroups: testAllowedGroups,
-	}, &memorySessions{}, slog.New(slog.NewTextHandler(logs, nil)))
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// The proxy's X-Forwarded-Proto was not honoured: the request looks like
-	// plain http while the public URL is https.
-	plain := publicRouter(a, "http")
-	for _, req := range []*http.Request{
-		httptest.NewRequest(http.MethodGet, LoginPath, nil),
-		httptest.NewRequest(http.MethodGet, CallbackPath+"?code=c&state=s", nil),
-		httptest.NewRequest(http.MethodPost, LogoutPath, nil),
-	} {
-		rec := httptest.NewRecorder()
-		plain.ServeHTTP(rec, req)
-		if rec.Code != http.StatusBadRequest {
-			t.Errorf("%s %s over http = %d, want 400", req.Method, req.URL.Path, rec.Code)
-		}
-		if loc := rec.Header().Get("Location"); loc != "" {
-			t.Errorf("%s %s over http redirected to %q", req.Method, req.URL.Path, loc)
-		}
-		if c := rec.Result().Cookies(); len(c) != 0 {
-			t.Errorf("%s %s over http set cookies %v", req.Method, req.URL.Path, c)
-		}
-	}
-	if !strings.Contains(logs.String(), "trusted peer") {
-		t.Errorf("logs = %q, want the scheme mismatch explained", logs.String())
-	}
-
-	bare := chi.NewRouter()
-	a.Mount(bare)
-	rec := httptest.NewRecorder()
-	bare.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, LoginPath, nil))
-	if rec.Code != http.StatusInternalServerError {
-		t.Errorf("login without a resolved origin = %d, want 500", rec.Code)
-	}
-}
-
-// TestLoginBehindTraefik resolves each request with forwarded.Resolve, as
-// the server's middleware does, trusting Traefik only.
-func TestLoginBehindTraefik(t *testing.T) {
-	t.Parallel()
-	p := newFakeProvider(t)
-	a, err := New(context.Background(), Config{
-		Issuer:        p.issuer(),
-		ClientID:      testClientID,
-		ClientSecret:  testClientSecret,
-		PublicURL:     testPublicURL,
-		SecretKey:     []byte("server-secret-key"),
-		AllowedGroups: testAllowedGroups,
-	}, &memorySessions{}, slog.New(slog.NewTextHandler(io.Discard, nil)))
-	if err != nil {
-		t.Fatal(err)
-	}
-	trusted, err := forwarded.ParseTrusted("10.60.1.33")
-	if err != nil {
-		t.Fatal(err)
-	}
-	r := chi.NewRouter()
-	r.Use(func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-			o, err := forwarded.Resolve(req, trusted)
-			if err != nil {
-				t.Errorf("Resolve: %v", err)
-				return
-			}
-			next.ServeHTTP(w, req.WithContext(forwarded.NewContext(req.Context(), o)))
-		})
-	})
-	a.Mount(r)
-
-	login := func(peer string) *http.Response {
-		// Traefik reaches the service over plain http on its internal
-		// address; the host headers must not leak into the redirect URI.
-		req := httptest.NewRequest(http.MethodGet, "http://mealquest:8080"+LoginPath, nil)
-		req.RemoteAddr = peer
-		req.Header.Set("X-Forwarded-For", "203.0.113.7")
-		req.Header.Set("X-Forwarded-Proto", "https")
-		req.Header.Set("X-Forwarded-Host", "evil.example.com")
-		rec := httptest.NewRecorder()
-		r.ServeHTTP(rec, req)
-		return rec.Result()
-	}
-
-	resp := login("10.60.1.33:41000")
-	if resp.StatusCode != http.StatusFound {
-		t.Fatalf("login via Traefik status = %d, want 302", resp.StatusCode)
-	}
-	loc, err := url.Parse(resp.Header.Get("Location"))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if got, want := loc.Query().Get("redirect_uri"), "https://meals.example.com/auth/callback"; got != want {
-		t.Errorf("redirect_uri = %q, want %q from the public URL", got, want)
-	}
-	cookies := resp.Cookies()
-	if len(cookies) != 1 || cookies[0].Name != loginCookieName || !cookies[0].Secure {
-		t.Errorf("login cookies = %v, want one Secure %s", cookies, loginCookieName)
-	}
-
-	// The same headers from an untrusted peer are ignored: the request is
-	// plain http, so sign-in is refused rather than setting a Secure cookie
-	// the browser would drop.
-	resp = login("198.51.100.9:5000")
-	if resp.StatusCode != http.StatusBadRequest || len(resp.Cookies()) != 0 {
-		t.Errorf("login spoofing X-Forwarded-Proto = %d with cookies %v, want 400 and none",
-			resp.StatusCode, resp.Cookies())
-	}
-}
-
 func TestLoginStateIsFreshEachTime(t *testing.T) {
 	t.Parallel()
 	h := newHarness(t)
@@ -1049,7 +904,8 @@ func TestCallbackRefusesTokenRedirectToHTTP(t *testing.T) {
 			}
 			// Trust the test server's certificate; the redirect policy stays.
 			a.client.Transport = tlsToken.Client().Transport
-			r := publicRouter(a, testPublicURL.Scheme)
+			r := chi.NewRouter()
+			a.Mount(r)
 			h := &harness{provider: p, sessions: sessions, router: r, logs: logs}
 			l := h.login(t, LoginPath)

@@ -13,6 +13,8 @@ import (
 	"testing"
 	"time"

+	"github.com/go-chi/chi/v5"
+
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/migrations"
 )
@@ -70,7 +72,8 @@ func newSessionHarness(t *testing.T) *sessionHarness {
 	if err != nil {
 		t.Fatal(err)
 	}
-	r := publicRouter(a, testPublicURL.Scheme)
+	r := chi.NewRouter()
+	a.Mount(r)
 	h.harness = &harness{provider: p, router: r, logs: logs}
 	h.auth = a
 	return h
@@ -0,0 +1,140 @@
+package server
+
+import (
+	"context"
+	"encoding/json"
+	"io"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+	"testing/fstest"
+	"time"
+
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/auth"
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded"
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store"
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/migrations"
+)
+
+// newDiscoveryProvider serves the OIDC discovery document auth.New needs;
+// login only redirects to its authorization endpoint.
+func newDiscoveryProvider(t *testing.T) *httptest.Server {
+	t.Helper()
+	var srv *httptest.Server
+	srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/.well-known/openid-configuration" {
+			http.NotFound(w, r)
+			return
+		}
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]any{
+			"issuer":                                srv.URL,
+			"authorization_endpoint":                srv.URL + "/authorize",
+			"token_endpoint":                        srv.URL + "/token",
+			"jwks_uri":                              srv.URL + "/jwks",
+			"userinfo_endpoint":                     srv.URL + "/userinfo",
+			"end_session_endpoint":                  srv.URL + "/end-session/",
+			"response_types_supported":              []string{"code"},
+			"subject_types_supported":               []string{"public"},
+			"id_token_signing_alg_values_supported": []string{"RS256"},
+		})
+	}))
+	t.Cleanup(srv.Close)
+	return srv
+}
+
+// newLoginRouter wires the production router as cmd/server does: the real
+// auth flow and session manager, trusting Traefik only.
+func newLoginRouter(t *testing.T, issuer string) http.Handler {
+	t.Helper()
+	ctx := context.Background()
+	db, err := store.Open(ctx, t.TempDir())
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		if err := db.Close(); err != nil {
+			t.Error(err)
+		}
+	})
+	if err := store.Migrate(ctx, db.Writer, migrations.FS); err != nil {
+		t.Fatal(err)
+	}
+	sessions, err := auth.NewSessionManager(db, 720*time.Hour, 4320*time.Hour)
+	if err != nil {
+		t.Fatal(err)
+	}
+	logger := slog.New(forwarded.LogHandler(slog.NewTextHandler(io.Discard, nil)))
+	flow, err := auth.New(ctx, auth.Config{
+		Issuer:        issuer,
+		ClientID:      "mealquest",
+		ClientSecret:  "client-s3cret",
+		PublicURL:     &url.URL{Scheme: "https", Host: "meals.example.com"},
+		SecretKey:     []byte("server-secret-key"),
+		AllowedGroups: []string{"mealquest-admin"},
+	}, sessions, logger)
+	if err != nil {
+		t.Fatal(err)
+	}
+	trusted, err := forwarded.ParseTrusted(traefik)
+	if err != nil {
+		t.Fatal(err)
+	}
+	r, err := NewRouter(logger, "dev", trusted, flow, sessions,
+		fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	return r
+}
+
+// TestLoginThroughRouter starts a login through the production router. The
+// redirect URI and the Secure login cookie come from MEALQUEST_PUBLIC_URL
+// whatever the direct peer: forwarded headers from an untrusted peer are
+// ignored but never fail sign-in.
+func TestLoginThroughRouter(t *testing.T) {
+	t.Parallel()
+	provider := newDiscoveryProvider(t)
+	router := newLoginRouter(t, provider.URL)
+	// Traefik reaches the service over plain http on its internal address;
+	// the host headers must not leak into the redirect URI.
+	headers := map[string]string{
+		forwarded.HeaderFor:   "203.0.113.7",
+		forwarded.HeaderProto: "https",
+		"X-Forwarded-Host":    "evil.example.com",
+	}
+	tests := []struct {
+		name string
+		peer string
+	}{
+		{"trusted proxy", traefik + ":41000"},
+		{"untrusted peer", "172.17.0.1:5000"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			rec := httptest.NewRecorder()
+			router.ServeHTTP(rec, originRequest("http://mealquest:8080"+auth.LoginPath, tt.peer, headers))
+			resp := rec.Result()
+			if resp.StatusCode != http.StatusFound {
+				t.Fatalf("login status = %d, want 302", resp.StatusCode)
+			}
+			loc, err := url.Parse(resp.Header.Get("Location"))
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got, want := loc.Scheme+"://"+loc.Host+loc.Path, provider.URL+"/authorize"; got != want {
+				t.Errorf("redirect to %s, want %s", got, want)
+			}
+			if got, want := loc.Query().Get("redirect_uri"), "https://meals.example.com/auth/callback"; got != want {
+				t.Errorf("redirect_uri = %q, want %q from the public URL", got, want)
+			}
+			cookies := resp.Cookies()
+			if len(cookies) != 1 || cookies[0].Name != "__Host-mealquest_login" || !cookies[0].Secure || !cookies[0].HttpOnly {
+				t.Errorf("login cookies = %v, want one Secure, HttpOnly __Host-mealquest_login", cookies)
+			}
+		})
+	}
+}