Copilot commented on code in PR #1135:
URL:
https://github.com/apache/skywalking-banyandb/pull/1135#discussion_r3265242869
##########
pkg/fs/remote/local/local.go:
##########
@@ -89,10 +108,75 @@ func (l *fs) List(_ context.Context, prefix string)
([]string, error) {
}
func (l *fs) Delete(_ context.Context, path string) error {
- fullPath := filepath.Join(l.baseDir, path)
+ fullPath, err := l.fullPath(path)
+ if err != nil {
+ return err
+ }
return os.Remove(fullPath)
}
func (l *fs) Close() error {
return nil
}
+
+func (l *fs) fullPath(path string) (string, error) {
+ if filepath.IsAbs(path) || hasVolumeName(path) {
+ return "", fmt.Errorf("path %q escapes base directory", path)
+ }
+ cleanPath := filepath.Clean(path)
+ if hasVolumeName(cleanPath) || cleanPath == ".." ||
strings.HasPrefix(cleanPath, ".."+string(filepath.Separator)) {
Review Comment:
`fullPath` currently allows `path == ""` and `path == "."` (since
`filepath.Clean("")` becomes `"."`). That means `Delete(".", ...)` targets
`l.baseDir` itself, and `Download(".")` opens the base directory; depending on
runtime conditions (e.g., empty directory), this broadens what a caller can
affect beyond a “file under baseDir”. Consider rejecting `"."`/empty explicitly
(or at least disallowing them for `Delete`), returning the same “escapes base
directory” error to keep behavior consistent.
##########
pkg/fs/remote/local/local.go:
##########
@@ -89,10 +108,75 @@ func (l *fs) List(_ context.Context, prefix string)
([]string, error) {
}
func (l *fs) Delete(_ context.Context, path string) error {
- fullPath := filepath.Join(l.baseDir, path)
+ fullPath, err := l.fullPath(path)
+ if err != nil {
+ return err
+ }
return os.Remove(fullPath)
}
func (l *fs) Close() error {
return nil
}
+
+func (l *fs) fullPath(path string) (string, error) {
+ if filepath.IsAbs(path) || hasVolumeName(path) {
+ return "", fmt.Errorf("path %q escapes base directory", path)
+ }
+ cleanPath := filepath.Clean(path)
+ if hasVolumeName(cleanPath) || cleanPath == ".." ||
strings.HasPrefix(cleanPath, ".."+string(filepath.Separator)) {
+ return "", fmt.Errorf("path %q escapes base directory", path)
+ }
+ fullPath := filepath.Join(l.baseDir, cleanPath)
+ relPath, err := filepath.Rel(l.baseDir, fullPath)
+ if err != nil {
+ return "", err
+ }
+ if relPath == ".." || strings.HasPrefix(relPath,
".."+string(filepath.Separator)) || filepath.IsAbs(relPath) {
+ return "", fmt.Errorf("path %q escapes base directory", path)
+ }
+ if err := l.ensureResolvedWithinBase(fullPath); err != nil {
+ return "", fmt.Errorf("path %q escapes base directory: %w",
path, err)
+ }
+ return fullPath, nil
+}
+
+func (l *fs) ensureResolvedWithinBase(path string) error {
+ existingPath := path
+ for {
+ if _, err := os.Lstat(existingPath); err == nil {
+ break
+ } else if !os.IsNotExist(err) {
+ return err
+ }
+ parentPath := filepath.Dir(existingPath)
+ if parentPath == existingPath {
+ return os.ErrNotExist
+ }
+ existingPath = parentPath
+ }
+
+ realPath, err := filepath.EvalSymlinks(existingPath)
+ if err != nil {
+ return err
+ }
Review Comment:
There is a check-then-use window between
`fullPath`/`ensureResolvedWithinBase` and the subsequent filesystem operations
(`MkdirAll`, `Create`, `Open`, `Remove`). An attacker with write access under
`baseDir` could potentially flip a directory component into a symlink after
validation but before the operation (TOCTOU), reintroducing a symlink-escape
vector. If this code is used in an untrusted environment, consider tightening
this by validating the resolved parent directory immediately before the final
open/create, or by using OS-specific “no-follow” primitives where feasible
(noting portability tradeoffs).
##########
banyand/backup/restore.go:
##########
@@ -244,6 +243,38 @@ func restoreByName(fs remote.FS, timeDir, rootPath,
catalogName string) error {
return nil
}
+func validatedRemoteRelPath(timeDir, catalogName, remoteFile string) (string,
error) {
+ remotePath := filepath.ToSlash(remoteFile)
+ if path.IsAbs(remotePath) || hasRemoteVolumeName(remotePath) {
+ return "", fmt.Errorf("remote file %q escapes backup prefix",
remoteFile)
+ }
+ prefix := path.Clean(filepath.ToSlash(filepath.Join(timeDir,
catalogName)))
+ cleanRemotePath := path.Clean(remotePath)
+ if hasRemoteVolumeName(cleanRemotePath) || cleanRemotePath == "." ||
prefix == "." {
+ return "", fmt.Errorf("remote file %q escapes backup prefix",
remoteFile)
+ }
+ relPath, err := filepath.Rel(filepath.FromSlash(prefix),
filepath.FromSlash(cleanRemotePath))
+ if err != nil {
+ return "", fmt.Errorf("failed to get relative path for %s: %w",
remoteFile, err)
+ }
+ relPath = filepath.ToSlash(relPath)
+ if relPath == "." || strings.HasPrefix(relPath, "../") || relPath ==
".." || path.IsAbs(relPath) {
+ return "", fmt.Errorf("remote file %q escapes backup prefix",
remoteFile)
+ }
+ return relPath, nil
+}
Review Comment:
`validatedRemoteRelPath` mixes `path` (POSIX `/`) and `filepath`
(OS-specific) and requires multiple `ToSlash`/`FromSlash` conversions, which
makes correctness harder to reason about (especially on Windows). Since
`remoteFile` values are treated as “remote paths” (slash-separated), consider
using `path.Clean` + `path.Rel` end-to-end (plus your volume-name guard) and
keeping `filepath` only for local filesystem paths. This reduces
platform-dependent behavior and simplifies the invariant: “remote paths always
use `/`”.
##########
pkg/fs/remote/local/local.go:
##########
@@ -60,15 +73,21 @@ func (l *fs) Upload(_ context.Context, path string, data
io.Reader) error {
}
func (l *fs) Download(_ context.Context, path string) (io.ReadCloser, error) {
- fullPath := filepath.Join(l.baseDir, path)
+ fullPath, err := l.fullPath(path)
+ if err != nil {
+ return nil, err
+ }
return os.Open(fullPath)
}
func (l *fs) List(_ context.Context, prefix string) ([]string, error) {
var files []string
- fullPath := filepath.Join(l.baseDir, prefix)
+ fullPath, err := l.fullPath(prefix)
+ if err != nil {
+ return nil, err
+ }
- err := filepath.Walk(fullPath, func(path string, info os.FileInfo, err
error) error {
+ err = filepath.Walk(fullPath, func(path string, info os.FileInfo, err
error) error {
if err != nil {
return err
Review Comment:
`List` will return an error when `prefix` doesn’t exist because
`filepath.Walk(fullPath, ...)` fails with `os.ErrNotExist`. This directly
contradicts the new `TestFSListMissingPrefixReturnsEmpty` test, which expects
`(empty slice, nil)`. Handle the missing-root case explicitly (e.g., detect
`os.IsNotExist(err)` from `Walk`/`Stat` and return an empty list without error).
##########
banyand/backup/restore.go:
##########
@@ -244,6 +243,38 @@ func restoreByName(fs remote.FS, timeDir, rootPath,
catalogName string) error {
return nil
}
+func validatedRemoteRelPath(timeDir, catalogName, remoteFile string) (string,
error) {
+ remotePath := filepath.ToSlash(remoteFile)
+ if path.IsAbs(remotePath) || hasRemoteVolumeName(remotePath) {
+ return "", fmt.Errorf("remote file %q escapes backup prefix",
remoteFile)
+ }
+ prefix := path.Clean(filepath.ToSlash(filepath.Join(timeDir,
catalogName)))
+ cleanRemotePath := path.Clean(remotePath)
+ if hasRemoteVolumeName(cleanRemotePath) || cleanRemotePath == "." ||
prefix == "." {
+ return "", fmt.Errorf("remote file %q escapes backup prefix",
remoteFile)
+ }
+ relPath, err := filepath.Rel(filepath.FromSlash(prefix),
filepath.FromSlash(cleanRemotePath))
+ if err != nil {
+ return "", fmt.Errorf("failed to get relative path for %s: %w",
remoteFile, err)
+ }
+ relPath = filepath.ToSlash(relPath)
+ if relPath == "." || strings.HasPrefix(relPath, "../") || relPath ==
".." || path.IsAbs(relPath) {
+ return "", fmt.Errorf("remote file %q escapes backup prefix",
remoteFile)
+ }
+ return relPath, nil
+}
+
+func hasRemoteVolumeName(path string) bool {
+ if filepath.VolumeName(filepath.FromSlash(path)) != "" {
+ return true
+ }
+ return len(path) >= 2 && isASCIILetter(path[0]) && path[1] == ':'
+}
+
+func isASCIILetter(c byte) bool {
+ return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')
+}
Review Comment:
`hasRemoteVolumeName`/`isASCIILetter` duplicate the equivalent helpers
introduced in `pkg/fs/remote/local/local.go`. To prevent the two
implementations drifting over time, consider extracting a shared helper (e.g.,
a small internal package) or reusing a single implementation if the repository
layout allows it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
