-
Notifications
You must be signed in to change notification settings - Fork 62
feat(protectedResourceHandler): support more protected resource identifiers #84
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.
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
feat(protectedResourceHandler): support more protected resource identifiers #84
Conversation
|
Makes sense! I think adding the
Fair, but I think that field will ultimately always be a URL. So maybe we can change the docstring and leave the variable? |
|
Could add the optional params |
Ah yeah, I think we could just update the doc string. 👍
We could do that; however, I'm worried that makes it easier to use incorrectly. If we have the ability to set // app/.well-known/oauth-protected-resource/foo/route.ts
const handler = protectedResourceHandler({
resource: "/bar", // ← Doesn't match the URL ("/foo").
// Also, need to remember to use
// a relative path.
authServerUrls: [authServerUrl],
});For this reason, I think it's better if we can figure out
I was checking out this section of the RFC:
It says "between the host component and the path and/or query components". I suppose instead of the const resourceUrl = new URL(req.url);
resourceUrl.pathname = resourceUrl.pathname
.replace(/^\/\.well-known\/oauth-protected-resource/, "");This seems to be the inverse of the RFC specifies to do, and so should be robust enough? |
|
Makes sense, however—in that same section, the RFC also mentions that there can be custom
In this case the aforementioned pathname parsing wouldn't work. |
|
Good point. I think we have a couple options here. WDYT?
|
|
Yeah, there's always the workaround of using the |
…rotected-resource-identifiers
bc20e33 to
fce6a32
Compare
fce6a32 to
31359d2
Compare
allenzhou101
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda prefer the readability of this instead of the ternary but up to you:
let resource = resourceUrl.toString();
if (resourceUrl.pathname === '/') {
resource = resource.replace(/\/$/, '');
}
Assume I setup my MCP server at
https://example.com/api/mcp.I could consider all of
https://example.comto be my protected resource, as we currently do in the README.md. In this case, the following resource metadata URL, the returned resource identifier (resource), and the WWW-Authenticate header are correct:{ "resource": "https://example.com", "authorization_servers": ["https://authorization-server.com"] }However, IIUC RFC 9728, I could also consider
https://example.com/api/mcpto be the protected resource. In this case, the following resource metadata URL, the returned resource identifier (resource), and the WWW-Authenticate header would be correct:{ "resource": "https://example.com/api/mcp", "authorization_servers": ["https://authorization-server.com"] }In order to support this, we'd call
protectedResourceHandlerfromand update the way that the resource identifier (
resource) is computed.Instead of using
we could do
which should work for both cases:
req.urlAlso, I kept the variable name
resourceUrl; however, I don't think that's semantically correct:https://github.com/vercel/mcp-adapter/blob/592125c3c567213fdf5e5ca5cc7e7b1a090469e0/src/auth/auth-metadata.ts#L52
A resource server can host multiple protected resources, and so I think the variable would be better named
resourceand documented as something like this:If we agree, I could open a follow-up PR to address this.