Fix prefix-boundary hole in filer root-escape checks#5497
Open
simonfaltum wants to merge 3 commits into
Open
Conversation
The root containment check in WorkspaceRootPath.Join and localRootPath.Join used a bare prefix match, so paths resolving to a sibling directory that shares the root as a string prefix (for example "/root-evil" for root "/root") passed the check. Require a separator boundary after the root, keep exact-root joins allowed, and preserve the unrooted local filer and "/"-rooted filer behavior. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
Collaborator
Integration test reportCommit: 6959000
36 interesting tests: 15 SKIP, 14 flaky, 7 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
requested review from
janniklasrose
and removed request for
shreyas-goenka
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Comment on lines
+23
to
+26
| // An empty root carries no restriction (cmd/fs constructs unrooted local filers). | ||
| if rp.rootPath == "" { | ||
| return absPath, nil | ||
| } |
Contributor
There was a problem hiding this comment.
what if name = "/i-am-root-now"?
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
3 participants
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.
Why
Found during a full-repo review of the CLI. All filers guard against relative paths escaping their root, but the guard in
libs/filer/workspace_root_path.goandlibs/filer/local_root_path.gowas a bare string prefix check. A path that resolves to a sibling directory sharing the root as a name prefix slipped through: with root/Users/me/proj, joining../proj-evil/xresolves to/Users/me/proj-evil/x, which passesstrings.HasPrefix. Every filer (workspace files, DBFS, UC volumes, local) shares these helpers, andbundle inittemplates write template-author-controlled paths through them.Changes
Before, a joined path only had to start with the root string; now it must either be exactly the root or extend it past a separator boundary.
WorkspaceRootPath.JoinandlocalRootPath.Joincompare against the root with a trailing separator and explicitly allow the exact-root result. Joins likeReadDir(".")resolve to exactly the root and keep working./(used by thefscommands) keep working: the separator is only appended when the cleaned root does not already end in one. Same for Windows drive roots likeC:\.NewLocalRootPath(""), used byfsfor local paths) keeps accepting any path.Test plan
../path-evil,../path-evil/x,../pathx) tolibs/filer/workspace_root_path_test.goandlibs/filer/local_root_path_test.go(Unix and Windows variants); these fail without the fix../path/xstill joins under the rootTestLocalRootPathEmptyRootcovering the unrooted local filer passthroughJoin(""),Join("."),Join("/")) pass unchangedgo test ./libs/filer/passesgo test ./libs/template/... ./libs/sync/...(direct consumers of these helpers) passes./task fmt-q,./task lint-q,./task checkspassThis pull request and its description were written by Isaac.