From e8dafda534c15f607166fcc63d277c9fbd74c113 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 09:45:33 -0400 Subject: [PATCH] Require HTTPS for OIDC and public URLs; stop logging token responses Address review findings on a10b24c: - F1: the OIDC issuer and the discovered authorization, token and JWKS endpoints must be https://; plain http:// is allowed only for the loopback hosts localhost, 127.0.0.1 and [::1]. Config rejects a remote http:// issuer at startup; auth.New rejects a discovered remote http:// endpoint, naming it, before it is used. - F2: a failed code exchange is reduced to a fixed category plus the HTTP status; the token endpoint's body and error_description are never logged or wrapped into the returned error. - F3: MEALQUEST_PUBLIC_URL must be https:// unless its host is loopback, so the Secure login cookie is always sent back on the callback. The rule lives once, in internal/secureurl.Check. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 +- internal/auth/auth.go | 56 +++++++++- internal/auth/auth_test.go | 148 ++++++++++++++++++++++++++- internal/config/config.go | 18 +++- internal/config/config_test.go | 27 +++++ internal/secureurl/secureurl.go | 32 ++++++ internal/secureurl/secureurl_test.go | 40 ++++++++ 7 files changed, 314 insertions(+), 11 deletions(-) create mode 100644 internal/secureurl/secureurl.go create mode 100644 internal/secureurl/secureurl_test.go diff --git a/README.md b/README.md index 27339ea..d08ebbf 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,9 @@ make dev (which proxies `/api`, `/auth` and `/media` to `:8080`). Open http://localhost:5173. Configuration is environment-only and fails fast: every variable above is required, and secrets are read only from files named -by `*_FILE` variables. +by `*_FILE` variables. `MEALQUEST_PUBLIC_URL`, `MEALQUEST_OIDC_ISSUER` and the +endpoints the issuer advertises must be `https://`; plain `http://` is +accepted only for `localhost`, `127.0.0.1` and `[::1]`. ## Common commands diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 3596a9b..ed351ce 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -17,6 +17,8 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/go-chi/chi/v5" "golang.org/x/oauth2" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/secureurl" ) // Routes owned by this package. @@ -85,12 +87,33 @@ func New(ctx context.Context, cfg Config, sessions SessionStarter, logger *slog. return nil, errors.New("auth: session starter is required") } + if err := checkEndpoint("issuer", cfg.Issuer); err != nil { + return nil, err + } + client := &http.Client{Timeout: providerTimeout} provider, err := oidc.NewProvider(oidc.ClientContext(ctx, client), cfg.Issuer) if err != nil { return nil, fmt.Errorf("auth: OIDC discovery: %w", err) } endpoint := provider.Endpoint() + var discovered struct { + JWKSURI string `json:"jwks_uri"` + } + if err := provider.Claims(&discovered); err != nil { + return nil, fmt.Errorf("auth: OIDC discovery: %w", err) + } + // The token endpoint receives the client secret and the key set decides + // which tokens verify, so every endpoint used must pass the same rule. + for _, e := range []struct{ name, raw string }{ + {"authorization endpoint", endpoint.AuthURL}, + {"token endpoint", endpoint.TokenURL}, + {"JWKS endpoint", discovered.JWKSURI}, + } { + if err := checkEndpoint(e.name, e.raw); err != nil { + return nil, err + } + } // client_secret_basic, the OIDC default; no auto-detection retries. endpoint.AuthStyle = oauth2.AuthStyleInHeader @@ -113,6 +136,19 @@ func New(ctx context.Context, cfg Config, sessions SessionStarter, logger *slog. }, nil } +// checkEndpoint applies secureurl.Check to the provider URL raw, naming the +// endpoint in the error. +func checkEndpoint(name, raw string) error { + u, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("auth: OIDC %s %q is not a valid URL", name, raw) + } + if err := secureurl.Check(u); err != nil { + return fmt.Errorf("auth: OIDC %s %q: %w", name, raw, err) + } + return nil +} + // Mount registers GET LoginPath and GET CallbackPath on r. func (a *Auth) Mount(r chi.Router) { r.Get(LoginPath, a.login) @@ -199,7 +235,7 @@ func (a *Auth) verify(ctx context.Context, code string, state loginState) (Claim token, err := a.oauth.Exchange(context.WithValue(ctx, oauth2.HTTPClient, a.client), code, oauth2.VerifierOption(state.Verifier)) if err != nil { - return Claims{}, fmt.Errorf("exchange code: %w", err) + return Claims{}, exchangeError(err) } raw, ok := token.Extra("id_token").(string) if !ok || raw == "" { @@ -221,3 +257,21 @@ func (a *Auth) verify(ctx context.Context, code string, state loginState) (Claim } return claims, nil } + +// exchangeError reduces a failed code exchange to a fixed category and safe +// metadata. The oauth2 error text can carry the token endpoint's response +// body or error_description, which may echo credentials, so it is dropped. +func exchangeError(err error) error { + var retrieve *oauth2.RetrieveError + var transport *url.Error + switch { + case errors.As(err, &retrieve): + return fmt.Errorf("exchange code: token endpoint rejected the request (HTTP %d)", retrieve.Response.StatusCode) + case errors.As(err, &transport): + // Request failures never carry the response; the URL is the token + // endpoint from discovery. + return fmt.Errorf("exchange code: token request failed: %w", transport) + default: + return errors.New("exchange code: token endpoint returned an unusable response") + } +} diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index da351cf..45db766 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1,6 +1,7 @@ package auth import ( + "bytes" "context" "crypto" "crypto/hmac" @@ -12,6 +13,7 @@ import ( "errors" "io" "log/slog" + "maps" "math/big" "net/http" "net/http/httptest" @@ -42,6 +44,16 @@ type fakeProvider struct { mu sync.Mutex codes map[string]issuedCode + // discoveryOverrides replace fields of the discovery document. + discoveryOverrides map[string]any + // tokenReply, when set, is returned by the token endpoint instead of a + // token. + tokenReply *tokenReply +} + +type tokenReply struct { + status int + body string } type issuedCode struct { @@ -68,7 +80,7 @@ func newFakeProvider(t *testing.T) *fakeProvider { func (p *fakeProvider) issuer() string { return p.srv.URL } func (p *fakeProvider) discovery(w http.ResponseWriter, _ *http.Request) { - writeJSON(w, map[string]any{ + doc := map[string]any{ "issuer": p.issuer(), "authorization_endpoint": p.issuer() + "/authorize", "token_endpoint": p.issuer() + "/token", @@ -76,7 +88,11 @@ func (p *fakeProvider) discovery(w http.ResponseWriter, _ *http.Request) { "response_types_supported": []string{"code"}, "subject_types_supported": []string{"public"}, "id_token_signing_alg_values_supported": []string{"RS256", "HS256"}, - }) + } + p.mu.Lock() + maps.Copy(doc, p.discoveryOverrides) + p.mu.Unlock() + writeJSON(w, doc) } func (p *fakeProvider) jwks(w http.ResponseWriter, _ *http.Request) { @@ -93,6 +109,15 @@ func (p *fakeProvider) jwks(w http.ResponseWriter, _ *http.Request) { // token checks client authentication, the code and the PKCE verifier before // returning the code's ID token. func (p *fakeProvider) token(w http.ResponseWriter, r *http.Request) { + p.mu.Lock() + reply := p.tokenReply + p.mu.Unlock() + if reply != nil { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(reply.status) + io.WriteString(w, reply.body) + return + } id, secret, ok := r.BasicAuth() if !ok || id != testClientID || secret != testClientSecret { http.Error(w, `{"error":"invalid_client"}`, http.StatusUnauthorized) @@ -226,25 +251,45 @@ type harness struct { provider *fakeProvider sessions *memorySessions router http.Handler + logs *syncBuffer +} + +// syncBuffer is a bytes.Buffer safe for concurrent log writes. +type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *syncBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() } func newHarness(t *testing.T) *harness { t.Helper() p := newFakeProvider(t) sessions := &memorySessions{} + logs := &syncBuffer{} a, err := New(context.Background(), Config{ Issuer: p.issuer(), ClientID: testClientID, ClientSecret: testClientSecret, PublicURL: testPublicURL, SecretKey: []byte("server-secret-key"), - }, sessions, slog.New(slog.DiscardHandler)) + }, sessions, slog.New(slog.NewTextHandler(logs, nil))) if err != nil { t.Fatal(err) } r := chi.NewRouter() a.Mount(r) - return &harness{provider: p, sessions: sessions, router: r} + return &harness{provider: p, sessions: sessions, router: r, logs: logs} } func (h *harness) get(target string, cookies ...*http.Cookie) *http.Response { @@ -548,6 +593,57 @@ func TestCallbackSessionError(t *testing.T) { } } +// A failed code exchange is logged by category and status only: the token +// endpoint's body and error_description may echo credentials. +func TestCallbackExchangeFailureDoesNotLogProviderResponse(t *testing.T) { + t.Parallel() + const leaked = "leaked-" + testClientSecret + tests := []struct { + name string + reply tokenReply + wantLogs string + }{ + { + name: "error response", + reply: tokenReply{http.StatusBadRequest, `{"error":"invalid_grant","error_description":"` + leaked + `"}`}, + wantLogs: "HTTP 400", + }, + { + name: "unparseable error response", + reply: tokenReply{http.StatusInternalServerError, leaked}, + wantLogs: "HTTP 500", + }, + { + name: "success without access token", + reply: tokenReply{http.StatusOK, `{"id_token":"` + leaked + `"}`}, + wantLogs: "unusable response", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + h := newHarness(t) + h.provider.mu.Lock() + h.provider.tokenReply = &tc.reply + h.provider.mu.Unlock() + l := h.login(t, LoginPath) + + resp := h.callback(l, l.query.Get("state"), "unused") + + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("callback status = %d, want 401", resp.StatusCode) + } + logs := h.logs.String() + if strings.Contains(logs, leaked) { + t.Errorf("logs contain the provider response: %s", logs) + } + if !strings.Contains(logs, tc.wantLogs) { + t.Errorf("logs = %q, want %q", logs, tc.wantLogs) + } + }) + } +} + func TestLoginCookieExpires(t *testing.T) { t.Parallel() c := newLoginCookie([]byte("server-secret-key")) @@ -635,3 +731,47 @@ func assertLoginCookieCleared(t *testing.T, resp *http.Response) { } t.Errorf("callback did not clear the %s cookie", loginCookieName) } + +func TestNewRejectsRemoteHTTPIssuer(t *testing.T) { + t.Parallel() + for _, issuer := range []string{"http://auth.example.com/application/o/mealquest/", "http://192.0.2.10"} { + _, err := New(context.Background(), Config{ + Issuer: issuer, + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + }, &memorySessions{}, slog.New(slog.DiscardHandler)) + if err == nil || !strings.Contains(err.Error(), "issuer") { + t.Errorf("New(issuer %q) error = %v, want issuer rejected", issuer, err) + } + } +} + +func TestNewRejectsRemoteHTTPEndpoint(t *testing.T) { + t.Parallel() + tests := []struct { + field, name string + }{ + {"authorization_endpoint", "authorization endpoint"}, + {"token_endpoint", "token endpoint"}, + {"jwks_uri", "JWKS endpoint"}, + } + for _, tc := range tests { + t.Run(tc.field, func(t *testing.T) { + t.Parallel() + p := newFakeProvider(t) + p.discoveryOverrides = map[string]any{tc.field: "http://auth.example.com/" + tc.field} + _, err := New(context.Background(), Config{ + Issuer: p.issuer(), + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + }, &memorySessions{}, slog.New(slog.DiscardHandler)) + if err == nil || !strings.Contains(err.Error(), tc.name) { + t.Errorf("New error = %v, want %s rejected", err, tc.name) + } + }) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 5c821e2..113c855 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -11,6 +11,8 @@ import ( "strconv" "strings" "time" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/secureurl" ) // Environment variable names. @@ -59,12 +61,12 @@ func load(lookup func(string) (string, bool)) (Config, error) { r := reader{lookup: lookup} cfg := Config{ HTTPAddr: r.listenAddr(EnvHTTPAddr), - PublicURL: r.httpURL(EnvPublicURL), + PublicURL: r.secureURL(EnvPublicURL), DataDir: r.dir(EnvDataDir), ShutdownTimeout: r.duration(EnvShutdownTimeout), SecretKey: r.secretFile(EnvSecretKeyFile), - OIDCIssuer: r.httpURL(EnvOIDCIssuer), + OIDCIssuer: r.secureURL(EnvOIDCIssuer), OIDCClientID: r.str(EnvOIDCClientID), OIDCClientSecret: r.secretFile(EnvOIDCClientSecretFile), OIDCAllowedGroups: r.list(EnvOIDCAllowedGroups), @@ -195,15 +197,21 @@ func validHostname(h string) bool { return true } -func (r *reader) httpURL(name string) *url.URL { +// secureURL reads an absolute URL that must pass secureurl.Check: https://, +// or http:// to a loopback host only. +func (r *reader) secureURL(name string) *url.URL { v, ok := r.required(name) if !ok { return nil } // The value is never echoed: it may carry credentials. u, err := url.Parse(v) - if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Hostname() == "" { - r.fail(name, "invalid URL (want absolute http:// or https:// URL)") + if err != nil { + r.fail(name, "invalid URL (want absolute https:// URL)") + return nil + } + if err := secureurl.Check(u); err != nil { + r.fail(name, "invalid URL: %v", err) return nil } if u.User != nil { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index da434ac..c00286b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -150,6 +150,24 @@ func TestLoad(t *testing.T) { env: with(map[string]string{EnvPublicURL: "http://:8080"}), badVars: []string{EnvPublicURL}, }, + { + name: "plain HTTP public URL on a remote host", + env: with(map[string]string{EnvPublicURL: "http://meals.example.com"}), + badVars: []string{EnvPublicURL}, + }, + { + name: "plain HTTP public URL on a non-loopback IP", + env: with(map[string]string{EnvPublicURL: "http://192.168.1.20:8080"}), + badVars: []string{EnvPublicURL}, + }, + { + name: "plain HTTP public URL on 127.0.0.1", + env: with(map[string]string{EnvPublicURL: "http://127.0.0.1:8080"}), + }, + { + name: "plain HTTP public URL on [::1]", + env: with(map[string]string{EnvPublicURL: "http://[::1]:8080"}), + }, { name: "relative public URL", env: with(map[string]string{EnvPublicURL: "meals.example.com"}), @@ -200,6 +218,15 @@ func TestLoad(t *testing.T) { env: with(map[string]string{EnvOIDCIssuer: "ftp://auth.example.com"}), badVars: []string{EnvOIDCIssuer}, }, + { + name: "plain HTTP OIDC issuer on a remote host", + env: with(map[string]string{EnvOIDCIssuer: "http://auth.example.com/application/o/mealquest/"}), + badVars: []string{EnvOIDCIssuer}, + }, + { + name: "plain HTTP OIDC issuer on localhost", + env: with(map[string]string{EnvOIDCIssuer: "http://localhost:9000/application/o/mealquest/"}), + }, { name: "several allowed groups", env: with(map[string]string{EnvOIDCAllowedGroups: "mealquest-admin, family"}), diff --git a/internal/secureurl/secureurl.go b/internal/secureurl/secureurl.go new file mode 100644 index 0000000..09f3129 --- /dev/null +++ b/internal/secureurl/secureurl.go @@ -0,0 +1,32 @@ +// Package secureurl holds the one transport rule for URLs that carry +// credentials or session cookies: HTTPS, or plain HTTP to a loopback host for +// local development and tests. +package secureurl + +import ( + "errors" + "fmt" + "net/url" + "strings" +) + +// loopbackHosts are the only hosts plain HTTP is allowed for, compared +// case-insensitively. url.URL's Hostname strips the brackets from "[::1]". +var loopbackHosts = map[string]bool{"localhost": true, "127.0.0.1": true, "::1": true} + +// Check reports an error unless u is an absolute https:// URL, or an http:// +// URL whose host is localhost, 127.0.0.1 or ::1. +func Check(u *url.URL) error { + switch { + case u.Hostname() == "": + return errors.New("URL has no host") + case u.Scheme == "https": + return nil + case u.Scheme == "http" && loopbackHosts[strings.ToLower(u.Hostname())]: + return nil + case u.Scheme == "http": + return errors.New("plain http:// is allowed only for localhost, 127.0.0.1 or [::1]; use https://") + default: + return fmt.Errorf("unsupported scheme %q (want https://)", u.Scheme) + } +} diff --git a/internal/secureurl/secureurl_test.go b/internal/secureurl/secureurl_test.go new file mode 100644 index 0000000..bebc707 --- /dev/null +++ b/internal/secureurl/secureurl_test.go @@ -0,0 +1,40 @@ +package secureurl + +import ( + "net/url" + "testing" +) + +func TestCheck(t *testing.T) { + t.Parallel() + tests := []struct { + raw string + ok bool + }{ + {"https://meals.example.com", true}, + {"https://auth.example.com/application/o/mealquest/", true}, + {"https://192.0.2.10:8443", true}, + {"http://localhost:8080", true}, + {"http://LOCALHOST:8080", true}, + {"http://127.0.0.1:8080/path", true}, + {"http://[::1]:8080", true}, + {"http://[::1]", true}, + {"http://meals.example.com", false}, + {"http://192.0.2.10", false}, + {"http://127.0.0.2", false}, + {"http://localhost.example.com", false}, + {"http://0.0.0.0:8080", false}, + {"ftp://localhost", false}, + {"https://", false}, + {"meals.example.com", false}, + } + for _, tc := range tests { + u, err := url.Parse(tc.raw) + if err != nil { + t.Fatal(err) + } + if err := Check(u); (err == nil) != tc.ok { + t.Errorf("Check(%q) error = %v, want ok=%v", tc.raw, err, tc.ok) + } + } +}