fosite icon indicating copy to clipboard operation
fosite copied to clipboard

Investigate missing `redirect_uri` check when performing OIDC flows

Open aeneasr opened this issue 2 years ago • 4 comments

Preflight checklist

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

aeneasr avatar Jul 19 '22 12:07 aeneasr

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):

https://github.com/ory/fosite/blob/29de878cba1f35ed03a883444c3ef1bd019d083c/authorize_helper.go#L83-L99

Also See (note the check of if redirect URI is nil):

https://github.com/ory/fosite/blob/29de878cba1f35ed03a883444c3ef1bd019d083c/authorize_request.go#L60-L75

Maybe making the following changes would be viable:

  • Add a func to the Client interface to facilitate per-client customization which is either:
    1. A function with the signature MatchRedirectURI(rawurl string) (*url.URL, err error)
    2. A function with the signature GetRedirectURIMatcher() func(rawurl string, client Client) (*url.URL, err error)
  • Add func to the Client interface with the signature MatchRedirectURIWithClientRedirectURIs(rawurl string) (*url.URL, err error) - Rename fosite.MatchRedirectURIWithClientRedirectURIs to fosite.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 call fosite.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.

james-d-elliott avatar Jul 22 '22 00:07 james-d-elliott

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:

https://github.com/ory/fosite/blob/29de878cba1f35ed03a883444c3ef1bd019d083c/authorize_request_handler.go#L180-L193

james-d-elliott avatar Jul 22 '22 00:07 james-d-elliott

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.

vivshankar avatar Jul 22 '22 01:07 vivshankar

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
}

james-d-elliott avatar Jul 22 '22 02:07 james-d-elliott