feat: Allow configuring redirect secure checker everywhere #489
Conversation
authorize_helper.go
Outdated
| @@ -131,10 +131,6 @@ func isLoopbackURI(requested *url.URL, registeredURI string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| if registered.Scheme != "http" || !isLoopbackAddress(registered.Host) { | |||
There was a problem hiding this comment.
This is redundant. So I removed it.
There was a problem hiding this comment.
While that's currently true, removing explicit checks for implicit ones opens up a big opportunity for regression issues. Given that this is security sensitive, and that you want to have this removed, please either write a test covering this exact case or point to the test case that already covers this logic. Thanks! :)
There was a problem hiding this comment.
But this is logically redundant. It is literally the opposite than the next case. It is De Morgan law.
Compare:
registered.Scheme != "http" || !isLoopbackAddress(registered.Host)
and:
requested.Scheme == "http" && isLoopbackAddress(requested.Host)
There is no way one can write a test case here because it is behaving exactly the same with or without this.
There was a problem hiding this comment.
Oh, you just want a test case where using http scheme returns false and using a http scheme without loopback address returns false? I think those are already covered.
There was a problem hiding this comment.
Exactly!
That test tests IsRedirectURISecure which does not use isLoopbackURI so I think this is neither covered explicitly nor implicitly.
| } | ||
|
|
||
| func IsLocalhost(redirectURI *url.URL) bool { | ||
| hn := redirectURI.Hostname() | ||
| return strings.HasSuffix(hn, ".localhost") || hn == "127.0.0.1" || hn == "localhost" | ||
| return strings.HasSuffix(hn, ".localhost") || hn == "127.0.0.1" || hn == "::1" || hn == "localhost" |
There was a problem hiding this comment.
IPv6 localhost is still localhost.
| @@ -38,6 +39,7 @@ import ( | |||
| type OpenIDConnectRequestValidator struct { | |||
| AllowedPrompt []string | |||
| Strategy jwt.JWTStrategy | |||
| IsRedirectURISecure func(*url.URL) bool | |||
There was a problem hiding this comment.
Adding this for completeness. We are allowing configuring IsRedirectURISecure in some places, but not in all. I added it here so that we can configure it here as well.
| @@ -75,7 +90,7 @@ func (v *OpenIDConnectRequestValidator) ValidatePrompt(ctx context.Context, req | |||
| // be processed as if no previous request had been approved. | |||
|
|
|||
| if stringslice.Has(prompt, "none") { | |||
| if !(req.GetRedirectURI().Scheme == "https" || (fosite.IsLocalhost(req.GetRedirectURI()) && req.GetRedirectURI().Scheme == "http")) { | |||
| if !v.secureChecker()(req.GetRedirectURI()) { | |||
There was a problem hiding this comment.
So instead of duplicating code, I moved all this code to IsRedirectURISecure.
authorize_helper.go
Outdated
| @@ -131,10 +131,6 @@ func isLoopbackURI(requested *url.URL, registeredURI string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| if registered.Scheme != "http" || !isLoopbackAddress(registered.Host) { | |||
There was a problem hiding this comment.
While that's currently true, removing explicit checks for implicit ones opens up a big opportunity for regression issues. Given that this is security sensitive, and that you want to have this removed, please either write a test covering this exact case or point to the test case that already covers this logic. Thanks! :)
authorize_helper.go
Outdated
| @@ -131,10 +131,6 @@ func isLoopbackURI(requested *url.URL, registeredURI string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| if registered.Scheme != "http" || !isLoopbackAddress(registered.Host) { | |||
There was a problem hiding this comment.
Exactly!
That test tests IsRedirectURISecure which does not use isLoopbackURI so I think this is neither covered explicitly nor implicitly.
|
OK. Are you open to having |
|
Addressed review comments. |
Proposed changes
I have been reading this part of the code and found out some things I could improve.
Checklist
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
Further comments
This is changing/cleaning some security logic used by fosite. I do not think any of these changes are changing anything really exploitable, but I still think it will harden and cleanup things a bit. Or at least make it more consistent.