From 2ef63e3b25cc2d2e773f628bc7f3eb829d755f02 Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 5 Oct 2026 11:51:01 -0400 Subject: [PATCH] fix(oauth): introspection describes only the caller's own client's access tokens (BUG-3398) (#1791) * fix(oauth): introspection describes only the caller's own client's access tokens (BUG-3398) Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * test(oauth): two clients share the test redirect URI (BUG-3398) Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * test(server): the census names the shared introspection caller reader (BUG-3398) Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS --- internal/server/app_tokens.go | 23 +++++++-- internal/server/body_reader_census_test.go | 7 +-- internal/server/bug3398_introspect_test.go | 57 ++++++++++++++++++++++ internal/server/handlers_oauth.go | 25 ++++++++-- 4 files changed, 101 insertions(+), 11 deletions(-) create mode 100644 internal/server/bug3398_introspect_test.go diff --git a/internal/server/app_tokens.go b/internal/server/app_tokens.go index ff8dc0eb4..0b86236f0 100644 --- a/internal/server/app_tokens.go +++ b/internal/server/app_tokens.go @@ -221,13 +221,26 @@ func (s *Server) introspectAppToken(ctx context.Context, token string) (*AppToke // (fosite.AccessTokenFromRequest) first, and only without one, the Basic // credentials with the client id percent-decoded. func (s *Server) introspectionCallerIsInstall(r *http.Request) bool { + id := s.introspectionCallerClientID(r) + if id == "" { + return false + } + c, err := s.store.GetOAuthClient(id) + return err == nil && c.IsInstallClient() +} + +// introspectionCallerClientID is the client a public /oauth/introspect +// request authenticates as, read as fosite's NewIntrospectionRequest reads +// it: the client of a bearer access token (header, form or query) first, +// and only without one, the Basic or form client id. "" when it cannot be +// read; fosite then refuses the request on its own terms. +func (s *Server) introspectionCallerClientID(r *http.Request) string { if bearer := fosite.AccessTokenFromRequest(r); bearer != "" { ar, _, err := s.oauthServer.IntrospectToken(r.Context(), bearer) - if err != nil || ar == nil { - return false + if err != nil || ar == nil || ar.GetClient() == nil { + return "" } - c, cerr := s.store.GetOAuthClient(ar.GetClient().GetID()) - return cerr == nil && c.IsInstallClient() + return ar.GetClient().GetID() } - return s.installClientFor(r) != nil + return requestClientID(r) } diff --git a/internal/server/body_reader_census_test.go b/internal/server/body_reader_census_test.go index 76a8bc0f3..73fa877dd 100644 --- a/internal/server/body_reader_census_test.go +++ b/internal/server/body_reader_census_test.go @@ -469,9 +469,10 @@ func TestEveryRequestHandoffIsClassified(t *testing.T) { "handlers_oauth.go::Server.handleOAuthRevoke::github.com/ory/fosite.OAuth2Provider.NewRevocationRequest": 1, "handlers_oauth.go::Server.handleOAuthIntrospect::github.com/ory/fosite.OAuth2Provider.NewIntrospectionRequest": 1, // TASK-3394: fosite's own bearer extraction (header, then the form's - // access_token, parsing multipart), so the install-caller refusal - // reads the caller exactly as NewIntrospectionRequest will. - "app_tokens.go::Server.introspectionCallerIsInstall::github.com/ory/fosite.AccessTokenFromRequest": 1, + // access_token, parsing multipart), so the caller is read exactly as + // NewIntrospectionRequest will. Shared since BUG-3398 by the + // install-caller refusal and the token-ownership check. + "app_tokens.go::Server.introspectionCallerClientID::github.com/ory/fosite.AccessTokenFromRequest": 1, } handoffs := collectRequestHandoffs(t, ".") diff --git a/internal/server/bug3398_introspect_test.go b/internal/server/bug3398_introspect_test.go new file mode 100644 index 000000000..cc78c6c33 --- /dev/null +++ b/internal/server/bug3398_introspect_test.go @@ -0,0 +1,57 @@ +package server + +import ( + "net/http" + "net/url" + "testing" +) + +// BUG-3398: the public /oauth/introspect describes a token only to that +// token's own client, and never describes a refresh token. Either refusal is +// the bare {"active": false} an unknown token gets, so neither is an oracle. +func TestBug3398_IntrospectionIsOwnClientAccessTokensOnly(t *testing.T) { + t.Parallel() + srv, _ := oauthEnabledTestServer(t) + _, sessionToken := loginTestUser(t, srv) + csrfTok := readCSRFFromCookie(t, srv, sessionToken) + clientA := registerTestClient(t, srv, "https://app.test/cb") + clientB := registerTestClient(t, srv, "https://app.test/cb") + + a1 := runAuthCodeFlow(t, srv, sessionToken, csrfTok, clientA, "verifier-b3398-a1-quick-brown-fox-1234567890") + a2 := runAuthCodeFlow(t, srv, sessionToken, csrfTok, clientA, "verifier-b3398-a2-quick-brown-fox-1234567890") + b1 := runAuthCodeFlow(t, srv, sessionToken, csrfTok, clientB, "verifier-b3398-b1-quick-brown-fox-1234567890") + str := func(m map[string]any, k string) string { + v, _ := m[k].(string) + if v == "" { + t.Fatalf("token response lacks %s: %v", k, m) + } + return v + } + + introspect := func(token, bearer string) map[string]any { + t.Helper() + rr := postOAuthFormBearer(srv, "/oauth/introspect", url.Values{"token": {token}}, bearer) + if rr.Code != http.StatusOK { + t.Fatalf("introspect: %d %s", rr.Code, rr.Body.String()) + } + var out map[string]any + parseJSON(t, rr, &out) + return out + } + inactive := func(where string, got map[string]any) { + t.Helper() + if len(got) != 1 || got["active"] != false { + t.Errorf("%s: want exactly {\"active\": false}, got %v", where, got) + } + } + + // The control: the same client's other access token is described. + if got := introspect(str(a1, "access_token"), str(a2, "access_token")); got["active"] != true || got["client_id"] != clientA { + t.Fatalf("same client: want active with client_id %s, got %v", clientA, got) + } + // Another client's token, though the caller's own token is active. + inactive("another client's access token", introspect(str(a1, "access_token"), str(b1, "access_token"))) + // A refresh token, even the caller's own client's. + inactive("own client's refresh token", introspect(str(a1, "refresh_token"), str(a2, "access_token"))) + inactive("another client's refresh token", introspect(str(a1, "refresh_token"), str(b1, "access_token"))) +} diff --git a/internal/server/handlers_oauth.go b/internal/server/handlers_oauth.go index 49c896e8f..ce9b5ae80 100644 --- a/internal/server/handlers_oauth.go +++ b/internal/server/handlers_oauth.go @@ -1440,6 +1440,10 @@ func (s *Server) handleOAuthRevoke(w http.ResponseWriter, r *http.Request) { // bearer is itself active and not the same token being introspected // (avoid trivial "introspect yourself with yourself" auth). // +// What is described (BUG-3398): only an ACCESS token, and only to that +// token's own client, read from the bearer's client (or the Basic client +// id). Any other token reads as {"active": false}, as an unknown one does. +// // In practice the consumer of this endpoint is sub-PR E's // MCPBearerAuth — but that integration uses fosite.IntrospectToken // directly (server-side, no HTTP roundtrip), so this public @@ -1468,8 +1472,8 @@ func (s *Server) handleOAuthIntrospect(w http.ResponseWriter, r *http.Request) { // Installed apps' clients do not use the public endpoint, and it never // describes their tokens (lead ruling R2 on TASK-3394): the app API - // checks its tokens in-process. The general caller-ownership gap is - // BUG-3398. + // checks its tokens in-process. Every other caller is told only about + // its own client's access tokens (BUG-3398, below). if err := r.ParseForm(); err != nil { http.Error(w, "Invalid form body", http.StatusBadRequest) return @@ -1481,8 +1485,23 @@ func (s *Server) handleOAuthIntrospect(w http.ResponseWriter, r *http.Request) { ir, err := s.oauthServer.Provider().NewIntrospectionRequest(ctx, r, session) if err == nil && ir.IsActive() { - if c, cerr := s.store.GetOAuthClient(ir.GetAccessRequester().GetClient().GetID()); cerr == nil && c.IsInstallClient() { + inspected := ir.GetAccessRequester().GetClient().GetID() + switch { + case ir.GetTokenUse() != fosite.AccessToken: + // A refresh token is never described (BUG-3398): it is not a + // bearer anywhere (/mcp refuses one), and describing it tells a + // holder of the string that it still mints access tokens. err = fosite.ErrInactiveToken + case inspected != s.introspectionCallerClientID(r): + // A token is described only to its own client (BUG-3398). RFC + // 7662 §2.2 lets the server answer inactive when the caller may + // not know about the token, and that answer is the same as for + // an unknown one, so this is not an oracle. + err = fosite.ErrInactiveToken + default: + if c, cerr := s.store.GetOAuthClient(inspected); cerr == nil && c.IsInstallClient() { + err = fosite.ErrInactiveToken + } } } if err != nil {