Copilot commented on code in PR #1135:
URL:
https://github.com/apache/skywalking-banyandb/pull/1135#discussion_r3263045788
##########
banyand/backup/restore.go:
##########
@@ -193,13 +194,12 @@ func restoreByName(fs remote.FS, timeDir, rootPath,
catalogName string) error {
logger.Infof("Restoring %s to %s from %s, remote total %d files",
catalogName, localDir, remotePrefix, len(remoteFiles))
remoteRelSet := make(map[string]bool)
- var relPath string
for _, remoteFile := range remoteFiles {
- relPath, err = filepath.Rel(timeDir, remoteFile)
- if err != nil {
- return fmt.Errorf("failed to get relative path for %s:
%w", remoteFile, err)
+ relPath, relPathErr := validatedRemoteRelPath(timeDir,
catalogName, remoteFile)
+ if relPathErr != nil {
+ return relPathErr
}
- remoteRelSet[filepath.ToSlash(relPath)] = true
+ remoteRelSet[path.Join(catalogName, relPath)] = true
}
Review Comment:
`remoteRelSet` keys are built with `path.Join(...)` (always `/` separators),
but later lookups use `filepath.Join(catalogName, localRelPath)` which is
OS-dependent. On Windows this can cause mismatched keys and unintended local
deletions during restore. Consider normalizing both sides to slash paths (e.g.,
use `path.Join` consistently or apply `filepath.ToSlash` before
inserting/looking up).
##########
pkg/fs/remote/local/local.go:
##########
@@ -89,10 +104,32 @@ 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) {
+ return "", fmt.Errorf("path %q escapes base directory", path)
+ }
+ cleanPath := filepath.Clean(path)
Review Comment:
`fullPath` rejects absolute paths via `filepath.IsAbs`, but on Windows some
paths with a volume name (e.g. `C:foo`) are not considered absolute yet can
escape `baseDir` semantics in `filepath.Join`. Consider rejecting any input
with a non-empty `filepath.VolumeName(path)` / `filepath.VolumeName(cleanPath)`
to make the base-dir containment check robust across platforms.
##########
banyand/backup/restore.go:
##########
@@ -244,6 +243,27 @@ 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) {
+ return "", fmt.Errorf("remote file %q escapes backup prefix",
remoteFile)
+ }
+ prefix := path.Clean(filepath.ToSlash(filepath.Join(timeDir,
catalogName)))
+ cleanRemotePath := path.Clean(remotePath)
Review Comment:
`validatedRemoteRelPath` only uses `path.IsAbs(remotePath)`, which won’t
treat Windows drive-letter paths like `C:/...` as absolute. If restore is run
on Windows (or receives keys with drive letters), this may allow unexpected
prefixes to pass validation. Consider additionally rejecting when
`filepath.IsAbs(filepath.FromSlash(remotePath))` is true or when
`filepath.VolumeName(filepath.FromSlash(remotePath))` is non-empty.
##########
pkg/fs/remote/local/local.go:
##########
@@ -89,10 +104,32 @@ 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) {
+ return "", fmt.Errorf("path %q escapes base directory", path)
+ }
+ cleanPath := filepath.Clean(path)
+ if 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)
+ }
+ return fullPath, nil
Review Comment:
The containment checks in `fullPath` are purely lexical;
`Upload`/`Download`/`Delete` can still be redirected outside `baseDir` via
symlinks inside `baseDir` (e.g., `baseDir/link -> /etc`, then operating on
`link/passwd`). If the intent is to ensure operations *resolve* within the base
directory, consider resolving symlinks for `baseDir` and the target’s existing
parent directories and rejecting when the resolved path is outside the resolved
base.
--
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]
