diff --git a/pkg/authserver/server/doc.go b/pkg/authserver/server/doc.go index 62c966deb0..19788d7b01 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..28b572d32e 100644 --- a/pkg/authserver/server/handlers/authorize_test.go +++ b/pkg/authserver/server/handlers/authorize_test.go @@ -7,13 +7,17 @@ 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" + "github.com/stacklok/toolhive/pkg/oauthproto" ) func TestAuthorizeHandler_MissingClientID(t *testing.T) { @@ -252,3 +256,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, + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodNone, + }) + 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"}, + 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"}, + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretBasic, + }) + 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 37b3c955e5..7722650b1d 100644 --- a/pkg/authserver/server/handlers/dcr_test.go +++ b/pkg/authserver/server/handlers/dcr_test.go @@ -323,8 +323,8 @@ func TestRegisterClientHandler_ClientIsStored(t *testing.T) { require.NotNil(t, storedClient) // DCR now stores the package's DCR-issued public client shape - // (*registration.publicClient), which embeds the LoopbackClient behaviour. - // Assert on the public surface rather than the unexported concrete type. + // (*registration.publicClient). Assert on the public surface rather than + // the unexported concrete type. 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()) @@ -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) { 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 984d1340f1..f085ebf868 100644 --- a/pkg/authserver/server/registration/client.go +++ b/pkg/authserver/server/registration/client.go @@ -22,85 +22,66 @@ import ( "encoding/base64" "fmt" "log/slog" + "net" "net/url" + "slices" "strings" "github.com/ory/fosite" - "github.com/stacklok/toolhive/pkg/networking" "github.com/stacklok/toolhive/pkg/oauthproto" ) -// LoopbackClient wraps a fosite.DefaultOpenIDConnectClient with RFC 8252 -// Section 7.3 loopback redirect URI matching helpers (MatchRedirectURI, -// GetMatchingRedirectURI, defined below). +// 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. // -// 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 -// -// What this type does NOT do: fosite's own authorize-path redirect matching -// (MatchRedirectURIWithClientRedirectURIs → isMatchingAsLoopback) reads only -// GetRedirectURIs() and never calls this type's methods, so they take no -// effect on that path. Fosite's own loopback matching (isLoopbackAddress, -// net.ParseIP().IsLoopback()) covers IP literals (127.0.0.1, [::1]) but not -// the "localhost" hostname — net.ParseIP("localhost") returns nil — so a -// client registered with "http://localhost/callback" gets exact-match only -// against fosite's matcher; a dynamic-port authorize request like -// "http://localhost:57403/callback" (the pattern VS Code, Claude Code, and -// other native apps use) fails today. MatchRedirectURI/GetMatchingRedirectURI -// exist for callers that do their own matching outside fosite's authorize -// path; they are not a fosite hook. This type's live value in the codebase is -// carrying the OIDC client shape (so GetTokenEndpointAuthMethod survives) -// through storage's DCR round-trip. -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). -// -// Note: fosite's redirect-matching path does not call MatchRedirectURI — -// MatchRedirectURIWithClientRedirectURIs reads only GetRedirectURIs() and -// applies fosite's own loopback handling (isMatchingAsLoopback), which covers -// loopback IP literals but not the "localhost" hostname. This wrapper's value -// is carrying the OIDC client shape (so GetTokenEndpointAuthMethod survives) -// for callers that do their own matching via MatchRedirectURI/ -// GetMatchingRedirectURI; it is not a fosite hook. -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. @@ -211,17 +192,18 @@ type dcrIssuedMarker struct{} func (dcrIssuedMarker) dcrIssued() {} // publicClient is the DCR-issued public client shape: an OIDC client (so the -// "none" method is recorded and enforced) with loopback redirect matching for -// native apps. +// "none" method is recorded and enforced). RFC 8252 Section 7.3 loopback +// dynamic-port matching for native apps is provided separately by +// RegisteredLoopbackRedirectURI, not by this type. type publicClient struct { dcrIssuedMarker - *LoopbackClient + *fosite.DefaultOpenIDConnectClient } // confidentialClient is the DCR-issued confidential client shape: an OIDC // client so fosite pins and enforces the registered auth method at the token -// endpoint. It is deliberately NOT a LoopbackClient — a secret-holding client -// gets no dynamic-port matching. +// endpoint. RegisteredLoopbackRedirectURI refuses dynamic-port matching for +// any non-public client, so a secret-holding client never gets it. type confidentialClient struct { dcrIssuedMarker *fosite.DefaultOpenIDConnectClient @@ -241,11 +223,11 @@ func GenerateClientSecret() (string, error) { } // New creates a fosite.Client from the given configuration. -// Public clients ("none") are wrapped in LoopbackClient to support RFC 8252 -// Section 7.3 compliant loopback redirect URI matching for native OAuth -// clients. Confidential clients (client_secret_basic / client_secret_post) -// require a Secret, have it SHA-256 hashed (see SHA256Hasher), and are not -// loopback-wrapped. +// 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 SHA-256 hashed +// (see SHA256Hasher) as required by fosite for credential validation. func New(cfg Config) (fosite.Client, error) { // Validate the auth method explicitly: silently defaulting an empty or // unknown value would reclassify the client one layer up, the same @@ -306,11 +288,8 @@ func New(cfg Config) (fosite.Client, error) { TokenEndpointAuthMethod: cfg.TokenEndpointAuthMethod, } - // Public clients get the LoopbackClient wrapper for RFC 8252 Section 7.3 - // dynamic port matching on native-app loopback redirect URIs; confidential - // clients do not (no dynamic-port matching for a secret holder). if public { - return &publicClient{LoopbackClient: NewLoopbackClient(oidcClient)}, nil + return &publicClient{DefaultOpenIDConnectClient: oidcClient}, nil } return &confidentialClient{DefaultOpenIDConnectClient: oidcClient}, nil } @@ -377,21 +356,6 @@ func NewConfidentialPlain(cfg Config) (fosite.Client, error) { return MarkDCRIssued(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. // @@ -411,6 +375,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" { @@ -418,7 +393,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 } @@ -427,13 +402,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 } @@ -441,13 +421,36 @@ func matchesAsLoopback(requestedURI, registeredURI string) bool { return true } -// isLoopbackURI checks if the URI uses a loopback address. +// isLoopbackURI reports whether uri's host is a loopback address (an IP +// loopback literal or "localhost", case-insensitively) — a hostname-only +// check, with no DNS resolution. Used by ValidateConfidentialRedirectURIs in +// dcr.go to reject a confidential registration's redirect_uri that targets +// loopback, which RFC 8252 §7.3 reserves for native/public clients. func isLoopbackURI(uri string) bool { parsed, err := url.Parse(uri) if err != nil { return false } - return networking.IsLocalhost(parsed.Hostname()) + return isLoopbackHostname(parsed.Hostname()) +} + +// 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 + } + 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 307fda6195..1ecc26fbe9 100644 --- a/pkg/authserver/server/registration/client_test.go +++ b/pkg/authserver/server/registration/client_test.go @@ -23,110 +23,168 @@ import ( "github.com/stretchr/testify/require" ) -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.(*publicClient) - assert.True(t, isLoopback, "public client should be the DCR-issued loopback-wrapped shape") + // Public clients are the DCR-issued publicClient shape (an OIDC client so + // TokenEndpointAuthMethod is set). + _, isPublic := client.(*publicClient) + assert.True(t, isPublic, "public client should be the DCR-issued publicClient shape") // 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 053d88455f..327da98b65 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,39 +270,11 @@ func buildFositeClient(doc *cimd.ClientMetadataDocument, resolvedScopes []string Public: true, } - openIDClient := &fosite.DefaultOpenIDConnectClient{ + // RFC 8252 §7.3 dynamic-port matching for this client's loopback redirect + // URIs is provided generically by registration.RegisteredLoopbackRedirectURI + // (keyed on IsPublic() + GetRedirectURIs()), so no wrapper type is needed here. + return &fosite.DefaultOpenIDConnectClient{ DefaultClient: defaultClient, TokenEndpointAuthMethod: tokenEndpointAuthMethod, } - - // Wrap in LoopbackClient when any redirect URI targets localhost. This does - // NOT make RFC 8252 §7.3 dynamic port matching work: fosite's own - // authorize-path redirect matching reads only GetRedirectURIs() and never - // calls LoopbackClient's methods, and fosite's own loopback matcher - // supports IP literals (127.0.0.1, [::1]) but not the "localhost" - // hostname — a "http://localhost/callback" registration still gets - // exact-match only against a dynamic-port authorize request. The wrap's - // value here is carrying the OIDC client shape so TokenEndpointAuthMethod - // is preserved — LoopbackClient 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 408919e1a7..36ef9b72e3 100644 --- a/pkg/authserver/storage/redis_test.go +++ b/pkg/authserver/storage/redis_test.go @@ -293,6 +293,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"}, + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodNone, + }) + 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"}, + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretBasic, + }) + 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_ClientAuthMethodPersistence pins the fail-closed read/write // behaviour for token_endpoint_auth_method: drift between the Public flag and // the stored method may only ever add secret verification, never remove it,