fix(#43): validate AdminClient inputs against an allow-list before writing privileged Special Keys - #105
Merged
Conversation
…iting privileged Special Keys
AdminClient spliced caller-supplied bytes directly into the
privileged Special Key paths
(\xff\xff/management/tenant/map/<name>,
\xff\xff/management/excluded/<addr>,
\xff\xff/management/force_recovery,
\xff\xff/configuration/{redundancy,storage})
with no content validation. A tenant name like 'foo/bar' silently
wrote \xff\xff/management/tenant/map/foo/bar (a different
management sub-key than intended), and a server address like
'127.0.0.1/24' silently produced
\xff\xff/management/excluded/127.0.0.1/24. configure() accepted
any whitespace-split string with no token validation.
The fix routes every caller-supplied identifier through private
helpers (validateTenantName, validateAddress, validateToken,
parseConfiguration) on AdminClient that throw
\InvalidArgumentException with a printable rendering of the
offending input before any transaction begins. Allow-list:
createTenant/deleteTenant [A-Za-z0-9._-] start alnum 256B
exclude/include/reboot [A-Za-z0-9._:-] 256B
configure (per token) [A-Za-z0-9_-] 64B
forceRecovery [A-Za-z0-9_-] 64B
Unit tests (47 cases) cover every allow-list boundary, length
bound, leading-character rule, printable-output safety, and
parseConfiguration rejection shapes. Integration tests (24 cases)
exercise createTenant, deleteTenant, excludeServer, includeServer,
configure against a live cluster and assert that rejected inputs
do not reach the Special Key range.
Affected files:
- src/AdminClient.php
- docs/admin.md
- CHANGELOG.md
- tests/Unit/AdminClientInputValidationTest.php (new)
- tests/Integration/AdminInputValidationTest.php (new)
5 tasks
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
1 participant
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.
Closes #43
Changes
Branch resurrects
fix/issue-43-adminclient-input-validation(cherry-picked onto currentmaster) and completes the second half of the issue: special-key paths are now verified against the documented FDB special-key spec (7.x) for the supported API version.1. Input validation (allow-list at the PHP trust boundary)
Every caller-supplied input written into a privileged
\xff\xff/management/...key is validated before the transaction opens:createTenant[A-Za-z0-9._-], start alnumdeleteTenantexcludeServer/includeServer/rebootWorker[A-Za-z0-9._:-]configure[A-Za-z0-9_-]per tokenforceRecovery[A-Za-z0-9_-]Rejections throw
\InvalidArgumentExceptionsynchronously with a printable (control-byte-escaped) rendering of the offending value — reproductions likecreateTenant('foo/bar')orexcludeServer('127.0.0.1/24')can no longer silently address a different management sub-key.2. Unverified special-key paths corrected (removed)
Verified against the special-keys documentation and FDB sources:
\xff\xff/configuration/module only covers process class types and coordinators — there is noredundancy/storagekey. Live-cluster check confirmed writes fail at commit withspecial_keys_no_module_found.fdbcli force_recovery_with_data_loss), not a special key — no\xff\xff/management/force_recoveryexists.Therefore
configure()andforceRecovery()are deprecated and now throw\LogicExceptionsynchronously (after input validation) instead of failing opaquely at commit.\xff\xff/management/consistency_check_suspendedand the tenant/excluded paths are documented and are covered by live integration tests.Tests
AdminClientInputValidationTest(47 cases),AdminClientTest(+4 cases for theLogicExceptionpaths)AdminInputValidationTest— tenant create/delete happy paths, exclude/include with special-key read-back, rejection paths, and "nothing written" assertions for the removed operations (26 tests, green against the 5-node Docker cluster; full integration suite 265 tests green)Docs
docs/admin.md— Cluster Configuration / Force Recovery sections rewritten; validation contract table updatedCHANGELOG.md—[#43]entries under Added and ChangedCI:
composer lint+composer test:unit+ full integration suite green locally.