fix: open redirect vulnerability in completeAuthorization #92
Conversation
🦋 Changeset detectedLatest commit: b845bf0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
b0aec94 to
1b4fa4f
Compare
|
This validation is reasonable to do as a defensive measure, but note that this doesn't really solve any "open redirect" issue because an attacker can just use dynamic client registration to register a client with whatever redirect_uri they want. Moreover, it should not be possible for an attacker to inject a bogus request_uri between |
This is true. If the authorization server allows open, unauthenticated Dynamic Client Registration, then an attacker doesn't need to exploit a flaw in an existing client; they can just create their own malicious client. However, I believe this is a separate and distinct vulnerability. The threat is not an open redirect, but rather an "unvalidated client registration" flow. I'm happy to discuss how this can be fixed and make relevant changes in my PR.
The fix in this PR is still necessary because it protects against an attacker hijacking the session of a legitimate, already-registered client. The two fixes address two different attack vectors. In a perfect world, the application developer would handle the I've commonly seen the following implementation where the application serializes the entire const res = await getAuthorizationURL({
client_id: c.env.CLIENT_ID,
redirect_uri: new URL('/oauth/callback', c.req.url).href,
state: oauthReqInfo,
scopes,
})Then, it trusts the const oauthReqInfo = AuthRequestSchemaWithExtraParams.parse(JSON.parse(atob(state)))
// Get the oathReqInfo out of KV
if (!oauthReqInfo.clientId) {
throw new McpError('Invalid State', 400)
}
// Return back to the MCP client a new token
const { redirectTo } = await c.env.OAUTH_PROVIDER.completeAuthorization({
request: oauthReqInfo,
userId: user.id,
metadata: {
label: user.email,
},
scope: oauthReqInfo.scope,
props: {
type: 'user_token',
user,
accounts,
accessToken,
refreshToken,
} satisfies AuthProps,
})The old API assumed the developer would do the right thing. If they made a mistake (like misusing the state parameter), the application became vulnerable. The new API is defensive. It does not trust its caller. By re-validating the |
|
Hi, as you both correctly discussed as long as you can arbitrarily register a client, the mitigation won't help against the phishing issue. That's why I was also asking to do csrf checking on the application/client side as well. Preventing these kind of phishing attacks where a victim just copy/pastes an URL and it directly causes an action in the logged in env. I agree that the suggested fix would still improve the current situation. Moreover, I'd like to see some recommendations / sample client usage that users/customers could refer to. |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @cloudflare/workers-oauth-provider@0.0.12 ### Patch Changes - [#92](#92) [`5a59d78`](5a59d78) Thanks [@roerohan](https://github.com/roerohan)! - fix: open redirect vulnerability in completeAuthorization Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
#96 adds a helper method to allow users of the library to validate a client before it gets registered |
Summary of Fix
This change addresses an open redirect vulnerability in the
completeAuthorizationmethod.The Vulnerability
The
completeAuthorizationmethod previously trusted theredirect_uriprovided within theAuthRequestobject without re-validating it. An application using this library could inadvertently pass anAuthRequestobject constructed from untrusted data (e.g., a state parameter from the URL). We were trusting the implementor of the library to ensure that a validatedredirect_uriis passed incompleteAuthorization.For example, if the implementor stores the
redirect_uriin the "?code=...&state=..." URL string, it gives an attacker a vector to modify theredirect_uri. An attacker can craft a malicious authorization link with a modified state parameter containing an attacker-controlledredirect_uri. If a victim authenticated through this link, the authorization code would be sent to the attacker's server, leading to a potential account takeover.The Solution
This fix adds a validation block at the beginning of the
completeAuthorizationmethod. Before any grant is created, the method now:clientIdfrom the request.redirectUrifrom the request is present in the client's list of registeredredirectUris.This change ensures that the
redirect_uriis always validated before use, making the library secure by default and protecting all downstream applications from this vulnerability.