Repository navigation
Add validation for blocked session note names in UserSessionNoteMapper - #53559
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Client updates and existing or imported mappers can bypass validation and still expose credentials.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds validation intended to prevent OIDC session-note mappers from exposing federated credentials.
Changes:
- Blocks four credential-bearing session-note names.
- Adds create, update, bulk, and allowed-name tests.
| File | Description |
|---|---|
UserSessionNoteMapper.java |
Adds blocked-note validation. |
UserSessionNoteMapperValidationTest.java |
Tests mapper configuration validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostEncSignedAssertionsAndDocumentTestKeycloak CI - Adapter IT Strict Cookies |
fd6513f to
1f96a3a
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Client-scope validation remains bypassable, runtime protections lack integration coverage, and the documentation incorrectly describes V2 authorization.
Review effort: Balanced
Findings: 1
Open (5)
Resolved since last review (1)
5aa39c0 to
e774a1e
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Security-critical issuance fallback and client-scope validation paths lack regression coverage.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (5)
e774a1e to
9f9e3b1
Compare
9f9e3b1 to
e701d59
Compare
e701d59 to
24a4268
Compare
cbc79d5 to
0b0479a
Compare
6e5dcee to
0346798
Compare
There was a problem hiding this comment.
@jimmychakkalakal Thanks! I've added 2 comments inline.
Besides it, can you please also check if we don't have other "user session notes" in Keycloak, which have similar problems like those and which allow to disclose some data, which are not supposed to be added to tokens?
AFAIK we have built-in support for Kerberos GSS credential, which is OK (and it is even documented ). I do not recall anything else, but can you please doublecheck and confirm?
EDIT: I've asked this to you before, can you please confirm if you did not found anything else?
@mposolda Thanks for the comments. I checked every place that writes a session note in the core server. Only the IdP tokens and the transient-user model are sensitive, and both are blocked. The Kerberos credential is allowed on purpose. Everything else is harmless metadata. |
There was a problem hiding this comment.
🟡 Changes recommended
Client registration responses omit newly persisted pairwise mapper salts, so reusing a response in a subsequent PUT can regenerate the salt and change users’ subject identifiers.
1 open issue
1 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Human review is needed to confirm third-party compatibility and issuance/import behavior because whole-client writes and realm imports invoke every installed mapper’s validator, including configuration-mutating validators.
Review effort: Balanced
There was a problem hiding this comment.
@jimmychakkalakal Thanks for the updates! Can you please rebase to fix the conflict?
Also there is one inline comment (related to copilot review). Can you please check it and confirm if it is valid or not? If valid comment, can you please fix it?
There was a problem hiding this comment.
🟡 Changes recommended
Critical issues cause valid client creation to fail with NullPointerException/500 and registration-token-authenticated updates to fail with 401 after token rotation.
2 open issues
1 resolved since last review
Review effort: Balanced
Thanks @mposolda I have rebased the branch with latest changes. I have fixed the issue related to copilot comment |
There was a problem hiding this comment.
🟡 Changes recommended
Critical authentication regressions cause valid client creation requests to return 500 and registration-token updates to return 401, while rebuilding the creation response also drops default roles.
3 open issues
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because realm-import validation may miss the separately imported admin-permissions client when fine-grained permissions V2 is enabled, allowing a blocked session-note mapper.
No open issues 🎉
3 resolved since last review
Review effort: Balanced
Prevent OIDC/SAML session note mappers from exposing federated IdP tokens (e.g. FEDERATED_ACCESS_TOKEN), which allowed bypassing the broker read-token role. - Reject sensitive notes in mapper and client validation (APIs, DCR, CIMD, imports). - Omit blocked notes at token issuance to safeguard legacy or imported mappers. - Return HTTP 400 instead of 500 for invalid clients during realm import. - Document breaking change in 26.8.1 upgrading guide. Closes keycloak#53545 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com> # Conflicts: # docs/documentation/upgrading/topics/changes/changes-26_8_1.adoc # Conflicts: # docs/documentation/upgrading/topics/changes/changes-26_8_1.adoc # Conflicts: # services/src/main/java/org/keycloak/validation/DefaultClientValidationProvider.java
Prevent OIDC/SAML session note mappers from exposing federated IdP tokens (e.g. FEDERATED_ACCESS_TOKEN), which allowed bypassing the broker read-token role. - Reject sensitive notes in mapper and client validation (APIs, DCR, CIMD, imports). - Omit blocked notes at token issuance to safeguard legacy or imported mappers. - Return HTTP 400 instead of 500 for invalid clients during realm import. - Document breaking change in 26.8.1 upgrading guide. Closes keycloak#53545 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com> # Conflicts: # docs/documentation/upgrading/topics/changes/changes-26_8_1.adoc
Closes keycloak#53545 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com> # Conflicts: # docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc # Conflicts: # docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc
…nses Validators can initialize protocol mapper configuration (e.g. pairwise mapper salt). The current implementation returns a response built BEFORE validation, so validation-generated values are lost. For CREATE and UPDATE, refresh the client representation from the database after validation to ensure validation-generated config is included in the response. Fixes Copilot comment on protocol mapper validation: if the salt is omitted from the CREATE/UPDATE response, a subsequent PUT with that response overwrites the stored salt, causing regeneration and changing users' sub values. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
Closes keycloak#53545 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The CVE remediation changes OIDC/SAML credential-exposure boundaries across client and realm-import paths, requiring end-to-end security and import-compatibility validation beyond static review.
No open issues 🎉
Review effort: Balanced
There was a problem hiding this comment.
@jimmychakkalakal Thanks for the patience with this one! I hope to finally merge once tests are OK (hope there will not be another conflict in the meantime :-) )
Fixes a security vulnerability (CVE-2026-105302 / #53545) where OIDC and SAML UserSessionNoteMapper could expose sensitive broker credentials stored in user session notes (such as FEDERATED_ACCESS_TOKEN, FEDERATED_REFRESH_TOKEN, etc.) to unauthorized clients.
Closes #53545
Fixed with help of Claude