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.
| 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.
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.