-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: Support WWW-Authenticate
scope
param for SEP-835
#983
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
base: main
Are you sure you want to change the base?
fix: Support WWW-Authenticate
scope
param for SEP-835
#983
Conversation
src/client/sse.ts
Outdated
if (response.status === 401 && response.headers.has('www-authenticate')) { | ||
this._resourceMetadataUrl = extractResourceMetadataUrl(response); | ||
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); | ||
this._resourceMetadataUrl = resourceMetadataUrl; | ||
this._scope = scope; | ||
} |
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.
this section of code only caches header params if the header exists (&& response.headers.has('www-authenticate')
). If the header does not exist, it would still use the same values cached from the previous www-authenticate
header.
Would it make more sense for it to clear the cached values if a new 401 is received without the header? It would mean simply removing the logic that checks for the header and instead let the extractWWWAuthenticateParams
handle it:
if (response.status === 401) {
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
this._resourceMetadataUrl = resourceMetadataUrl;
this._scope = scope;
}
In this case, if the header does not exist, it will reset both cached values to undefined
and fall back to the oauth metadata values.
I think there's also some server side work to be done here as well, as I am seeing this line of code conflict with the concept of step up: typescript-sdk/src/server/auth/handlers/authorize.ts Lines 127 to 132 in 856d9ec
|
I was thinking the step up concept is for access tokens, not clients. So if a client is limited to a certain set of scopes, it is still limited to them. I think this is for the case where I have a client that has |
The DCR spec does not mention that a client can't ask for more.
It says that it can use but it doesn't say it can't use any alternative value. As such, I think we remove this check from Also, your example makes sense until the MCP Server introduces new functionality that increases the list of all scopes to |
If that is the intended behavior then it seems like that block of code should be removed as there is no such thing as "allowed scopes". I can't speak for why it was included to begin with though. |
Yeah, I can't either. Just something @localden and I agreed to on Discord. |
0806e81
to
e338f1c
Compare
const allowedScopes = new Set(client.scope?.split(' ')); | ||
|
||
// Check each requested scope against allowed scopes | ||
for (const scope of requestedScopes) { | ||
if (!allowedScopes.has(scope)) { | ||
throw new InvalidScopeError(`Client was not registered with scope ${scope}`); | ||
} | ||
} |
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.
Removing this check in order to support the scope step up flow. Clients should be able to step up to scopes beyond the initial client registration scopes.
describe('Scope validation', () => { | ||
it('validates requested scopes against client registered scopes', async () => { | ||
const response = await supertest(app).get('/authorize').query({ | ||
client_id: 'valid-client', | ||
redirect_uri: 'https://example.com/callback', | ||
response_type: 'code', | ||
code_challenge: 'challenge123', | ||
code_challenge_method: 'S256', | ||
scope: 'profile email admin' // 'admin' not in client scopes | ||
}); | ||
|
||
expect(response.status).toBe(302); | ||
const location = new URL(response.header.location); | ||
expect(location.searchParams.get('error')).toBe('invalid_scope'); | ||
}); | ||
|
||
it('accepts valid scopes subset', async () => { | ||
const response = await supertest(app).get('/authorize').query({ | ||
client_id: 'valid-client', | ||
redirect_uri: 'https://example.com/callback', | ||
response_type: 'code', | ||
code_challenge: 'challenge123', | ||
code_challenge_method: 'S256', | ||
scope: 'profile' // subset of client scopes | ||
}); | ||
|
||
expect(response.status).toBe(302); | ||
const location = new URL(response.header.location); | ||
expect(location.searchParams.has('code')).toBe(true); | ||
}); | ||
}); |
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.
This updates the auth flow to prefer the
scope
param from theWWW-Authenticate
header if it exists.Motivation and Context
This is motivated by SEP-835. The current implementation defaults to the
scopes_supported
from the metadata file and ignores thescope
param from theWWW-Authenticate
header. SEP-835 says thescope
provided by theWWW-Authenticate
header should be preferred.How Has This Been Tested?
Local tests have been updated and are passing. This has not been tested in a real application.
Breaking Changes
No breaking changes.
Types of changes
Checklist