fix: don't crash when /admin/serverinfo omits systemInfo - #185
Open
phillipfickl wants to merge 1 commit into
Open
phillipfickl wants to merge 1 commit into
phillipfickl wants to merge 1 commit into
Conversation
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 Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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