-
Notifications
You must be signed in to change notification settings - Fork 109
feat: added helper methods for storing and retrieving "AuthRequest" objects in KV #95
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?
Conversation
🦋 Changeset detectedLatest commit: b313a3e 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: |
kentonv
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 don't think we should add this.
The problem that this feature is trying to solve is a problem that arises when acting as an OAuth client: how to protect the state value from forgery. But this library is not meant to be used by OAuth clients, it is meant to be used by OAuth servers. It just so happens that a common use case for this library is a type of application that acts as a sort of "OAuth proxy", such that it is both a client and a server. And such proxies very commonly need to stash the auth request somewhere, using the state to look it up again later, hence this PR is trying to offer a way.
But securely storing values associated with the state file in the upstream request is clearly an OAuth client responsibility, not an OAuth server responsibility. I don't think it makes sense for workers-oauth-provider, which is an OAuth server library, to be trying to assist with client responsibilities. Instead, people should be using some sort of OAuth client library for that.
More concretely, one problem with the solution proposed in this PR is that it can only store an authRequest. But depending on the application, that might not be the only thing they want to put in their "state". What if they have additional metadata they need to stash? Now they have to go build their own storage anyway. We could solve this by extending storeAuthRequest() to a more general storeStuff() that lets you store any arbitrary metadata, but then it becomes obvious that this has nothing to do with workers-oauth-provider and doesn't really belong in this library.
Another issue I have with this proposal is that it doesn't fully solve the security problem it's trying to help with. Since the KV storage is shared between the victim and the attacker, it's possible for an attacker to initiate a request, thus getting their authRequest stored into KV, and then trick the victim into continuing with their token.
To solve this, applications should really store this metadata in the client's browser storage (cookies, localStorage, etc.). That way, an attacker cannot inject their own value at all. (The state token should still be a random string which is also stored in local storage, so that if the flow comes back with a mismatching state token, the request can be rejected...)
|
I agree that the core responsibility for securely storing and validating the state parameter lies with the OAuth client, not the server. You’re right that by handling this inside the provider library, we blur that boundary and risk encouraging insecure or misleading usage patterns. The motivation behind this PR was mainly to simplify things for proxy-style applications (where the same app acts as both client and server), but I see how that use case probably falls outside the intended scope of My thinking was that such applications could store the auth request in KV, while using PKCE to store the That said, I agree that this pattern is probably better handled by applications themselves rather than baked into the library. I think it makes sense to close this PR and instead document best practices for “OAuth proxy” setups - for example, explaining that such applications should handle state persistence themselves (e.g., via cookies, localStorage, or a client-managed store) and that If that sounds reasonable, I can draft some docs or an example to clarify this pattern instead. |
Description
This PR introduces two new helper methods,
storeAuthRequestandgetAuthRequest, to theOAuthHelpersobject. These methods provide developers with the ability to manually store and retrieveAuthRequestobjects from the KV store, offering greater flexibility in managing the authorization flow.This is particularly useful in scenarios where the authorization process is split across different contexts or invocations, allowing developers to persist and retrieve the authorization state as needed.
Example usage
Real world example