fix(sources/filesystem): order resume comparison by path component (#5041)
* fix(sources/filesystem): order resume comparison by path component
When resuming a filesystem scan, scanDir compared the directory path
against the stored resume point with a raw string comparison
(`path < resumeAfter`). Raw comparison does not match the depth-first
traversal order produced by os.ReadDir, because the separator '/' (0x2F)
sorts after characters that are valid inside a path component such as
'-' (0x2D) and '.' (0x2E).
So with sibling directories where one name is a prefix of the other,
e.g. `blue-team` and `blue-team-deprecated`, resuming inside `blue-team`
made scanDir evaluate `"/root/blue-team-deprecated" < "/root/blue-team/file"`
as true and skip `blue-team-deprecated` entirely, silently dropping every
file beneath it.
Compare the paths component by component instead, which matches the order
os.ReadDir returns entries in ("blue-team" before "blue-team-deprecated").
Adds an integration test for the prefix-sibling case and a unit test for
the comparison helper.
Closes #5039
* refactor(sources/filesystem): simplify comparePathsForResume with slices.Compare
Replace the manual component-wise loop with slices.Compare over the
split path components. Equivalent ordering (a shared-prefix ancestor
path still sorts before its descendants) with a smaller body.
This commit is contained in:
@@ -5,6 +5,7 @@ import (
|
||||
"io"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"slices"
|
||||
"strings"
|
||||
|
||||
"github.com/go-errors/errors"
|
||||
@@ -229,6 +230,25 @@ func (s *Source) scanSymlink(
|
||||
return nil
|
||||
}
|
||||
|
||||
// comparePathsForResume orders two cleaned paths the way a depth-first walk over
|
||||
// os.ReadDir-sorted entries does: component by component. A raw string comparison
|
||||
// is wrong here because the separator '/' (0x2F) sorts after characters that are
|
||||
// valid in a path component, such as '-' (0x2D) or '.' (0x2E). For example, as raw
|
||||
// strings "/root/blue-team-deprecated" < "/root/blue-team/file.txt" (the '-' after
|
||||
// "blue-team" sorts before '/'), which would make scanDir wrongly skip the sibling
|
||||
// directory "blue-team-deprecated" when resuming inside "blue-team". Comparing per
|
||||
// component matches os.ReadDir's ordering, where "blue-team" sorts before
|
||||
// "blue-team-deprecated". Returns -1, 0, or 1.
|
||||
func comparePathsForResume(a, b string) int {
|
||||
// slices.Compare walks the components lexicographically and, when one path is
|
||||
// a prefix of the other, orders the shorter (ancestor) path first — matching
|
||||
// the depth-first walk order described above.
|
||||
return slices.Compare(
|
||||
strings.Split(a, string(filepath.Separator)),
|
||||
strings.Split(b, string(filepath.Separator)),
|
||||
)
|
||||
}
|
||||
|
||||
func (s *Source) scanDir(
|
||||
ctx trContext.Context,
|
||||
chunksChan chan *sources.Chunk,
|
||||
@@ -258,8 +278,8 @@ func (s *Source) scanDir(
|
||||
if resumeAfter != "" && !strings.HasPrefix(resumeAfter, path+string(filepath.Separator)) && resumeAfter != path {
|
||||
// Resume point is not in this subtree. Compare paths to determine if we
|
||||
// should skip this directory (already scanned) or process it (already passed).
|
||||
if path < resumeAfter {
|
||||
// This directory comes before the resume point lexicographically,
|
||||
if comparePathsForResume(path, resumeAfter) < 0 {
|
||||
// This directory comes before the resume point in traversal order,
|
||||
// meaning it was already fully scanned. Skip it entirely.
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -679,6 +679,88 @@ func TestResumptionWithNestedDirectories(t *testing.T) {
|
||||
assert.Equal(t, 1, len(reporter.Chunks), "expected exactly 1 file to be scanned")
|
||||
}
|
||||
|
||||
func TestResumptionWithPrefixSiblingDirectories(t *testing.T) {
|
||||
ctx := trContext.Background()
|
||||
|
||||
// Create sibling directories where one name is a prefix of the other:
|
||||
// root/
|
||||
// blue-team/
|
||||
// project-notes.txt
|
||||
// blue-team-deprecated/
|
||||
// AWSCredentials.txt
|
||||
//
|
||||
// os.ReadDir sorts "blue-team" before "blue-team-deprecated", so a scan that
|
||||
// resumes inside blue-team must still descend into blue-team-deprecated. A raw
|
||||
// string comparison of the directory path against the resume point would skip
|
||||
// blue-team-deprecated, because '-' (0x2D) sorts before the separator '/' (0x2F).
|
||||
rootDir, err := os.MkdirTemp("", "trufflehog-resumption-prefix-test")
|
||||
require.NoError(t, err)
|
||||
t.Cleanup(func() { _ = os.RemoveAll(rootDir) })
|
||||
|
||||
dirs := map[string]string{
|
||||
"blue-team": "project-notes.txt",
|
||||
"blue-team-deprecated": "AWSCredentials.txt",
|
||||
}
|
||||
for dir, file := range dirs {
|
||||
dirPath := filepath.Join(rootDir, dir)
|
||||
require.NoError(t, os.Mkdir(dirPath, 0755))
|
||||
require.NoError(t, os.WriteFile(filepath.Join(dirPath, file), []byte("content of "+file), 0644))
|
||||
}
|
||||
|
||||
conn, err := anypb.New(&sourcespb.Filesystem{})
|
||||
require.NoError(t, err)
|
||||
|
||||
s := Source{}
|
||||
err = s.Init(ctx, "test resumption prefix sibling", 0, 0, true, conn, 1)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Resume inside blue-team. project-notes.txt is the resume point (already
|
||||
// scanned); AWSCredentials.txt in the sibling blue-team-deprecated comes after
|
||||
// it in traversal order and must still be scanned.
|
||||
resumePoint := filepath.Join(rootDir, "blue-team", "project-notes.txt")
|
||||
s.SetEncodedResumeInfoFor(rootDir, resumePoint)
|
||||
|
||||
reporter := sourcestest.TestReporter{}
|
||||
err = s.ChunkUnit(ctx, sources.CommonSourceUnit{ID: rootDir}, &reporter)
|
||||
require.NoError(t, err)
|
||||
|
||||
scannedFiles := make(map[string]bool)
|
||||
for _, chunk := range reporter.Chunks {
|
||||
scannedFiles[filepath.Base(chunk.SourceMetadata.GetFilesystem().GetFile())] = true
|
||||
}
|
||||
|
||||
assert.False(t, scannedFiles["project-notes.txt"], "project-notes.txt should have been skipped (the resume point itself)")
|
||||
assert.True(t, scannedFiles["AWSCredentials.txt"], "AWSCredentials.txt should have been scanned (sibling dir after the resume point)")
|
||||
assert.Equal(t, 1, len(reporter.Chunks), "expected exactly 1 file to be scanned")
|
||||
}
|
||||
|
||||
func TestComparePathsForResume(t *testing.T) {
|
||||
sep := string(filepath.Separator)
|
||||
p := func(parts ...string) string { return sep + filepath.Join(parts...) }
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
a string
|
||||
b string
|
||||
want int
|
||||
}{
|
||||
{"equal", p("root", "aaa"), p("root", "aaa"), 0},
|
||||
{"before sibling", p("root", "aaa"), p("root", "bbb", "file.txt"), -1},
|
||||
{"after sibling", p("root", "ccc"), p("root", "bbb", "file.txt"), 1},
|
||||
{"ancestor before descendant", p("root", "aaa"), p("root", "aaa", "file.txt"), -1},
|
||||
// Prefix-sibling: "blue-team-deprecated" must sort AFTER a resume point
|
||||
// inside "blue-team", matching os.ReadDir order (regression for the raw
|
||||
// string comparison where '-' < '/').
|
||||
{"prefix sibling sorts after", p("root", "blue-team-deprecated"), p("root", "blue-team", "notes.txt"), 1},
|
||||
{"prefix sibling reverse", p("root", "blue-team", "notes.txt"), p("root", "blue-team-deprecated"), -1},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
assert.Equal(t, tt.want, comparePathsForResume(tt.a, tt.b))
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestResumptionWithOutOfSubtreeResumePoint(t *testing.T) {
|
||||
ctx := trContext.Background()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user