osfs: Replace filepath-securejoin with os.Root#158
Open
pjbgf wants to merge 6 commits intogo-git:mainfrom
Open
osfs: Replace filepath-securejoin with os.Root#158pjbgf wants to merge 6 commits intogo-git:mainfrom
filepath-securejoin with os.Root#158pjbgf wants to merge 6 commits intogo-git:mainfrom
Conversation
|
pjbgf
force-pushed
the
goroot
branch
2 times, most recently
from
0adffac to
cfcfa7b
Compare
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.
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>
Capabilities() used bitwise AND instead of OR, reporting no capabilities. translateError() swallowed errors when Unwrap returned nil and relied on exact string matching. Stat() created the base dir outside os.Root as a side-effect. Abs-to-relative conversion was duplicated across six methods. MkdirAll() silently discarded the caller's permission mode. Chroot() downgraded FromRoot instances to per-operation mode. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io>
Cache os.Root in newBoundOS to eliminate per-operation OpenRoot/Close syscall pairs. Defer symlink resolution in OpenFile to a retry path that only triggers on path-escape errors, removing an Lstat from every non-create open. Simplify fsRoot and Chroot now that os.Root is always cached. Add benchmarks for Create, Stat, Lstat, Rename and Remove. Fix WalkDir benchmark to use fs.WalkDir via iofs adapter for all implementations. Exclude setup cost from Rename and Remove benchmarks. Add TestOpenAbsSymlinkInsideRoot to prove the symlink fallback in OpenFile is needed: os.Root rejects absolute symlink targets that point inside the root, and BoundOS resolves them to relative paths. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+135
to
+144
| f, err := root.Open(name) | ||
| if err != nil { | ||
| return nil, translateError(err, name) | ||
| } | ||
|
|
||
| e, err := f.ReadDir(-1) | ||
| if err != nil { | ||
| return nil, translateError(err, name) | ||
| } | ||
| return e, nil |
There was a problem hiding this comment.
ReadDir opens a directory file via root.Open but never closes it. This will leak file descriptors over time (and can exhaust them under load). Ensure the opened handle is closed (e.g., defer Close) after reading entries, including on error paths.
Comment on lines
+169
to
+177
| func (fs *BoundOS) MkdirAll(name string, perm gofs.FileMode) error { | ||
| root, err := fs.fsRoot() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.MkdirAll(dir, perm) | ||
|
|
||
| // os.Root errors when perm contains bits other than the nine least-significant bits (0o777). | ||
| err = root.MkdirAll(name, perm&0o777) | ||
| return translateError(err, name) |
There was a problem hiding this comment.
MkdirAll does not call toRelative on the provided path, unlike most other methods. As a result, passing an absolute path that is still within baseDir (e.g. filepath.Join(fs.Root(), "sub")) will be treated by os.Root as an escape and fail. Consider normalizing with toRelative before calling root.MkdirAll so BoundOS consistently accepts absolute-inside-root paths.
|
|
||
| err = fs.createDir(root, newname) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Symlink returns the raw error from createDir. If createDir fails due to an os.Root path-escape, callers won’t get ErrPathEscapesParent like they do for other operations. Translate the createDir error with translateError (using newname) for consistent error semantics.
Suggested change
| return err | |
| return translateError(err, newname) |
| return err | ||
| } | ||
| return os.Chmod(abspath, mode) | ||
| return root.Chmod(path, mode) |
There was a problem hiding this comment.
Chmod returns root.Chmod errors directly without mapping os.Root path-escape failures to ErrPathEscapesParent. For consistency with the rest of BoundOS (OpenFile/Stat/Remove/etc.), wrap the result with translateError.
Suggested change
| return root.Chmod(path, mode) | |
| return translateError(root.Chmod(path, mode), path) |
Comment on lines
+316
to
+331
| fi, err := root.Lstat(path) | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| err := root.MkdirAll(path, defaultDirectoryMode) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to auto create dir: %w", err) | ||
| } | ||
| } else if err != nil { | ||
| return nil, err | ||
| } | ||
| if fi != nil && !fi.IsDir() { | ||
| return nil, fmt.Errorf("cannot chroot: path is not dir") | ||
| } | ||
|
|
||
| childRoot, err := root.OpenRoot(path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to chroot: %w", err) |
There was a problem hiding this comment.
Chroot uses the provided path directly with root.Lstat/MkdirAll/OpenRoot, without applying toRelative or translateError. This can break callers that pass absolute paths within baseDir and can also leak os.Root’s internal escape error instead of ErrPathEscapesParent. Normalize the path (toRelative + special-case ""/".") and translate path-escape errors consistently (including errors from the auto-MkdirAll).
Suggested change
| fi, err := root.Lstat(path) | |
| if errors.Is(err, os.ErrNotExist) { | |
| err := root.MkdirAll(path, defaultDirectoryMode) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to auto create dir: %w", err) | |
| } | |
| } else if err != nil { | |
| return nil, err | |
| } | |
| if fi != nil && !fi.IsDir() { | |
| return nil, fmt.Errorf("cannot chroot: path is not dir") | |
| } | |
| childRoot, err := root.OpenRoot(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to chroot: %w", err) | |
| relPath := fs.toRelative(path) | |
| if relPath == "" || relPath == "." { | |
| childRoot, err := root.OpenRoot(".") | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to chroot: %w", translateError(err, path)) | |
| } | |
| return FromRoot(childRoot), nil | |
| } | |
| fi, err := root.Lstat(relPath) | |
| if errors.Is(err, os.ErrNotExist) { | |
| err := root.MkdirAll(relPath, defaultDirectoryMode) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to auto create dir: %w", translateError(err, path)) | |
| } | |
| } else if err != nil { | |
| return nil, translateError(err, path) | |
| } | |
| if fi != nil && !fi.IsDir() { | |
| return nil, fmt.Errorf("cannot chroot: path is not dir") | |
| } | |
| childRoot, err := root.OpenRoot(relPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("unable to chroot: %w", translateError(err, path)) |
Comment on lines
+374
to
+387
| func isPathEscapeError(err error) bool { | ||
| return err != nil && strings.Contains(err.Error(), ErrPathEscapesParent.Error()) | ||
| } | ||
|
|
||
| // insideBaseDirEval checks whether filename is contained within | ||
| // a dir that is within the fs.baseDir, by first evaluating any symlinks | ||
| // that either filename or fs.baseDir may contain. | ||
| func (fs *BoundOS) insideBaseDirEval(filename string) (bool, error) { | ||
| if fs.baseDir == "/" || fs.baseDir == "" || fs.baseDir == filename { | ||
| return true, nil | ||
| } | ||
| dir, err := filepath.EvalSymlinks(filepath.Dir(filename)) | ||
| if dir == "" || os.IsNotExist(err) { | ||
| dir = filepath.Dir(filename) | ||
| func translateError(err error, file string) error { | ||
| if err == nil { | ||
| return nil | ||
| } | ||
| wd, err := filepath.EvalSymlinks(fs.baseDir) | ||
| if wd == "" || os.IsNotExist(err) { | ||
| wd = fs.baseDir | ||
| } | ||
| if filename != wd && dir != wd && !strings.HasPrefix(dir, wd+string(filepath.Separator)) { | ||
| return false, fmt.Errorf("%q: path outside base dir %q: %w", filename, fs.baseDir, os.ErrNotExist) | ||
|
|
||
| if isPathEscapeError(err) { | ||
| return fmt.Errorf("%w: %q", ErrPathEscapesParent, file) | ||
| } | ||
| return true, nil | ||
|
|
||
| return err |
There was a problem hiding this comment.
isPathEscapeError relies on substring matching of err.Error(). This is brittle across Go versions/OSes and may fail to detect (or falsely detect) path-escape conditions. If possible, prefer a structured check (e.g., errors.As to *fs.PathError and inspection of the underlying error) and keep the string-match as a last resort.
Comment on lines
+50
to
77
| // BoundOS is a fs implementation based on the OS filesystem which relies on | ||
| // Go's os.Root. Prefer this fs implementation over ChrootOS. | ||
| // | ||
| // An [os.Root] is opened once and reused for all operations. When created | ||
| // via [FromRoot], the caller is responsible for its lifecycle. When created | ||
| // via [New] with [WithBoundOS], the root is opened eagerly and released by | ||
| // the garbage collector's finalizer (same as [os.File]). | ||
| // | ||
| // Behaviours of note: | ||
| // 1. Read and write operations can only be directed to files which descends | ||
| // from the base dir. | ||
| // 2. Symlinks don't have their targets modified, and therefore can point | ||
| // to locations outside the base dir or to non-existent paths. | ||
| // 3. Readlink and Lstat ensures that the link file is located within the base | ||
| // dir, evaluating any symlinks that file or base dir may contain. | ||
| // 3. Operations leading to escapes to outside the [os.Root] location results | ||
| // in [ErrPathEscapesParent]. | ||
| type BoundOS struct { | ||
| baseDir string | ||
| deduplicatePath bool | ||
| baseDir string | ||
| root *os.Root | ||
| rootError error | ||
| } | ||
|
|
||
| func newBoundOS(d string, deduplicatePath bool) billy.Filesystem { | ||
| return &BoundOS{baseDir: d, deduplicatePath: deduplicatePath} | ||
| func newBoundOS(d string) billy.Filesystem { | ||
| r, err := os.OpenRoot(d) | ||
| if err != nil { | ||
| return &BoundOS{baseDir: d, rootError: err} | ||
| } | ||
| return &BoundOS{baseDir: d, root: r} | ||
| } |
There was a problem hiding this comment.
The PR description states the default behavior opens/closes an os.Root per operation, but this implementation opens os.Root once in newBoundOS and retains it for all operations (relying on GC finalization for cleanup). Either update the PR description (and/or doc comment) to reflect the actual lifecycle, or adjust the implementation to match the described per-operation behavior if that’s the intended default.
Comment on lines
+54
to
66
| func BenchmarkOpen(b *testing.B) { | ||
| e := newBenchEnv(b, true) | ||
| b.Run("memfs", benchmarkOpen(e.mem)) | ||
| b.Run("chrootOS", benchmarkOpen(e.chroot)) | ||
| b.Run("boundOS", benchmarkOpen(e.bound)) | ||
| b.Run("go-lib", func(b *testing.B) { | ||
| for b.Loop() { | ||
| _, err := root.Open(fileName) | ||
| _, err := e.root.Open(fileName) | ||
| if err != nil { | ||
| b.Fatal("cannot open file", "error", err) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
BenchmarkOpen’s go-lib sub-benchmark opens a file each iteration but never closes it. This can exhaust file descriptors and skew benchmark results. Close the returned file on each iteration (and similarly ensure billy fs opens are closed if they return OS-backed handles).
Comment on lines
+28
to
+52
| func newBenchEnv(b *testing.B, withFile bool) benchEnv { | ||
| b.Helper() | ||
| b.StopTimer() | ||
|
|
||
| baseDir := b.TempDir() | ||
| root, err := os.OpenRoot(baseDir) | ||
| require.NoError(b, err) | ||
|
|
||
| osfn := filepath.Join(baseDir, fileName) | ||
|
|
||
| err = os.WriteFile(osfn, []byte("test"), 0o600) | ||
| require.NoError(b, err) | ||
|
|
||
| m := memfs.New() | ||
| err = util.WriteFile(m, fileName, []byte("test"), 0o600) | ||
| require.NoError(b, err) | ||
| if withFile { | ||
| err = os.WriteFile(filepath.Join(baseDir, fileName), []byte("test"), 0o600) | ||
| require.NoError(b, err) | ||
| err = util.WriteFile(m, fileName, []byte("test"), 0o600) | ||
| require.NoError(b, err) | ||
| } | ||
|
|
||
| b.StartTimer() | ||
| return benchEnv{ | ||
| baseDir: baseDir, | ||
| root: root, | ||
| mem: m, | ||
| chroot: osfs.New(baseDir, osfs.WithChrootOS()), | ||
| bound: osfs.New(baseDir, osfs.WithBoundOS()), | ||
| } | ||
| } |
There was a problem hiding this comment.
newBenchEnv/newBenchEnvMany open an *os.Root but never close it. Add b.Cleanup (or defer outside the timed section) to close the root to avoid leaking descriptors across benchmark runs/sub-benchmarks.
Comment on lines
+165
to
+185
| func BenchmarkRename(b *testing.B) { | ||
| e := newBenchEnv(b, false) | ||
| b.Run("memfs", benchmarkRename(e.mem)) | ||
| b.Run("chrootOS", benchmarkRename(e.chroot)) | ||
| b.Run("boundOS", benchmarkRename(e.bound)) | ||
| b.Run("go-lib", func(b *testing.B) { | ||
| for b.Loop() { | ||
| b.StopTimer() | ||
| f, err := e.root.Create("rename-src") | ||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| f.Close() | ||
| b.StartTimer() | ||
|
|
||
| err = e.root.Rename("rename-src", "rename-dst") | ||
| if err != nil { | ||
| b.Fatal("cannot rename file", "error", err) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
BenchmarkRename renames to a constant destination ("rename-dst") without removing/resetting it between iterations. On Windows, rename fails if the destination exists, so this benchmark can fail after the first iteration. Consider using unique dst names per iteration or removing/renaming back as part of the per-iteration setup (outside the timed section).
Introduce RootOS, a high-performance filesystem backed by a caller-managed os.Root that is reused across all operations. BoundOS becomes a thin wrapper that opens and closes an os.Root per operation, avoiding leaked directory handles on Windows. FromRoot now returns (*RootOS, error), validating the root upfront rather than checking on every operation. RootOS.Chroot returns a child RootOS; BoundOS.Chroot returns a child BoundOS. Add rootOS sub-benchmarks alongside the existing implementations. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io>
Signed-off-by: Paulo Gomes <paulo@entire.io>
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
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.
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.