Repository navigation
Client uris validation improvements - #54062
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes follow existing validation patterns and include regression coverage, with no blocking issues identified; tests were not executed.
0 open findings
What changed in this PR
Extends Trusted Hosts client registration validation to post-logout redirect URIs, addressing #54046.
Changes:
- Applies existing trusted-host checks to post-logout redirects while excluding special markers.
- Rejects null hosts safely.
- Adds regression coverage for rejected create/update requests and accepted redirects.
| File | Description |
|---|---|
| tests/base/src/test/java/org/keycloak/tests/client/ClientRegistrationPoliciesTest.java | Tests post-logout redirect validation during registration and updates. |
| services/src/main/java/org/keycloak/services/clientregistration/policy/impl/TrustedHostClientRegistrationPolicy.java | Validates post-logout redirect hosts and handles null hosts. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Should backchannel_logout_uri and frontchannel_logout_uri also be checked against the trusted hosts here?
An anonymous registration with a redirect_uris entry on an untrusted host gets 403. The same registration with either logout URI on that host gets 201, and for the backchannel one Keycloak sends the logout token to that host when the user logs out.
With this PR, the Trusted Hosts policy checks rootUrl, baseUrl, adminUrl, redirect_uris and now post_logout_redirect_uris (verifyClientUrls). The two logout URIs are stored from the registration request (backchannel, frontchannel), but the policy doesn't check them.
How I tested, on this PR's head with a probe test in ClientRegistrationPoliciesTest:
- Trusted host set to
localhost. An HTTP listener on127.0.0.1plays the untrusted host. - Registered a client anonymously with one URI field on
127.0.0.1at a time. - For the backchannel client, logged in a user, then logged that user out from the admin API. To log in I turned off consent and enabled direct grants on that client through the admin API. Neither change touches the URIs.
I didn't check the frontchannel iframe in a browser. #50530 tracks the same two fields for CIMD.
Field on 127.0.0.1 |
Expected | Actual |
|---|---|---|
redirect_uris |
403 | 403 |
post_logout_redirect_uris |
403 | 403 (this PR) |
frontchannel_logout_uri |
403 | 201, client created |
backchannel_logout_uri |
403 | 201, client created; on logout, 127.0.0.1 received POST /backchannel with a logout_token |
|
Thanks @graziang. I pulled this change on 26.8.1 and verified against the original report: untrusted Consider adding a |
@oleksii-udovychenko added validation also for |
@girishkvs updated the test, can you please check? |
|
Thanks @graziang. I re-ran the same probe on the latest commit, and both logout URIs are now rejected. Since the client can't be registered with an untrusted backchannel URI, the logout step doesn't run any more.
|
|
@graziang |
There was a problem hiding this comment.
I see the possible problem of why the URIs like post_logout_redirect_uris or "frontchannel logout uri" should be checked, but other URIs like for example "logo URI" are not checked?
The documentation is not very specific . It mentions just "URLs of newly registered client must also use just those trusted hosts or domains."
As usual, I afraid that if we accept this one, we will have follow-up reports that client URI x is checked, but client URI y is not checked.
We have few places, where we check client URIs. Maybe there are more, but I am aware of:
DefaultClientValidationProviderSecureClientUrisPatternExecutorSecureClientUrisExecutorTrustedHostClientRegistrationProvider
IMO it would be great if all those make sure that they check the same set of URIs. Can we have the list of URIs configured somewhere on single place and make sure that all those consume it from this single place?
Right now, we have metadata in SecureClientUrisPatternExecutorFactory.ALL_CLIENT_URI_FIELDS and DefaultClientValidationProvider.FieldMessages . Can we somehow unify those and update somehow the format of those metadata, so that it can be consumed by all places?
There was a problem hiding this comment.
Thanks @graziang, I have reviewed the changes LGTM, I approve PR from me end
d4facd8 to
ac3eccf
Compare
There was a problem hiding this comment.
@graziang Thanks!
I've added one minor inline comment, but it is OK to merge without that. Just added that in case that there are some changes needed anyway for this PR.
However there is failing tests in Account UI, which failed for 3 re-runs. Can you check if it is related to this PR or not? It seems to me that changes in this PR are rather unrelated, but not 100% sure.
| Client URI validation and client policies now share a catalog of URI fields. Existing policy configuration keys, selected field names, regular expressions, and wildcard options retain their meaning. | ||
| The following additional checks can reject previously accepted client configurations: | ||
|
|
||
| * With *Client URIs Must Match* enabled, the *Trusted Hosts* registration policy checks all supplied URI fields on creation and update, including post-logout redirects, logout endpoints, web origins, JWKS and request URIs, CIBA notification endpoints, logo, policy, terms of service, sector identifier attributes, and SAML endpoints and metadata URLs. Add any legitimate external hosts to the trusted-host list before updating these clients. Post-logout `*` is rejected because it permits redirects to any host. Explicit URLs accompanying `+` or `-` are also checked. |
There was a problem hiding this comment.
| * With *Client URIs Must Match* enabled, the *Trusted Hosts* registration policy checks all supplied URI fields on creation and update, including post-logout redirects, logout endpoints, web origins, JWKS and request URIs, CIBA notification endpoints, logo, policy, terms of service, sector identifier attributes, and SAML endpoints and metadata URLs. Add any legitimate external hosts to the trusted-host list before updating these clients. Post-logout `*` is rejected because it permits redirects to any host. Explicit URLs accompanying `+` or `-` are also checked. | |
| * With *Client URIs Must Match* enabled, the *Trusted Hosts* client registration policy checks all supplied URI fields on creation and update, including post-logout redirects, logout endpoints, web origins, JWKS and request URIs, CIBA notification endpoints, logo, policy, terms of service, sector identifier attributes, and SAML endpoints and metadata URLs. Add any legitimate external hosts to the trusted-host list before updating these clients. Post-logout `*` is rejected because it permits redirects to any host. Explicit URLs accompanying `+` or `-` are also checked. |
Just a nitpick, but not a blocker for this PR from my point of view. Might be added just in case there are more issues in this PR (so that PR will need to be updated anyway).
|
@mposolda @graziang I did a local repro and it seems that the Account UI failure is related to this PR: I also pulled latest commit head One more change that may affect upgrades: with |
Closes keycloak#54046 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Closes keycloak#54046 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
|
@girishkvs Thanks, confirmed and fixed the web-origin regression. Also relative logos now work with Trusted Hosts. |
Signed-off-by: Marek Posolda <mposolda@gmail.com>
| return value; | ||
| } | ||
|
|
||
| return rootUrl == null || rootUrl.isEmpty() ? null : rootUrl + value; |
Closes keycloak#54046 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com> Signed-off-by: Marek Posolda <mposolda@gmail.com> Co-authored-by: Marek Posolda <mposolda@gmail.com>
Closes #54046
This PR adds
ClientUriField, a single catalog of the client URI fields with the common code to read and validate them, and makes all validators use it. Existing configuration keys, field names, patterns, wildcard options and the handling of relative URIs,data:logos and the+,*,-andnullmarkers are unchanged.ClientUriFieldas the shared list of client URI fields, with common code to read values and run validation.DefaultClientValidationProviderto use the shared list and keep existing validation rules and error messages.TrustedHostClientRegistrationPolicyto check all listed URI fields on creation and update, including post-logout redirects.SecureClientUrisExecutorto also check sector identifier attributes and SAML endpoint and metadata URLs.SamlSecureClientUrisExecutorto check the shared list for SAML clients, including stored URLs during login and logout.SecureClientUrisPatternExecutorand its factory to use the shared list while keeping field selection and literal regex matching.