Restrict OAuth client branding URLs to http(s)
CIMD and registration accepted any URI scheme for client_uri and logo_uri, so allowlisted metadata could surface javascript: links on sign-in. Validate absolute http/https at ingest and only expose those schemes in branding. Signed-off-by: Bryan Frimin <bryan@probo.com>
This commit is contained in:
@@ -647,12 +647,26 @@ func NewCIMDClient(
|
|||||||
}
|
}
|
||||||
|
|
||||||
if logoURI != nil && *logoURI != "" {
|
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
|
client.LogoURI = &u
|
||||||
}
|
}
|
||||||
|
|
||||||
if clientURI != nil && *clientURI != "" {
|
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
|
client.ClientURI = &u
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -22,8 +22,10 @@ package coredata_test
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"testing"
|
"testing"
|
||||||
|
"time"
|
||||||
|
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
|
"github.com/stretchr/testify/require"
|
||||||
"go.probo.inc/probo/pkg/coredata"
|
"go.probo.inc/probo/pkg/coredata"
|
||||||
"go.probo.inc/probo/pkg/uri"
|
"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)
|
||||||
|
},
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|||||||
@@ -40,6 +40,7 @@ import (
|
|||||||
"go.probo.inc/probo/pkg/coredata"
|
"go.probo.inc/probo/pkg/coredata"
|
||||||
"go.probo.inc/probo/pkg/gid"
|
"go.probo.inc/probo/pkg/gid"
|
||||||
"go.probo.inc/probo/pkg/netx"
|
"go.probo.inc/probo/pkg/netx"
|
||||||
|
"go.probo.inc/probo/pkg/uri"
|
||||||
)
|
)
|
||||||
|
|
||||||
const (
|
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
|
authMethod := doc.TokenEndpointAuthMethod
|
||||||
if authMethod == "" {
|
if authMethod == "" {
|
||||||
authMethod = string(coredata.OAuth2ClientTokenEndpointAuthMethodNone)
|
authMethod = string(coredata.OAuth2ClientTokenEndpointAuthMethodNone)
|
||||||
@@ -310,6 +319,22 @@ func validateCIMDRedirectURI(redirectURI string) error {
|
|||||||
return nil
|
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) {
|
func (f *cimdFetcher) loadCache(clientIDURL string) (*ClientMetadataDocument, bool) {
|
||||||
raw, ok := f.cache.Load(clientIDURL)
|
raw, ok := f.cache.Load(clientIDURL)
|
||||||
if !ok {
|
if !ok {
|
||||||
|
|||||||
@@ -159,6 +159,45 @@ func TestValidateClientMetadataDocument(t *testing.T) {
|
|||||||
require.Error(t, err)
|
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) {
|
func TestCIMDFetcherFetch(t *testing.T) {
|
||||||
|
|||||||
@@ -53,12 +53,14 @@ func ClientBrandingFromClient(client *coredata.OAuth2Client) *ClientBranding {
|
|||||||
Name: client.ClientName,
|
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()
|
clientURL := client.ClientURI.String()
|
||||||
branding.ClientURL = &clientURL
|
branding.ClientURL = &clientURL
|
||||||
}
|
}
|
||||||
|
|
||||||
if client.LogoURI != nil {
|
if client.LogoURI != nil && client.LogoURI.IsHTTP() {
|
||||||
logoURL := client.LogoURI.String()
|
logoURL := client.LogoURI.String()
|
||||||
branding.LogoURL = &logoURL
|
branding.LogoURL = &logoURL
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -28,6 +28,9 @@ import (
|
|||||||
func TestClientBrandingFromClient(t *testing.T) {
|
func TestClientBrandingFromClient(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
|
t.Run("exposes http and https client urls", func(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
clientURL := uri.URI("https://example.com")
|
clientURL := uri.URI("https://example.com")
|
||||||
logoURL := uri.URI("https://example.com/logo.png")
|
logoURL := uri.URI("https://example.com/logo.png")
|
||||||
|
|
||||||
@@ -43,6 +46,27 @@ func TestClientBrandingFromClient(t *testing.T) {
|
|||||||
assert.Equal(t, "Acme", branding.Name)
|
assert.Equal(t, "Acme", branding.Name)
|
||||||
assert.Equal(t, "https://example.com", *branding.ClientURL)
|
assert.Equal(t, "https://example.com", *branding.ClientURL)
|
||||||
assert.Equal(t, "https://example.com/logo.png", *branding.LogoURL)
|
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) {
|
func TestClientBranding_EmptyClientID(t *testing.T) {
|
||||||
|
|||||||
71
pkg/iam/oauth2/register_client_test.go
Normal file
71
pkg/iam/oauth2/register_client_test.go
Normal file
@@ -0,0 +1,71 @@
|
|||||||
|
// Copyright (c) 2026 Probo Inc <hello@probo.com>.
|
||||||
|
//
|
||||||
|
// 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)
|
||||||
|
},
|
||||||
|
)
|
||||||
|
}
|
||||||
@@ -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 (
|
var (
|
||||||
plaintextSecret string
|
plaintextSecret string
|
||||||
secretHash []byte
|
secretHash []byte
|
||||||
|
|||||||
@@ -43,6 +43,21 @@ func Parse(raw string) (URI, error) {
|
|||||||
|
|
||||||
func (u URI) String() string { return string(u) }
|
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 {
|
func (u *URI) UnmarshalText(text []byte) error {
|
||||||
parsed, err := Parse(string(text))
|
parsed, err := Parse(string(text))
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
@@ -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) {
|
func TestURIUnmarshalText(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user