internal/oauthex: address URL attacks in PRM#539
internal/oauthex: address URL attacks in PRM#539jba merged 1 commit intomodelcontextprotocol:mainfrom
Conversation
Fix Protected Resource Metadata to prevent URL attacks, as described in the issue. For modelcontextprotocol#526
findleyr
left a comment
findleyr
left a comment
There was a problem hiding this comment.
This LGTM (though I have only superficial knowledge of the problem).
However, changes like this should include a test.
rolandshoemaker
left a comment
rolandshoemaker
left a comment
There was a problem hiding this comment.
Approved with one documentation comment, should probably also have a test.
| return err | ||
| } | ||
| scheme := strings.ToLower(uu.Scheme) | ||
| if scheme == "javascript" || scheme == "data" || scheme == "vbscript" { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.}
There was a problem hiding this comment.
Seems fine with me. I think we should document that somewhere though.
|
Can we land this today? Would be good to include in the next release (planned soon). |
|
I merged this, but the comments should be addressed in a follow up. @samthanawalla |
5 participants
Fix Protected Resource Metadata to prevent URL attacks, as described in the issue.
For #526