[Bug]: Validation of redirect_uri is not performed before redirect

[ccbrown]

Preflight Checklist

• I could not find a solution in the documentation, the existing issues or discussions
• I have joined the ZITADEL chat

Version

No response

Describe the problem caused by this bug

Authorization servers MUST NOT redirect the user to redirect URIs unless they are valid:

If the Redirection URI is invalid, the Authorization Server MUST NOT redirect the User Agent to the invalid Redirection URI.

Redirect URIs are only valid if they match a pre-registered URI:

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] (Simple String Comparison).

I believe the spec is clear here. However, this repo happily redirects users to any redirect URI, with no validation performed, on a variety of errors. This also has security implications.

To reproduce

Make a request like so:

/authorize?client_id=invalid&redirect_uri=https://invalid.com

The redirect_uri is invalid, as it is not a pre-registered value, but the client will be redirected anyway.

Screenshots

No response

Expected behavior

No response

Additional Context

No response

[muhlemmer]

I've just tested using our example, and cannot reproduce.:

go run github.com/zitadel/oidc/v3/example/server

I modified the client's requested redirect URL, so the auth request looks like:

http://localhost:9998/auth?client_id=web&prompt=Welcome+back%21&redirect_uri=http%3A%2F%2Ffoo%3A9999%2Fauth%2Fcallback&response_type=code&scope=openid+profile&state=2bf03663-a989-4ad6-94f2-46e324c7640d

The browser prints the correct error message:

The requested redirect_uri is missing in the client configuration. If you have any questions, you may contact the administrator of the application.

If you somehow able to pass invalid redirects, can you be more clear with:

1. Version of the package you are using
2. A minimal go implementation of an OIDC Provider or op.Server that allows an invalid redirect URL.

[ccbrown]

@muhlemmer Thanks for the prompt response. This can be reproduced using any version of the package, including the latest main (b9bcd6aef9c19673fa9ef942a41ef987bf825a56).

It can be reproduced using your example server without modifications.

For example:

http://localhost:9998/auth?client_id=invalid&redirect_uri=https://invalid.com

Will redirect to:

https://invalid.com/?error=server_error&error_description=unable+to+retrieve+client+by+id

Many error cases such as that one do not perform any validity checks on redirect URIs.

[muhlemmer]

I see what's happening. As the client ID is incorrect, the lib can't find the client. In that case it returns a server_error, which allows redirect to happen. The only error type that disables redirect is the ErrInvalidRequestRedirectURI, which is never returned in this case as we don't get to validating any URI.

I will do some research what the correct error type should be for a non-existing client (server_error sounds wrong also) and disable the redirect.

[muhlemmer]

Thanks for the report BTW.

[muhlemmer]

The Oauth2 RFC 6749, section 4.1.2.1 is also pretty clear on this:

If the request fails due to a missing, invalid, or mismatching redirection URI, or if the client identifier is missing or invalid, the authorization server SHOULD inform the resource owner of the error and MUST NOT automatically redirect the user-agent to the invalid redirection URI.

[ccbrown]

Any movement on this? If this was categorized as a "small issue" I believe you should reconsider. This issue means every user of your library is susceptible to what's typically around a 6.1 CVSS vulnerability. Respectfully, I don't see anything in your "Sprint Backlog" or "In Progress" columns that should be a higher priority than this for a company specializing in identity infrastructure.

If you don't have the resources, let me know and I'll be happy to take a stab at a pull request.

[muhlemmer]

Hi, small issue means we don't need to plan it to fix it, because it's supposedly under a day work. We work on small issues between prioritized issues. It doesn't mean it is low priority.

I already did som preliminary work, but halted for a bit because I need to rethink some of the op.Servers AuthRequest verification logic. As in there we first verify the content of the request, and after that obtain the client. I intend to finish the fix this week.

[ccbrown]

Thanks for the update! I appreciate it!

[github-actions]

🎉 This issue has been resolved in version 3.27.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀