From df692ef21490b021f91bfbb84e2aadbdbbdab140 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 11:32:36 -0400 Subject: [PATCH] Feature 03 integration fix: sanitize ID-token verification errors (#26) Co-authored-by: dev_agent Co-committed-by: dev_agent --- internal/auth/auth.go | 13 ++++++- internal/auth/auth_test.go | 74 +++++++++++++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index aa17cdd..d82d33d 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -353,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") @@ -404,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) }) diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 8af6771..407bbb8 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -60,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 { @@ -110,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, @@ -308,6 +319,7 @@ func (m *memorySessions) started() []Claims { } type harness struct { + auth *Auth provider *fakeProvider sessions *memorySessions router http.Handler @@ -350,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 { @@ -728,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"))