diff --git a/pkg/coredata/oauth2_client.go b/pkg/coredata/oauth2_client.go index c1e457a01..b33bab3e4 100644 --- a/pkg/coredata/oauth2_client.go +++ b/pkg/coredata/oauth2_client.go @@ -647,12 +647,26 @@ func NewCIMDClient( } if logoURI != nil && *logoURI != "" { - u := uri.URI(*logoURI) + u, err := uri.Parse(*logoURI) + if err != nil { + return nil, fmt.Errorf("cannot parse logo_uri: %w", err) + } + if !u.IsHTTP() { + return nil, fmt.Errorf("logo_uri must be an absolute http or https URL") + } + client.LogoURI = &u } if clientURI != nil && *clientURI != "" { - u := uri.URI(*clientURI) + u, err := uri.Parse(*clientURI) + if err != nil { + return nil, fmt.Errorf("cannot parse client_uri: %w", err) + } + if !u.IsHTTP() { + return nil, fmt.Errorf("client_uri must be an absolute http or https URL") + } + client.ClientURI = &u } diff --git a/pkg/coredata/oauth2_client_test.go b/pkg/coredata/oauth2_client_test.go index b7868108d..4477f9077 100644 --- a/pkg/coredata/oauth2_client_test.go +++ b/pkg/coredata/oauth2_client_test.go @@ -22,8 +22,10 @@ package coredata_test import ( "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.probo.inc/probo/pkg/coredata" "go.probo.inc/probo/pkg/uri" ) @@ -100,3 +102,72 @@ func TestOAuth2Client_IsRedirectURIAllowed(t *testing.T) { ) } } + +func TestNewCIMDClient_WebURIs(t *testing.T) { + t.Parallel() + + t.Run( + "accepts https client_uri and logo_uri", + func(t *testing.T) { + t.Parallel() + + clientURI := "https://mcp.example.com" + logoURI := "https://mcp.example.com/logo.png" + + client, err := coredata.NewCIMDClient( + "https://mcp.example.com/oauth/metadata.json", + "Example MCP", + []string{"https://mcp.example.com/callback"}, + nil, + &logoURI, + &clientURI, + time.Now(), + ) + require.NoError(t, err) + require.NotNil(t, client.ClientURI) + require.NotNil(t, client.LogoURI) + assert.Equal(t, "https://mcp.example.com", client.ClientURI.String()) + assert.Equal(t, "https://mcp.example.com/logo.png", client.LogoURI.String()) + }, + ) + + t.Run( + "rejects non-web client_uri", + func(t *testing.T) { + t.Parallel() + + clientURI := "javascript://example.com/%0Aalert(1)" + + _, err := coredata.NewCIMDClient( + "https://mcp.example.com/oauth/metadata.json", + "Example MCP", + []string{"https://mcp.example.com/callback"}, + nil, + nil, + &clientURI, + time.Now(), + ) + require.Error(t, err) + }, + ) + + t.Run( + "rejects non-web logo_uri", + func(t *testing.T) { + t.Parallel() + + logoURI := "data://example.com/image" + + _, err := coredata.NewCIMDClient( + "https://mcp.example.com/oauth/metadata.json", + "Example MCP", + []string{"https://mcp.example.com/callback"}, + nil, + &logoURI, + nil, + time.Now(), + ) + require.Error(t, err) + }, + ) +} diff --git a/pkg/iam/oauth2/cimd.go b/pkg/iam/oauth2/cimd.go index e1099b08a..f2bae7f8f 100644 --- a/pkg/iam/oauth2/cimd.go +++ b/pkg/iam/oauth2/cimd.go @@ -40,6 +40,7 @@ import ( "go.probo.inc/probo/pkg/coredata" "go.probo.inc/probo/pkg/gid" "go.probo.inc/probo/pkg/netx" + "go.probo.inc/probo/pkg/uri" ) const ( @@ -257,6 +258,14 @@ func validateClientMetadataDocument(clientIDURL string, doc *ClientMetadataDocum } } + if err := validateCIMDWebURI(doc.ClientURI, "client_uri"); err != nil { + return err + } + + if err := validateCIMDWebURI(doc.LogoURI, "logo_uri"); err != nil { + return err + } + authMethod := doc.TokenEndpointAuthMethod if authMethod == "" { authMethod = string(coredata.OAuth2ClientTokenEndpointAuthMethodNone) @@ -310,6 +319,22 @@ func validateCIMDRedirectURI(redirectURI string) error { return nil } +func validateCIMDWebURI(raw, field string) error { + if raw == "" { + return nil + } + + parsed, err := uri.Parse(raw) + if err != nil || !parsed.IsHTTP() { + return NewError( + ErrInvalidClient, + WithDescription("client metadata document contains invalid "+field), + ) + } + + return nil +} + func (f *cimdFetcher) loadCache(clientIDURL string) (*ClientMetadataDocument, bool) { raw, ok := f.cache.Load(clientIDURL) if !ok { diff --git a/pkg/iam/oauth2/cimd_test.go b/pkg/iam/oauth2/cimd_test.go index 2c565c3ca..eda41f39d 100644 --- a/pkg/iam/oauth2/cimd_test.go +++ b/pkg/iam/oauth2/cimd_test.go @@ -159,6 +159,45 @@ func TestValidateClientMetadataDocument(t *testing.T) { require.Error(t, err) }, ) + + t.Run( + "https client_uri and logo_uri allowed", + func(t *testing.T) { + t.Parallel() + + good := doc + good.ClientURI = "https://mcp.example.com" + good.LogoURI = "https://mcp.example.com/logo.png" + + require.NoError(t, validateClientMetadataDocument(clientID, &good)) + }, + ) + + t.Run( + "non-web client_uri rejected", + func(t *testing.T) { + t.Parallel() + + bad := doc + bad.ClientURI = "javascript://example.com/%0Aalert(1)" + + err := validateClientMetadataDocument(clientID, &bad) + require.Error(t, err) + }, + ) + + t.Run( + "non-web logo_uri rejected", + func(t *testing.T) { + t.Parallel() + + bad := doc + bad.LogoURI = "data://example.com/image" + + err := validateClientMetadataDocument(clientID, &bad) + require.Error(t, err) + }, + ) } func TestCIMDFetcherFetch(t *testing.T) { diff --git a/pkg/iam/oauth2/client_branding.go b/pkg/iam/oauth2/client_branding.go index 013d2df57..42ea10bb8 100644 --- a/pkg/iam/oauth2/client_branding.go +++ b/pkg/iam/oauth2/client_branding.go @@ -53,12 +53,14 @@ func ClientBrandingFromClient(client *coredata.OAuth2Client) *ClientBranding { Name: client.ClientName, } - if client.ClientURI != nil { + // Only expose absolute http(s) URLs. Metadata may historically contain + // non-web schemes; branding must not surface those as links or image src. + if client.ClientURI != nil && client.ClientURI.IsHTTP() { clientURL := client.ClientURI.String() branding.ClientURL = &clientURL } - if client.LogoURI != nil { + if client.LogoURI != nil && client.LogoURI.IsHTTP() { logoURL := client.LogoURI.String() branding.LogoURL = &logoURL } diff --git a/pkg/iam/oauth2/client_branding_test.go b/pkg/iam/oauth2/client_branding_test.go index 2c39f0403..a4a5f776b 100644 --- a/pkg/iam/oauth2/client_branding_test.go +++ b/pkg/iam/oauth2/client_branding_test.go @@ -28,21 +28,45 @@ import ( func TestClientBrandingFromClient(t *testing.T) { t.Parallel() - clientURL := uri.URI("https://example.com") - logoURL := uri.URI("https://example.com/logo.png") + t.Run("exposes http and https client urls", func(t *testing.T) { + t.Parallel() - branding := ClientBrandingFromClient( - &coredata.OAuth2Client{ - ClientName: "Acme", - ClientURI: &clientURL, - LogoURI: &logoURL, - }, - ) + clientURL := uri.URI("https://example.com") + logoURL := uri.URI("https://example.com/logo.png") - require.NotNil(t, branding) - assert.Equal(t, "Acme", branding.Name) - assert.Equal(t, "https://example.com", *branding.ClientURL) - assert.Equal(t, "https://example.com/logo.png", *branding.LogoURL) + branding := ClientBrandingFromClient( + &coredata.OAuth2Client{ + ClientName: "Acme", + ClientURI: &clientURL, + LogoURI: &logoURL, + }, + ) + + require.NotNil(t, branding) + assert.Equal(t, "Acme", branding.Name) + assert.Equal(t, "https://example.com", *branding.ClientURL) + assert.Equal(t, "https://example.com/logo.png", *branding.LogoURL) + }) + + t.Run("omits non-web client and logo urls", func(t *testing.T) { + t.Parallel() + + clientURL := uri.URI("javascript://example.com/%0Aalert(1)") + logoURL := uri.URI("data://example.com/image") + + branding := ClientBrandingFromClient( + &coredata.OAuth2Client{ + ClientName: "Acme", + ClientURI: &clientURL, + LogoURI: &logoURL, + }, + ) + + require.NotNil(t, branding) + assert.Equal(t, "Acme", branding.Name) + assert.Nil(t, branding.ClientURL) + assert.Nil(t, branding.LogoURL) + }) } func TestClientBranding_EmptyClientID(t *testing.T) { diff --git a/pkg/iam/oauth2/register_client_test.go b/pkg/iam/oauth2/register_client_test.go new file mode 100644 index 000000000..8967c9f1f --- /dev/null +++ b/pkg/iam/oauth2/register_client_test.go @@ -0,0 +1,71 @@ +// 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 oauth2 + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "go.gearno.de/kit/log" + "go.probo.inc/probo/pkg/coredata" + "go.probo.inc/probo/pkg/uri" +) + +func TestRegisterClient_RejectsNonWebURIs(t *testing.T) { + t.Parallel() + + svc := NewService(nil, nil, "", log.NewLogger()) + + t.Run( + "non-web client_uri", + func(t *testing.T) { + t.Parallel() + + clientURI := uri.URI("javascript://example.com/%0Aalert(1)") + + _, _, err := svc.RegisterClient( + context.Background(), + &RegisterClientRequest{ + ClientName: "Acme", + Visibility: coredata.OAuth2ClientVisibilityPublic, + RedirectURIs: []uri.URI{"https://example.com/callback"}, + ClientURI: &clientURI, + }, + ) + require.Error(t, err) + }, + ) + + t.Run( + "non-web logo_uri", + func(t *testing.T) { + t.Parallel() + + logoURI := uri.URI("data://example.com/image") + + _, _, err := svc.RegisterClient( + context.Background(), + &RegisterClientRequest{ + ClientName: "Acme", + Visibility: coredata.OAuth2ClientVisibilityPublic, + RedirectURIs: []uri.URI{"https://example.com/callback"}, + LogoURI: &logoURI, + }, + ) + require.Error(t, err) + }, + ) +} diff --git a/pkg/iam/oauth2/service.go b/pkg/iam/oauth2/service.go index 13f242708..ca12d7a6d 100644 --- a/pkg/iam/oauth2/service.go +++ b/pkg/iam/oauth2/service.go @@ -1103,6 +1103,24 @@ func (s *Service) RegisterClient( } } + if req.ClientURI != nil && !req.ClientURI.IsHTTP() { + return gid.Nil, + "", + NewError( + ErrInvalidRequest, + WithDescription("client_uri must be an absolute http or https URL"), + ) + } + + if req.LogoURI != nil && !req.LogoURI.IsHTTP() { + return gid.Nil, + "", + NewError( + ErrInvalidRequest, + WithDescription("logo_uri must be an absolute http or https URL"), + ) + } + var ( plaintextSecret string secretHash []byte diff --git a/pkg/uri/uri.go b/pkg/uri/uri.go index 003cd2642..977e18054 100644 --- a/pkg/uri/uri.go +++ b/pkg/uri/uri.go @@ -43,6 +43,21 @@ func Parse(raw string) (URI, error) { func (u URI) String() string { return string(u) } +// IsHTTP reports whether u is an absolute http or https URL. +func (u URI) IsHTTP() bool { + parsed, err := url.Parse(string(u)) + if err != nil || parsed.Host == "" { + return false + } + + switch parsed.Scheme { + case "http", "https": + return true + default: + return false + } +} + func (u *URI) UnmarshalText(text []byte) error { parsed, err := Parse(string(text)) if err != nil { diff --git a/pkg/uri/uri_test.go b/pkg/uri/uri_test.go index eaa4700f2..c0e963347 100644 --- a/pkg/uri/uri_test.go +++ b/pkg/uri/uri_test.go @@ -113,6 +113,32 @@ func TestParse(t *testing.T) { } } +func TestURIIsHTTP(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + uri URI + want bool + }{ + {name: "https", uri: URI("https://example.com"), want: true}, + {name: "http", uri: URI("http://localhost:3000"), want: true}, + {name: "custom scheme", uri: URI("myapp://callback"), want: false}, + {name: "javascript scheme", uri: URI("javascript://example.com/%0Aalert(1)"), want: false}, + } + + for _, tt := range tests { + t.Run( + tt.name, + func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, tt.uri.IsHTTP()) + }, + ) + } +} + func TestURIUnmarshalText(t *testing.T) { t.Parallel()