Skip to content

fix: don't crash when /admin/serverinfo omits systemInfo - #185

Open
phillipfickl wants to merge 1 commit into
fschmtt:mainfrom
phillipfickl:fix/version-detection-fail-open
Open

phillipfickl wants to merge 1 commit into
fschmtt:mainfrom
phillipfickl:fix/version-detection-fail-open

Conversation

@phillipfickl

Copy link
Copy Markdown
Contributor

Since Keycloak 26.4, GET /admin/serverinfo returns 200 but omits systemInfo for anyone who is not a master realm administrator (26.7 tightened this further to require manage-realm). fetchVersion() dereferenced the absent systemInfo unconditionally, so every resource accessor raised "Error: Call to a member function getVersion() on null" from library internals. A response carrying systemInfo without a version hit a second path, a TypeError out of SystemInfo::getVersion(), which is declared ': string' over a nullable property.

The version is only consumed by AttributeNormalizer::normalize() to strip outgoing properties gated by #[Since], the highest of which is major 26, and there are no #[Until] attributes at all. On every server affected by this bug the filter is therefore a guaranteed no-op: the library was making a privileged request on every accessor to compute nothing.

So version detection now degrades instead of throwing. A null version already means "no filtering", which on 26.4+ is identical to a successful detection. Detection is memoised via a separate flag so a failure is not retried on every subsequent accessor call, and the caught throwable is retained so the real cause survives.

getVersion() is the one explicit ask that cannot be answered without it, so that alone throws the new VersionDetectionException, naming both cause and remedy and chaining the original failure. This also repairs its return of a ?string from a ': string' signature.

Builder::withVersion() pins the version and skips the request. It is an optimisation, not the fix: with no #[Until] attributes, over-stating the version can never strip a field, so pinning is the only option that can be actively wrong when it drifts from the server.

Also rebuilds queryExecutor alongside serializer and commandExecutor, which otherwise kept a null-version serializer for the object's whole lifetime.

closes #184

Since Keycloak 26.4, GET /admin/serverinfo returns 200 but withholds
systemInfo from insufficiently privileged accounts. fetchVersion()
dereferenced the absent systemInfo unconditionally, so every resource
accessor raised "Error: Call to a member function getVersion() on null"
from library internals. A response carrying systemInfo without a version
hit a second path, a TypeError out of SystemInfo::getVersion(), which is
declared ': string' over a nullable property.

The version is only consumed by AttributeNormalizer::normalize() to strip
outgoing properties gated by #[Since], the highest of which is major 26,
and there are no #[Until] attributes at all. On every server affected by
this bug the filter is therefore a guaranteed no-op: the library was making
a privileged request on every accessor to compute nothing.

So version detection now degrades instead of throwing. A null version
already means "no filtering", which on 26.4+ is identical to a successful
detection. Detection is memoised via a separate flag so a failure is not
retried on every subsequent accessor call, and the caught throwable is
retained so the real cause survives.

getVersion() is the one explicit ask that cannot be answered without it, so
that alone throws the new VersionDetectionException, naming both cause and
remedy and chaining the original failure. This also repairs its return of a
?string from a ': string' signature.

Builder::withVersion() pins the version and skips the request. It is an
optimisation, not the fix: with no #[Until] attributes, over-stating the
version can never strip a field, so pinning is the only option that can be
actively wrong when it drifts from the server.

Also rebuilds queryExecutor alongside serializer and commandExecutor, which
otherwise kept a null-version serializer for the object's whole lifetime.

The privilege rules in the exception message and README were measured
against real 26.3.5 / 26.4.7 / 26.5.7 / 26.6.4 / 26.7.3 servers rather than
taken from the upgrading guide: 26.4 alone restricts systemInfo to master
realm administrators, while 26.5+ gate it on manage-realm in the account's
own realm, with no master realm membership required.

Refs fschmtt#184
@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.24%. Comparing base (1c29894) to head (1874835).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
+ Coverage   92.18%   93.24%   +1.06%     
==========================================
  Files          32       32              
  Lines         972      977       +5     
==========================================
+ Hits          896      911      +15     
+ Misses         76       66      -10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@phillipfickl phillipfickl changed the title fix: fail open when the Keycloak version cannot be detected fix: don't crash when /admin/serverinfo omits systemInfo Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Allow bypassing /admin/serverinfo call to avoid requiring manage-realm privileges

1 participant