Skip to content

internal/oauthex: address URL attacks in PRM #539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 30, 2025
Merged

internal/oauthex: address URL attacks in PRM #539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions internal/oauthex/oauth2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,21 @@ func getJSON[T any](ctx context.Context, c *http.Client, url string, limit int64
}
return &t, nil
}

// checkURLScheme ensures that its argument is a valid URL with a scheme
// that prevents XSS attacks.
// See #526.
func checkURLScheme(u string) error {
if u == "" {
return nil
}
uu, err := url.Parse(u)
if err != nil {
return err
}
scheme := strings.ToLower(uu.Scheme)
if scheme == "javascript" || scheme == "data" || scheme == "vbscript" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that we can't just blanket block everything except http(s), because it's plausible that some servers may want to authenticate using a custom app protocol, but that is a little worrying, since it's not immediately clear to me that those couldn't similarly be used for attacks.

I see this is what other SDKs have done though, so perhaps this is the best option we have. There should at least be a comment here explaining why we block these particular schemes, and why we cannot just only allow http(s).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the list of schemes from https://github.com/modelcontextprotocol/typescript-sdk/pull/877/files#diff-a0fe23c86bd448c6fa4c60fe4d380daa65451fea86a9e760673a5a7c65292e88, line 20. There's no explanation in that PR for where that list comes from. Clearly, they are all suspect, but I don't know if there are other suspect schemes as well. However, the article you cited, https://verialabs.com/blog/from-mcp-to-shell, seems to think that the above PR is sufficient. (See the section called the most impactful fix.}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine with me. I think we should document that somewhere though.

return fmt.Errorf("URL has disallowed scheme %q", scheme)
}
return nil
}
14 changes: 10 additions & 4 deletions internal/oauthex/resource_meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,18 +159,24 @@ func GetProtectedResourceMetadataFromHeader(ctx context.Context, header http.Hea

// getPRM makes a GET request to the given URL, and validates the response.
// As part of the validation, it compares the returned resource field to wantResource.
func getPRM(ctx context.Context, url string, c *http.Client, wantResource string) (*ProtectedResourceMetadata, error) {
if !strings.HasPrefix(strings.ToUpper(url), "HTTPS://") {
return nil, fmt.Errorf("resource URL %q does not use HTTPS", url)
func getPRM(ctx context.Context, purl string, c *http.Client, wantResource string) (*ProtectedResourceMetadata, error) {
if !strings.HasPrefix(strings.ToUpper(purl), "HTTPS://") {
return nil, fmt.Errorf("resource URL %q does not use HTTPS", purl)
}
prm, err := getJSON[ProtectedResourceMetadata](ctx, c, url, 1<<20)
prm, err := getJSON[ProtectedResourceMetadata](ctx, c, purl, 1<<20)
if err != nil {
return nil, err
}
// Validate the Resource field to thwart impersonation attacks (section 3.3).
if prm.Resource != wantResource {
return nil, fmt.Errorf("got metadata resource %q, want %q", prm.Resource, wantResource)
}
// Validate the authorization server URLs to prevent XSS attacks (see #526).
for _, u := range prm.AuthorizationServers {
if err := checkURLScheme(u); err != nil {
return nil, err
}
}
return prm, nil
}

Expand Down
4 changes: 3 additions & 1 deletion oauthex/oauthex.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
// Package oauthex implements extensions to OAuth2.
package oauthex

import "github.com/modelcontextprotocol/go-sdk/internal/oauthex"
import (
"github.com/modelcontextprotocol/go-sdk/internal/oauthex"
)

// ProtectedResourceMetadata is the metadata for an OAuth 2.0 protected resource,
// as defined in section 2 of https://www.rfc-editor.org/rfc/rfc9728.html.
Expand Down