Investigate missing `redirect_uri` check when performing OIDC flows

[aeneasr]

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

[james-d-elliott]

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

fosite/authorize_helper.go

Lines 83 to 99 in 29de878

	func MatchRedirectURIWithClientRedirectURIs(rawurl string, client Client) (*url.URL, error) {
	if rawurl == "" && len(client.GetRedirectURIs()) == 1 {
	if redirectURIFromClient, err := url.Parse(client.GetRedirectURIs()[0]); err == nil && IsValidRedirectURI(redirectURIFromClient) {
	// If no redirect_uri was given and the client has exactly one valid redirect_uri registered, use that instead
	return redirectURIFromClient, nil
	}
	} else if redirectTo, ok := isMatchingRedirectURI(rawurl, client.GetRedirectURIs()); rawurl != "" && ok {
	// If a redirect_uri was given and the clients knows it (simple string comparison!)
	// return it.
	if parsed, err := url.Parse(redirectTo); err == nil && IsValidRedirectURI(parsed) {
	// If no redirect_uri was given and the client has exactly one valid redirect_uri registered, use that instead
	return parsed, nil
	}
	}
	return nil, errorsx.WithStack(ErrInvalidRequest.WithHint("The 'redirect_uri' parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls."))
	}

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

fosite/authorize_request.go

Lines 60 to 75 in 29de878

	func (d *AuthorizeRequest) IsRedirectURIValid() bool {
	if d.GetRedirectURI() == nil {
	return false
	}
	raw := d.GetRedirectURI().String()
	if d.GetClient() == nil {
	return false
	}
	redirectURI, err := MatchRedirectURIWithClientRedirectURIs(raw, d.GetClient())
	if err != nil {
	return false
	}
	return IsValidRedirectURI(redirectURI)
	}

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]

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

	func (f *Fosite) validateAuthorizeRedirectURI(_ *http.Request, request *AuthorizeRequest) error {
	// Fetch redirect URI from request
	rawRedirURI := request.Form.Get("redirect_uri")
	// 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
	}

[vivshankar]

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.

[james-d-elliott]

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
}