Reduce ID token verification errors to a fixed category

go-oidc's RemoteKeySet puts the JWKS endpoint's response body in its error
for non-200 and malformed 200 responses; Verify wrapped it and the callback
logged it. verifyError now keeps only a fixed category, plus the expiry for
an expired token. A test with a sentinel JWKS body checks it reaches
neither the returned error, the response nor the logs.

Addresses feature-review finding F03-03 on PR #24.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/auth/auth.go

@@ -13,6 +13,7 @@ import (
 	"net/http"
 	"net/url"
 	"slices"
+	"sync/atomic"
 	"time"
 
 	"github.com/coreos/go-oidc/v3/oidc"
@@ -107,9 +108,12 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger
 	}
 
 	client := &http.Client{Timeout: providerTimeout, CheckRedirect: checkRedirect}
-	provider, err := oidc.NewProvider(oidc.ClientContext(ctx, client), cfg.Issuer)
+	discovery := *client
+	recorder := &statusRecorder{next: http.DefaultTransport}
+	discovery.Transport = recorder
+	provider, err := oidc.NewProvider(oidc.ClientContext(ctx, &discovery), cfg.Issuer)
 	if err != nil {
-		return nil, fmt.Errorf("auth: OIDC discovery: %w", err)
+		return nil, discoveryError(err, int(recorder.status.Load()))
 	}
 	endpoint := provider.Endpoint()
 	var discovered struct {
@@ -159,6 +163,32 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger
 	}, nil
 }
 
+// statusRecorder records the status of the last response it carries.
+type statusRecorder struct {
+	next   http.RoundTripper
+	status atomic.Int32
+}
+
+func (s *statusRecorder) RoundTrip(req *http.Request) (*http.Response, error) {
+	resp, err := s.next.RoundTrip(req)
+	if err == nil {
+		s.status.Store(int32(resp.StatusCode))
+	}
+	return resp, err
+}
+
+// discoveryError reports a failed discovery. go-oidc puts the provider's
+// response body in its error for a non-200 discovery response, so that
+// error is replaced by the status alone. Transport errors carry no response
+// content and keep their cause.
+func discoveryError(err error, status int) error {
+	var urlErr *url.Error
+	if !errors.As(err, &urlErr) && status != 0 && status != http.StatusOK {
+		return fmt.Errorf("auth: OIDC discovery: provider answered HTTP %d", status)
+	}
+	return fmt.Errorf("auth: OIDC discovery: %w", err)
+}
+
 // checkEndpoint applies secureurl.Check to the provider URL raw, naming the
 // endpoint in the error.
 func checkEndpoint(name, raw string) error {
@@ -323,7 +353,7 @@ func (a *Auth) verify(ctx context.Context, code string, state loginState) (Claim
 	}
 	idToken, err := a.verifier.Verify(ctx, raw)
 	if err != nil {
-		return Claims{}, fmt.Errorf("verify ID token: %w", err)
+		return Claims{}, verifyError(err)
 	}
 	if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(state.Nonce)) != 1 {
 		return Claims{}, errors.New("ID token nonce mismatch")
@@ -374,6 +404,17 @@ func userinfoError(err error) error {
 	return errors.New("userinfo: endpoint returned an unusable response")
 }
 
+// verifyError reduces a failed ID token verification to a fixed category.
+// go-oidc's error text can carry the JWKS endpoint's response body or
+// token-supplied values, so only the expiry of an expired token is kept.
+func verifyError(err error) error {
+	var expired *oidc.TokenExpiredError
+	if errors.As(err, &expired) {
+		return fmt.Errorf("verify ID token: token expired at %s", expired.Expiry.UTC().Format(time.RFC3339))
+	}
+	return errors.New("verify ID token: token or provider key set rejected")
+}
+
 // member reports whether groups contains an allowed group.
 func (a *Auth) member(groups []string) bool {
 	return slices.ContainsFunc(groups, func(g string) bool { return slices.Contains(a.allowed, g) })

internal/auth/auth_test.go

@@ -11,6 +11,7 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"log/slog"
 	"maps"
@@ -59,6 +60,8 @@ type fakeProvider struct {
 	userinfoHits int
 	// userinfoReply, when set, is returned by the userinfo endpoint instead.
 	userinfoReply *tokenReply
+	// jwksReply, when set, is returned by the JWKS endpoint instead.
+	jwksReply *tokenReply
 }
 
 type tokenReply struct {
@@ -109,6 +112,15 @@ func (p *fakeProvider) discovery(w http.ResponseWriter, _ *http.Request) {
 }
 
 func (p *fakeProvider) jwks(w http.ResponseWriter, _ *http.Request) {
+	p.mu.Lock()
+	reply := p.jwksReply
+	p.mu.Unlock()
+	if reply != nil {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(reply.status)
+		io.WriteString(w, reply.body)
+		return
+	}
 	writeJSON(w, map[string]any{"keys": []map[string]string{{
 		"kty": "RSA",
 		"kid": testKID,
@@ -307,6 +319,7 @@ func (m *memorySessions) started() []Claims {
 }
 
 type harness struct {
+	auth     *Auth
 	provider *fakeProvider
 	sessions *memorySessions
 	router   http.Handler
@@ -349,7 +362,7 @@ func newHarness(t *testing.T) *harness {
 	}
 	r := chi.NewRouter()
 	a.Mount(r)
-	return &harness{provider: p, sessions: sessions, router: r, logs: logs}
+	return &harness{auth: a, provider: p, sessions: sessions, router: r, logs: logs}
 }
 
 func (h *harness) get(target string, cookies ...*http.Cookie) *http.Response {
@@ -727,6 +740,66 @@ func TestCallbackExchangeFailureDoesNotLogProviderResponse(t *testing.T) {
 	}
 }
 
+func TestCallbackKeySetFailureDoesNotLogProviderResponse(t *testing.T) {
+	t.Parallel()
+	const sentinel = "sentinel-jwks-body-5c1e"
+	tests := []struct {
+		name  string
+		reply tokenReply
+	}{
+		{"error response", tokenReply{http.StatusInternalServerError, sentinel}},
+		{"malformed success", tokenReply{http.StatusOK, `{"keys":"` + sentinel + `"`}},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			h := newHarness(t)
+			h.provider.mu.Lock()
+			h.provider.jwksReply = &tc.reply
+			h.provider.mu.Unlock()
+
+			// The returned error, checked through verify directly.
+			l := h.login(t, LoginPath)
+			req := httptest.NewRequest(http.MethodGet, CallbackPath, nil)
+			req.AddCookie(l.cookie)
+			state, err := h.auth.cookie.read(req, time.Now())
+			if err != nil {
+				t.Fatal(err)
+			}
+			code := h.provider.authorize(l.query.Get("code_challenge"), h.provider.signRS256(h.provider.claims(l.query.Get("nonce"))))
+			_, err = h.auth.verify(context.Background(), code, state)
+			if err == nil {
+				t.Fatal("verify succeeded with an unusable key set")
+			}
+			if strings.Contains(err.Error(), sentinel) {
+				t.Errorf("verify error contains the JWKS response: %v", err)
+			}
+
+			// The logs and response, through the callback.
+			l = h.login(t, LoginPath)
+			resp := h.callback(l, l.query.Get("state"), h.provider.signRS256(h.provider.claims(l.query.Get("nonce"))))
+
+			if resp.StatusCode != http.StatusUnauthorized {
+				t.Errorf("callback status = %d, want 401", resp.StatusCode)
+			}
+			body, _ := io.ReadAll(resp.Body)
+			if strings.Contains(string(body), sentinel) {
+				t.Errorf("response contains the JWKS response: %s", body)
+			}
+			logs := h.logs.String()
+			if strings.Contains(logs, sentinel) {
+				t.Errorf("logs contain the JWKS response: %s", logs)
+			}
+			if want := "token or provider key set rejected"; !strings.Contains(logs, want) {
+				t.Errorf("logs = %q, want %q", logs, want)
+			}
+			if len(h.sessions.started()) != 0 {
+				t.Error("a session started")
+			}
+		})
+	}
+}
+
 func TestLoginCookieExpires(t *testing.T) {
 	t.Parallel()
 	c := newLoginCookie([]byte("server-secret-key"))
@@ -789,6 +862,41 @@ func TestNewFailsWhenDiscoveryFails(t *testing.T) {
 	}
 }
 
+func TestNewDiscoveryErrorOmitsProviderBody(t *testing.T) {
+	t.Parallel()
+	const sentinel = "sentinel-discovery-body-7f3a"
+	for _, status := range []int{http.StatusNotFound, http.StatusInternalServerError} {
+		t.Run(http.StatusText(status), func(t *testing.T) {
+			t.Parallel()
+			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				w.WriteHeader(status)
+				io.WriteString(w, sentinel)
+			}))
+			t.Cleanup(srv.Close)
+			logs := &syncBuffer{}
+			_, err := New(context.Background(), Config{
+				Issuer:        srv.URL,
+				ClientID:      testClientID,
+				ClientSecret:  testClientSecret,
+				PublicURL:     testPublicURL,
+				SecretKey:     []byte("k"),
+				AllowedGroups: testAllowedGroups,
+			}, &memorySessions{}, slog.New(slog.NewTextHandler(logs, nil)))
+			if err == nil {
+				t.Fatal("New succeeded without a discovery document")
+			}
+			// cmd/server logs the returned error at startup.
+			slog.New(slog.NewTextHandler(logs, nil)).Error("startup failed", "err", err)
+			if strings.Contains(logs.String(), sentinel) {
+				t.Errorf("logs contain the provider response body: %s", logs)
+			}
+			if want := fmt.Sprintf("auth: OIDC discovery: provider answered HTTP %d", status); err.Error() != want {
+				t.Errorf("error = %q, want %q", err, want)
+			}
+		})
+	}
+}
+
 func TestNewFailsOnIssuerMismatch(t *testing.T) {
 	t.Parallel()
 	p := newFakeProvider(t)

internal/server/csrf.go

@@ -12,7 +12,7 @@ import (
 // GET /api/v1/me.
 const CSRFHeader = "X-MealQuest-CSRF"
 
-// requireCSRF passes GET, HEAD and OPTIONS requests to next; every other
+// requireCSRF passes GET requests to next; every other
 // request must carry CSRFHeader equal to its session's CSRF token or get 403
 // csrf. It runs behind requireSession, which puts the session in the context.
 func requireCSRF(logger *slog.Logger) func(http.Handler) http.Handler {
@@ -21,7 +21,7 @@ func requireCSRF(logger *slog.Logger) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			switch r.Method {
-			case http.MethodGet, http.MethodHead, http.MethodOptions:
+			case http.MethodGet:
 				next.ServeHTTP(w, r)
 				return
 			}

internal/server/csrf_test.go

@@ -38,10 +38,9 @@ func TestCSRFThroughRouter(t *testing.T) {
 		{"POST with right header", http.MethodPost, "/api/v1/me", []string{testSession.CSRFToken}, http.StatusMethodNotAllowed, methodNotAllowedJSON},
 		{"PATCH with right header", http.MethodPatch, "/api/v1/settings/household", []string{testSession.CSRFToken}, http.StatusNotFound, notFoundJSON},
 		{"GET without header", http.MethodGet, "/api/v1/me", nil, http.StatusOK, ""},
-		// HEAD and OPTIONS pass the check and reach the router, which
-		// answers neither for /api/v1/me.
-		{"HEAD without header", http.MethodHead, "/api/v1/me", nil, http.StatusMethodNotAllowed, ""},
-		{"OPTIONS without header", http.MethodOptions, "/api/v1/me", nil, http.StatusMethodNotAllowed, methodNotAllowedJSON},
+		// Only GET is exempt; HEAD and OPTIONS need the header too.
+		{"HEAD without header", http.MethodHead, "/api/v1/me", nil, http.StatusForbidden, csrfJSON},
+		{"OPTIONS without header", http.MethodOptions, "/api/v1/me", nil, http.StatusForbidden, csrfJSON},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
@@ -89,6 +88,8 @@ func TestRequireCSRF(t *testing.T) {
 		{"right header reaches handler", http.MethodPost, &testSession, testSession.CSRFToken, http.StatusNoContent},
 		{"wrong header", http.MethodPut, &testSession, "nope", http.StatusForbidden},
 		{"GET needs no header", http.MethodGet, &testSession, "", http.StatusNoContent},
+		{"HEAD needs the header", http.MethodHead, &testSession, "", http.StatusForbidden},
+		{"OPTIONS needs the header", http.MethodOptions, &testSession, "", http.StatusForbidden},
 		{"no session in context", http.MethodPost, nil, "", http.StatusInternalServerError},
 		{"session without token", http.MethodPost, &auth.Session{Subject: "user-123"}, "", http.StatusInternalServerError},
 	} {

internal/server/server.go

@@ -27,8 +27,7 @@ type RouteMounter interface {
 // only from a peer in trusted (see resolveOrigin). version is reported by
 // GET /health; auth mounts the /auth routes; sessions guards /api/v1 and
 // /media; assets is the web build served for every other GET. /health, /auth
-// and the web build are public. Requests to /api/v1 other than GET, HEAD and
-// OPTIONS also need the session's CSRF token in CSRFHeader.
+// and the web build are public. Requests to /api/v1 other than GET also need the session's CSRF token in CSRFHeader.
 func NewRouter(logger *slog.Logger, version string, trusted forwarded.Trusted, auth RouteMounter, sessions SessionLookup, assets fs.FS) (http.Handler, error) {
 	notFound := apiErrorHandler(logger, http.StatusNotFound, "not_found", "not found")
 	methodNotAllowed := apiErrorHandler(logger, http.StatusMethodNotAllowed, "method_not_allowed", "method not allowed")