fix(oauth): three oauth discovery and registration issues #489
Conversation
github-actions
bot
added
T-core
| pub client_id: String, | ||
| pub client_secret: Option<String>, | ||
| pub client_name: String, | ||
| pub client_name: Option<String>, |
There was a problem hiding this comment.
It is recommended but not required to send client_name
https://datatracker.ietf.org/doc/html/rfc7591
| Ok(None) | ||
| } | ||
|
|
||
| async fn fetch_resource_metadata_url(&self) -> Result<Option<Url>, AuthError> { |
There was a problem hiding this comment.
gpeal
force-pushed
the
gpeal/www-authenticate-header
branch
from
ce3df3a to
73d4574
Compare
| Ok(token) => token, | ||
| Err(RequestTokenError::Parse(_, body)) => { | ||
| match serde_json::from_slice::<OAuthTokenResponse>(&body) { | ||
| Ok(parsed) => { |
There was a problem hiding this comment.
Parse errors can happen during successful token exchanges
https://github.com/ramosbugs/oauth2-rs/blob/main/src/error.rs#L125
gpeal
force-pushed
the
gpeal/www-authenticate-header
branch
from
73d4574 to
8704262
Compare
gpeal
force-pushed
the
gpeal/www-authenticate-header
branch
from
8704262 to
a415b2e
Compare
gpeal
changed the title
| /// fragment. Quoted values support escaped characters (e.g. `\"`). The parser skips leading | ||
| /// whitespace before reading either a quoted or token value. If no well-formed value is found, | ||
| /// `None` is returned. | ||
| fn parse_next_header_value(header_fragment: &str) -> Option<(String, usize)> { |
There was a problem hiding this comment.
I would do some revs on whether there are any valid header formats not handled by this parsing logic. It looks good to me for the formats I know about, but I asked our dear friend ChatGPT and it mentioned a few that might cause issues:
not sure the validity of these possibilities, and how widely used any of them would be in servers that matter, but worth doing a sanity check before merge.
I'll skip the first since it's deprecated and we'll see if any reports of popular MCPs report issues with it. I'll add support for semicolons because it's easy enough even though it's not a part of the spec. Let's punt on escaped/embedded commas in urls. Hopefully that isn't an issue in practice. |
gpeal
force-pushed
the
gpeal/www-authenticate-header
branch
from
a415b2e to
6a9c393
Compare
[Release notes](https://github.com/modelcontextprotocol/rust-sdk/releases) Notably, this picks up two of my PRs that have four separate fixes for oauth dynamic client registration and auth modelcontextprotocol/rust-sdk#489 modelcontextprotocol/rust-sdk#476
2 participants
Motivation and Context
While trying to get Supabase to login as part of openai/codex#5254, I encountered three separate oauth issues. Comments are inline
How Has This Been Tested?
Logged in to the Supabase MCP with Codex for the first time.
Sanity checked Linear, Notion, and Todoist and verified that they all still work.
Breaking Changes
None
Types of changes
Checklist