diff --git a/pkg/coredata/oauth2_client.go b/pkg/coredata/oauth2_client.go index f0bcae8cb..0bf40d374 100644 --- a/pkg/coredata/oauth2_client.go +++ b/pkg/coredata/oauth2_client.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "maps" + "net/url" "slices" "time" @@ -26,6 +27,7 @@ import ( "go.gearno.de/kit/pg" "go.probo.inc/probo/pkg/gid" "go.probo.inc/probo/pkg/iam/policy" + "go.probo.inc/probo/pkg/netx" "go.probo.inc/probo/pkg/page" "go.probo.inc/probo/pkg/uri" ) @@ -54,7 +56,42 @@ type ( ) func (c *OAuth2Client) IsRedirectURIAllowed(rawURI string) bool { - return slices.Contains(c.RedirectURIs, uri.URI(rawURI)) + if slices.Contains(c.RedirectURIs, uri.URI(rawURI)) { + return true + } + + // RFC 8252 ยง7.3: native apps using a loopback redirect URI choose an + // ephemeral port at request time, so the port must be ignored when + // matching against the registered loopback redirect URIs. + requested, err := url.Parse(rawURI) + if err != nil || !netx.IsLoopback(requested.Hostname()) { + return false + } + + for _, registered := range c.RedirectURIs { + candidate, err := url.Parse(registered.String()) + if err != nil { + continue + } + + if candidate.Scheme != requested.Scheme { + continue + } + + if !netx.IsLoopback(candidate.Hostname()) { + continue + } + + if candidate.Hostname() != requested.Hostname() { + continue + } + + if candidate.Path == requested.Path && candidate.RawQuery == requested.RawQuery { + return true + } + } + + return false } func (c *OAuth2Client) HasGrantType(grantType OAuth2GrantType) bool { diff --git a/pkg/coredata/oauth2_client_test.go b/pkg/coredata/oauth2_client_test.go new file mode 100644 index 000000000..303a0d632 --- /dev/null +++ b/pkg/coredata/oauth2_client_test.go @@ -0,0 +1,96 @@ +// Copyright (c) 2026 Probo Inc . +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +// OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +package coredata_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.probo.inc/probo/pkg/coredata" + "go.probo.inc/probo/pkg/uri" +) + +func TestOAuth2Client_IsRedirectURIAllowed(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + registered []uri.URI + requested string + want bool + }{ + { + name: "exact https match", + registered: []uri.URI{"https://chatgpt.com/connector/oauth/callback"}, + requested: "https://chatgpt.com/connector/oauth/callback", + want: true, + }, + { + name: "https mismatch", + registered: []uri.URI{"https://chatgpt.com/connector/oauth/callback"}, + requested: "https://evil.example/callback", + want: false, + }, + { + name: "loopback ignores port when registered without port", + registered: []uri.URI{"http://localhost/callback"}, + requested: "http://localhost:3118/callback", + want: true, + }, + { + name: "loopback ignores port when registered with a different port", + registered: []uri.URI{"http://127.0.0.1:53682/callback"}, + requested: "http://127.0.0.1:8080/callback", + want: true, + }, + { + name: "loopback host must match (localhost vs 127.0.0.1)", + registered: []uri.URI{"http://127.0.0.1/callback"}, + requested: "http://localhost:3118/callback", + want: false, + }, + { + name: "loopback path must match", + registered: []uri.URI{"http://localhost/callback"}, + requested: "http://localhost:3118/other", + want: false, + }, + { + name: "loopback scheme must match", + registered: []uri.URI{"http://localhost/callback"}, + requested: "https://localhost:3118/callback", + want: false, + }, + { + name: "non-loopback host does not get port flexibility", + registered: []uri.URI{"https://app.example.com/callback"}, + requested: "https://app.example.com:8443/callback", + want: false, + }, + } + + for _, tt := range tests { + t.Run( + tt.name, + func(t *testing.T) { + t.Parallel() + + client := &coredata.OAuth2Client{RedirectURIs: tt.registered} + + assert.Equal(t, tt.want, client.IsRedirectURIAllowed(tt.requested)) + }, + ) + } +} diff --git a/pkg/iam/oauth2/cimd.go b/pkg/iam/oauth2/cimd.go index 4b2dd673b..9868d675a 100644 --- a/pkg/iam/oauth2/cimd.go +++ b/pkg/iam/oauth2/cimd.go @@ -33,7 +33,7 @@ import ( "go.probo.inc/probo/pkg/cachecontrol" "go.probo.inc/probo/pkg/coredata" "go.probo.inc/probo/pkg/gid" - "go.probo.inc/probo/pkg/net" + "go.probo.inc/probo/pkg/netx" ) const ( @@ -239,7 +239,7 @@ func validateCIMDRedirectURI(redirectURI string) error { switch parsed.Scheme { case "https": case "http": - if !net.IsLoopback(parsed.Hostname()) { + if !netx.IsLoopback(parsed.Hostname()) { return NewError( ErrInvalidClient, WithDescription("client metadata document contains invalid redirect_uri"), @@ -255,47 +255,6 @@ func validateCIMDRedirectURI(redirectURI string) error { return nil } -func cimdRedirectURIAllowed(doc *ClientMetadataDocument, redirectURI string) bool { - for _, allowed := range doc.RedirectURIs { - if redirectURI == allowed { - return true - } - - if cimdLoopbackRedirectMatches(allowed, redirectURI) { - return true - } - } - - return false -} - -func cimdLoopbackRedirectMatches(registered, requested string) bool { - registeredURL, err := url.Parse(registered) - if err != nil { - return false - } - - requestedURL, err := url.Parse(requested) - if err != nil { - return false - } - - if registeredURL.Scheme != requestedURL.Scheme { - return false - } - - if !net.IsLoopback(registeredURL.Hostname()) || !net.IsLoopback(requestedURL.Hostname()) { - return false - } - - if registeredURL.Hostname() != requestedURL.Hostname() { - return false - } - - return registeredURL.Path == requestedURL.Path && - registeredURL.RawQuery == requestedURL.RawQuery -} - func (f *cimdFetcher) loadCache(clientIDURL string) (*ClientMetadataDocument, bool) { raw, ok := f.cache.Load(clientIDURL) if !ok { @@ -338,7 +297,6 @@ func (s *Service) resolveClient( ctx context.Context, tx pg.Tx, clientIDRaw string, - redirectURI string, ) (*coredata.OAuth2Client, error) { if clientID, err := gid.ParseGID(clientIDRaw); err == nil { if tx != nil { @@ -373,10 +331,6 @@ func (s *Service) resolveClient( return nil, err } - if redirectURI != "" && !cimdRedirectURIAllowed(doc, redirectURI) { - return nil, ErrInvalidRedirectURI - } - client, err := s.upsertCIMDClient(ctx, tx, clientIDRaw, doc) if err != nil { return nil, err diff --git a/pkg/iam/oauth2/cimd_test.go b/pkg/iam/oauth2/cimd_test.go index 3b8ba1a07..3847d6d60 100644 --- a/pkg/iam/oauth2/cimd_test.go +++ b/pkg/iam/oauth2/cimd_test.go @@ -141,30 +141,6 @@ func TestValidateClientMetadataDocument(t *testing.T) { ) } -func TestCIMDRedirectURIAllowed(t *testing.T) { - t.Parallel() - - doc := &ClientMetadataDocument{ - RedirectURIs: []string{ - "https://chatgpt.com/connector/oauth/callback", - "http://127.0.0.1:53682/callback", - }, - } - - assert.True( - t, - cimdRedirectURIAllowed(doc, "https://chatgpt.com/connector/oauth/callback"), - ) - assert.True( - t, - cimdRedirectURIAllowed(doc, "http://127.0.0.1:8080/callback"), - ) - assert.False( - t, - cimdRedirectURIAllowed(doc, "https://evil.example/callback"), - ) -} - func TestCIMDFetcherFetch(t *testing.T) { t.Parallel() diff --git a/pkg/iam/oauth2/service.go b/pkg/iam/oauth2/service.go index d7262d254..77ee03662 100644 --- a/pkg/iam/oauth2/service.go +++ b/pkg/iam/oauth2/service.go @@ -32,7 +32,7 @@ import ( "go.probo.inc/probo/pkg/crypto/rand" "go.probo.inc/probo/pkg/gid" "go.probo.inc/probo/pkg/iam/oauth2scope" - "go.probo.inc/probo/pkg/net" + "go.probo.inc/probo/pkg/netx" "go.probo.inc/probo/pkg/page" "go.probo.inc/probo/pkg/uri" ) @@ -1060,7 +1060,7 @@ func (s *Service) RegisterClient( } case coredata.OAuth2ClientVisibilityPrivate: if parsed.Scheme == "http" { - if !net.IsLoopback(parsed.Hostname()) { + if !netx.IsLoopback(parsed.Hostname()) { return gid.Nil, "", NewError( @@ -1433,13 +1433,11 @@ func (s *Service) Authorize( if err := s.pg.WithTx( ctx, func(ctx context.Context, tx pg.Tx) error { - client, err := s.resolveClient(ctx, tx, req.ClientIDRaw, req.RedirectURI) + client, err := s.resolveClient(ctx, tx, req.ClientIDRaw) if err != nil { return err } - fmt.Printf("X: %+v\n", client) - if !client.IsRedirectURIAllowed(req.RedirectURI) { return ErrInvalidRedirectURI } @@ -1738,7 +1736,7 @@ func (s *Service) AuthenticateClient( clientIDRaw string, clientSecret string, ) (*coredata.OAuth2Client, error) { - client, err := s.resolveClient(ctx, nil, clientIDRaw, "") + client, err := s.resolveClient(ctx, nil, clientIDRaw) if err != nil { return nil, err } diff --git a/pkg/net/net.go b/pkg/netx/netx.go similarity index 98% rename from pkg/net/net.go rename to pkg/netx/netx.go index 20524d0b7..7e7539c53 100644 --- a/pkg/net/net.go +++ b/pkg/netx/netx.go @@ -12,7 +12,7 @@ // OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. -package net +package netx import "net" diff --git a/pkg/net/net_test.go b/pkg/netx/netx_test.go similarity index 96% rename from pkg/net/net_test.go rename to pkg/netx/netx_test.go index 5f0850e9f..28d5fd9eb 100644 --- a/pkg/net/net_test.go +++ b/pkg/netx/netx_test.go @@ -12,13 +12,13 @@ // OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. -package net_test +package netx_test import ( "testing" "github.com/stretchr/testify/assert" - "go.probo.inc/probo/pkg/net" + "go.probo.inc/probo/pkg/netx" ) func TestIsLoopback(t *testing.T) { @@ -77,7 +77,7 @@ func TestIsLoopback(t *testing.T) { func(t *testing.T) { t.Parallel() - assert.Equal(t, tt.want, net.IsLoopback(tt.host)) + assert.Equal(t, tt.want, netx.IsLoopback(tt.host)) }, ) }