From 0bd9c5c88ded8d6e1ddb278f68f0d699f38f0d2d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 4 Aug 2026 23:24:55 +0200 Subject: [PATCH 1/2] Accept localhost dynamic-port redirect_uris Native MCP clients registered via DCR (VS Code, Claude Code) register a portless loopback redirect_uri such as http://localhost/callback and then listen on an ephemeral port. RFC 8252 Section 7.3 requires the authorization server to allow any port for loopback redirects, but fosite's matcher recognises only IP literals (127.0.0.1, [::1]) -- it compares "localhost" by exact string equality, so the dynamic-port request is rejected and the flow cannot complete. /authorize now rewrites the request form's redirect_uri to the client's registered portless literal so fosite's exact-match validation accepts it, then restores the requested dynamic-port URI for the pending authorization. The authorization code stays bound to the exact port the client asked for, so the token-endpoint check is unaffected. Error paths keep the client's real listener as the redirect target. A wrapper supplies the dynamic-port URI to fosite's error writer and widens its redirect_uri validity check to cover the localhost loopback case it cannot recognise on its own. The wrapper only ever widens that answer, never narrows it, so a client whose redirect_uri fosite would have accepted unaided still receives a proper error redirect rather than a bare JSON body. Loopback matching is stricter than fosite's: escaped paths are compared literally, a bare "?" is significant, and fragments and userinfo are rejected. Exact registered matches take precedence over dynamic-port matches so a client that pinned a port is never rewritten to a different registered entry. Dynamic-port matching is restricted to public clients, since RFC 8252 loopback redirects are a native-app pattern. IP-literal loopback clients are left untouched, as fosite already matches those natively on both success and error paths. Co-Authored-By: Claude Opus 5 --- pkg/authserver/server/doc.go | 4 +- pkg/authserver/server/handlers/authorize.go | 177 ++++++++++- .../server/handlers/authorize_test.go | 276 +++++++++++++++++ pkg/authserver/server/handlers/callback.go | 11 +- .../server/handlers/callback_test.go | 44 +++ pkg/authserver/server/handlers/dcr_test.go | 11 +- .../server/handlers/helpers_test.go | 7 +- .../server/handlers/loopback_e2e_test.go | 139 +++++++++ pkg/authserver/server/registration/client.go | 170 ++++++----- .../server/registration/client_test.go | 279 ++++++++++++------ pkg/authserver/storage/cimd_decorator.go | 42 +-- pkg/authserver/storage/cimd_decorator_test.go | 50 +++- pkg/authserver/storage/redis_test.go | 53 ++++ 13 files changed, 1037 insertions(+), 226 deletions(-) create mode 100644 pkg/authserver/server/handlers/loopback_e2e_test.go diff --git a/pkg/authserver/server/doc.go b/pkg/authserver/server/doc.go index f07738c548..b6a8983ebf 100644 --- a/pkg/authserver/server/doc.go +++ b/pkg/authserver/server/doc.go @@ -22,8 +22,8 @@ // // The server package is organized into focused sub-packages: // -// - server/registration: OAuth client types including RFC 8252 compliant LoopbackClient -// for native applications with dynamic port matching +// - server/registration: OAuth client construction plus RFC 8252 loopback +// redirect_uri matching for native applications with dynamic ports // - server/crypto: Cryptographic utilities for key loading, PKCE, and signing // - server/session: Session management linking issued tokens to upstream IdP tokens // diff --git a/pkg/authserver/server/handlers/authorize.go b/pkg/authserver/server/handlers/authorize.go index ba767e88c9..d22f1ce32a 100644 --- a/pkg/authserver/server/handlers/authorize.go +++ b/pkg/authserver/server/handlers/authorize.go @@ -4,14 +4,17 @@ package handlers import ( + "context" "crypto/rand" "log/slog" "net/http" + "net/url" "time" "github.com/ory/fosite" "github.com/stacklok/toolhive/pkg/authserver/server/crypto" + "github.com/stacklok/toolhive/pkg/authserver/server/registration" "github.com/stacklok/toolhive/pkg/authserver/storage" "github.com/stacklok/toolhive/pkg/authserver/upstream" ) @@ -44,16 +47,31 @@ func newUpstreamAuthSecrets() *upstreamAuthSecrets { func (h *Handler) AuthorizeHandler(w http.ResponseWriter, req *http.Request) { ctx := req.Context() + // See rewriteLoopbackRedirectURI's doc comment for what this does and why. + rewrittenFrom := h.rewriteLoopbackRedirectURI(ctx, req) + // Let fosite validate everything: client_id, redirect_uri, response_type, PKCE, scopes ar, err := h.provider.NewAuthorizeRequest(ctx, req) if err != nil { - h.provider.WriteAuthorizeError(ctx, w, ar, err) + // If the rewrite above fired, fosite validated (and stored) the + // registered portless literal, not the dynamic port the client + // actually requested. Wrapping ar restores the real listener as the + // error-redirect target -- see loopbackAuthorizeRequester's doc + // comment for how. + h.provider.WriteAuthorizeError(ctx, w, wrapLoopbackErrorRequester(ar, rewrittenFrom), err) return } // Extract validated data from the authorize request clientID := ar.GetClient().GetID() + // Use the original requested redirect_uri (before the loopback rewrite above) + // so the dynamic port survives into PendingAuthorization and everything + // downstream. rewrittenFrom is only non-empty when a rewrite actually + // happened; every other case falls through to ar.GetRedirectURI() unchanged. redirectURI := ar.GetRedirectURI().String() + if rewrittenFrom != "" { + redirectURI = rewrittenFrom + } state := ar.GetState() codeChallenge := ar.GetRequestForm().Get("code_challenge") codeChallengeMethod := ar.GetRequestForm().Get("code_challenge_method") @@ -122,3 +140,160 @@ func (h *Handler) AuthorizeHandler(w http.ResponseWriter, req *http.Request) { // Redirect user to upstream IDP http.Redirect(w, req, upstreamURL, http.StatusFound) } + +// loopbackAuthorizeRequester wraps a fosite.AuthorizeRequester to make the +// client's real, dynamic-port loopback redirect_uri the error-redirect target +// for fosite's WriteAuthorizeError, instead of the registered portless +// literal that rewriteLoopbackRedirectURI substituted for validation. +// +// WriteAuthorizeError touches the requester only through the +// fosite.AuthorizeRequester interface (GetResponseMode, IsRedirectURIValid, +// GetRedirectURI, GetState, plus a G11NContext type-assertion covered below) +// -- it never type-asserts to the concrete *fosite.AuthorizeRequest -- so an +// embedding wrapper survives it intact. +// +// Overriding GetRedirectURI is the only way to restore the dynamic port: +// fosite.AuthorizeRequester has no SetRedirectURI method, and RedirectURI is +// a plain field on the concrete *fosite.AuthorizeRequest, so it can't be +// mutated back without a concrete-type assertion. +// +// Overriding IsRedirectURIValid is required alongside it: WriteAuthorizeError +// gates the redirect on this check, and fosite's own implementation cannot +// recognize "localhost" as loopback any more than the original /authorize +// request could -- it would reject the dynamic-port URI and fall back to a +// bare JSON body. The override only ever widens fosite's own answer with the +// loopback matcher's; see the method's doc comment for why it can't narrow. +// +// Known side effect: fosite's getLangFromRequester (i18n_helper.go) type- +// asserts the requester to fosite.G11NContext to read GetLang(), which lives +// on the embedded concrete *fosite.Request, not on the AuthorizeRequester +// interface -- so the wrapper always falls back to language.English. This is +// harmless here because no MessageCatalog is configured on the provider, but +// it's a real behavioural difference from an unwrapped requester. +type loopbackAuthorizeRequester struct { + fosite.AuthorizeRequester + // redirectURI is the client's real, requested dynamic-port URI. + redirectURI *url.URL +} + +// wrapLoopbackErrorRequester wraps ar in loopbackAuthorizeRequester when +// rewrittenFrom is non-empty (i.e. rewriteLoopbackRedirectURI fired), so a +// later validation failure redirects the error to the client's real listener +// instead of the registered portless placeholder. Returns ar unwrapped when +// no rewrite happened, or when rewrittenFrom fails to parse. +func wrapLoopbackErrorRequester(ar fosite.AuthorizeRequester, rewrittenFrom string) fosite.AuthorizeRequester { + if rewrittenFrom == "" { + return ar + } + parsed, err := url.Parse(rewrittenFrom) + if err != nil { + return ar + } + return &loopbackAuthorizeRequester{AuthorizeRequester: ar, redirectURI: parsed} +} + +// IsRedirectURIValid may only ever WIDEN what fosite considers a valid +// redirect target, never narrow it: it first defers to the embedded +// requester's own check, and falls back to the loopback matcher only when +// that check says no. Narrowing would stop WriteAuthorizeError from +// delivering an error to a client whose redirect_uri fosite's own logic +// would have accepted on its own -- degrading a proper error redirect to a +// bare JSON body. +// +// The fallback adds exactly the "localhost" dynamic-port case fosite's own +// matcher cannot recognize (see rewriteLoopbackRedirectURI). It cannot be +// the sole check: registration.RegisteredLoopbackRedirectURI is +// public-clients-only (it early-returns false for any confidential client), +// so relying on it alone would reject confidential clients that fosite's +// own check would have validated. +func (r *loopbackAuthorizeRequester) IsRedirectURIValid() bool { + if r.AuthorizeRequester.IsRedirectURIValid() { + return true + } + client := r.GetClient() + if client == nil { + return false + } + _, ok := registration.RegisteredLoopbackRedirectURI(client, r.redirectURI.String()) + return ok +} + +// GetRedirectURI returns a copy of redirectURI, never the stored pointer: +// WriteAuthorizeError mutates the returned *url.URL in place (clearing +// Fragment, overwriting RawQuery), which would otherwise corrupt +// r.redirectURI on first use. +func (r *loopbackAuthorizeRequester) GetRedirectURI() *url.URL { + u := *r.redirectURI + return &u +} + +// rewriteLoopbackRedirectURI checks whether req's redirect_uri is a genuine +// RFC 8252 §7.3 loopback dynamic-port match against a client's registered +// redirect_uri (via registration.RegisteredLoopbackRedirectURI, so this works +// regardless of which concrete fosite.Client type the storage backend +// returns) and, if so, rewrites req.Form's redirect_uri to the registered +// (portless) literal so fosite's exact-match validation accepts it. It +// returns the original requested redirect_uri if a rewrite was made, or "" +// otherwise (including when req.Form can't be parsed, the client can't be +// resolved, isn't a loopback client, or the hostname is an IP literal -- +// fosite's own validation and native loopback matching handle those cases +// as they do today). +// +// Why this rewrite exists at all: fosite has no client-side hook for +// "localhost" loopback matching (see registration.RegisteredLoopbackRedirectURI) +// -- it only matches redirect_uri by exact string equality against a client's registered +// URIs (checked first) or its own IP-literal-only loopback exception, so this +// rewrite is what makes fosite's exact-match branch accept a "localhost" +// dynamic-port request. See loopbackAuthorizeRequester above for how the +// error path recovers the dynamic port that this rewrite hides from fosite. +func (h *Handler) rewriteLoopbackRedirectURI(ctx context.Context, req *http.Request) string { + if err := req.ParseForm(); err != nil { + return "" + } + + requestedRedirectURI := req.Form.Get("redirect_uri") + clientID := req.Form.Get("client_id") + if requestedRedirectURI == "" || clientID == "" { + return "" + } + + // Cheap, storage-free pre-check before doing a second client lookup (on top + // of fosite's own internal one): a rewrite can only ever be needed for an + // http redirect_uri whose hostname is "localhost" specifically. IP + // literals (127.0.0.1, [::1]) are intentionally excluded: fosite's own + // isMatchingAsLoopback already matches those natively, dynamic port and + // all, on both success and error paths, so treating them as needing the + // rewrite below would only add risk for no benefit. This also keeps the + // extra storage lookup below off the common case (non-loopback + // redirect_uris), narrowing its cost/availability impact to genuine + // "localhost" loopback requests only. + parsed, err := url.Parse(requestedRedirectURI) + if err != nil || parsed.Scheme != "http" || !registration.IsLocalhostHostname(parsed.Hostname()) { + return "" + } + + // A lookup error here is not necessarily fatal to the request: fosite's + // own independent lookup below can still succeed and validate the + // (unrewritten) redirect_uri normally. It only becomes user-visible as a + // generic "redirect_uri does not match any pre-registered redirect urls" + // when BOTH lookups fail, or when this is a transient error (e.g. a + // storage timeout) that fosite's second lookup doesn't hit -- in which + // case the rewrite silently never happens. Log it so that case is + // diagnosable; client_id is not a secret. + client, err := h.storage.GetClient(ctx, clientID) + if err != nil { + slog.WarnContext(ctx, "failed to look up client for loopback redirect_uri rewrite", + "client_id", clientID, + "error", err, + ) + return "" + } + + registered, ok := registration.RegisteredLoopbackRedirectURI(client, requestedRedirectURI) + if !ok || registered == requestedRedirectURI { + return "" + } + + req.Form.Set("redirect_uri", registered) + return requestedRedirectURI +} diff --git a/pkg/authserver/server/handlers/authorize_test.go b/pkg/authserver/server/handlers/authorize_test.go index 14415e6c81..6211487388 100644 --- a/pkg/authserver/server/handlers/authorize_test.go +++ b/pkg/authserver/server/handlers/authorize_test.go @@ -7,13 +7,16 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" + "github.com/ory/fosite" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stacklok/toolhive/pkg/authserver/server" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" + "github.com/stacklok/toolhive/pkg/authserver/server/registration" ) func TestAuthorizeHandler_MissingClientID(t *testing.T) { @@ -252,3 +255,276 @@ func TestAuthorizeHandler_RedirectsToUpstream(t *testing.T) { // Verify the challenge matches the stored verifier assert.Equal(t, servercrypto.ComputePKCEChallenge(pending.UpstreamPKCEVerifier), mockUpstream.capturedCodeChallenge) } + +// registerLoopbackClient creates a public client with loopback redirect URIs (as +// DCR/CIMD would) and registers it in storState so the mock GetClient call the +// handler makes can resolve it. +func registerLoopbackClient(t *testing.T, storState *testStorageState, clientID string, redirectURIs ...string) { + t.Helper() + + client, err := registration.New(registration.Config{ + ID: clientID, + RedirectURIs: redirectURIs, + Public: true, + }) + require.NoError(t, err) + storState.clients[clientID] = client +} + +func TestAuthorizeHandler_LoopbackLocalhostDynamicPortIsAccepted(t *testing.T) { + t.Parallel() + handler, storState, mockUpstream := handlerTestSetup(t) + + const clientID = "loopback-localhost-client" + registerLoopbackClient(t, storState, clientID, "http://localhost/callback") + + params := url.Values{ + "client_id": {clientID}, + "redirect_uri": {"http://localhost:54321/callback"}, + "response_type": {"code"}, + "state": {"client-state"}, + "code_challenge": {"challenge123"}, + "code_challenge_method": {"S256"}, + } + req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+params.Encode(), nil) + rec := httptest.NewRecorder() + + handler.AuthorizeHandler(rec, req) + + require.Equal(t, http.StatusFound, rec.Code, "response body: %s", rec.Body.String()) + assert.Contains(t, rec.Header().Get("Location"), "https://idp.example.com/authorize") + + pending, ok := storState.pendingAuths[mockUpstream.capturedState] + require.True(t, ok, "pending authorization should be stored") + assert.Equal(t, "http://localhost:54321/callback", pending.RedirectURI, + "the dynamic-port redirect_uri, not the portless registered one, must be preserved") +} + +// TestAuthorizeHandler_LoopbackCaseInsensitiveLocalhostIsAccepted pins that a +// mixed-case "localhost" hostname is matched the same as lowercase -- +// registration.hostnamesMatch treats "localhost" case-insensitively, and +// rewriteLoopbackRedirectURI's own pre-filter must not be stricter than the +// matcher it's gating (it previously used networking.IsLocalhost, a +// case-SENSITIVE check, which silently rejected "LOCALHOST" before the +// case-insensitive matcher ever saw it). +func TestAuthorizeHandler_LoopbackCaseInsensitiveLocalhostIsAccepted(t *testing.T) { + t.Parallel() + handler, storState, mockUpstream := handlerTestSetup(t) + + const clientID = "loopback-uppercase-localhost-client" + registerLoopbackClient(t, storState, clientID, "http://localhost/callback") + + params := url.Values{ + "client_id": {clientID}, + "redirect_uri": {"http://LOCALHOST:54321/callback"}, + "response_type": {"code"}, + "state": {"client-state"}, + "code_challenge": {"challenge123"}, + "code_challenge_method": {"S256"}, + } + req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+params.Encode(), nil) + rec := httptest.NewRecorder() + + handler.AuthorizeHandler(rec, req) + + require.Equal(t, http.StatusFound, rec.Code, "response body: %s", rec.Body.String()) + + pending, ok := storState.pendingAuths[mockUpstream.capturedState] + require.True(t, ok, "pending authorization should be stored") + assert.Equal(t, "http://LOCALHOST:54321/callback", pending.RedirectURI) +} + +func TestAuthorizeHandler_Loopback127001DynamicPortStillWorks(t *testing.T) { + t.Parallel() + handler, storState, mockUpstream := handlerTestSetup(t) + + const clientID = "loopback-127001-client" + registerLoopbackClient(t, storState, clientID, "http://127.0.0.1/callback") + + params := url.Values{ + "client_id": {clientID}, + "redirect_uri": {"http://127.0.0.1:54321/callback"}, + "response_type": {"code"}, + "state": {"client-state"}, + "code_challenge": {"challenge123"}, + "code_challenge_method": {"S256"}, + } + req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+params.Encode(), nil) + rec := httptest.NewRecorder() + + handler.AuthorizeHandler(rec, req) + + require.Equal(t, http.StatusFound, rec.Code, "response body: %s", rec.Body.String()) + + pending, ok := storState.pendingAuths[mockUpstream.capturedState] + require.True(t, ok, "pending authorization should be stored") + assert.Equal(t, "http://127.0.0.1:54321/callback", pending.RedirectURI) +} + +func TestAuthorizeHandler_LoopbackUnregisteredRedirectURIRejected(t *testing.T) { + t.Parallel() + handler, storState, _ := handlerTestSetup(t) + + const clientID = "loopback-unregistered-client" + registerLoopbackClient(t, storState, clientID, "http://localhost/callback") + + params := url.Values{ + "client_id": {clientID}, + "redirect_uri": {"http://localhost:54321/not-the-registered-path"}, + "response_type": {"code"}, + "state": {"client-state"}, + "code_challenge": {"challenge123"}, + "code_challenge_method": {"S256"}, + } + req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+params.Encode(), nil) + rec := httptest.NewRecorder() + + handler.AuthorizeHandler(rec, req) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "invalid_request") +} + +// TestAuthorizeHandler_LoopbackErrorRedirectsToDynamicPort proves that a +// validation failure occurring after redirect_uri validation succeeds (here, +// an unsupported response_type) redirects to the client's real dynamic-port +// listener, not the registered portless placeholder that +// rewriteLoopbackRedirectURI substituted for fosite's exact-match check. See +// loopbackAuthorizeRequester for the mechanism. +func TestAuthorizeHandler_LoopbackErrorRedirectsToDynamicPort(t *testing.T) { + t.Parallel() + handler, storState, _ := handlerTestSetup(t) + + const clientID = "loopback-error-redirect-client" + registerLoopbackClient(t, storState, clientID, "http://localhost/callback") + + params := url.Values{ + "client_id": {clientID}, + "redirect_uri": {"http://localhost:54321/callback"}, + "response_type": {"token"}, // implicit flow not supported + "state": {"client-state"}, + } + req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+params.Encode(), nil) + rec := httptest.NewRecorder() + + handler.AuthorizeHandler(rec, req) + + // fosite uses 303 See Other for error redirects per RFC 6749 + assert.Equal(t, http.StatusSeeOther, rec.Code) + location := rec.Header().Get("Location") + assert.Contains(t, location, "error=unsupported_response_type") + assert.Contains(t, location, "state=client-state") + assert.True(t, strings.HasPrefix(location, "http://localhost:54321/callback"), + "error redirect must target the client's real dynamic-port listener, got: %s", location) +} + +// TestAuthorizeHandler_Loopback127001ErrorRedirectsToDynamicPort proves an +// IP-literal loopback client (127.0.0.1) keeps working exactly as it did +// before the "localhost" loopback rewrite was introduced: +// rewriteLoopbackRedirectURI skips IP literals entirely, since fosite's own +// native loopback matching already preserves the dynamic port for these on +// both success and error redirects. +func TestAuthorizeHandler_Loopback127001ErrorRedirectsToDynamicPort(t *testing.T) { + t.Parallel() + handler, storState, _ := handlerTestSetup(t) + + const clientID = "loopback-127001-error-redirect-client" + registerLoopbackClient(t, storState, clientID, "http://127.0.0.1/callback") + + params := url.Values{ + "client_id": {clientID}, + "redirect_uri": {"http://127.0.0.1:54321/callback"}, + "response_type": {"token"}, // implicit flow not supported + "state": {"client-state"}, + } + req := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+params.Encode(), nil) + rec := httptest.NewRecorder() + + handler.AuthorizeHandler(rec, req) + + assert.Equal(t, http.StatusSeeOther, rec.Code) + location := rec.Header().Get("Location") + assert.Contains(t, location, "error=unsupported_response_type") + assert.True(t, strings.HasPrefix(location, "http://127.0.0.1:54321/callback"), + "error redirect must preserve the dynamic port for IP-literal loopback clients, got: %s", location) +} + +func TestLoopbackAuthorizeRequester_IsRedirectURIValid(t *testing.T) { + t.Parallel() + + loopbackClient, err := registration.New(registration.Config{ + ID: "wrapper-test-client", + RedirectURIs: []string{"http://localhost/callback"}, + Public: true, + }) + require.NoError(t, err) + + confidentialClient, err := registration.New(registration.Config{ + ID: "wrapper-test-confidential-client", + Secret: "s3cr3t-plaintext", + RedirectURIs: []string{"https://example.com/callback"}, + Public: false, + }) + require.NoError(t, err) + + tests := []struct { + name string + // arRedirectURI, when non-empty, seeds the embedded requester's own + // RedirectURI, exercising fosite's own IsRedirectURIValid. Left empty + // (nil ar.RedirectURI) for cases that must exercise only the fallback. + arRedirectURI string + client fosite.Client + redirectURI string + wantValid bool + }{ + { + name: "nil client is invalid", + client: nil, + redirectURI: "http://localhost:54321/callback", + wantValid: false, + }, + { + name: "genuine loopback dynamic-port match is valid", + client: loopbackClient, + redirectURI: "http://localhost:54321/callback", + wantValid: true, + }, + { + name: "unregistered path is not a loopback match", + client: loopbackClient, + redirectURI: "http://localhost:54321/not-registered", + wantValid: false, + }, + { + // Pins the widen-only invariant (see IsRedirectURIValid's doc + // comment): would fail if the override reverted to consulting + // only the public-clients-only loopback matcher, since + // RegisteredLoopbackRedirectURI unconditionally rejects + // confidential clients via its !IsPublic() guard. + name: "confidential client with exact-match redirect_uri is valid via fosite's own check", + arRedirectURI: "https://example.com/callback", + client: confidentialClient, + redirectURI: "https://example.com/callback", + wantValid: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + redirectURI, err := url.Parse(tt.redirectURI) + require.NoError(t, err) + + ar := fosite.NewAuthorizeRequest() + ar.Client = tt.client + if tt.arRedirectURI != "" { + ar.RedirectURI, err = url.Parse(tt.arRedirectURI) + require.NoError(t, err) + } + wrapped := &loopbackAuthorizeRequester{AuthorizeRequester: ar, redirectURI: redirectURI} + + assert.Equal(t, tt.wantValid, wrapped.IsRedirectURIValid()) + }) + } +} diff --git a/pkg/authserver/server/handlers/callback.go b/pkg/authserver/server/handlers/callback.go index 7f7114c33b..1b110c2283 100644 --- a/pkg/authserver/server/handlers/callback.go +++ b/pkg/authserver/server/handlers/callback.go @@ -359,7 +359,16 @@ func (h *Handler) buildAuthorizeRequesterFromPending( if client, err := h.storage.GetClient(ctx, pending.ClientID); err == nil { ar.Client = client } - return ar + + // ar.RedirectURI keeps the real dynamic-port URI from pending.RedirectURI. + // fosite's own IsRedirectURIValid can't recognize "localhost" as loopback + // any more than the original /authorize request could (see + // rewriteLoopbackRedirectURI in authorize.go), so wrap here. Wrapping + // unconditionally is safe because the wrapper's override only ever widens + // validity (see loopbackAuthorizeRequester.IsRedirectURIValid), so a + // non-loopback or confidential client keeps exactly the validity fosite + // would have given it. + return wrapLoopbackErrorRequester(ar, pending.RedirectURI) } // handleUpstreamError handles error responses from the upstream IDP. diff --git a/pkg/authserver/server/handlers/callback_test.go b/pkg/authserver/server/handlers/callback_test.go index 4d5173f3b6..a4efe103d8 100644 --- a/pkg/authserver/server/handlers/callback_test.go +++ b/pkg/authserver/server/handlers/callback_test.go @@ -7,6 +7,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -93,6 +94,49 @@ func TestCallbackHandler_UpstreamError(t *testing.T) { assert.False(t, ok, "pending authorization should be deleted") } +// TestCallbackHandler_UpstreamError_LoopbackLocalhostRedirectsToDynamicPort +// proves that an upstream-IDP error (e.g. the user denies consent) for a +// "localhost" loopback client redirects to the client's real dynamic-port +// listener from pending.RedirectURI, not the registered portless literal and +// not a bare JSON body. See loopbackAuthorizeRequester (authorize.go) and +// wrapLoopbackErrorRequester's use in buildAuthorizeRequesterFromPending for +// the mechanism. +func TestCallbackHandler_UpstreamError_LoopbackLocalhostRedirectsToDynamicPort(t *testing.T) { + t.Parallel() + handler, storState, _ := handlerTestSetup(t) + + const clientID = "loopback-callback-error-client" + registerLoopbackClient(t, storState, clientID, "http://localhost/callback") + + internalState := testInternalState + pending := &storage.PendingAuthorization{ + ClientID: clientID, + RedirectURI: "http://localhost:54321/callback", + State: "client-state", + PKCEChallenge: "challenge123", + PKCEMethod: "S256", + Scopes: []string{"openid"}, + InternalState: internalState, + SessionID: "session-loopback-upstream-error", + UpstreamProviderName: "test-upstream", + CreatedAt: time.Now(), + } + storState.pendingAuths[internalState] = pending + + req := httptest.NewRequest(http.MethodGet, "/oauth/callback?error=access_denied&error_description=User+denied&state="+internalState, nil) + rec := httptest.NewRecorder() + + handler.CallbackHandler(rec, req) + + require.Equal(t, http.StatusSeeOther, rec.Code, + "the error must redirect to the client's real listener, not fall back to a JSON body; body: %s", rec.Body.String()) + location := rec.Header().Get("Location") + assert.Contains(t, location, "error=access_denied") + assert.Contains(t, location, "state=client-state") + assert.True(t, strings.HasPrefix(location, "http://localhost:54321/callback"), + "error redirect must target the client's real dynamic-port listener, got: %s", location) +} + func TestCallbackHandler_ExchangeCodeFailure(t *testing.T) { t.Parallel() handler, storState, mockUpstream := handlerTestSetup(t) diff --git a/pkg/authserver/server/handlers/dcr_test.go b/pkg/authserver/server/handlers/dcr_test.go index 2491b4f500..cc8bed96f8 100644 --- a/pkg/authserver/server/handlers/dcr_test.go +++ b/pkg/authserver/server/handlers/dcr_test.go @@ -303,13 +303,10 @@ func TestRegisterClientHandler_ClientIsStored(t *testing.T) { require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) require.NotNil(t, storedClient) - loopbackClient, ok := storedClient.(*registration.LoopbackClient) - require.True(t, ok, "expected *registration.LoopbackClient, got %T", storedClient) - - assert.Equal(t, resp.ClientID, loopbackClient.GetID()) - assert.True(t, loopbackClient.IsPublic()) - assert.Equal(t, []string{"http://127.0.0.1:8080/callback"}, loopbackClient.GetRedirectURIs()) - assert.Equal(t, fosite.Arguments(allowedAudiences), loopbackClient.GetAudience(), + assert.Equal(t, resp.ClientID, storedClient.GetID()) + assert.True(t, storedClient.IsPublic()) + assert.Equal(t, []string{"http://127.0.0.1:8080/callback"}, storedClient.GetRedirectURIs()) + assert.Equal(t, fosite.Arguments(allowedAudiences), storedClient.GetAudience(), "DCR client must inherit server's AllowedAudiences so refresh token requests with resource= succeed") } diff --git a/pkg/authserver/server/handlers/helpers_test.go b/pkg/authserver/server/handlers/helpers_test.go index 2ee8ede7b2..a391a2a0b9 100644 --- a/pkg/authserver/server/handlers/helpers_test.go +++ b/pkg/authserver/server/handlers/helpers_test.go @@ -184,14 +184,15 @@ func baseTestSetup(t *testing.T, opts ...baseTestSetupOption) (fosite.OAuth2Prov } storState.clients[testAuthClientID] = testClient - // Setup mock expectations for GetClient - stor.EXPECT().GetClient(gomock.Any(), testAuthClientID).DoAndReturn(func(_ context.Context, id string) (fosite.Client, error) { + // Setup mock expectations for GetClient. Looks up storState.clients so tests + // can register additional clients (e.g. a loopback client under its own ID) + // after baseTestSetup returns. + stor.EXPECT().GetClient(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, id string) (fosite.Client, error) { if c, ok := storState.clients[id]; ok { return c, nil } return nil, fosite.ErrNotFound }).AnyTimes() - stor.EXPECT().GetClient(gomock.Any(), gomock.Not(testAuthClientID)).Return(nil, fosite.ErrNotFound).AnyTimes() // Token issuance renews the public client's registration TTL (best-effort). // Record the calls so tests can assert the renewal fired on success. diff --git a/pkg/authserver/server/handlers/loopback_e2e_test.go b/pkg/authserver/server/handlers/loopback_e2e_test.go new file mode 100644 index 0000000000..ed0163a441 --- /dev/null +++ b/pkg/authserver/server/handlers/loopback_e2e_test.go @@ -0,0 +1,139 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package handlers + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" +) + +const loopbackE2EClientID = "loopback-e2e-client" + +// driveLoopbackAuthorizeAndCallback runs a loopback dynamic-port client through +// /oauth/authorize and the simulated upstream callback, returning the handler +// (so the caller can exchange the code at /oauth/token) and the minted +// authorization code. It proves the fix end to end: AuthorizeHandler rewrites +// the redirect_uri for fosite's validation but preserves the dynamic port in +// PendingAuthorization, and CallbackHandler's code issuance sources the +// client redirect from that same PendingAuthorization. +func driveLoopbackAuthorizeAndCallback(t *testing.T, dynamicRedirectURI string) (*Handler, string) { + t.Helper() + + handler, storState, mockUpstream := handlerTestSetup(t) + registerLoopbackClient(t, storState, loopbackE2EClientID, "http://localhost/callback") + + pkceChallenge := servercrypto.ComputePKCEChallenge(testPKCEVerifier) + + authParams := url.Values{ + "client_id": {loopbackE2EClientID}, + "redirect_uri": {dynamicRedirectURI}, + "response_type": {"code"}, + "state": {"client-state"}, + "code_challenge": {pkceChallenge}, + "code_challenge_method": {"S256"}, + } + authReq := httptest.NewRequest(http.MethodGet, "/oauth/authorize?"+authParams.Encode(), nil) + authRec := httptest.NewRecorder() + handler.AuthorizeHandler(authRec, authReq) + require.Equal(t, http.StatusFound, authRec.Code, + "authorize should redirect to upstream, got %d: %s", authRec.Code, authRec.Body.String()) + + internalState := mockUpstream.capturedState + require.NotEmpty(t, internalState, "upstream authorization URL should have been built with the internal state") + + callbackReq := httptest.NewRequest(http.MethodGet, "/oauth/callback?code=upstream-code&state="+internalState, nil) + callbackRec := httptest.NewRecorder() + handler.CallbackHandler(callbackRec, callbackReq) + require.Equal(t, http.StatusSeeOther, callbackRec.Code, + "callback should redirect with an authorization code, got %d: %s", callbackRec.Code, callbackRec.Body.String()) + + location := callbackRec.Header().Get("Location") + require.True(t, strings.HasPrefix(location, dynamicRedirectURI), + "callback should redirect back to the dynamic-port redirect_uri actually used at /authorize, got: %s", location) + + redirectURL, err := url.Parse(location) + require.NoError(t, err) + code := redirectURL.Query().Get("code") + require.NotEmpty(t, code, "callback redirect should include an authorization code") + + return handler, code +} + +// loopbackTokenExchange posts the given redirect_uri and code to /oauth/token. +func loopbackTokenExchange(t *testing.T, handler *Handler, code, redirectURI string) *httptest.ResponseRecorder { + t.Helper() + + form := url.Values{ + "grant_type": {"authorization_code"}, + "client_id": {loopbackE2EClientID}, + "redirect_uri": {redirectURI}, + "code": {code}, + "code_verifier": {testPKCEVerifier}, + } + req := httptest.NewRequest(http.MethodPost, "/oauth/token", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + + handler.TokenHandler(rec, req) + return rec +} + +// TestLoopbackLocalhost_FullFlow_TokenExchangeSucceeds is the core +// verification: a client registered with a portless http://localhost/callback +// requests authorization with a dynamic port (RFC 8252 §7.3), completes the +// upstream callback, and presents that SAME dynamic-port redirect_uri at +// /oauth/token. The token exchange must succeed -- proving the dynamic port +// survives authorize -> callback -> token without a redirect_uri mismatch. +func TestLoopbackLocalhost_FullFlow_TokenExchangeSucceeds(t *testing.T) { + t.Parallel() + + const dynamicRedirectURI = "http://localhost:54321/callback" + handler, code := driveLoopbackAuthorizeAndCallback(t, dynamicRedirectURI) + + rec := loopbackTokenExchange(t, handler, code, dynamicRedirectURI) + + require.Equal(t, http.StatusOK, rec.Code, + "token exchange with the same dynamic-port redirect_uri should succeed, got %d: %s", rec.Code, rec.Body.String()) + assert.Contains(t, rec.Body.String(), "access_token") +} + +// TestLoopbackLocalhost_FullFlow_TokenExchangeRejectsDifferentPort proves the +// fix doesn't weaken RFC 6749 §10.6's authorization-code/redirect-URI binding: +// presenting a different port than what was used at /authorize must still be +// rejected at /oauth/token. +func TestLoopbackLocalhost_FullFlow_TokenExchangeRejectsDifferentPort(t *testing.T) { + t.Parallel() + + const dynamicRedirectURI = "http://localhost:54321/callback" + handler, code := driveLoopbackAuthorizeAndCallback(t, dynamicRedirectURI) + + rec := loopbackTokenExchange(t, handler, code, "http://localhost:9999/callback") + + assert.Equal(t, http.StatusBadRequest, rec.Code, + "token exchange with a different port than used at /authorize must be rejected, got %d: %s", rec.Code, rec.Body.String()) + assert.Contains(t, rec.Body.String(), "invalid_grant") +} + +// TestLoopbackLocalhost_FullFlow_TokenExchangeRejectsDifferentPath proves the +// same binding holds for a different path, not just a different port. +func TestLoopbackLocalhost_FullFlow_TokenExchangeRejectsDifferentPath(t *testing.T) { + t.Parallel() + + const dynamicRedirectURI = "http://localhost:54321/callback" + handler, code := driveLoopbackAuthorizeAndCallback(t, dynamicRedirectURI) + + rec := loopbackTokenExchange(t, handler, code, "http://localhost:54321/other-path") + + assert.Equal(t, http.StatusBadRequest, rec.Code, + "token exchange with a different path than used at /authorize must be rejected, got %d: %s", rec.Code, rec.Body.String()) + assert.Contains(t, rec.Body.String(), "invalid_grant") +} diff --git a/pkg/authserver/server/registration/client.go b/pkg/authserver/server/registration/client.go index d6cdcf39a5..05c8d53576 100644 --- a/pkg/authserver/server/registration/client.go +++ b/pkg/authserver/server/registration/client.go @@ -18,69 +18,65 @@ package registration import ( "fmt" + "net" "net/url" + "slices" "strings" "github.com/ory/fosite" "golang.org/x/crypto/bcrypt" - - "github.com/stacklok/toolhive/pkg/networking" ) -// LoopbackClient is a fosite.Client implementation that supports RFC 8252 Section 7.3 -// compliant loopback redirect URI matching for native OAuth clients. -// -// RFC 8252 Section 7.3 specifies that: -// - Loopback redirect URIs use "http" (not "https") -// - The host must be "127.0.0.1", "[::1]", or "localhost" -// - The authorization server MUST allow any port -// - The path and query components must match exactly +// RegisteredLoopbackRedirectURI returns the registered redirect URI of c that +// requestedURI matches -- exactly, or under RFC 8252 Section 7.3 loopback +// dynamic-port rules -- or ("", false) if none matches. // -// This client extends fosite's built-in loopback support to also handle "localhost" -// as a loopback address. Fosite's isMatchingAsLoopback uses isLoopbackAddress() -// which only supports IP addresses (net.ParseIP().IsLoopback()), not the "localhost" -// hostname. This is needed for DCR with clients like VS Code, Claude Code, and other -// native apps that register redirect URIs like "http://localhost/callback" and then -// request authorization with dynamic ports like "http://localhost:57403/callback". -type LoopbackClient struct { - *fosite.DefaultOpenIDConnectClient +// Loopback dynamic-port matching is restricted to public clients: RFC 8252 +// loopback redirects are a native-app pattern and a confidential client must +// never get dynamic-port flexibility on its registered redirect_uri. Keeping +// that guard here rather than in a per-client method means no storage backend +// can reconstruct a client that silently skips it. +func RegisteredLoopbackRedirectURI(c fosite.Client, requestedURI string) (string, bool) { + if !c.IsPublic() { + return "", false + } + return matchLoopbackRedirectURI(c.GetRedirectURIs(), requestedURI) } -// NewLoopbackClient creates a new LoopbackClient wrapping the provided client. -// The wrapper preserves all OIDC fields (including TokenEndpointAuthMethod) -// while adding RFC 8252 §7.3 dynamic port matching for loopback redirect URIs. -func NewLoopbackClient(client *fosite.DefaultOpenIDConnectClient) *LoopbackClient { - return &LoopbackClient{DefaultOpenIDConnectClient: client} +// IsLocalhostHostname reports whether host (as returned by url.Hostname()) is +// the string "localhost", matched case-insensitively -- the same definition +// hostnamesMatch and isLoopbackHostname use. Exported so callers outside this +// package that need to know "is this specifically localhost, not an IP +// loopback literal" (e.g. deciding whether fosite's own IP-literal-only +// loopback matching already handles a redirect_uri, or whether it needs this +// package's help) share this package's one definition instead of +// re-implementing it and risking drift. +func IsLocalhostHostname(host string) bool { + return strings.EqualFold(host, "localhost") } -// MatchRedirectURI checks if the given redirect URI matches one of the client's -// registered redirect URIs, with RFC 8252 Section 7.3 loopback support. +// matchLoopbackRedirectURI returns the registered URI (from registeredURIs) +// that requestedURI matches, either exactly or under RFC 8252 Section 7.3 +// loopback dynamic-port rules, or ("", false) if none matches. // -// For loopback URIs (127.0.0.1, [::1], or localhost), the port is allowed to -// vary while the scheme, host, path, and query must match exactly. -func (c *LoopbackClient) MatchRedirectURI(requestedURI string) bool { - for _, registeredURI := range c.GetRedirectURIs() { - if matchesRedirectURI(requestedURI, registeredURI) { - return true - } +// Exact matches take precedence over loopback matches: a full pass over +// registeredURIs checks for an exact match first, and only then falls back to +// loopback matching. Without this, a client registered with both +// "http://localhost/callback" and "http://localhost:54321/callback" would +// have a request for the second rewritten to the first, because loopback +// matching against the first entry succeeds before the loop ever reaches the +// second entry's exact match. A client that registered a specific port must +// not have its request silently redirected to a different registered entry. +func matchLoopbackRedirectURI(registeredURIs []string, requestedURI string) (string, bool) { + if slices.Contains(registeredURIs, requestedURI) { + return requestedURI, true } - return false -} - -// GetMatchingRedirectURI returns the matching redirect URI if found, or an empty string. -// For loopback URIs, returns the requested URI (with its port) if it matches a registered -// loopback pattern. -func (c *LoopbackClient) GetMatchingRedirectURI(requestedURI string) string { - for _, registeredURI := range c.GetRedirectURIs() { - if matchesRedirectURI(requestedURI, registeredURI) { - // For loopback matches, return the requested URI to preserve the dynamic port - if isLoopbackURI(requestedURI) { - return requestedURI - } - return registeredURI + for _, registeredURI := range registeredURIs { + if matchesAsLoopback(requestedURI, registeredURI) { + return registeredURI, true } } - return "" + return "", false } // DefaultScopes are the default OAuth 2.0 scopes for registered clients. @@ -121,8 +117,9 @@ type Config struct { } // New creates a fosite.Client from the given configuration. -// Public clients are wrapped in LoopbackClient to support RFC 8252 Section 7.3 -// compliant loopback redirect URI matching for native OAuth clients. +// Public clients get TokenEndpointAuthMethod "none" via DefaultOpenIDConnectClient; +// RFC 8252 Section 7.3 loopback redirect URI matching for native OAuth clients is +// provided separately by RegisteredLoopbackRedirectURI, not by the client type itself. // Confidential clients with secrets have their Secret field bcrypt-hashed // as required by fosite for credential validation. func New(cfg Config) (fosite.Client, error) { @@ -167,36 +164,19 @@ func New(cfg Config) (fosite.Client, error) { defaultClient.Secret = hashedSecret } - // Wrap public clients in LoopbackClient for RFC 8252 Section 7.3 - // dynamic port matching for native app loopback redirect URIs. - // Use DefaultOpenIDConnectClient so TokenEndpointAuthMethod ("none" for - // public clients) is preserved through the LoopbackClient wrapper. + // Use DefaultOpenIDConnectClient for public clients so TokenEndpointAuthMethod + // ("none") is set; RegisteredLoopbackRedirectURI provides RFC 8252 §7.3 + // dynamic port matching for these clients' loopback redirect URIs. if cfg.Public { - oidcClient := &fosite.DefaultOpenIDConnectClient{ + return &fosite.DefaultOpenIDConnectClient{ DefaultClient: defaultClient, TokenEndpointAuthMethod: "none", - } - return NewLoopbackClient(oidcClient), nil + }, nil } return defaultClient, nil } -// Compile-time interface compliance check -var _ fosite.Client = (*LoopbackClient)(nil) - -// matchesRedirectURI checks if a requested URI matches a registered URI. -// Implements RFC 8252 Section 7.3 loopback matching. -func matchesRedirectURI(requestedURI, registeredURI string) bool { - // Exact match always works - if requestedURI == registeredURI { - return true - } - - // Try loopback matching - return matchesAsLoopback(requestedURI, registeredURI) -} - // matchesAsLoopback checks if the requested URI matches the registered URI // using RFC 8252 Section 7.3 loopback rules. // @@ -216,6 +196,17 @@ func matchesAsLoopback(requestedURI, registeredURI string) bool { return false } + // RFC 6749 Section 3.1.2: the redirection endpoint URI MUST NOT include a + // fragment component, and must not carry userinfo. Fosite's own + // IsValidRedirectURI enforces this on whatever redirect_uri it actually + // validates -- but a loopback match here is what causes the ORIGINAL + // requested URI (not fosite's validated one) to become the effective + // redirect target, so the same check must run here too, or an invalid + // requested URI could reach storage/token-issuance unvalidated. + if requested.Fragment != "" || requested.User != nil { + return false + } + // RFC 8252 Section 7.3: Loopback redirect URIs use the "http" scheme. // Dynamic port matching only applies to http loopback URIs, not https. if requested.Scheme != "http" || registered.Scheme != "http" { @@ -223,7 +214,7 @@ func matchesAsLoopback(requestedURI, registeredURI string) bool { } // Both must be loopback addresses - if !networking.IsLocalhost(requested.Hostname()) || !networking.IsLocalhost(registered.Hostname()) { + if !isLoopbackHostname(requested.Hostname()) || !isLoopbackHostname(registered.Hostname()) { return false } @@ -232,13 +223,18 @@ func matchesAsLoopback(requestedURI, registeredURI string) bool { return false } - // Path must match exactly - if requested.Path != registered.Path { + // Path must match exactly. EscapedPath() (not Path) is compared: Path is + // percent-decoded, so an encoded separator (e.g. registered + // "/callback%2Fchild") would otherwise compare equal to a literal, + // unencoded path ("/callback/child") that was never actually registered. + if requested.EscapedPath() != registered.EscapedPath() { return false } - // Query must match exactly - if requested.RawQuery != registered.RawQuery { + // Query must match exactly, including whether a bare "?" was present at + // all (ForceQuery): RawQuery alone can't distinguish "/callback" from + // "/callback?", since both parse to an empty RawQuery. + if requested.RawQuery != registered.RawQuery || requested.ForceQuery != registered.ForceQuery { return false } @@ -246,13 +242,23 @@ func matchesAsLoopback(requestedURI, registeredURI string) bool { return true } -// isLoopbackURI checks if the URI uses a loopback address. -func isLoopbackURI(uri string) bool { - parsed, err := url.Parse(uri) - if err != nil { - return false +// isLoopbackHostname reports whether host (as returned by url.Hostname(), so +// already stripped of brackets and port) is one of the RFC 8252 §7.3 loopback +// forms: "localhost" (case-insensitive, matching hostnamesMatch below) or an +// IP loopback literal (127.0.0.1, ::1). +// +// This is deliberately self-contained rather than delegating to +// networking.IsLocalhost: that helper (via oauthproto.IsLoopbackHost) is a +// case-SENSITIVE prefix check requiring the bracketed "[::1]" form, which +// url.Hostname() never produces -- using it here would silently make +// "LOCALHOST" and "::1" both unmatchable despite hostnamesMatch's own +// case-insensitive "localhost" contract. +func isLoopbackHostname(host string) bool { + if IsLocalhostHostname(host) { + return true } - return networking.IsLocalhost(parsed.Hostname()) + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() } // hostnamesMatch checks if two hostnames (as returned by url.Hostname()) should diff --git a/pkg/authserver/server/registration/client_test.go b/pkg/authserver/server/registration/client_test.go index 37f6280006..eb6c93d5a6 100644 --- a/pkg/authserver/server/registration/client_test.go +++ b/pkg/authserver/server/registration/client_test.go @@ -23,110 +23,168 @@ import ( "golang.org/x/crypto/bcrypt" ) -func TestNewLoopbackClient(t *testing.T) { - t.Parallel() - - defaultClient := &fosite.DefaultClient{ - ID: "test-client", - RedirectURIs: []string{"http://127.0.0.1/callback"}, - Public: true, - } - - client := NewLoopbackClient(&fosite.DefaultOpenIDConnectClient{DefaultClient: defaultClient}) - - assert.NotNil(t, client) - assert.Equal(t, "test-client", client.GetID()) - assert.Equal(t, []string{"http://127.0.0.1/callback"}, client.GetRedirectURIs()) - assert.True(t, client.IsPublic()) -} - -func TestLoopbackClient_MatchRedirectURI(t *testing.T) { +// TestRegisteredLoopbackRedirectURI covers RegisteredLoopbackRedirectURI, the +// sole production-reachable matcher for loopback clients. +func TestRegisteredLoopbackRedirectURI(t *testing.T) { t.Parallel() tests := []struct { name string registeredURIs []string requestedURI string - shouldMatch bool + expectedURI string + expectedOK bool }{ - // Exact matches { name: "exact match - https", registeredURIs: []string{"https://example.com/callback"}, requestedURI: "https://example.com/callback", - shouldMatch: true, + expectedURI: "https://example.com/callback", + expectedOK: true, }, { name: "exact match - http loopback with port", registeredURIs: []string{"http://127.0.0.1:8080/callback"}, requestedURI: "http://127.0.0.1:8080/callback", - shouldMatch: true, + expectedURI: "http://127.0.0.1:8080/callback", + expectedOK: true, }, // RFC 8252 Section 7.3 - IPv4 loopback (127.0.0.1) { - name: "loopback IPv4 - dynamic port matches", + name: "loopback IPv4 - dynamic port matches, returns registered portless URI", registeredURIs: []string{"http://127.0.0.1/callback"}, - requestedURI: "http://127.0.0.1:57403/callback", - shouldMatch: true, - }, - { - name: "loopback IPv4 - different dynamic port matches", - registeredURIs: []string{"http://127.0.0.1/callback"}, - requestedURI: "http://127.0.0.1:8080/callback", - shouldMatch: true, + requestedURI: "http://127.0.0.1:54321/callback", + expectedURI: "http://127.0.0.1/callback", + expectedOK: true, }, { name: "loopback IPv4 - no port in request matches registered without port", registeredURIs: []string{"http://127.0.0.1/callback"}, requestedURI: "http://127.0.0.1/callback", - shouldMatch: true, + expectedURI: "http://127.0.0.1/callback", + expectedOK: true, }, { name: "loopback IPv4 - path must match", registeredURIs: []string{"http://127.0.0.1/callback"}, requestedURI: "http://127.0.0.1:57403/other", - shouldMatch: false, + expectedURI: "", + expectedOK: false, }, { name: "loopback IPv4 - query must match", registeredURIs: []string{"http://127.0.0.1/callback?foo=bar"}, requestedURI: "http://127.0.0.1:57403/callback?foo=bar", - shouldMatch: true, + expectedURI: "http://127.0.0.1/callback?foo=bar", + expectedOK: true, }, { name: "loopback IPv4 - query mismatch fails", registeredURIs: []string{"http://127.0.0.1/callback"}, requestedURI: "http://127.0.0.1:57403/callback?extra=param", - shouldMatch: false, + expectedURI: "", + expectedOK: false, + }, + { + name: "loopback IPv4 - fragment on requested URI rejected", + registeredURIs: []string{"http://127.0.0.1/callback"}, + requestedURI: "http://127.0.0.1:57403/callback#frag", + expectedURI: "", + expectedOK: false, }, // RFC 8252 Section 7.3 - localhost { - name: "loopback localhost - dynamic port matches", + name: "localhost loopback - returns registered portless URI", registeredURIs: []string{"http://localhost/callback"}, - requestedURI: "http://localhost:57403/callback", - shouldMatch: true, + requestedURI: "http://localhost:54321/callback", + expectedURI: "http://localhost/callback", + expectedOK: true, }, { name: "loopback localhost - path must match", registeredURIs: []string{"http://localhost/callback"}, requestedURI: "http://localhost:57403/other", - shouldMatch: false, + expectedURI: "", + expectedOK: false, + }, + // A percent-encoded separator in the registered path must NOT match an + // unencoded literal that merely decodes to the same string: Path is + // decoded, so comparing it (instead of EscapedPath) would treat + // "/callback%2Fchild" (registered) and "/callback/child" (requested, + // never actually registered) as equal. + { + name: "encoded path separator does not match unencoded literal", + registeredURIs: []string{"http://localhost/callback%2Fchild"}, + requestedURI: "http://localhost:57403/callback/child", + expectedURI: "", + expectedOK: false, + }, + // A bare trailing "?" (ForceQuery) must not be treated as equivalent + // to no query string at all: both parse to an empty RawQuery, so + // comparing RawQuery alone can't tell "/callback" from "/callback?". + { + name: "bare trailing question mark does not match no query string", + registeredURIs: []string{"http://localhost/callback"}, + requestedURI: "http://localhost:57403/callback?", + expectedURI: "", + expectedOK: false, + }, + + // RFC 6749 §3.1.2: the redirection endpoint URI MUST NOT include a + // fragment component or userinfo; a dynamic-port match must not let + // either through unvalidated. + { + name: "loopback localhost - fragment on requested URI rejected", + registeredURIs: []string{"http://localhost/callback"}, + requestedURI: "http://localhost:57403/callback#frag", + expectedURI: "", + expectedOK: false, + }, + { + name: "loopback localhost - userinfo on requested URI rejected", + registeredURIs: []string{"http://localhost/callback"}, + requestedURI: "http://user:pass@localhost:57403/callback", + expectedURI: "", + expectedOK: false, + }, + + // isLoopbackHostname is self-contained (not networking.IsLocalhost, which + // has a separate, wider-blast-radius bug: a case-sensitive prefix check + // requiring the bracketed "[::1]" form that url.Hostname() never + // produces -- gating ~15 unrelated HTTPS-exemption/DCR/discovery call + // sites, tracked separately, out of scope for #6189). So both [::1] and + // mixed-case "localhost" work correctly here. + { + name: "IPv6 loopback [::1] - dynamic port matches", + registeredURIs: []string{"http://[::1]/callback"}, + requestedURI: "http://[::1]:54321/callback", + expectedURI: "http://[::1]/callback", + expectedOK: true, + }, + { + name: "case-insensitive localhost - dynamic port matches", + registeredURIs: []string{"http://localhost/callback"}, + requestedURI: "http://LOCALHOST:54321/callback", + expectedURI: "http://localhost/callback", + expectedOK: true, }, // Cross-hostname matching should NOT work (security requirement) { - name: "localhost and 127.0.0.1 are different", + name: "localhost and 127.0.0.1 are different (registered 127.0.0.1)", registeredURIs: []string{"http://127.0.0.1/callback"}, requestedURI: "http://localhost:57403/callback", - shouldMatch: false, + expectedURI: "", + expectedOK: false, }, { - name: "127.0.0.1 and localhost are different", + name: "wrong loopback host - localhost does not match 127.0.0.1 (registered localhost)", registeredURIs: []string{"http://localhost/callback"}, - requestedURI: "http://127.0.0.1:57403/callback", - shouldMatch: false, + requestedURI: "http://127.0.0.1:54321/callback", + expectedURI: "", + expectedOK: false, }, // Non-loopback should use exact matching only @@ -134,13 +192,15 @@ func TestLoopbackClient_MatchRedirectURI(t *testing.T) { name: "non-loopback - exact match required", registeredURIs: []string{"https://example.com/callback"}, requestedURI: "https://example.com:8080/callback", - shouldMatch: false, + expectedURI: "", + expectedOK: false, }, { name: "non-loopback - different host fails", registeredURIs: []string{"https://example.com/callback"}, requestedURI: "https://other.com/callback", - shouldMatch: false, + expectedURI: "", + expectedOK: false, }, // HTTPS loopback should NOT get dynamic port matching (RFC 8252 says http) @@ -148,7 +208,8 @@ func TestLoopbackClient_MatchRedirectURI(t *testing.T) { name: "https loopback - no dynamic port matching", registeredURIs: []string{"https://127.0.0.1/callback"}, requestedURI: "https://127.0.0.1:57403/callback", - shouldMatch: false, + expectedURI: "", + expectedOK: false, }, // Multiple registered URIs @@ -156,13 +217,29 @@ func TestLoopbackClient_MatchRedirectURI(t *testing.T) { name: "multiple URIs - matches first", registeredURIs: []string{"http://127.0.0.1/callback", "https://example.com/callback"}, requestedURI: "http://127.0.0.1:8080/callback", - shouldMatch: true, + expectedURI: "http://127.0.0.1/callback", + expectedOK: true, }, { name: "multiple URIs - matches second", registeredURIs: []string{"http://127.0.0.1/callback", "https://example.com/callback"}, requestedURI: "https://example.com/callback", - shouldMatch: true, + expectedURI: "https://example.com/callback", + expectedOK: true, + }, + + // A registered port is not pinned: matchesAsLoopback only checks + // scheme/hostname/path/query, so a registered port is ignored on both + // sides, not just the requested one. Defensible under RFC 8252 (native + // apps don't have a fixed listening port to pin to), but this is the + // security-relevant edge of the loopback carve-out, so it must be + // explicitly pinned by a test rather than left implicit. + { + name: "registered port is not pinned - any requested port still matches", + registeredURIs: []string{"http://localhost:8080/callback"}, + requestedURI: "http://localhost:54321/callback", + expectedURI: "http://localhost:8080/callback", + expectedOK: true, }, // Edge cases @@ -170,25 +247,36 @@ func TestLoopbackClient_MatchRedirectURI(t *testing.T) { name: "empty registered URIs", registeredURIs: []string{}, requestedURI: "http://127.0.0.1:8080/callback", - shouldMatch: false, + expectedURI: "", + expectedOK: false, }, { name: "invalid requested URI", registeredURIs: []string{"http://127.0.0.1/callback"}, requestedURI: "://invalid", - shouldMatch: false, + expectedURI: "", + expectedOK: false, + }, + { + name: "no match - returns empty string and false", + registeredURIs: []string{"https://example.com/callback"}, + requestedURI: "https://other.com/callback", + expectedURI: "", + expectedOK: false, }, { name: "empty path matches empty path", registeredURIs: []string{"http://127.0.0.1"}, requestedURI: "http://127.0.0.1:8080", - shouldMatch: true, + expectedURI: "http://127.0.0.1", + expectedOK: true, }, { name: "root path matches root path", registeredURIs: []string{"http://127.0.0.1/"}, requestedURI: "http://127.0.0.1:8080/", - shouldMatch: true, + expectedURI: "http://127.0.0.1/", + expectedOK: true, }, } @@ -196,69 +284,77 @@ func TestLoopbackClient_MatchRedirectURI(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - client := NewLoopbackClient(&fosite.DefaultOpenIDConnectClient{ + client := &fosite.DefaultOpenIDConnectClient{ DefaultClient: &fosite.DefaultClient{ ID: "test-client", RedirectURIs: tt.registeredURIs, Public: true, }, - }) + } - result := client.MatchRedirectURI(tt.requestedURI) - assert.Equal(t, tt.shouldMatch, result) + uri, ok := RegisteredLoopbackRedirectURI(client, tt.requestedURI) + assert.Equal(t, tt.expectedURI, uri) + assert.Equal(t, tt.expectedOK, ok) }) } } -func TestLoopbackClient_GetMatchingRedirectURI(t *testing.T) { +// TestRegisteredLoopbackRedirectURI_ConfidentialClientNeverMatches pins the +// IsPublic() guard: RegisteredLoopbackRedirectURI must never grant loopback +// dynamic-port matching to a confidential client, even if constructed +// directly with loopback-shaped redirect URIs (bypassing New's own +// public-only wrapping). +func TestRegisteredLoopbackRedirectURI_ConfidentialClientNeverMatches(t *testing.T) { t.Parallel() + client := &fosite.DefaultOpenIDConnectClient{ + DefaultClient: &fosite.DefaultClient{ + ID: "test-client", + RedirectURIs: []string{"http://localhost/callback"}, + Public: false, + }, + } + + uri, ok := RegisteredLoopbackRedirectURI(client, "http://localhost:54321/callback") + assert.Equal(t, "", uri) + assert.False(t, ok, "a confidential client must not get loopback dynamic-port matching") +} + +// TestRegisteredLoopbackRedirectURI_ExactMatchPrecedesLoopbackMatch pins that +// exact registered matches take precedence over loopback dynamic-port +// matches: when a public client is registered with both a portless loopback +// URI and an exact-port loopback URI, a request for the exact-port URI must +// return that exact entry, not the portless entry that would also +// loopback-match. Order of registration must not affect the outcome. +func TestRegisteredLoopbackRedirectURI_ExactMatchPrecedesLoopbackMatch(t *testing.T) { + t.Parallel() + + portless := "http://localhost/callback" + exact := "http://localhost:54321/callback" + tests := []struct { name string registeredURIs []string - requestedURI string - expectedURI string }{ - { - name: "loopback - returns requested URI with port", - registeredURIs: []string{"http://127.0.0.1/callback"}, - requestedURI: "http://127.0.0.1:57403/callback", - expectedURI: "http://127.0.0.1:57403/callback", - }, - { - name: "non-loopback exact match - returns registered URI", - registeredURIs: []string{"https://example.com/callback"}, - requestedURI: "https://example.com/callback", - expectedURI: "https://example.com/callback", - }, - { - name: "no match - returns empty string", - registeredURIs: []string{"https://example.com/callback"}, - requestedURI: "https://other.com/callback", - expectedURI: "", - }, - { - name: "localhost loopback - returns requested URI", - registeredURIs: []string{"http://localhost/callback"}, - requestedURI: "http://localhost:8080/callback", - expectedURI: "http://localhost:8080/callback", - }, + {name: "portless registered first", registeredURIs: []string{portless, exact}}, + {name: "exact registered first", registeredURIs: []string{exact, portless}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - client := NewLoopbackClient(&fosite.DefaultOpenIDConnectClient{ + client := &fosite.DefaultOpenIDConnectClient{ DefaultClient: &fosite.DefaultClient{ ID: "test-client", RedirectURIs: tt.registeredURIs, Public: true, }, - }) + } - result := client.GetMatchingRedirectURI(tt.requestedURI) - assert.Equal(t, tt.expectedURI, result) + uri, ok := RegisteredLoopbackRedirectURI(client, exact) + require.True(t, ok) + assert.Equal(t, exact, uri, "exact match must win over the portless loopback match") }) } } @@ -275,9 +371,10 @@ func TestNewClient_PublicClient(t *testing.T) { client, err := New(cfg) require.NoError(t, err) - // Public clients should be wrapped in LoopbackClient - _, isLoopback := client.(*LoopbackClient) - assert.True(t, isLoopback, "public client should be wrapped in LoopbackClient") + // Public clients are DefaultOpenIDConnectClient (not DefaultClient) so + // TokenEndpointAuthMethod is set. + _, isOIDC := client.(*fosite.DefaultOpenIDConnectClient) + assert.True(t, isOIDC, "public client should be *fosite.DefaultOpenIDConnectClient") // Check basic properties assert.Equal(t, "test-public-client", client.GetID()) diff --git a/pkg/authserver/storage/cimd_decorator.go b/pkg/authserver/storage/cimd_decorator.go index d8a8c5c508..ab49630753 100644 --- a/pkg/authserver/storage/cimd_decorator.go +++ b/pkg/authserver/storage/cimd_decorator.go @@ -6,7 +6,6 @@ package storage import ( "context" "fmt" - "net/url" "slices" "strings" "time" @@ -108,6 +107,17 @@ func (d *CIMDStorageDecorator) fetchOrCached(ctx context.Context, id string) (fo // context detached from the caller so that one caller cancelling does not // abort the in-flight request for other waiters. The HTTP client inside // FetchClientMetadataDocument enforces its own 5-second timeout. + // + // Deliberately NOT negatively cached: an unreachable/invalid client_id is + // refetched on every call (including the authorize handler's own + // loopback-redirect_uri pre-check on top of fosite's internal client + // lookup, doubling that specific cost) rather than caching the failure. + // Two reasons: the CIMD draft (client-id-metadata-document §5.2) requires + // error responses and invalid/malformed documents not be cached, and a + // shared failure+success cache lets an unauthenticated caller evict + // legitimate entries for free by cycling through bogus client_ids up to + // the LRU's bound -- a cheaper, more effective attack than the modest + // double-fetch this would have saved. fetchCtx := context.WithoutCancel(ctx) result, err, _ := d.sf.Do(id, func() (interface{}, error) { // Re-check cache inside singleflight (another goroutine may have populated it) @@ -221,8 +231,6 @@ var defaultCIMDResponseTypes = []string{"code"} const defaultCIMDTokenEndpointAuthMethod = "none" // buildFositeClient converts a ClientMetadataDocument into a fosite.Client. -// Redirect URIs containing http://localhost are wrapped in a LoopbackClient -// so that RFC 8252 §7.3 dynamic port matching applies. // resolvedScopes is the already-validated scope list computed by fetch() via // registration.ValidateScopes; when empty, DefaultScopes is used — this occurs when // the decorator has no ScopesSupported restriction (unconstrained AS). @@ -262,34 +270,8 @@ func buildFositeClient(doc *cimd.ClientMetadataDocument, resolvedScopes []string Public: true, } - openIDClient := &fosite.DefaultOpenIDConnectClient{ + return &fosite.DefaultOpenIDConnectClient{ DefaultClient: defaultClient, TokenEndpointAuthMethod: tokenEndpointAuthMethod, } - - // Wrap in LoopbackClient when any redirect URI targets localhost so that - // RFC 8252 §7.3 dynamic port matching works for native app clients. - // Pass openIDClient directly so TokenEndpointAuthMethod is preserved — - // LoopbackClient now embeds *fosite.DefaultOpenIDConnectClient. - if hasLoopbackRedirectURI(doc.RedirectURIs) { - return registration.NewLoopbackClient(openIDClient) - } - - return openIDClient -} - -// hasLoopbackRedirectURI returns true when any of the redirect URIs in the -// list targets a loopback address over HTTP. The host is parsed from each URI -// to prevent bypass via hosts like "http://localhost.evil.com/". -func hasLoopbackRedirectURI(uris []string) bool { - for _, uri := range uris { - parsed, err := url.Parse(uri) - if err != nil { - continue - } - if parsed.Scheme == "http" && oauthproto.IsLoopbackHost(parsed.Hostname()) { - return true - } - } - return false } diff --git a/pkg/authserver/storage/cimd_decorator_test.go b/pkg/authserver/storage/cimd_decorator_test.go index 1666cc81de..1c75081c31 100644 --- a/pkg/authserver/storage/cimd_decorator_test.go +++ b/pkg/authserver/storage/cimd_decorator_test.go @@ -282,6 +282,37 @@ func TestCIMDStorageDecorator_FetchOrCached_FetchFailureReturnsNotFound(t *testi assert.ErrorIs(t, err, fosite.ErrNotFound, "fetch failure must be wrapped as fosite.ErrNotFound") } +// TestCIMDStorageDecorator_FetchOrCached_FetchFailureIsNotCached pins CIMD +// draft compliance (client-id-metadata-document §5.2: error responses and +// invalid/malformed documents must not be cached) and closes off the +// eviction-based DoS a shared failure+success cache would otherwise allow: an +// unauthenticated caller cycling through bogus client_ids could flush +// legitimate cache entries for free. The accepted tradeoff is a fresh network +// fetch on every call for a persistently-failing client_id. +func TestCIMDStorageDecorator_FetchOrCached_FetchFailureIsNotCached(t *testing.T) { + t.Parallel() + + var fetchCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + fetchCount.Add(1) + w.WriteHeader(http.StatusInternalServerError) + })) + t.Cleanup(srv.Close) + + dec := newEnabledDecorator(t, newTestBase(t), 10, time.Minute) + id := srv.URL + "/meta.json" + + _, err := dec.fetchOrCached(context.Background(), id) + require.Error(t, err) + + _, err = dec.fetchOrCached(context.Background(), id) + require.Error(t, err) + assert.ErrorIs(t, err, fosite.ErrNotFound) + + assert.Equal(t, int32(2), fetchCount.Load(), + "a failing client_id must be refetched every call, not cached (CIMD draft §5.2)") +} + func TestCIMDStorageDecorator_FetchOrCached_ExpiredCacheEntryRefetches(t *testing.T) { t.Parallel() @@ -371,7 +402,7 @@ func TestBuildFositeClient_ScopeParsing(t *testing.T) { assert.ElementsMatch(t, []string{"openid", "profile", "email"}, []string(got.GetScopes())) } -func TestBuildFositeClient_LoopbackRedirectWrapsInLoopbackClient(t *testing.T) { +func TestBuildFositeClient_LoopbackRedirectGetsDynamicPortMatching(t *testing.T) { t.Parallel() doc := &cimd.ClientMetadataDocument{ @@ -380,16 +411,17 @@ func TestBuildFositeClient_LoopbackRedirectWrapsInLoopbackClient(t *testing.T) { } got := buildFositeClient(doc, nil) - // LoopbackClient adds MatchRedirectURI — check the distinctive method is present. - type loopbackMatcher interface { - MatchRedirectURI(string) bool - } - _, ok := got.(loopbackMatcher) - assert.True(t, ok, "loopback redirect URI must produce a LoopbackClient") - // TokenEndpointAuthMethod must be preserved through the LoopbackClient wrapper. + // A client built for a loopback CIMD document must get RFC 8252 §7.3 + // dynamic-port matching: a request for a different port than registered + // still resolves to the registered portless URI. + uri, ok := registration.RegisteredLoopbackRedirectURI(got, "http://localhost:54321/callback") + require.True(t, ok, "loopback redirect URI must get dynamic-port matching") + assert.Equal(t, "http://localhost/callback", uri) + + // TokenEndpointAuthMethod must be preserved through buildFositeClient. oidc, ok := got.(fosite.OpenIDConnectClient) - require.True(t, ok, "LoopbackClient must implement fosite.OpenIDConnectClient") + require.True(t, ok, "got must implement fosite.OpenIDConnectClient") assert.Equal(t, "none", oidc.GetTokenEndpointAuthMethod(), "loopback client must preserve TokenEndpointAuthMethod from the OIDC client") } diff --git a/pkg/authserver/storage/redis_test.go b/pkg/authserver/storage/redis_test.go index cb9d562811..06d6d44ef8 100644 --- a/pkg/authserver/storage/redis_test.go +++ b/pkg/authserver/storage/redis_test.go @@ -26,6 +26,7 @@ import ( "github.com/stretchr/testify/require" tcredis "github.com/stacklok/toolhive-core/redis" + "github.com/stacklok/toolhive/pkg/authserver/server/registration" "github.com/stacklok/toolhive/pkg/authserver/server/session" ) @@ -273,6 +274,58 @@ func TestRedisStorage_RegisterClient(t *testing.T) { }) } +// TestRedisStorage_GetClient_SupportsLoopbackRedirectMatching pins that a +// public client round-tripped through Redis still supports RFC 8252 §7.3 +// loopback dynamic-port redirect_uri matching via +// registration.RegisteredLoopbackRedirectURI. Redis reconstructs GetClient's +// result as its own *redisClient type, so this only holds because +// RegisteredLoopbackRedirectURI works against any fosite.Client regardless of +// concrete type. A regression here would silently +// disable the embedded auth server's localhost-loopback fix for any client +// registered via DCR against Redis-backed storage. +func TestRedisStorage_GetClient_SupportsLoopbackRedirectMatching(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + client, err := registration.New(registration.Config{ + ID: "loopback-redis-client", + RedirectURIs: []string{"http://localhost/callback"}, + Public: true, + }) + require.NoError(t, err) + require.NoError(t, s.RegisterClient(ctx, client)) + + retrieved, err := s.GetClient(ctx, "loopback-redis-client") + require.NoError(t, err) + + registered, ok := registration.RegisteredLoopbackRedirectURI(retrieved, "http://localhost:54321/callback") + require.True(t, ok, "a dynamic-port localhost request must still match the registered portless URI") + assert.Equal(t, "http://localhost/callback", registered) + }) +} + +// TestRedisStorage_GetClient_ConfidentialClientDoesNotMatchAsLoopback pins that +// a confidential (non-public) client stored in Redis never matches as a +// loopback client, even if its redirect_uris happen to look loopback-shaped -- +// RegisteredLoopbackRedirectURI restricts dynamic-port matching to public +// clients, since RFC 8252 loopback redirects are a native-app pattern. +func TestRedisStorage_GetClient_ConfidentialClientDoesNotMatchAsLoopback(t *testing.T) { + withRedisStorage(t, func(ctx context.Context, s *RedisStorage, _ *miniredis.Miniredis) { + client, err := registration.New(registration.Config{ + ID: "confidential-redis-client", + Secret: "s3cr3t", + RedirectURIs: []string{"http://localhost/callback"}, + Public: false, + }) + require.NoError(t, err) + require.NoError(t, s.RegisterClient(ctx, client)) + + retrieved, err := s.GetClient(ctx, "confidential-redis-client") + require.NoError(t, err) + + _, ok := registration.RegisteredLoopbackRedirectURI(retrieved, "http://localhost:54321/callback") + assert.False(t, ok, "a confidential client must not get loopback dynamic-port matching") + }) +} + // TestRedisStorage_RenewClientTTL verifies that RenewClientTTL extends the // registration TTL for public (DCR) clients — keeping actively-used clients from // being evicted mid-lifecycle — while leaving confidential clients (which have no From a899108ae8e9934c094a64a5deb4418266eed480 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 11 Aug 2026 23:27:32 +0200 Subject: [PATCH 2/2] Update tests for merged registration.Config API The main-branch merge replaced Config.Public (bool) with TokenEndpointAuthMethod (string) and dropped the LoopbackClient wrapper type; these two non-conflicted test files still referenced the old shapes and failed to build. --- .../server/handlers/authorize_test.go | 21 ++++++++++--------- pkg/authserver/server/handlers/dcr_test.go | 10 ++++----- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/pkg/authserver/server/handlers/authorize_test.go b/pkg/authserver/server/handlers/authorize_test.go index 6211487388..28b572d32e 100644 --- a/pkg/authserver/server/handlers/authorize_test.go +++ b/pkg/authserver/server/handlers/authorize_test.go @@ -17,6 +17,7 @@ import ( "github.com/stacklok/toolhive/pkg/authserver/server" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" "github.com/stacklok/toolhive/pkg/authserver/server/registration" + "github.com/stacklok/toolhive/pkg/oauthproto" ) func TestAuthorizeHandler_MissingClientID(t *testing.T) { @@ -263,9 +264,9 @@ func registerLoopbackClient(t *testing.T, storState *testStorageState, clientID t.Helper() client, err := registration.New(registration.Config{ - ID: clientID, - RedirectURIs: redirectURIs, - Public: true, + ID: clientID, + RedirectURIs: redirectURIs, + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodNone, }) require.NoError(t, err) storState.clients[clientID] = client @@ -453,17 +454,17 @@ func TestLoopbackAuthorizeRequester_IsRedirectURIValid(t *testing.T) { t.Parallel() loopbackClient, err := registration.New(registration.Config{ - ID: "wrapper-test-client", - RedirectURIs: []string{"http://localhost/callback"}, - Public: true, + ID: "wrapper-test-client", + RedirectURIs: []string{"http://localhost/callback"}, + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodNone, }) require.NoError(t, err) confidentialClient, err := registration.New(registration.Config{ - ID: "wrapper-test-confidential-client", - Secret: "s3cr3t-plaintext", - RedirectURIs: []string{"https://example.com/callback"}, - Public: false, + ID: "wrapper-test-confidential-client", + Secret: "s3cr3t-plaintext", + RedirectURIs: []string{"https://example.com/callback"}, + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretBasic, }) require.NoError(t, err) diff --git a/pkg/authserver/server/handlers/dcr_test.go b/pkg/authserver/server/handlers/dcr_test.go index c4db643a72..7722650b1d 100644 --- a/pkg/authserver/server/handlers/dcr_test.go +++ b/pkg/authserver/server/handlers/dcr_test.go @@ -686,7 +686,7 @@ func TestRegisterClientHandler_ConfidentialDCR(t *testing.T) { func TestRegisterClientHandler_ConfidentialClientStored(t *testing.T) { t.Parallel() - t.Run("stored without LoopbackClient wrapper and carries auth method", func(t *testing.T) { + t.Run("stored without loopback dynamic-port matching and carries auth method", func(t *testing.T) { t.Parallel() cfg := confidentialConfig(true) w, captured := runDCR(t, cfg, @@ -700,11 +700,9 @@ func TestRegisterClientHandler_ConfidentialClientStored(t *testing.T) { assert.False(t, captured.IsPublic(), "confidential client must not be public") assert.Equal(t, []string{"https://app.example/cb"}, captured.GetRedirectURIs()) - // A secret-holding client must NOT get the LoopbackClient dynamic-port - // matching wrapper. - _, isLoopback := captured.(*registration.LoopbackClient) - assert.False(t, isLoopback, - "confidential client must not be a *registration.LoopbackClient") + // A secret-holding client must NOT get RFC 8252 dynamic-port matching. + _, ok = registration.RegisteredLoopbackRedirectURI(captured, "https://app.example/cb") + assert.False(t, ok, "confidential client must not get loopback dynamic-port matching") }) t.Run("audience is preserved on stored confidential client", func(t *testing.T) {