osfs: Replace filepath-securejoin with os.Root#158
osfs: Replace filepath-securejoin with os.Root#158pjbgf wants to merge 2 commits intogo-git:mainfrom
filepath-securejoin with os.Root#158Conversation
|
This change removes the previous on-demand costly evaluation of paths
with the Go's traversal resistent primitive os.Root.
The benchmarks in /test/ indicate that in most scenarios this represent
a positive performance:
│ base.txt │ pr.txt │
│ sec/op │ sec/op vs base │
Compare/osfs.boundOS_open-4 15.78µ ± 4% 13.38µ ± 2% -15.22% (p=0.002 n=6)
Compare/osfs.boundOS_read-4 88.45µ ± 1% 91.08µ ± 0% +2.97% (p=0.002 n=6)
Compare/osfs.boundOS_write-4 924.4µ ± 23% 867.2µ ± 18% ~ (p=0.180 n=6)
Compare/osfs.boundOS_create-4 31.39µ ± 51% 23.10µ ± 72% ~ (p=0.065 n=6)
Compare/osfs.boundOS_stat-4 9.493µ ± 2% 4.244µ ± 2% -55.30% (p=0.002 n=6)
Compare/osfs.boundOS_rename-4 68.14µ ± 1% 42.76µ ± 3% -37.26% (p=0.002 n=6)
Compare/osfs.boundOS_remove-4 30.14µ ± 2% 24.31µ ± 3% -19.34% (p=0.002 n=6)
Compare/osfs.boundOS_mkdirall-4 18.25µ ± 351% 17.74µ ± 303% ~ (p=0.699 n=6)
Compare/osfs.boundOS_tempfile-4 39.63µ ± 5% 34.35µ ± 2% -13.31% (p=0.002 n=6)
│ base.txt │ pr.txt │
│ B/op │ B/op vs base │
Compare/osfs.boundOS_open-4 1032.0 ± 0% 424.0 ± 0% -58.91% (p=0.002 n=6)
Compare/osfs.boundOS_read-4 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=6) ¹
Compare/osfs.boundOS_write-4 1507.0 ± 3% 261.0 ± 0% -82.68% (p=0.002 n=6)
Compare/osfs.boundOS_create-4 1536.5 ± 3% 278.0 ± 1% -81.91% (p=0.002 n=6)
Compare/osfs.boundOS_stat-4 1120.0 ± 0% 240.0 ± 0% -78.57% (p=0.002 n=6)
Compare/osfs.boundOS_rename-4 4845.0 ± 0% 122.0 ± 1% -97.48% (p=0.002 n=6)
Compare/osfs.boundOS_remove-4 1055.00 ± 0% 63.00 ± 0% -94.03% (p=0.002 n=6)
Compare/osfs.boundOS_mkdirall-4 2123.5 ± 20% 199.0 ± 8% -90.63% (p=0.002 n=6)
Compare/osfs.boundOS_tempfile-4 183.0 ± 1% 336.0 ± 0% +83.61% (p=0.002 n=6)
│ base.txt │ pr.txt │
│ allocs/op │ allocs/op vs base │
Compare/osfs.boundOS_open-4 21.000 ± 0% 9.000 ± 0% -57.14% (p=0.002 n=6)
Compare/osfs.boundOS_read-4 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=6) ¹
Compare/osfs.boundOS_write-4 25.000 ± 4% 8.000 ± 0% -68.00% (p=0.002 n=6)
Compare/osfs.boundOS_create-4 26.000 ± 4% 8.000 ± 0% -69.23% (p=0.002 n=6)
Compare/osfs.boundOS_stat-4 19.000 ± 0% 3.000 ± 0% -84.21% (p=0.002 n=6)
Compare/osfs.boundOS_rename-4 68.000 ± 0% 8.000 ± 0% -88.24% (p=0.002 n=6)
Compare/osfs.boundOS_remove-4 19.000 ± 0% 3.000 ± 0% -84.21% (p=0.002 n=6)
Compare/osfs.boundOS_mkdirall-4 32.500 ± 14% 8.000 ± 12% -75.38% (p=0.002 n=6)
Compare/osfs.boundOS_tempfile-4 6.000 ± 0% 14.000 ± 0% +133.33% (p=0.002 n=6)
A new ErrPathEscapesParent was introduced to represent when an operation is
attempting to escape the root/bound dir used for the bound OS.
Signed-off-by: Paulo Gomes <pjbgf@linux.com>
Without ensuring the all required modules are present, they will be downloaded on demand, which may result in the benchmark file starting with: go: downloading <module_name> <version> This in turn breaks benchstat, which becomes unable to compare the two benchmark results. Signed-off-by: Paulo Gomes <pjbgf@linux.com>
There was a problem hiding this comment.
Pull request overview
This pull request replaces the third-party filepath-securejoin library with Go 1.25's native os.Root API for path traversal protection in the BoundOS filesystem implementation. The change aims to improve performance through more efficient path validation while leveraging Go's built-in security primitives.
Changes:
- Replaces
filepath-securejoindependency withos.RootAPI, requiring Go 1.25.0 - Introduces
ErrPathEscapesParenterror andFromRootfunction for flexible root management - Updates all filesystem operations to use
os.Rootwith consistent error translation - Migrates tests from string-based error matching to sentinel error comparison using
errors.Is
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| osfs/os_bound.go | Core rewrite to use os.Root for all operations; adds FromRoot function, translateError helper, and dual-mode root lifecycle management |
| osfs/os_bound_test.go | Updates test assertions to use errors.Is; adds TestFromRoot; removes TestAbs and TestInsideBaseDirEval; adds new symlink test cases |
| osfs/os_options.go | Moves WithBoundOS and WithChrootOS option functions to separate file; removes WithDeduplicatePath option |
| osfs/os.go | Simplifies New function by removing deduplicatePath option handling |
| go.mod | Bumps Go version to 1.25.0; removes filepath-securejoin dependency |
| go.sum | Removes filepath-securejoin dependency entries |
| .github/workflows/bench-regression.yml | Adds explicit go mod download steps for better caching |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (fs *BoundOS) MkdirAll(name string, _ gofs.FileMode) error { | ||
| root, cleanup, err := fs.fsRoot() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.MkdirAll(dir, perm) | ||
| defer cleanup() | ||
|
|
||
| // os.Root errors when perm contains bits other than the nine least-significant bits (0o777). | ||
| err = root.MkdirAll(name, 0o777) | ||
| return translateError(err, name) |
There was a problem hiding this comment.
The MkdirAll method ignores the provided perm parameter and always uses 0o777 (line 184). While this is mentioned in the comment, it represents a behavioral change from standard filesystem semantics where the caller expects to control directory permissions. This could be surprising to users and may cause issues if specific permissions are required. Consider documenting this more prominently in the method's godoc comment, or applying the permission to the final directory in the path after creation if os.Root allows it.
| dir, err := fs.abs(path) | ||
| f, err := root.OpenFile(name, flag, perm) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
The error returned from root.OpenFile is not being translated, which means that path escape errors from os.Root won't be wrapped with ErrPathEscapesParent. This is inconsistent with other methods in this file that call translateError on errors returned from root operations. Consider changing line 120 to return nil, translateError(err, name) for consistency.
| return nil, err | |
| return nil, translateError(err, name) |
| // Ensure the target directory exists. | ||
| err = root.MkdirAll(filepath.Dir(to), defaultDirectoryMode) | ||
| if err == nil { | ||
| err = root.Rename(from, to) | ||
| } | ||
|
|
||
| return os.Rename(f, t) | ||
| return translateError(err, to) |
There was a problem hiding this comment.
The Rename method translates errors using only the to parameter. However, errors can occur from both the from and to paths (e.g., if from escapes the root). While the current implementation may be acceptable if the error message is sufficient, consider whether the error should include information about which path caused the issue. If both paths could cause errors, you may want to handle them separately or include both in the error message.
|
|
||
| _, err = os.Stat(fs.baseDir) | ||
| if err != nil && os.IsNotExist(err) { | ||
| err = os.MkdirAll(fs.baseDir, defaultDirectoryMode) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
The Stat method creates the base directory if it doesn't exist when an absolute path is provided (lines 199-205). This side effect is unexpected for a read operation like Stat. This behavior could lead to unintended directory creation and may not be consistent with standard filesystem semantics where Stat is a read-only operation. Consider whether this auto-creation is intentional and necessary, or if it should be removed.
| _, err = os.Stat(fs.baseDir) | |
| if err != nil && os.IsNotExist(err) { | |
| err = os.MkdirAll(fs.baseDir, defaultDirectoryMode) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } |
| defer childRoot.Close() | ||
|
|
||
| return New(childRoot.Name(), WithBoundOS()), nil |
There was a problem hiding this comment.
The childRoot is closed immediately after being opened (line 384), but then childRoot.Name() is used to create a new filesystem (line 386). After the defer statement executes, the childRoot is closed, so the new filesystem created on line 386 will have a base directory pointing to a path from a now-closed os.Root. While childRoot.Name() returns a string path that remains valid after closing, this pattern is confusing and could be error-prone. Consider storing childRoot.Name() in a variable before the defer, or restructure to make the lifecycle clearer.
| defer childRoot.Close() | |
| return New(childRoot.Name(), WithBoundOS()), nil | |
| childRootPath := childRoot.Name() | |
| if err := childRoot.Close(); err != nil { | |
| return nil, fmt.Errorf("unable to close child root: %w", err) | |
| } | |
| return New(childRootPath, WithBoundOS()), nil |
| dir = os.TempDir() | ||
| f, err = fs.TempFile("../../../above/cwd", "prefix") | ||
| require.ErrorIs(t, err, ErrPathEscapesParent) | ||
| assert.Nil(f) | ||
|
|
||
| dir = filepath.Join(dir, "/tmp") | ||
| // For windows, volume name must be removed. | ||
| if v := filepath.VolumeName(dir); v != "" { | ||
| dir = strings.TrimPrefix(dir, v) | ||
| } | ||
|
|
||
| f, err = fs.TempFile(dir, "prefix") |
There was a problem hiding this comment.
The dir variable is reassigned to os.TempDir() on line 384, which changes the reference point for subsequent tests. This appears to be a bug because the test cases after this line are checking paths relative to the reassigned dir, which is no longer the bound directory of the filesystem. The filesystem fs was created with the original dir (from line 369), so the later assertions are testing with an unrelated directory path. This looks like it may have been a copy-paste error from previous test code.
| dir = os.TempDir() | |
| f, err = fs.TempFile("../../../above/cwd", "prefix") | |
| require.ErrorIs(t, err, ErrPathEscapesParent) | |
| assert.Nil(f) | |
| dir = filepath.Join(dir, "/tmp") | |
| // For windows, volume name must be removed. | |
| if v := filepath.VolumeName(dir); v != "" { | |
| dir = strings.TrimPrefix(dir, v) | |
| } | |
| f, err = fs.TempFile(dir, "prefix") | |
| f, err = fs.TempFile("../../../above/cwd", "prefix") | |
| require.ErrorIs(t, err, ErrPathEscapesParent) | |
| assert.Nil(f) | |
| outsideDir := filepath.Join(os.TempDir(), "/tmp") | |
| // For windows, volume name must be removed. | |
| if v := filepath.VolumeName(outsideDir); v != "" { | |
| outsideDir = strings.TrimPrefix(outsideDir, v) | |
| } | |
| f, err = fs.TempFile(outsideDir, "prefix") |
| // When not creating, read symlink links so that they can be made | ||
| // relative and therefore work. | ||
| if flag&os.O_CREATE == 0 { | ||
| fi, err := root.Lstat(name) | ||
| if err == nil && fi.Mode()&gofs.ModeSymlink != 0 { | ||
| fn, err := root.Readlink(name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| name = fn | ||
| } | ||
| } | ||
|
|
||
| if filepath.IsAbs(name) { | ||
| fn, err := filepath.Rel(fs.baseDir, name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| name = fn | ||
| } |
There was a problem hiding this comment.
In the OpenFile method, when a symlink is detected (lines 94-101), the code reads the symlink target and replaces name with it. However, if the symlink target is an absolute path, the subsequent check on line 104 will try to make it relative to fs.baseDir. This could cause issues if the symlink target is an absolute path that doesn't descend from fs.baseDir, as filepath.Rel may return an error or an unexpected path. Consider handling the case where the symlink target is an absolute path that escapes the base directory more explicitly.
This change removes the previous on-demand costly evaluation of paths and replaces it with Go's traversal resistent primitive
os.Root.The benchmark tests in
/test/show that in most scenarios this has a positive performance impact. The numbers below are based usingFromRootwhich enables reuse of an activeos.Rootacross several operations:The default behaviour is for each operation to open and close a
os.Root, which increases the cost per operation but still looks largely better than the previous implementation, with the exception of:For a full break-down, refer to the comment below.
A new
ErrPathEscapesParentwas introduced to represent when an operation isattempting to escape the root/bound dir used for the bound OS.
Requires Go
1.25.Fixes #135.
Relates to #101.