-
Notifications
You must be signed in to change notification settings - Fork 154
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
fix: oauth2 implicit grant #2510
Conversation
add to the authentification web url : - clientId with the document id - response_type to token - state to prevent CSRF attack but not enable for the moment
src/main/redux/sagas/auth.ts
Outdated
// see also the callback response : it must use the id query parameter to indicate the Authentication Document identifier | ||
// https://drafts.opds.io/authentication-for-opds-1.0#342-a-shared-client-identifier:~:text=it%20must%20use%20the%20id%20query%20parameter%20to%20indicate%20the%20Authentication%20Document%20identifier | ||
|
||
browserUrlParsed.searchParams.set("client_id", encodeURIComponent_RFC3986(auth?.id || "http://opds-spec.org/auth/client")); |
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 think using the auth?.id
here is incorrect. The id
specified in the OPDS Authentication Document is the unique identifier of the document itself, not the id for the client to use when authenticating.
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.
From the documentation:
Example of an OPDS callback containing an Access Token
opds://authorize/?id=http%3A%2F%2Fexample.org%2Fauth.json&access_token=9b3dc428-df5f-4bd2-9f0d-72497cbf8464&token_type=bearer
Using the example as a guide, the OAuth Server should return an opds://authorize
redirect after successful authentication should include a querystring parameter with the opds document id in it.
if (data.state !== getImplicitNonceForImplicitAuthentication()) { | ||
debug("received state parameter is not equal to the nonce sent", "value=", data.state); | ||
debug("the state verification is IGNORED and bypassed to ensure a legacy compatibility with all OPDS OAUTH2 server"); | ||
// TODO: improve this and enable the state verification |
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 think a possibility for enabling backwards-compatible behaviour would be to only ignore the state
nonce if it is missing in the response from the OAuth server. Something like:
// https://auth0.com/docs/secure/attack-protection/state-parameters
if (data.state) {
if (data.state === getImplicitNonceForImplicitAuthentication()) {
debug("State parameter was included in callback response and is VERIFIED and CORRECT");
}
else {
// Todo: Reject the response, user is potentially compromised
debug("received state parameter is not equal to the nonce sent", "value=", data.state);
}
}
else {
debug("State verification missing in response - verification checks IGNORED and bypassed to ensure legacy compatibility with all OPDS OAUTH2 server");
}
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.
Thinking about it a little more, this probably doesn't provide much additional security. If a user's connection is compromised, a malicious actor could just omit the state
parameter in the crafted response and the check would be bypassed.
* Changes to OAuth 2.0 Implicit Grant Flow handling: * Not using the OPDS Authentication Document's Id as the client_id, using "http://opds-spec.org/auth/client" in all cases * OPDS Authentication Document Id is validated against the id returned in the callback_url (validation skipped if missing for backwards compatability) * Basic OPDS Authentication Document validation * Debug messaging improvements to provide developers with context about why requests are rejected * Documentation of OAuth parameters and any workarounds for OPDS Authentication/OAuth specification conflicts * Allowing misconfigured opds authentication documents when they are missing non-essential data
I'm merging now, let's continue the discussion in several separate issues. Many Thanks @mpdunlop |
add parameter to the authentification web url :
Fixes #2506