Allow public clients (e.g. SPAs using implicit flow or PKCE) to have redirect URLs other than localhost #1822
Conversation
| } | ||
| } | ||
| // For non-public clients, only named RedirectURIs are allowed. | ||
| if !client.Public { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Our use case is to use PKCE for a web app that doesn't run on "localhost".
Therefore, we would like that RedirectURIs are also considered for public clients.
PKCE was definitely also intended for web apps, and you can already configure such web apps e.g. in Okta or Auth0.
The following code to allow any localhost URI, redirectURIOOB or deviceCallbackURI for public clients is still active.
An alternative could be to no longer allow these special URLs implicitly when RedirectURIs is non-empty.
I don't have a strong opinion if this would be better. The maintainers should decide.
There was a problem hiding this comment.
I think the implementation is good as it is. The alternative of not supporting the special URLs, when redirectURIs is defined, would break currently working configurations.
There was a problem hiding this comment.
I would personally prefer the logic to only allow whitelisted redirect uris, if the list is non-empty.
This would tighten the security and would not break existing configurations that do not use RedirectURIs for public clients.
And I bet the most configurations that do currently use it for public clients operate under the false assumption that the list is actually respected (I know we did in our deployment for some time).
There was a problem hiding this comment.
I fear that there are configurations that may be working by accident, because currently it is possible to have the RedirectURIs wrongly configured, and it still works.
Let's see what the maintainers decide.
There was a problem hiding this comment.
I think we can fix that by properly documenting this change or mentioning it in the change log.
There was a problem hiding this comment.
I would personally prefer the logic to only allow whitelisted redirect uris, if the list is non-empty.
@tkleczek AFAICT this is not the case now, but the rest of the conditions provide backwards compatibility and limit redirect uris to redirectURIOOB, deviceCallbackURI or localhost for public clients which seems fine. WDYT?
There was a problem hiding this comment.
My concern is that for SPAs allowing localhost redirects seems like an unnecessary risk.
But if we care about potential backwards compatibility issues more, then I am fine with the change as it is.
There was a problem hiding this comment.
My concern is that for SPAs allowing localhost redirects seems like an unnecessary risk.
That's actually true. Consider that e.g. a malicious desktop app could obtain a token intended for https://webapp.com , since the desktop app can run a server listening on http://localhost .
If the user is already logged in at the IDP in the browser, then this could probably be done without user interaction.
The desktop app could then call the API of https://webapp.com with the token.
So I would prefer not to allow these special URLs implicitly if RedirectURIs is non-empty.
In the future, three additional boolean options could be added to explicitly (dis)allow each of redirectURIOOB, deviceCallbackURI or localhost-ish URLs. But this would definitely break backwards compatibility, and might be over-engineering.
There was a problem hiding this comment.
@sagikazarmark I added more tests for these special URIs in c15e288 .
Then I made the change suggested by @tkleczek in 162073b : if !client.Public || len(client.RedirectURIs) > 0 { return false }
As you can see in the tests, it's still possible to explicitly add redirectURIOOB and deviceCallbackURI to the RedirectURIs if needed. So most mobile/desktop/localhost apps should just continue to work, and all others just need to update their configuration.
The only edge case that now doesn't work any more is a combination of 1) named RedirectURIs with 2) localhost URIs with random port. (For desktop apps, random ports must be used, since any hard-coded port might be occupied by another application.)
But this case is weird anyway (using the same client for web app + desktop app?). It can be solved by using two clients instead.
Please take another look.
|
@tkleczek I saw your comment in an e-mail, can't find it here any more:
What about SPAs without server component, hosted as static websites? Read more here: https://developer.okta.com/blog/2019/08/22/okta-authjs-pkce |
heidemn-faro
changed the title
|
@heidemn-faro Yes, I realized this soon after I've written my comment and therefore deleted it. |
There was a problem hiding this comment.
LGTM
@sagikazarmark I would propose to add this PR to v2.26.0 milestone, because it enables PKCE #1784 for single page web applications.
|
Hey, thanks for this! Just wanted to ask quickly what the timeline is on getting this merged and releasing v2.26.0? Thanks in advance! |
|
I'll try to get this merged and released this week. |
@sagikazarmark awesome, thank you sir! |
…ect URIs configured Signed-off-by: Martin Heide <martin.heide@faro.com>
Signed-off-by: Martin Heide <martin.heide@faro.com>
heidemn-faro
force-pushed
the
feature/redirect-uris-for-public-clients
branch
from
7e9dd88
to
1ea481b
Compare
Signed-off-by: Martin Heide <martin.heide@faro.com>
…Is is set Signed-off-by: Martin Heide <martin.heide@faro.com>
The PRs dexidp/dex#1784 and dexidp/dex#1822 are super useful, but the documentation can be confusing. First, it does not make sense to specify a secret in a public client because it will require you to pass the secret to the public client, which will make the secret "not secret". Also, as of dexidp/dex#1822, it is possible to use `redirectURIs` in a public client. Signed-off-by: leonnicolas <60091705+leonnicolas@users.noreply.github.com>
Relates to: