Respect incoming returnToUrl from query string
Closed this issue · 21 comments
After nodeSolidServer/node-solid-server#648, node-solid-server will send the returnToUrl through a query string rather than using sessions (here's why).
While oidc-auth-manager initially “sees” this query string, it gets lost on the way and is no longer present as redirect_uri later on.
Tell me more - what's the behavior you'd like to see?
So src/handlers/select-provider-request.js will receive returnToUrl through req.query.returnToUrl, and needs to preserve that on redirect to providerAuthUrl.
authUrlFor will need to look into req.query.returnToUrl rather than this.session. I.e., authUrlFor is now implicitly using the key returnToUrl in this.session as a contract.
Ahh I think I see. K!
This I gather is the issue where currently you can go to (eg) https://timbl.com/timbl/Public/Test/2018/RWWCrew%20only/Meeting%20on%20Apples/index.ttl#this go through the login popups, and end being redirected to https://timbl.com/timbl/Public/Test/2018/RWWCrew%20only/Meeting%20on%20Apples/index.ttl without the trailing #this . This bug has been around for a long time. longer than this issue...
@RubenVerborgh Here's some more thoughts on the issue here.
Preventing malicious post-auth redirects is one of the core goals of the OIDC protocol. This is why any values in redirect_uri have to be pre-registered by the client. As in, when a Solid POD does a dynamic registration to the oidc IDP, it says "here is a valid list of redirect URIs that I expect". And the IDP checks against that list, and flags an error for an arbitrary value of redirect_uri.
So that's why the returnToUrl was being stored in session. Because there's no good way to pre-register all possible return-to URLs.
But I agree, at the moment this is breaking, because the server is not seeing the hash fragment, and so can't store it in the session. So I think the solution would be a modification of your proposal -- have the OIDC Manager look in req.query.returnToUrl, and store that in the session. (So that the post-auth callback can resume from there.)
when a Solid POD does a dynamic registration to the oidc IDP, it says "here is a valid list of redirect URIs that I expect". And the IDP checks against that list, and flags an error for an arbitrary value of redirect_uri.
And it can't do that at the point of redirection?
So that's why the returnToUrl was being stored in session.
Can we think of a scenario where someone can do something evil when returnToUrl is not stored in the session?
So I think the solution would be a modification of your proposal -- have the OIDC Manager look in req.query.returnToUrl, and store that in the session.
My main worry here is clean contracts: it should be clear how the URL is passed from A to B and how it can be maintained throughout the flow. Right now, the contract of it being in session was undocumented, and the redirect_uri values weren't really doing anything.
Also, I'd like to avoid using session as much as possible, because such global state makes things really difficult to reason about. Another option would be to send an encrypted version of redirect_uri as a URL parameter or a hidden HTML field, so the client cannot modify it (but at least the client maintains state then). But for my understanding, I'd like to first see a scenario where the client modifying it could indeed pose a problem.
I absolutely agree -- we should document and clarify the contract (and the usage of session).
As for why session usage is required at all (why not use redirect_uri) - the OAuth 2.0 Threat Model and Security Considerations doc can shed some light on it:
- Section 3.5 - Redirect URI
- Threat: Open Redirectors on Client
- Section 5.2.3.5. - Validate Pre-Registered
redirect_uri
The TLDR; is -- flexible/open redirect_uri usage can lead to phishing & access tokens being stolen.
Here's a way to think about the contract / division of responsibilities. The redirect_uri is in charge of authenticating the client, and deals with secure handoff between zones of trust. So, it contains usually just one value, something like https://example.com/callback.
The session.returnToUrl has to do with -- after the OIDC workflow completes (and you're back at /callback), how to resume whatever URL within the zone of trust the user was trying to access. So that's more about UX / user convenience. Does that make more sense?
The only other way to do returnToUrl in OAuth2/OIDC without using an express session is to use the state parameter. Basically, the state parameter (which is kept through the OIDC redirect) is there for the client to implement a session-like mechanism (if they're not using session cookies or something).
So another option would be to encode (via JWT or something) the redirectToUrl into the state parameter. And the post-auth /callback handler can look inside that (instead of session.returnToUrl). I'm not quite clear that this method is preferable to just using session. What do you think?
Hi Dmitri,
Thanks for clarifying.
The redirect_uri is in charge of authenticating the client, and deals with secure handoff between zones of trust.
In the case of Solid, it can be any URL on the server though (including URLs with hashes, which caused this issue).
What we need is that /api/auth/select-provider only accepts a returnToUrl whose domain is equal to the current domain. E.g., https://abc/api/auth/select-provider?returnToUrl=… should validate that the returnToUrl value starts with https://abc/. Then we don't need session to store that URL on server A.
And then we just pass that URL to the OIDC endpoint on server B using the regular way (i.e., the JWS-encoded request URL parameter), and we're all good, right?
Yeah, that sounds good! (If I understand you correctly.)
The changes would be:
- We validate
returnToUrlin theselect-providerhandler, to make sure it's on this POD. - We pass the
returnToUrl(I'm assuming you meant encoded thestateparameter, yeah?) to the OIDC endpoint - We tweak the auth-callback-request handler to decode the
stateparam, and redirect the user to thereturnToUrl?
How it currently works, with a concrete example:
- I'm on https://drive.verborgh.org/ and click "Log in"
- I get redirected to https://drive.verborgh.org/common/popup.html, where I choose the custom IDP https://solid.community/
- The popup redirects me to https://solid.community/login?scope=openid&client_id=abc&response_type=id_token%20token&request=xyz, where xyz is the JWT-encoded version of:
{
"redirect_uri": "https://drive.verborgh.org/common/popup.html",
"display": "page",
"nonce": "blabla",
"key": {
"alg": "RS256",
"e": "AQAB",
"ext": true,
"key_ops": [
"verify"
],
"kty": "RSA",
"n": "blabla
}
}
We pass the returnToUrl (I'm assuming you meant the state parameter, yeah?) to the OIDC endpoint
So given the above, I did mean the request parameter.
Note that we don't do any registering as specified in https://tools.ietf.org/html/rfc6819#section-5.2.3.5. I.e., /common/popup.html isn't more special than any other URL.
Unless we want to make it special, but then we should register that URL when registering the client. And then we need to use state instead, so popup.html can read from there.
Quick question, before I go into explanation. Are you sure that in the example above, the "redirect_uri": property in the request param is "https://drive.verborgh.org/common/popup.html"? (As in, you decoded the JWS and that's what it was?)
Yes, I decoded it.
Ok, so in this particular case, popup.html is the Relying Party client. So it does actually dynamically register its own url with solid.community. If you clear the local storage & cookies, you should be able to observe the popup’s registration request.
So it does actually dynamically register its own url with solid.community.
From the client side then? Because the server doesn't know about popup.html.
But if that's the case, then any HTML page on the server can register itself with solid.community. So how meaningful is a registration then?
Also, note that authentication can and do happen from other pages, such as in nodeSolidServer/node-solid-server#571. Do they just all register themselves?
Do they just all register themselves?
Yes, any app (ie other pages) can dynamically register itself.
So how meaningful is a registration then?
Sure, that's a great question. :) So this is where we get into the disconnect between theory and current implementation.
In theory, the combination of dynamic registration + strict redirect_uri validation is still meaningful, when combined with the user consent screen. As in, the first time the user authenticates to that app, they are presented with an "Are you sure?" screen by the pod, which should include the pre-registered redirect uris so that the user can check that there's no dodgy redirects there. But beyond that first time, the server remembers the user's permission and continues the strict redirect_uri checking behind the scenes, and guards against phishing, malicious redirects etc.
In practice, the user consent page hasn't been implemented yet on Solid (we're skipping that step for various convenience reasons, but we should actually implement it at some point).
All that said, I still think that the right way (in terms of appropriate contract / division of responsibility) to solve this issue is to either a) Continue storing the returnToUrl in express session as currently, except fix it so that the hash fragment gets stored too (from the query param), or b) Encode the returnToUrl in the opaque state param, and have the client redirect the user after the auth flow ends.
As in, the first time the user authenticates to that app, they are presented with an "Are you sure?" screen by the pod, which should include the pre-registered redirect uris so that the user can check that there's no dodgy redirects there.
Good point—but does that make sense as long it's all on the same server? Because the session is shared across the whole server anyway.
a) Continue storing the returnToUrl in express session as currently, except fix it so that the hash fragment gets stored too (from the query param)
This would be my current preference. node-solid-server will be handing returnToUrl as a query parameter to /api/auth/select-provider (corresponding to SelectProviderRequest.get), which should extract it from there and verify the hostname is the same as the current host (and if not, block it).
does that make sense as long it's all on the same server? Because the session is shared across the whole server anyway.
Ahh, you're right. When we do end up implementing the consent screen, it should probably be skipped for apps on the same server.
This would be my current preference
Ok, copy that.
Implemented in #19.