Compare paths on segment boundaries in isSubPath() - #493
Merged
kingjia90 merged 1 commit intoAug 20, 2026
Conversation
isSubPath() used a plain str_starts_with(), so a sibling whose name merely starts with another path counted as its child - "/Catalog_Archive" was treated as a child of "/Catalog". removeSubPaths() therefore dropped such a sibling from the allowed main paths in the workspace query, and since the resulting terms filter on fullPath only covers a path and its descendants, the folder never matched: it stayed invisible in the element tree although the workspace granted list permission on exactly that path. The change is strictly narrowing, only the previous false positives disappear. For the root path the rtrim leaves "/", which every absolute path starts with, so "/" keeps matching everything below it.
|
All contributors have signed the CLA ✍️ ✅ |
timwiechers
marked this pull request as ready for review
August 10, 2026 12:55
pimcore-deployments
marked this pull request as draft
August 10, 2026 12:55
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes hierarchy checks so similarly prefixed sibling paths remain distinct.
Changes:
- Compares paths at
/segment boundaries. - Adds regression tests for prefix siblings and root paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Service/PathService.php |
Corrects subpath detection. |
tests/Unit/Service/PathServiceTest.php |
Adds regression coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
timwiechers
marked this pull request as ready for review
August 10, 2026 12:57
pimcore-deployments
marked this pull request as draft
August 10, 2026 12:58
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 pimcore/platform-version#314
Problem
PathService::isSubPath()compares with a plainstr_starts_with():There is no
/boundary, so a sibling whose name merely starts with another path counts as its child —/Catalog_Archiveis treated as a child of/Catalog.QueryService::addQueryByMainPath()runs the allowed workspace paths throughremoveSubPaths(), which therefore drops/Catalog_Archivefrom$allowedMainPaths. The resultingtermsfilter onsystem_fields.fullPathcovers a path and its descendants (path_hierarchytokenizer, splitting on/), and/Catalogdoes not match/Catalog_Archive— it is a sibling, not a descendant.Effect for the user: a folder with
listgranted on exactly its own path is missing from the element tree, while sibling folders without a prefix neighbour show up normally. Only the tree is affected, since it reads from the index; the SQL-level permission check reports the same folder as visible.isSubPath()also backscontainsSubPath(),getContainedSubPaths()andkeepDeclinedPathsWithinAllowed(), which all want real hierarchy as well.This is the index-side counterpart of pimcore/platform-version#214, where the same missing boundary was fixed for the SQL permission conditions in the core.
Fix
Compare against the parent path plus its separator:
Strictly narrowing — only the previous false positives disappear. The root path stays intact:
rtrim('/', '/') . '/'is/, which every absolute path starts with.Tests
Extended
tests/Unit/Service/PathServiceTest.php:testIsSubPath()— prefix siblings are not sub paths (/foobarand/foo_barunder/foo,/foo/barbazunder/foo/bar), and/foobaris still a sub path of/.testRemoveSubPaths()—['/foo', '/foo_bar']is kept as-is instead of collapsing to['/foo'].All pre-existing assertions in that file stay unchanged and green.