fix: openid ignores missing redirect uri #762
Conversation
Fixes an issue where Authorize Requests which were intended for an OpenID Connect 1.0 client would incorrectly be allowed when missing the redirect URI when it's required by the specification.
james-d-elliott
force-pushed
the
fix-oidc-redirect-uri
branch
from
f89f709 to
4273501
Compare
| // Hybrid Flow - https://openid.net/specs/openid-connect-core-1_0.html#HybridAuthRequest | ||
| // | ||
| // Note: as per the Hybrid Flow documentation the Hybrid Flow has the same requirements as the Authorization Code Flow. | ||
| if len(rawRedirURI) == 0 && request.GetRequestedScopes().Has("openid") { |
There was a problem hiding this comment.
Is this not already handled by the openid handler itself?
There was a problem hiding this comment.
I have added an integration test which shows this is unfortunately not the case. It only returns an error if the client has multiple redirect URI's. The issue is that OAuth 2.0 allows assuming the Redirect URI if absent provided there is only one, however OpenID Connect 1.0 explicitly requires the redirect URI parameter and has no such leeway.
It should also be noted that OAuth 2.1 does not alter this behavior and instead elaborates / clarifies the behavior for better or worse. The only alteration by my reading is in OpenID Connect 1.0.
There was a problem hiding this comment.
Also see the linked issue, you were actually the person to raise the issue. Maybe that's helpful?
There was a problem hiding this comment.
I checked this and I agree with @james-d-elliott: OAuth 2.0 says that if you included Redirect URI in authentication request, then it has also be included in the token request. And Fosite currently checks that.
But OIDC spec says that Redirect URI is required in the authentication request. And this PR adds this check.
There was a problem hiding this comment.
Thank you both - I checked and indeed the expectation is that the redirect-url is properly validated before we call HandleAuthorizeEndpointRequest. Given that this is openID specific though, this should be moved to the openid handler instead. I'll do that now
There was a problem hiding this comment.
I'm creating a separate PR as this is a breaking change for existing clients and it requires dedicated care for merging that to production on our infrastructure!
There was a problem hiding this comment.
As far as I can tell, this implementation does not respect hybrid or implicit flows of OIDC, so it definitely makes sense to move that to the appropriate handlers in my view.
There was a problem hiding this comment.
That sounds like a solid plan, glad this will get merged regardless.
| return request, err | ||
| } | ||
|
|
||
| if err := f.validateResponseMode(r, request); err != nil { | ||
| if err = f.validateResponseMode(r, request); err != nil { |
There was a problem hiding this comment.
Why these changes? I think they are unrelated and just change the style?
| @@ -363,27 +378,31 @@ func (f *Fosite) newAuthorizeRequest(ctx context.Context, r *http.Request, isPAR | |||
| return request, err | |||
| } | |||
|
|
|||
| if err := f.validateAuthorizeRedirectURI(r, request); err != nil { | |||
| if err = f.parseAuthorizeScope(r, request); err != nil { | |||
There was a problem hiding this comment.
I think that instead of splitting validateAuthorizeScope into parseAuthorizeScope and validateAuthorizeScope, it would be easier if you swapped the order of validation, first call (original) validateAuthorizeScope and then (updated) validateAuthorizeRedirectURI. In a way, if validateAuthorizeRedirectURI depends on parsed scopes, it is better if validateAuthorizeRedirectURI depends on parsed and validated scopes.
Fixes an issue where Authorize Requests which were intended for an OpenID Connect 1.0 client would incorrectly be allowed when missing the redirect URI when it's required by the specification. Closes ory#685 Closes ory#762 BREAKING CHANGES: Going forward, calls to `/oauth2/auth` which trigger OpenID Connect require the `redirect_uri` query parameter to be set.
|
Closing in favor of #814 |
Fixes an issue where Authorize Requests which were intended for an OpenID Connect 1.0 client would incorrectly be allowed when missing the redirect URI when it's required by the specification. Closes #685 Closes #762 BREAKING CHANGES: Going forward, calls to `/oauth2/auth` which trigger OpenID Connect require the `redirect_uri` query parameter to be set.
Fixes an issue where Authorize Requests which were intended for an OpenID Connect 1.0 client would incorrectly be allowed when missing the redirect URI when it's required by the specification.
Related Issue or Design Document
Fixes #685
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact security@ory.sh) from the maintainers to push the changes.
Further comments
The added tests fail prior to the fix and pass after.