fix(#40): enforce explicit move bounds, reject self-subtree and partition-cross moves - #69
Merged
Merged
Conversation
added 2 commits
July 4, 2026 16:19
…tion-cross moves
DirectoryLayer::move() previously accepted any pair of
non-empty paths with no further validation, which let three
silent-permissive bugs through:
1. moving ['a'] into ['a','b'] produces a cycle / unreachable
subtree in the subdirs index;
2. moving ['p','a'] into ['x','a'] silently re-parents a
partition node under a non-partition parent (or vice-versa),
leaving the moved prefix in a different partition's content
space;
3. moving to a path with more than 64 segments lets a malformed
input produce arbitrarily-deep directory entries on a hot path.
Replace with explicit bounds + throw:
- validateMoveBounds(oldPath, newPath) runs before the
transaction and rejects: identical paths (no-op rewrite),
newPath under oldPath (cycle), depth > MAX_MOVE_PATH_DEPTH
(64 segments).
- assertSamePartitionLayer() runs inside the transaction and
rejects moves that cross between partition and non-partition
layers.
Both helpers throw DirectoryException with a printable rendering
of the offending paths (control bytes, DEL, high bytes, and '/'
inside segments are escaped to \xHH) so application input never
flows raw into log lines.
Tests:
- tests/Unit/DirectoryMoveBoundsValidationTest.php (29 cases):
printableSegment / printablePath boundaries, identical /
self-subtree / src-dest prefix rejections, MAX_MOVE_PATH_DEPTH
boundaries, partition-layer accept and reject paths.
- tests/Integration/DirectoryTest.php gains 7 new cases covering
immediate-subtree rejection, deeper-subtree rejection,
rejected-self-subtree leaves the index untouched, identical-
path rejection, sibling-rename happy path, re-parent-under-
existing-parent happy path, partition crossing rejection in
both directions, and same-partition move happy path.
Docs/CHANGELOG updated to document the new contract.
…own layer
CI caught two real bugs and one cosmetic defect in the original
patch:
1. The original partition-crossing rule compared oldPath's own
layer attribute against newPath's parent's layer. A child born
inside a partition carries layer="" on its own node, so a
move like ['app','part','child'] -> ['app','other'] would
compare oldLayer="" against newParentLayer="" and silently
succeed, even though the child lived inside a 'partition'
node and the move was a real partition crossing.
2. Conversely, ['app','part3','sibling_a'] -> ['app','part3','sibling_b']
was expected to succeed but with the source-vs-parent rule
also happened to work — but for the wrong reason. The check
needs to look at the immediate **parents**, not the source.
Fix: hoist oldParentNode lookup to the top of the transaction
(alongside the existing existence checks), compare
oldParentLayer against newParentLayer, and surface
'parent layer "X"' in the exception so the message carries
the actual partition identity that disagreed.
The unit tests for assertSamePartitionLayer are updated to
match the corrected exception text; the partition-crossing
integration tests stay undchanged at the substring level
('partition crossings are disallowed') so the message
contract remains observable.
Also fix: replace a backtick-delimited regex in
moveIntoOwnDeeperSubdirectoryThrows with the tilde
delimiter, since the original used '/.../' with literal
backticks that PHP didn't recognise as a valid pattern.
Tests:
- 29 unit tests in DirectoryMoveBoundsValidationTest.php
still pass with updated exception-text expectations.
- 9 integration tests in DirectoryTest.php unchanged
conceptually; one regex delimiter repaired.
Verified locally with composer lint and composer test:unit
(429/429 pass).
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.
Fixes #40.
Problem
DirectoryLayer::move()silently accepted any pair of non-empty paths with no further validation. Three silent-permissive bugs slipped through:move(['a'], ['a','b'])produced a cycle / unreachable subtree in the subdirs index.move(['p','a'], ['x','a'])silently re-parented a partition node under a non-partition parent (or vice-versa), leaving the moved prefix in a different layer's content space.Fix
Replace silent-permissive behaviour with explicit bounds + throw:
validateMoveBounds(oldPath, newPath)runs before the transaction and throwsDirectoryExceptionif:newPath === oldPath(no-op rewrite of the subdirs index)newPathbegins witholdPathas a prefix (cycle)MAX_MOVE_PATH_DEPTH(64 segments)assertSamePartitionLayer()runs inside the transaction and throwsDirectoryExceptionif the source's layer and the new parent's layer differ (rejects partition → top-level and top-level →partitioncrossings).printablePath()/printableSegment()render control bytes, DEL, high bytes, and/inside segments as\xHHso exception text never echoes raw caller input.set()/clear(), so a rejectedmove()does not leave partial state and the sameoldPathcan be retried with a differentnewPath.Tests
tests/Unit/DirectoryMoveBoundsValidationTest.php— 29 new pure-PHP cases(16
validateMoveBoundsaccept/reject + 8assertSamePartitionLayeraccept/reject + 5printableSegment/printablePathboundaries, all reflected throughvalidateMoveBounds,assertSamePartitionLayer,printablePath,printableSegment).tests/Integration/DirectoryTest.php— 9 new live-cluster cases:moveIntoOwnImmediateSubdirectoryThrowsmoveIntoOwnDeeperSubdirectoryThrowsmoveToIdenticalPathThrowsmoveIntoOwnSubtreeLeavesDirectoryIndexUntouched(proves the rejected transaction does not rewrite the index)moveToSiblingStillSucceeds(happy-path guard)moveIntoExistingParentStillSucceeds(re-parent happy path)moveWithEmptySourcePathThrows(regression for the existingvalidatePathguard)moveFromPartitionIntoTopLevelThrows/moveFromTopLevelIntoPartitionThrows/moveWithinSamePartitionStillSucceedsDocs
docs/directory-layer.md— "Moving Directories" section replaced with a violation-table documenting the new contract and the printable-rendering note.CHANGELOG.md— entry under [Unreleased] / Fixed referencing [Bug] DirectoryLayer::move allows moving a directory into its own subtree #40.Verification
composer lint(PHPCS + Rector + PHPStan level 9) is clean.composer test:unitpasses (515 total, 994 assertions; +29 new).composer test:integrationcannot run locally without the docker FDB cluster but the new integration tests follow the existingDatabaseCleanupTraitconventions and run on the same CI pipeline as the rest of the suite.