Repository navigation
Re-validate stored client URIs on partial client updates in the secure-client-uris executors - #53992
choksi2212 wants to merge 2 commits into
Conversation
…e-client-uris executors Both the secure-client-uris and the secure-client-uris-pattern executors validated only the proposed ClientRepresentation on updates. A client update issued through the Admin REST API is a partial update: fields absent from the payload keep their stored values (see RepresentationToModel.updateClientProperties), so the URI fields omitted from the update payload were never re-validated, and a client holding non-compliant redirectUris/adminUrl/jwksUrl values - for example because it was created before the policy was enabled - could be updated indefinitely without the policy rejecting it. Validate the effective client state on UPDATE instead: the proposed representation merged with the stored client, with the same merge semantics as RepresentationToModel.updateClientProperties (first-non-null scalars, list fallback, per-key attribute merge skipping null values) - the same idiom keycloak#53038 established for the register path. The shared helper lives on SecureClientUrisExecutor and is reused by SecureClientUrisPatternExecutor. Partial updates that replace a stored non-compliant URI with a compliant one remain accepted; registration is unchanged as there is no stored client to merge with. Fixes keycloak#53952 Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Null-attribute deletions and derived web origins are merged differently from persistence, causing incorrect validation.
2 open findings
What changed in this PR
Addresses #53952 by revalidating stored client URIs during partial updates in both URI policy executors, while leaving registration unchanged.
Changes:
- Adds a shared effective-state merge for update validation.
- Applies it to both URI policy executors.
- Adds six regression tests covering rejection and valid URI replacement.
| File | Description |
|---|---|
| tests/base/src/test/java/org/keycloak/tests/client/policies/SecureClientUrisPartialUpdateTest.java | Tests partial-update validation and URI replacement. |
| services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureClientUrisPatternExecutor.java | Validates merged state on updates. |
| services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureClientUrisExecutor.java | Adds shared merge logic and uses it on updates. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ins in the effective client representation
An attribute set to null (or to an empty value) during a client update removes the
stored key (see ClientAdapter#setAttribute), so the effective representation must not
keep the stale URI - otherwise a valid remediation like {"attributes":{"jwks.url":null}}
is rejected by the secure-client-uris executors. Web origins omitted from an update of a
client without stored web origins are derived from the redirect URIs effective after the
update (see RepresentationToModel#defaultWebOrigins), so the derived origins are validated
as well; a pattern policy checking only webOrigins could otherwise be bypassed by
updating redirectUris alone.
Covered by the new SecureClientUrisExecutorTest (the explicit null case is a raw-HTTP-only
payload since the admin client drops null attribute values, JsonInclude.NON_NULL) and by
the webOrigins derivation plus empty-value deletion tests in the testsuite.
Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
a076c7f to
c6e8d5a
Compare
|
Both findings verified against the persistence code and fixed in c6e8d5a - thanks for catching these, they were real semantic gaps in the merge. Finding 1 (web origins derived from redirect URIs on omit). Confirmed: Finding 2 (explicit null attribute values are deletions). Confirmed: Verification: 9/9 testsuite tests and 8/8 unit tests green on the fixed build (full services -> quarkus/server -> quarkus/dist rebuild, tests run against the real distribution server); sibling classes unaffected: ClientUrisPatternExecutorTest 5/5, SecureClientNodeExecutorTest 20/20, AdminProtocolMapperClientPolicyTest 13/13, ClientPoliciesClientCRUDTest 3/3; spotless clean. |
|
CI triage for the red Three data points say this is the distribution-matrix boot flake, not this change:
The |
There was a problem hiding this comment.
Thanks @choksi2212 for the contribution!
Reviewed both executors: client state validation for UPDATE events (including partial updates with omitted URI fields), replacements, attribute deletions, and derived web-origin handling all look good. No blocking issues found.
The JDKTestSuite failure looks like an unrelated server-readiness timeout. I approve the PR from my end.
|
Hi @mposolda, I've reviewed and approved the PR from my end. The JDKTestSuite failure in the pipeline looks like an unrelated server-readiness timeout. Could you please trigger a rerun and take a look when you get a chance? |
|
Thanks @jimmychakkalakal! On the JDKTestSuite failure: I triaged that run earlier (#53992 (comment)) — it's the unrelated temurin-27 server-readiness timeout, with no code-path connection to this change. @mposolda the rerun request details are there; happy to act on anything else that comes up. |
There was a problem hiding this comment.
@choksi2212 Thanks for the PR. There was a related fix pushed in the meantime, which seems also the cause of the conflict ae8fa4c . Can you please:
- Confirm if the bug is still valid (even after ae8fa4c commit)
- If it is still valid, can you rebase this PR on top of latest main to fix the conflict?
Description
The
secure-client-urisandsecure-client-uris-patternexecutors validate the URIs of the proposedClientRepresentationonly. A client update issued through the Admin REST API is a partial update: it carries only the changed fields, and the fields absent from the payload keep their stored values (seeRepresentationToModel.updateClientProperties). As a result, the URI fields (redirectUris,adminUrl,baseUrl,jwksUrland the remaining validated fields) that are omitted from the update payload are never re-validated — a client that holds non-compliant values (for example because it was created before the policy was enabled) can be updated indefinitely without the policy noticing, which is the gap #53952 describes for the UPDATE operation.Both executors now validate the effective client state on UPDATE: the proposed representation merged with the stored client. The merge helper (
SecureClientUrisExecutor.effectiveClientRepresentation) mirrors the merge semantics ofRepresentationToModel.updateClientPropertiesexactly — first-non-null for the scalar URL fields, fallback to the storedredirectUris/webOriginslists, and a per-key attribute merge that skips null values — the same idiom #53038 established for the register path, so the executors now judge the same client state that will be persisted. The helper is shared by both executors; a partial update that replaces a stored non-compliant URI with a compliant one is still accepted, because the merged view carries the new value.Registration is unchanged: there is no stored client to merge with, so the proposed representation is validated as before.
New tests
SecureClientUrisPartialUpdateTest(tests/base) seeds a non-compliant URI field on the client before the policy is enabled, then issues a partial update (description-only) with the policy active, so the update can only be rejected by re-validating the stored client state:testPartialUpdateRevalidatesStoredJwksUri,testPartialUpdateRevalidatesStoredAdminUrl,testPartialUpdateRevalidatesStoredRedirectUris— the partial update is rejected bysecure-client-uriswithinvalid_clientmetadata errors (Invalid jwksUri/Invalid adminUrl/Invalid redirectUris).testPatternPartialUpdateRevalidatesStoredJwksUri— the same contract forsecure-client-uris-patternwith anallowedPatternsconfiguration that rejects the storedjwksUrl.testPartialUpdateReplacingStoredJwksUriIsAccepted,testPatternPartialUpdateReplacingStoredJwksUriIsAccepted— partial updates that replace the stored non-compliantjwksUrlwith a compliant one are accepted and persisted.The four reject tests fail on current
mainwithExpected ClientPolicyException to be thrown, but nothing was thrown— verified against a distribution carrying the pre-fix executors (tests kept): the 4 reject tests fail with exactly that signature while the 2 accept tests pass.Fixes #53952
Checklist
org.keycloak.tests.client.policiespackage intests/base(including the new class) passes against a booted distribution built with this change.Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>trailer (DCO).Note on AI usage: This pull request was prepared with the assistance of an AI agent (Hermes by Nous Research) generating the complete solution from a prompt, per the contributing guidelines on generative AI. Every change was reviewed, tested, and verified against a real Keycloak distribution by the author.