From 64acccd3eb150f007889b27ef71527f09207a23b Mon Sep 17 00:00:00 2001 From: Axel Etcheverry Date: Sun, 23 Aug 2026 21:22:46 +0200 Subject: [PATCH] feat(oauth2): add an error hook so server_error causes are observable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An RFC 6749 §5.2 response carries a code, a description and a URI — never the cause. A server_error therefore reached the client as an opaque 500 and was dropped everywhere else, leaving an operator with nothing to diagnose. RFC 7009 §2.2 made it worse on /revoke: a failed revocation still answers 200 OK, so the failure was unobservable too. ServerConfig.OnError (type ErrorHook) is called with the normalized *Error envelope — cause intact — at the point the server decides on its answer: every RFC 6749 §5.2 body, every /authorize redirect, the pre-redirect refusals answered with a bare 400, and the best-effort revocations /revoke swallows. grant.Config.OnError does the same for the family revocation a grant swallows during BCP §8.10.3 reuse detection. The hook is purely observational — no response, status code or redirect changes. Refs #63. --- CHANGELOG.md | 6 + docs/observability.md | 47 ++++- examples/oauth2/main.go | 14 ++ oauth2/authorize_endpoint.go | 37 ++-- oauth2/doc.go | 4 + oauth2/grant/grant.go | 16 ++ oauth2/grant/hook_test.go | 146 ++++++++++++++ oauth2/grant/refresh_token.go | 10 +- oauth2/hook.go | 34 ++++ oauth2/hook_test.go | 363 ++++++++++++++++++++++++++++++++++ oauth2/introspect_endpoint.go | 8 +- oauth2/metadata_endpoint.go | 2 +- oauth2/revoke_endpoint.go | 50 +++-- oauth2/server.go | 16 ++ oauth2/token_endpoint.go | 23 ++- 15 files changed, 731 insertions(+), 45 deletions(-) create mode 100644 oauth2/grant/hook_test.go create mode 100644 oauth2/hook.go create mode 100644 oauth2/hook_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 37ae71e..b58fc40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,12 @@ legacy packages (`authentication/`, `authorization/`, the in-tree - **Sessions** (`session`): stateless AES-256-GCM encrypted cookies with key rotation, a `Manager` (Login/Get/Touch/Rotate/Logout), and a synchronizer-token CSRF helper. +- **OAuth2 error hook**: `ServerConfig.OnError` and `grant.Config.OnError` + (both `oauth2.ErrorHook`) observe every error the authorization server + turns into an RFC 6749 §5.2 response — carrying the cause a `server_error` + never puts on the wire — plus the errors it swallows on purpose, such as a + best-effort revocation (RFC 7009 §2.2) or a family revocation that failed + during reuse detection. Purely observational: the responses are unchanged. - **Observability**: OpenTelemetry spans emitted directly by the core, `httpsec`, `grpcsec`, `connectrpcsec`, `jwtsec`, and `session`. See [docs/observability.md](docs/observability.md). diff --git a/docs/observability.md b/docs/observability.md index b2555d4..50f6d6f 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -25,7 +25,52 @@ dependency. Basic/Bearer authentication is still observable: the core `security.Manager.Authenticate` span records which authenticator ran via the `security.authenticator.name` attribute and an `authenticator.try` event per candidate. OAuth2 HTTP endpoints are observable through the host -server's HTTP instrumentation (e.g. `otelhttp`). +server's HTTP instrumentation (e.g. `otelhttp`) and, for their errors, +through the OAuth2 error hook described below. + +## OAuth2 error hook + +An RFC 6749 §5.2 response carries a code, a description and a URI — never +the cause. A `server_error` therefore reaches the client as an opaque 500, +and RFC 7009 §2.2 goes further: a revocation answers `200 OK` even when the +revocation itself failed. Both are protocol requirements, and both leave an +operator with nothing to diagnose. + +`oauth2.ServerConfig.OnError` closes that gap. It is an `oauth2.ErrorHook` +(`func(ctx context.Context, err error)`) called with the `*oauth2.Error` +envelope — cause intact — at the point the server decides on its answer: + +```go +srv, err := oauth2.NewServer(oauth2.ServerConfig{ + // ... + OnError: func(ctx context.Context, err error) { + if oauth2.IsCode(err) != oauth2.CodeServerError { + return // expected 4xx traffic + } + + slog.ErrorContext(ctx, "oauth2 server error", "err", err) + }, +}) +``` + +The hook fires for: + +- every error serialized as an RFC 6749 §5.2 body (`/token`, `/revoke`, + `/introspect`, metadata) and every error redirected back to the client by + `/authorize`, including the pre-redirect refusals answered with a bare + 400 (unknown client, unregistered `redirect_uri`); +- the best-effort revocations `/revoke` swallows to honour RFC 7009 §2.2. + +`grant.Config.OnError` is the same hook for the errors a grant swallows +before returning — today, a family revocation that failed during BCP §8.10.3 +reuse detection. Errors a grant *returns* travel to the server and reach +`ServerConfig.OnError`, so wire both fields to the same sink. + +The hook is purely observational: it MUST NOT influence the response, and it +runs synchronously on the request goroutine, so keep it fast. It receives no +secret — the envelope carries the code, the description and the wrapped Go +error, never a token or a client secret — but the cause comes from your own +storage layer, so apply the same redaction rules you apply to your logs. ## Span catalog diff --git a/examples/oauth2/main.go b/examples/oauth2/main.go index b6d7e05..0e3c274 100644 --- a/examples/oauth2/main.go +++ b/examples/oauth2/main.go @@ -92,6 +92,18 @@ type principal struct{ sub string } func (p principal) Subject() string { return p.sub } +// logOAuthError is the [oauth2.ErrorHook] shared by the server and the +// grants. The RFC 6749 §5.2 body carries no cause, so this is the only +// place a server_error becomes diagnosable; the expected 4xx traffic is +// filtered out to keep the log readable. +func logOAuthError(_ context.Context, err error) { + if oauth2.IsCode(err) != oauth2.CodeServerError { + return + } + + log.Printf("oauth2: server error: %v", err) +} + // buildServer wires the authorization server and the Bearer-protected // resource server onto a single mux. It is separate from main so the // end-to-end test can exercise the exact same wiring. @@ -121,6 +133,7 @@ func buildServer() (http.Handler, error) { AccessTTL: time.Hour, RefreshTTL: 24 * time.Hour, RotateRefreshTokens: true, + OnError: logOAuthError, } srv, err := oauth2.NewServer(oauth2.ServerConfig{ @@ -134,6 +147,7 @@ func buildServer() (http.Handler, error) { grant.NewRefreshToken(gcfg), }, ClientAuth: []oauth2.ClientAuthenticator{clientauth.NewBasic(), clientauth.NewPost()}, + OnError: logOAuthError, }) if err != nil { return nil, fmt.Errorf("oauth2.NewServer: %w", err) diff --git a/oauth2/authorize_endpoint.go b/oauth2/authorize_endpoint.go index fdc25fd..4331e7a 100644 --- a/oauth2/authorize_endpoint.go +++ b/oauth2/authorize_endpoint.go @@ -174,21 +174,25 @@ func (s *Server) AuthorizeHandler(cfg AuthorizeConfig, consent ConsentFunc) http func (s *Server) serveAuthorize(cfg AuthorizeConfig, consent ConsentFunc, w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet && r.Method != http.MethodPost { + s.notifyError(r.Context(), ErrInvalidRequest.WithDescription("/authorize requires GET or POST")) http.Error(w, "oauth2: /authorize requires GET or POST", http.StatusMethodNotAllowed) return } if err := r.ParseForm(); err != nil { + s.notifyError(r.Context(), ErrInvalidRequest.WithCause(err)) http.Error(w, "oauth2: malformed authorization request", http.StatusBadRequest) return } // Client and redirect URI come first: a failure here MUST NOT redirect - // (the redirect target is not yet trusted). + // (the redirect target is not yet trusted). The refusal is deliberately + // opaque on the wire, so the hook is the only place the cause shows up. client, err := s.cfg.ClientStore.LoadClient(r.Context(), r.FormValue("client_id")) if err != nil || client == nil { + s.notifyError(r.Context(), ErrInvalidClient.WithDescription("unknown or invalid client").WithCause(err)) http.Error(w, "oauth2: unknown or invalid client", http.StatusBadRequest) return @@ -196,6 +200,7 @@ func (s *Server) serveAuthorize(cfg AuthorizeConfig, consent ConsentFunc, w http redirectURI, ok := resolveRedirectURI(client, r.FormValue("redirect_uri")) if !ok { + s.notifyError(r.Context(), ErrInvalidRequest.WithDescription("missing or unregistered redirect_uri")) http.Error(w, "oauth2: missing or unregistered redirect_uri", http.StatusBadRequest) return @@ -208,7 +213,7 @@ func (s *Server) serveAuthorize(cfg AuthorizeConfig, consent ConsentFunc, w http flow, oerr := resolveFlow(cfg, r.FormValue("response_type")) if oerr != nil { // The response type is unknown — default to a query-string error. - redirectAuthorizeError(w, r, redirectURI, state, oerr, false) + s.redirectAuthorizeError(w, r, redirectURI, state, oerr, false) return } @@ -217,14 +222,14 @@ func (s *Server) serveAuthorize(cfg AuthorizeConfig, consent ConsentFunc, w http ar, oerr := s.parseAuthorizeRequest(r, client, redirectURI, flow) if oerr != nil { - redirectAuthorizeError(w, r, redirectURI, state, oerr, useFragment) + s.redirectAuthorizeError(w, r, redirectURI, state, oerr, useFragment) return } decision, err := consent(w, r, ar) if err != nil { - redirectAuthorizeError(w, r, redirectURI, ar.State, + s.redirectAuthorizeError(w, r, redirectURI, ar.State, ErrServerError.WithDescription("consent handler failed"), useFragment) return @@ -236,7 +241,7 @@ func (s *Server) serveAuthorize(cfg AuthorizeConfig, consent ConsentFunc, w http } if !decision.Approved { - redirectAuthorizeError(w, r, redirectURI, ar.State, + s.redirectAuthorizeError(w, r, redirectURI, ar.State, ErrAccessDenied.WithDescription("the resource owner denied the request"), useFragment) return @@ -334,7 +339,7 @@ func (s *Server) issueAuthorizationCode( ) { granted, err := grantedScope(ar, decision) if err != nil { - redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, + s.redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrInvalidScope.WithDescription("granted scope exceeds the request"), false) return @@ -342,7 +347,7 @@ func (s *Server) issueAuthorizationCode( raw, err := randomCode() if err != nil { - redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(err), false) + s.redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(err), false) return } @@ -365,7 +370,7 @@ func (s *Server) issueAuthorizationCode( } if err := s.cfg.Storage.SaveAuthorizationCode(r.Context(), code); err != nil { - redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(err), false) + s.redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(err), false) return } @@ -390,7 +395,7 @@ func (s *Server) issueImplicitToken( ) { granted, err := grantedScope(ar, decision) if err != nil { - redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, + s.redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrInvalidScope.WithDescription("granted scope exceeds the request"), true) return @@ -398,14 +403,14 @@ func (s *Server) issueImplicitToken( _, audience, ierr := s.resolveIssuer(r.Context(), r) if ierr != nil { - redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(ierr), true) + s.redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(ierr), true) return } raw, hash, err := cfg.ImplicitTokens.Generate(r.Context()) if err != nil { - redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(err), true) + s.redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(err), true) return } @@ -423,7 +428,7 @@ func (s *Server) issueImplicitToken( } if err := s.cfg.Storage.SaveAccessToken(r.Context(), at); err != nil { - redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(err), true) + s.redirectAuthorizeError(w, r, ar.RedirectURI, ar.State, ErrServerError.WithCause(err), true) return } @@ -497,14 +502,18 @@ func authorizeScope(requested string, allowed []string) (string, error) { } // redirectAuthorizeError sends an RFC 6749 §4.1.2.1 / §4.2.2.1 error -// response by redirecting back to the client's redirect URI. -func redirectAuthorizeError( +// response by redirecting back to the client's redirect URI. The envelope — +// cause included — is handed to the configured [ErrorHook] first: the +// redirect carries only the code and the description. +func (s *Server) redirectAuthorizeError( w http.ResponseWriter, r *http.Request, redirectURI, state string, oerr *Error, useFragment bool, ) { + s.notifyError(r.Context(), oerr) + params := url.Values{"error": {oerr.Code}} if oerr.Description != "" { params.Set("error_description", oerr.Description) diff --git a/oauth2/doc.go b/oauth2/doc.go index 4a58a4d..47f7ebb 100644 --- a/oauth2/doc.go +++ b/oauth2/doc.go @@ -21,6 +21,10 @@ // the jwt sub-module (no hard dependency from oauth2 to jwt). // - Stores expose atomic ConsumeAuthorizationCode and RotateRefreshToken // to guarantee single-use semantics and reuse-detection. +// - ServerConfig.OnError (and grant.Config.OnError) observe the errors the +// protocol hides: the cause of a server_error, which never reaches the +// wire, and the revocations RFC 7009 §2.2 requires to answer 200 OK +// regardless. See ErrorHook. // // Allowed dependencies: // - github.com/hyperscale-stack/security (core) diff --git a/oauth2/grant/grant.go b/oauth2/grant/grant.go index 8fa32ad..c771876 100644 --- a/oauth2/grant/grant.go +++ b/oauth2/grant/grant.go @@ -18,6 +18,7 @@ package grant import ( + "context" "time" "github.com/hyperscale-stack/security/oauth2" @@ -46,6 +47,21 @@ type Config struct { // /token?grant_type=refresh_token call and marks the old one // consumed; reuse triggers family revocation. Default true in BCP/21. RotateRefreshTokens bool + // OnError, when set, observes the errors a grant swallows to keep the + // protocol response intact — today, a family revocation that failed + // during reuse detection. Errors returned to the server travel to + // [oauth2.ServerConfig.OnError] instead, so wire both to the same sink. + // Optional; see [oauth2.ErrorHook]. + OnError oauth2.ErrorHook +} + +// notifyError hands err to the configured [oauth2.ErrorHook], if any. +func (c Config) notifyError(ctx context.Context, err error) { + if c.OnError == nil || err == nil { + return + } + + c.OnError(ctx, err) } // Request and Response are type aliases anchoring the contract in the diff --git a/oauth2/grant/hook_test.go b/oauth2/grant/hook_test.go new file mode 100644 index 0000000..761fd7a --- /dev/null +++ b/oauth2/grant/hook_test.go @@ -0,0 +1,146 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package grant_test + +import ( + "context" + "errors" + "net/url" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/grant" + "github.com/hyperscale-stack/security/oauth2/storage/memory" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// familyRevokeFailingStore fails the family revocation the reuse-detection +// path runs, so the swallowed error can be observed. +type familyRevokeFailingStore struct { + *memory.Store + + err error +} + +func (s *familyRevokeFailingStore) RevokeRefreshFamily(context.Context, string) error { + return s.err +} + +// consumedRefreshRequest stores an already-consumed refresh token and +// returns the matching /token request — the BCP §8.10.3 reuse case. +func consumedRefreshRequest(ctx context.Context, store oauth2.Storage) grant.Request { + raw := "reused-refresh-token" + + _ = store.SaveRefreshToken(ctx, &oauth2.RefreshToken{ + Token: raw, + TokenHash: oauth2.HashToken(nil, raw), + ClientID: clientID, + Subject: subject, + Scope: "read:mail", + FamilyID: "family-1", + Consumed: true, + IssuedAt: time.Date(2026, 5, 20, 12, 0, 0, 0, time.UTC), + ExpiresAt: time.Date(2026, 5, 21, 12, 0, 0, 0, time.UTC), + }) + + return grant.Request{ + Client: newClient(), + Form: url.Values{"refresh_token": {raw}}, + Issuer: "https://auth.example", + Audience: "api", + Now: time.Date(2026, 5, 20, 13, 0, 0, 0, time.UTC), + Profile: oauth2.Profile20BCP, + } +} + +func TestRefreshTokenOnErrorReportsFamilyRevokeFailure(t *testing.T) { + t.Parallel() + + ctx := t.Context() + store := &familyRevokeFailingStore{ + Store: memory.New(), + err: errors.New("revocation backend down"), + } + + var seen []error + + g := grant.NewRefreshToken(grant.Config{ + Storage: store, + AccessTokens: newAccessGen(), + RefreshTokens: newRefreshGen(), + AccessTTL: time.Hour, + RefreshTTL: 24 * time.Hour, + RotateRefreshTokens: true, + OnError: func(_ context.Context, err error) { + seen = append(seen, err) + }, + }) + + resp, err := g.Handle(ctx, consumedRefreshRequest(ctx, store)) + + // The protocol answer is unchanged: reuse stays invalid_grant. + assert.Nil(t, resp) + require.ErrorIs(t, err, oauth2.ErrRefreshTokenReused) + assert.Equal(t, oauth2.CodeInvalidGrant, oauth2.IsCode(err)) + + // The failed revocation is only visible through the hook. + require.Len(t, seen, 1) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(seen[0])) + assert.ErrorContains(t, seen[0], "revoke refresh family failed after reuse detection") + assert.ErrorContains(t, errors.Unwrap(seen[0]), "revocation backend down") +} + +func TestRefreshTokenOnErrorIsOptional(t *testing.T) { + t.Parallel() + + ctx := t.Context() + store := &familyRevokeFailingStore{ + Store: memory.New(), + err: errors.New("revocation backend down"), + } + + g := grant.NewRefreshToken(grant.Config{ + Storage: store, + AccessTokens: newAccessGen(), + RefreshTokens: newRefreshGen(), + AccessTTL: time.Hour, + RefreshTTL: 24 * time.Hour, + RotateRefreshTokens: true, + }) + + req := consumedRefreshRequest(ctx, store) + + assert.NotPanics(t, func() { + _, err := g.Handle(ctx, req) + assert.ErrorIs(t, err, oauth2.ErrRefreshTokenReused) + }) +} + +func TestRefreshTokenOnErrorQuietOnSuccessfulRevoke(t *testing.T) { + t.Parallel() + + ctx := t.Context() + store := memory.New() + + var seen []error + + g := grant.NewRefreshToken(grant.Config{ + Storage: store, + AccessTokens: newAccessGen(), + RefreshTokens: newRefreshGen(), + AccessTTL: time.Hour, + RefreshTTL: 24 * time.Hour, + RotateRefreshTokens: true, + OnError: func(_ context.Context, err error) { + seen = append(seen, err) + }, + }) + + _, err := g.Handle(ctx, consumedRefreshRequest(ctx, store)) + require.ErrorIs(t, err, oauth2.ErrRefreshTokenReused) + assert.Empty(t, seen, "a successful family revocation must stay quiet") +} diff --git a/oauth2/grant/refresh_token.go b/oauth2/grant/refresh_token.go index ce2367a..43667d3 100644 --- a/oauth2/grant/refresh_token.go +++ b/oauth2/grant/refresh_token.go @@ -59,8 +59,14 @@ func (g *RefreshToken) Handle(ctx context.Context, req Request) (*Response, erro } if rt.Consumed { - // Reuse detected — revoke the whole family and refuse. - _ = g.cfg.Storage.RevokeRefreshFamily(ctx, rt.FamilyID) + // Reuse detected — revoke the whole family and refuse. The refusal + // stays invalid_grant whatever the revocation did, so a failure + // there is only observable through the error hook. + if err := g.cfg.Storage.RevokeRefreshFamily(ctx, rt.FamilyID); err != nil { + g.cfg.notifyError(ctx, oauth2.ErrServerError. + WithDescription("revoke refresh family failed after reuse detection"). + WithCause(err)) + } return nil, oauth2.ErrRefreshTokenReused } diff --git a/oauth2/hook.go b/oauth2/hook.go new file mode 100644 index 0000000..a0ead70 --- /dev/null +++ b/oauth2/hook.go @@ -0,0 +1,34 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oauth2 + +import "context" + +// ErrorHook observes the errors the authorization server would otherwise +// keep to itself: +// +// - every error the server turns into an RFC 6749 §5.2 error response. +// The hook receives the [*Error] envelope with its Cause intact, which +// is the only place the cause of a server_error is ever exposed — the +// wire response deliberately drops it. +// - the errors the server swallows to stay protocol-compliant, such as a +// best-effort revocation that failed (RFC 7009 §2.2 mandates 200 OK +// whatever happened). +// +// The hook is a pure observability sink: it MUST NOT influence the +// response. It runs synchronously on the request goroutine, so a slow +// implementation slows the request down. +// +// Use [IsCode] to filter out the expected 4xx traffic, and errors.Unwrap / +// errors.As to reach the underlying cause: +// +// OnError: func(ctx context.Context, err error) { +// if oauth2.IsCode(err) != oauth2.CodeServerError { +// return +// } +// +// slog.ErrorContext(ctx, "oauth2 server error", "err", err) +// } +type ErrorHook func(ctx context.Context, err error) diff --git a/oauth2/hook_test.go b/oauth2/hook_test.go new file mode 100644 index 0000000..a27a3e0 --- /dev/null +++ b/oauth2/hook_test.go @@ -0,0 +1,363 @@ +// Copyright 2026 Hyperscale. All rights reserved. +// Use of this source code is governed by a MIT +// license that can be found in the LICENSE file. + +package oauth2_test + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "sync" + "testing" + "time" + + "github.com/hyperscale-stack/security/oauth2" + "github.com/hyperscale-stack/security/oauth2/clientauth" + "github.com/hyperscale-stack/security/oauth2/grant" + "github.com/hyperscale-stack/security/oauth2/storage/memory" + "github.com/hyperscale-stack/security/oauth2/token" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// errorSink records everything the [oauth2.ErrorHook] is handed. +type errorSink struct { + mu sync.Mutex + errs []error +} + +func (s *errorSink) hook(_ context.Context, err error) { + s.mu.Lock() + defer s.mu.Unlock() + + s.errs = append(s.errs, err) +} + +func (s *errorSink) collected() []error { + s.mu.Lock() + defer s.mu.Unlock() + + return append([]error(nil), s.errs...) +} + +func (s *errorSink) codes() []string { + out := make([]string, 0, len(s.errs)) + for _, err := range s.collected() { + out = append(out, oauth2.IsCode(err)) + } + + return out +} + +// revokeFailingStore fails the revocation paths so the swallowed errors +// have something to be observed by. +type revokeFailingStore struct { + *memory.Store + + err error +} + +func (s *revokeFailingStore) RevokeAccessToken(context.Context, string) error { return s.err } + +func (s *revokeFailingStore) RevokeRefreshFamily(context.Context, string) error { return s.err } + +// hookServerConfig is the baseline config shared by the error-hook tests: +// memory storage, one confidential client, Basic client authentication. +func hookServerConfig(store oauth2.Storage) oauth2.ServerConfig { + cfg := grant.Config{ + Storage: store, + AccessTokens: token.NewOpaque(32), + RefreshTokens: token.OpaqueRefreshAdapter{Opaque: token.NewOpaque(32)}, + AccessTTL: time.Hour, + RefreshTTL: 24 * time.Hour, + RotateRefreshTokens: true, + } + + return oauth2.ServerConfig{ + Profile: oauth2.Profile20BCP, + Storage: store, + ClientStore: &staticClientStore{clients: map[string]oauth2.Client{ + testClientID: &oauth2.DefaultClient{ + IDValue: testClientID, + Secret: testClientSecret, + TypeValue: oauth2.ClientConfidential, + RedirectURIValues: []string{redirectURI}, + ScopeValues: []string{"api:read"}, + }, + }}, + IssuerResolver: oauth2.StaticIssuer("https://auth.example", "api"), + Grants: []oauth2.Grant{grant.NewClientCredentials(cfg), grant.NewRefreshToken(cfg)}, + ClientAuth: []oauth2.ClientAuthenticator{clientauth.NewBasic()}, + } +} + +func TestOnErrorReportsServerErrorCause(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + cfg := hookServerConfig(memory.New()) + cfg.IssuerResolver = failingIssuer{} + cfg.OnError = sink.hook + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + rec := httptest.NewRecorder() + srv.TokenHandler().ServeHTTP(rec, formRequest("/token", + url.Values{"grant_type": {"client_credentials"}}, true)) + + // The wire body stays cause-free... + require.Equal(t, http.StatusInternalServerError, rec.Code) + assert.NotContains(t, rec.Body.String(), "issuer backend down") + + // ...but the hook sees the whole envelope, cause included. + got := sink.collected() + require.Len(t, got, 1) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(got[0])) + assert.ErrorContains(t, got[0], "oauth2: server_error") + assert.ErrorContains(t, errors.Unwrap(got[0]), "issuer backend down") +} + +func TestOnErrorReportsClientErrors(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + cfg := hookServerConfig(memory.New()) + cfg.OnError = sink.hook + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + cases := []struct { + name string + handler http.Handler + req *http.Request + want string + }{ + { + "token: wrong method", + srv.TokenHandler(), + httptest.NewRequest(http.MethodGet, "/token", nil), + oauth2.CodeInvalidRequest, + }, + { + "token: unknown grant_type", + srv.TokenHandler(), + formRequest("/token", url.Values{"grant_type": {"nope"}}, true), + oauth2.CodeUnsupportedGrantType, + }, + { + "token: unauthenticated client", + srv.TokenHandler(), + formRequest("/token", url.Values{"grant_type": {"client_credentials"}}, false), + oauth2.CodeInvalidClient, + }, + { + "revoke: missing token", + srv.RevokeHandler(), + formRequest("/revoke", url.Values{}, true), + oauth2.CodeInvalidRequest, + }, + { + "introspect: missing token", + srv.IntrospectHandler(), + formRequest("/introspect", url.Values{}, true), + oauth2.CodeInvalidRequest, + }, + } + + for _, tc := range cases { + tc.handler.ServeHTTP(httptest.NewRecorder(), tc.req) + } + + want := make([]string, 0, len(cases)) + for _, tc := range cases { + want = append(want, tc.want) + } + + assert.Equal(t, want, sink.codes()) +} + +func TestOnErrorIsOptional(t *testing.T) { + t.Parallel() + + cfg := hookServerConfig(memory.New()) + cfg.IssuerResolver = failingIssuer{} + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + rec := httptest.NewRecorder() + // No hook configured: the server must behave exactly as before. + assert.NotPanics(t, func() { + srv.TokenHandler().ServeHTTP(rec, formRequest("/token", + url.Values{"grant_type": {"client_credentials"}}, true)) + }) + assert.Equal(t, http.StatusInternalServerError, rec.Code) +} + +func TestOnErrorReportsAuthorizeErrors(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + query url.Values + want string + }{ + { + // Refused before the redirect URI is trusted: answered with a + // bare 400, so the hook is the only trace. + name: "unknown client", + query: url.Values{ + "response_type": {"code"}, + "client_id": {"ghost"}, + "redirect_uri": {redirectURI}, + }, + want: oauth2.CodeInvalidClient, + }, + { + name: "unregistered redirect_uri", + query: url.Values{ + "response_type": {"code"}, + "client_id": {testClientID}, + "redirect_uri": {"https://evil.example/cb"}, + }, + want: oauth2.CodeInvalidRequest, + }, + { + // Redirected back to the client: the code travels, nothing else. + name: "unsupported response_type", + query: url.Values{ + "response_type": {"magic"}, + "client_id": {testClientID}, + "redirect_uri": {redirectURI}, + }, + want: oauth2.CodeUnsupportedResponseType, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + cfg := hookServerConfig(memory.New()) + cfg.OnError = sink.hook + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + handler := srv.AuthorizeHandler(oauth2.AuthorizeConfig{}, + func(http.ResponseWriter, *http.Request, *oauth2.AuthorizeRequest) (*oauth2.Consent, error) { + return &oauth2.Consent{Approved: true, Subject: "alice"}, nil + }) + + handler.ServeHTTP(httptest.NewRecorder(), + httptest.NewRequest(http.MethodGet, "/authorize?"+tc.query.Encode(), nil)) + + assert.Equal(t, []string{tc.want}, sink.codes()) + }) + } +} + +func TestOnErrorReportsFailedRevocation(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + store := &revokeFailingStore{Store: memory.New(), err: errors.New("revocation backend down")} + cfg := hookServerConfig(store) + cfg.OnError = sink.hook + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + // Mint a token pair to revoke. + rec := httptest.NewRecorder() + srv.TokenHandler().ServeHTTP(rec, formRequest("/token", + url.Values{"grant_type": {"client_credentials"}}, true)) + require.Equal(t, http.StatusOK, rec.Code) + + var issued struct { + AccessToken string `json:"access_token"` + } + + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &issued)) + require.NotEmpty(t, issued.AccessToken) + require.Empty(t, sink.codes(), "a successful /token must not notify") + + // RFC 7009 §2.2: the revocation still answers 200 OK... + rec = httptest.NewRecorder() + srv.RevokeHandler().ServeHTTP(rec, formRequest("/revoke", + url.Values{"token": {issued.AccessToken}}, true)) + assert.Equal(t, http.StatusOK, rec.Code) + + // ...and the failure surfaces through the hook only. + got := sink.collected() + require.NotEmpty(t, got) + + for _, e := range got { + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(e)) + assert.ErrorContains(t, e, "revoke") + } + + assert.ErrorContains(t, got[0], "revoke access token failed") +} + +func TestOnErrorSkipsUnknownRevocationTarget(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + cfg := hookServerConfig(memory.New()) + cfg.OnError = sink.hook + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + rec := httptest.NewRecorder() + srv.RevokeHandler().ServeHTTP(rec, formRequest("/revoke", + url.Values{"token": {"never-issued"}}, true)) + + // An unknown token is not an incident: 200 OK and no notification. + assert.Equal(t, http.StatusOK, rec.Code) + assert.Empty(t, sink.codes()) +} + +func TestOnErrorReportsFailedFamilyRevocation(t *testing.T) { + t.Parallel() + + sink := &errorSink{} + store := &revokeFailingStore{Store: memory.New(), err: errors.New("revocation backend down")} + cfg := hookServerConfig(store) + cfg.OnError = sink.hook + + srv, err := oauth2.NewServer(cfg) + require.NoError(t, err) + + raw := "refresh-to-revoke" + require.NoError(t, store.SaveRefreshToken(t.Context(), &oauth2.RefreshToken{ + Token: raw, + TokenHash: oauth2.HashToken(nil, raw), + ClientID: testClientID, + Subject: "alice", + Scope: "api:read", + FamilyID: "family-1", + IssuedAt: time.Now(), + ExpiresAt: time.Now().Add(time.Hour), + })) + + rec := httptest.NewRecorder() + srv.RevokeHandler().ServeHTTP(rec, formRequest("/revoke", url.Values{"token": {raw}}, true)) + + // RFC 7009 §2.2 still mandates 200 OK; the hook carries the failure. + assert.Equal(t, http.StatusOK, rec.Code) + + got := sink.collected() + require.Len(t, got, 1) + assert.Equal(t, oauth2.CodeServerError, oauth2.IsCode(got[0])) + assert.ErrorContains(t, got[0], "revoke refresh family failed") + assert.ErrorContains(t, errors.Unwrap(got[0]), "revocation backend down") +} diff --git a/oauth2/introspect_endpoint.go b/oauth2/introspect_endpoint.go index 6ba7c65..f717572 100644 --- a/oauth2/introspect_endpoint.go +++ b/oauth2/introspect_endpoint.go @@ -20,26 +20,26 @@ func (s *Server) IntrospectHandler() http.Handler { func (s *Server) serveIntrospect(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { - writeOAuthError(w, ErrInvalidRequest.WithDescription("POST required")) + s.writeOAuthError(r.Context(), w, ErrInvalidRequest.WithDescription("POST required")) return } if err := r.ParseForm(); err != nil { - writeOAuthError(w, ErrInvalidRequest.WithCause(err)) + s.writeOAuthError(r.Context(), w, ErrInvalidRequest.WithCause(err)) return } if _, err := s.authenticateClient(r.Context(), r); err != nil { - writeOAuthError(w, err) + s.writeOAuthError(r.Context(), w, err) return } rawToken := r.PostFormValue("token") if rawToken == "" { - writeOAuthError(w, ErrInvalidRequest.WithDescription("missing token")) + s.writeOAuthError(r.Context(), w, ErrInvalidRequest.WithDescription("missing token")) return } diff --git a/oauth2/metadata_endpoint.go b/oauth2/metadata_endpoint.go index 2ec153f..a548f88 100644 --- a/oauth2/metadata_endpoint.go +++ b/oauth2/metadata_endpoint.go @@ -27,7 +27,7 @@ func (s *Server) MetadataHandler() http.Handler { func (s *Server) serveMetadata(w http.ResponseWriter, r *http.Request) { issuer, _, err := s.resolveIssuer(r.Context(), r) if err != nil { - writeOAuthError(w, err) + s.writeOAuthError(r.Context(), w, err) return } diff --git a/oauth2/revoke_endpoint.go b/oauth2/revoke_endpoint.go index 7d2b4db..59332a9 100644 --- a/oauth2/revoke_endpoint.go +++ b/oauth2/revoke_endpoint.go @@ -23,27 +23,27 @@ func (s *Server) RevokeHandler() http.Handler { func (s *Server) serveRevoke(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { - writeOAuthError(w, ErrInvalidRequest.WithDescription("POST required")) + s.writeOAuthError(r.Context(), w, ErrInvalidRequest.WithDescription("POST required")) return } if err := r.ParseForm(); err != nil { - writeOAuthError(w, ErrInvalidRequest.WithCause(err)) + s.writeOAuthError(r.Context(), w, ErrInvalidRequest.WithCause(err)) return } client, err := s.authenticateClient(r.Context(), r) if err != nil { - writeOAuthError(w, err) + s.writeOAuthError(r.Context(), w, err) return } rawToken := r.PostFormValue("token") if rawToken == "" { - writeOAuthError(w, ErrInvalidRequest.WithDescription("missing token")) + s.writeOAuthError(r.Context(), w, ErrInvalidRequest.WithDescription("missing token")) return } @@ -58,27 +58,47 @@ func (s *Server) serveRevoke(w http.ResponseWriter, r *http.Request) { } // bestEffortRevoke tries to revoke a token assuming it is an access token, -// then assuming it is a refresh token. The implementation is intentionally -// silent: per RFC 7009 §2.2, the response must not reveal whether the -// token existed. +// then assuming it is a refresh token. The wire response stays silent: per +// RFC 7009 §2.2 it must not reveal whether the token existed, and it stays +// 200 OK even when the revocation itself failed. A failed revocation is +// exactly the event an operator needs, so it is reported to the configured +// [ErrorHook] instead — a missing token is not, so the lookups stay quiet. func (s *Server) bestEffortRevoke(ctx context.Context, client Client, rawToken string) { hash := HashToken(nil, rawToken) if at, err := s.cfg.Storage.LookupAccessToken(ctx, hash); err == nil { - if at.ClientID == client.ID() { - _ = s.cfg.Storage.RevokeAccessToken(ctx, hash) - - if at.FamilyID != "" { - _ = s.cfg.Storage.RevokeRefreshFamily(ctx, at.FamilyID) - } - } + s.revokeAccess(ctx, client, at, hash) return } if rt, err := s.cfg.Storage.LookupRefreshToken(ctx, hash); err == nil { if rt.ClientID == client.ID() && rt.FamilyID != "" { - _ = s.cfg.Storage.RevokeRefreshFamily(ctx, rt.FamilyID) + s.revokeFamily(ctx, rt.FamilyID) } } } + +// revokeAccess revokes an access token — and the refresh family it belongs +// to — once the token is confirmed to belong to the calling client. +func (s *Server) revokeAccess(ctx context.Context, client Client, at *AccessToken, hash string) { + if at.ClientID != client.ID() { + return + } + + if err := s.cfg.Storage.RevokeAccessToken(ctx, hash); err != nil { + s.notifyError(ctx, ErrServerError.WithDescription("revoke access token failed").WithCause(err)) + } + + if at.FamilyID != "" { + s.revokeFamily(ctx, at.FamilyID) + } +} + +// revokeFamily revokes a refresh-token family, reporting a failure to the +// [ErrorHook] rather than dropping it. +func (s *Server) revokeFamily(ctx context.Context, familyID string) { + if err := s.cfg.Storage.RevokeRefreshFamily(ctx, familyID); err != nil { + s.notifyError(ctx, ErrServerError.WithDescription("revoke refresh family failed").WithCause(err)) + } +} diff --git a/oauth2/server.go b/oauth2/server.go index a5f0f51..57d36ad 100644 --- a/oauth2/server.go +++ b/oauth2/server.go @@ -58,6 +58,11 @@ type ServerConfig struct { // Now is the clock used to stamp issuance / expiry. Defaults to // time.Now (wall clock); inject a fixed clock in tests. Now func() time.Time + // OnError, when set, observes every error the server turns into an + // RFC 6749 §5.2 response — including the cause of a server_error, which + // never reaches the wire — plus the errors it swallows on purpose + // (best-effort revocation). Optional; see [ErrorHook]. + OnError ErrorHook } // Server is the OAuth2 authorization server. It exposes one @@ -117,6 +122,17 @@ func NewServer(cfg ServerConfig) (*Server, error) { // for endpoints (metadata, jwks) that need to introspect it. func (s *Server) Config() ServerConfig { return s.cfg } +// notifyError hands err to the configured [ErrorHook], if any. It is a +// no-op when no hook is registered or when err is nil, so call sites stay +// free of guards. +func (s *Server) notifyError(ctx context.Context, err error) { + if s.cfg.OnError == nil || err == nil { + return + } + + s.cfg.OnError(ctx, err) +} + // authenticateClient runs the configured client-authentication methods in // order and returns the first match. func (s *Server) authenticateClient(ctx context.Context, r *http.Request) (Client, error) { diff --git a/oauth2/token_endpoint.go b/oauth2/token_endpoint.go index e622040..a95640d 100644 --- a/oauth2/token_endpoint.go +++ b/oauth2/token_endpoint.go @@ -5,6 +5,7 @@ package oauth2 import ( + "context" "encoding/json" "errors" "net/http" @@ -26,41 +27,41 @@ func (s *Server) TokenHandler() http.Handler { func (s *Server) serveToken(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { - writeOAuthError(w, ErrInvalidRequest.WithDescription("POST required")) + s.writeOAuthError(r.Context(), w, ErrInvalidRequest.WithDescription("POST required")) return } if err := r.ParseForm(); err != nil { - writeOAuthError(w, ErrInvalidRequest.WithCause(err)) + s.writeOAuthError(r.Context(), w, ErrInvalidRequest.WithCause(err)) return } client, err := s.authenticateClient(r.Context(), r) if err != nil { - writeOAuthError(w, err) + s.writeOAuthError(r.Context(), w, err) return } grantType := r.PostFormValue("grant_type") if grantType == "" { - writeOAuthError(w, ErrInvalidRequest.WithDescription("missing grant_type")) + s.writeOAuthError(r.Context(), w, ErrInvalidRequest.WithDescription("missing grant_type")) return } handler, ok := s.dispatch[grantType] if !ok { - writeOAuthError(w, ErrUnsupportedGrantType.WithDescription("grant_type "+grantType+" not supported")) + s.writeOAuthError(r.Context(), w, ErrUnsupportedGrantType.WithDescription("grant_type "+grantType+" not supported")) return } issuer, audience, err := s.resolveIssuer(r.Context(), r) if err != nil { - writeOAuthError(w, err) + s.writeOAuthError(r.Context(), w, err) return } @@ -74,7 +75,7 @@ func (s *Server) serveToken(w http.ResponseWriter, r *http.Request) { Profile: s.cfg.Profile, }) if err != nil { - writeOAuthError(w, err) + s.writeOAuthError(r.Context(), w, err) return } @@ -130,12 +131,18 @@ type errorResponse struct { // writeOAuthError serializes err as an RFC 6749 §5.2 envelope. Non-OAuth // errors collapse to server_error so the wire response stays compliant. -func writeOAuthError(w http.ResponseWriter, err error) { +// +// The normalized envelope — cause included — is handed to the configured +// [ErrorHook] before anything is written, since the wire body carries only +// the code, the description and the URI. +func (s *Server) writeOAuthError(ctx context.Context, w http.ResponseWriter, err error) { var oe *Error if !errors.As(err, &oe) { oe = ErrServerError.WithCause(err) } + s.notifyError(ctx, oe) + body := errorResponse{ Error: oe.Code, ErrorDescription: oe.Description,