Investigate missing `redirect_uri` check when performing OIDC flows
aeneasr opened this issue · 4 comments
Preflight checklist
- I could not find a solution in the existing issues, docs, nor discussions.
- I agree to follow this project's Code of Conduct.
- I have read and am following this repository's Contribution Guidelines.
- This issue affects my Ory Cloud project.
- I have joined the Ory Community Slack.
- I am signed up to the Ory Security Patch Newsletter.
Describe the bug
Open ID Connect Core specification says:
redirect_uri
REQUIRED. Redirection URI to which the response will be sent. This URI MUST exactly match one of the Redirection URI values for the Client pre-registered at the OpenID Provider, with the matching performed as described in Section 6.2.1 of [[RFC3986]](https://openid.net/specs/openid-connect-core-1_0.html#RFC3986) (Simple String Comparison). When using this flow, the Redirection URI SHOULD use the https scheme; however, it MAY use the http scheme, provided that the Client Type is confidential, as defined in Section 2.1 of OAuth 2.0, and provided the OP allows the use of http Redirection URIs in this case. The Redirection URI MAY use an alternate scheme, such as one that is intended to identify a callback into a native application.
It does however not appear as if we are checking for this condition, as OAuth2 has the redirect URI marked as optional.
Reproducing the bug
This should be first confirmed with an integration test case
Relevant log output
No response
Relevant configuration
No response
Version
On which operating system are you observing this issue?
No response
In which environment are you deploying?
No response
Additional Context
No response
I had a brief look at this (wouldn't consider it to be an exhaustive look, just looked for references to GetRedirectURIs() and from there found the most likely sources). It appears what's occurring is if the clients have a single redirect URI and the redirect URI provided to the authorization endpoint has an empty or non-existent redirect URI the single configured redirect URI (in the client) is used.
See (also note the duplicate comment haha):
Lines 83 to 99 in 29de878
Also See (note the check of if redirect URI is nil):
Lines 60 to 75 in 29de878
Maybe making the following changes would be viable:
- Add a func to the Client interface to facilitate per-client customization which is either:
- A function with the signature
MatchRedirectURI(rawurl string) (*url.URL, err error)
- A function with the signature
GetRedirectURIMatcher() func(rawurl string, client Client) (*url.URL, err error)
- A function with the signature
- Add func to the Client interface with the signature
MatchRedirectURIWithClientRedirectURIs(rawurl string) (*url.URL, err error)
- Renamefosite.MatchRedirectURIWithClientRedirectURIs
tofosite.MatchOAuth2RedirectURIWithClientRedirectURIs
- Add a new
fosite.MatchRedirectURIWithClientRedirectURIs
which is effectively a clone of the original function but excludes the first conditional - Adjust
fosite.MatchOAuth2RedirectURIWithClientRedirectURIs
to have the first conditional only, then callfosite.MatchRedirectURIWithClientRedirectURIs
- If option 2 is used, we would check the result of
client.GetRedirectURIMatcher()
, if not nil then we'd immediately return the result of that func. If it is nil then we'd return the default behavior.
Just found the only call to IsRedirectURIValid() is in WriteAuthorizeError. Seems strange?
Ahhh I see why. Here's where it's actually used in NewAuthorizeRequest
:
fosite/authorize_request_handler.go
Lines 180 to 193 in 29de878
Wouldn't it be simpler to handle this by just modifying validateAuthorizeScope
in authorize_request_handler.go
?
Drop this at the bottom of that function after request.SetRequestedScopes(scope)
.
if request.GetRequestedScopes().Has("openid") && r.Form.Get("redirect_uri") == "" {
return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("Redirect URI information is required."))
}
This would satisfy the core spec that requires redirect_uri
to be included in the request if scope=openid
is used.
Considering it's not an OpenID Authorize Request without the openid
scope this indeed is a brilliant idea. I question if we should instead reverse the order of validateAuthorizeScope
and validateAuthorizeRedirectURI
and check this within validateAuthorizeRedirectURI
instead however. Similar to this:
func (f *Fosite) validateAuthorizeRedirectURI(_ *http.Request, request *AuthorizeRequest) error {
// Fetch redirect URI from request
rawRedirURI := request.Form.Get("redirect_uri")
if rawRedirURI == "" && request.GetRequestedScopes().Has("openid") {
return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("Redirect URI information is required."))
}
// Validate redirect uri
redirectURI, err := MatchRedirectURIWithClientRedirectURIs(rawRedirURI, request.Client)
if err != nil {
return err
} else if !IsValidRedirectURI(redirectURI) {
return errorsx.WithStack(ErrInvalidRequest.WithHintf("The redirect URI '%s' contains an illegal character (for example #) or is otherwise invalid.", redirectURI))
}
request.RedirectURI = redirectURI
return nil
}