Closed ccbrown closed 2 weeks ago
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:
op.Server
that allows an invalid redirect URL.@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.
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.
Thanks for the report BTW.
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.
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.
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.Server
s 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.
Thanks for the update! I appreciate it!
:tada: This issue has been resolved in version 3.27.1 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:
Preflight Checklist
Version
No response
Describe the problem caused by this bug
Authorization servers MUST NOT redirect the user to redirect URIs unless they are valid:
Redirect URIs are only valid if they match a pre-registered URI:
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:
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