Repository navigation
fix: do not leak SCIM resource existence to unauthorized callers - #52151
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Prevents SCIM endpoints from leaking whether a resource ID exists by ensuring unauthorized callers receive 403 even when the resource is missing, and adds a regression test to cover the scenario.
Changes:
- Add an authorization regression test ensuring missing resources return 403 for callers without access.
- Update
BaseResourceTypeProvider.get(...)to check realm-level view permission when the model is missing and throwForbiddenExceptionif unauthorized.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scim/tests/base/src/test/java/org/keycloak/tests/scim/tck/AuthorizationTest.java | Adds coverage for the “missing resource must still be 403 when unauthorized” behavior. |
| scim/core/src/main/java/org/keycloak/scim/resource/spi/BaseResourceTypeProvider.java | Adds permission gating on missing-model path to avoid leaking existence via 404 vs 403. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .build())); | ||
| assertAccessDenied(() -> noAccessClient.users().delete(missingId)); | ||
| } | ||
|
|
||
| @Test |
34088b6 to
2643e74
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Query-only callers can still distinguish missing resources from existing forbidden resources.
Review details
Suppressed comments (1)
scim/core/src/main/java/org/keycloak/scim/resource/spi/BaseResourceTypeProvider.java:59
canQuery()also acceptsQUERY_USERS/QUERY_GROUPS, but those roles do not permit reading an individual resource. A query-only caller therefore still gets 404 for a missing ID and 403 for an existing ID, so the existence leak remains; check realm-levelVIEWhere instead (and cover the query-only roles in the regression test).
if (!canQuery()) {
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
When a SCIM resource does not exist, BaseResourceTypeProvider.get() returned null, which the resource layer maps to a 404. A caller with no view permission could therefore distinguish a missing resource (404) from an existing-but- forbidden one (403), leaking whether a given resource id exists. Fix: when the model is null, first check realm-level view permission on the resource type and throw ForbiddenException (403) when the caller lacks it. The 404 path is preserved for callers who do hold view permission. Closes keycloak#52147 Signed-off-by: waterWang <waterWang@users.noreply.github.com>
2643e74 to
c7ea8c1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Query-only callers can still distinguish missing resources from existing forbidden resources.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| if (!canQuery()) { | ||
| throw new ForbiddenException(); | ||
| } |
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#salesPostTestKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testMultipleTabsParallelLoginKeycloak CI - Adapter IT Strict Cookies |
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#salesPostAssertionAndResponseSigTestKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestNoKeyNameInKeyInfoKeycloak CI - Adapter IT Strict Cookies |
Problem
SCIM mutative operations (patch, update, delete) and
GETperform an existence check before the authorization check. A caller with no view permission sees a 404 when the resource does not exist, but a 403 when it does — leaking whether a given resource id exists. This lets callers enumerate resource ids they cannot access.Root cause
BaseResourceTypeProvider.get(id, attributes, excludedAttributes)returnsnullwhen the model is not found, before checking any permission. The resource layer (ScimResourceTypeResource) maps thatnullto a 404. Only when the model does exist does the code reach thehasPermission(... VIEW)check that produces a 403.Fix
When the model is null, first check realm-level view permission on the resource type (
hasPermission(getRealmResourceType(), VIEW)). If the caller lacks it, throwForbiddenException(→ 403) so the response is indistinguishable from an existing-but-forbidden resource. Callers who do hold view permission still receive the 404 for a truly missing resource.Test
Added
testMissingResourceDoesNotLeakExistencetoAuthorizationTest: an access-less client issuingget,update,patchanddeleteon a non-existent user id now receives 403 for all four, instead of 404.Closes #52147