Create unit tests for pkg/tar-patch package#103
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive suite of unit and integration tests in pkg/tar-patch/apply_test.go to verify the Apply function and the FilesystemDataSource implementation. Feedback on the changes highlights that the path traversal test (TestApply_PathTraversal) is currently a false positive because it uses an empty mock data source, which would fail on any file lookup regardless of whether path traversal was successfully blocked. It is recommended to refactor this test using a real filesystem data source and a file created outside the temporary directory to ensure path traversal attempts are robustly caught.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func TestApply_PathTraversal(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| filename string | ||
| }{ | ||
| {"parent directory", "../etc/passwd"}, | ||
| {"absolute path", "/etc/passwd"}, | ||
| {"complex traversal", "../../etc/passwd"}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| delta := createDeltaStream(t, []deltaOp{ | ||
| {code: protocol.DeltaOpOpen, size: uint64(len(tc.filename)), data: []byte(tc.filename)}, | ||
| }) | ||
|
|
||
| var output bytes.Buffer | ||
| ds := newMockDataSource() | ||
|
|
||
| err := Apply(delta, ds, &output) | ||
| if err == nil { | ||
| t.Fatal("expected error for path traversal attempt, got nil") | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The current path traversal test is a false positive. It passes only because the mock data source ds is completely empty, meaning any file lookup (including a successfully neutralized path like etc/passwd) will fail with a 'file not found' error.
If a regression were introduced in CleanPath that allowed actual path traversal (e.g., returning ../etc/passwd directly), the mock data source would still return a 'file not found' error, and this test would still pass, failing to detect the vulnerability.
To make this security test robust, we should use NewFilesystemDataSource with a real temporary directory, create a file outside of that directory, and verify that attempting to access it via path traversal fails because the path is neutralized (resulting in a 'no such file or directory' error inside the temp directory) rather than successfully opening the outside file.
func TestApply_PathTraversal(t *testing.T) {
testCases := []struct {
name string
filename string
}{
{"parent directory", "../outside.txt"},
{"absolute path", "/outside.txt"},
{"complex traversal", "../../outside.txt"},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
tempDir := t.TempDir()
parentDir := filepath.Dir(tempDir)
outsideFile := filepath.Join(parentDir, "outside.txt")
if err := os.WriteFile(outsideFile, []byte("secret"), 0644); err != nil {
t.Fatalf("failed to create outside file: %v", err)
}
defer os.Remove(outsideFile)
delta := createDeltaStream(t, []deltaOp{
{code: protocol.DeltaOpOpen, size: uint64(len(tc.filename)), data: []byte(tc.filename)},
})
var output bytes.Buffer
ds := NewFilesystemDataSource(tempDir)
defer ds.Close()
err := Apply(delta, ds, &output)
if err == nil {
t.Fatal("expected error for path traversal attempt, got nil")
}
if !os.IsNotExist(err) && !strings.Contains(err.Error(), "no such file or directory") {
t.Errorf("expected no such file or directory error, got: %v", err)
}
})
}
}There was a problem hiding this comment.
@melkoudi-cmyk Have you taken a look at this? My claude is coming back with the same thing about this being a false positive because ds is empty. It suggested this as a fix:
func TestApply_PathTraversal(t *testing.T) {
// Create two separate temp directories
tempDir := t.TempDir()
outsideDir := t.TempDir()
// Create a file OUTSIDE the base directory
secretFile := filepath.Join(outsideDir, "secret.txt")
if err := os.WriteFile(secretFile, []byte("secret data"), 0644); err != nil {
t.Fatalf("failed to create secret file: %v", err)
}
testCases := []struct {
name string
filename string
}{
{"parent directory", "../secret.txt"},
{"absolute path", "/etc/passwd"},
{"complex traversal", "../../secret.txt"},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
delta := createDeltaStream(t, []deltaOp{
{code: protocol.DeltaOpOpen, size: uint64(len(tc.filename)), data:
[]byte(tc.filename)},
})
var output bytes.Buffer
ds := NewFilesystemDataSource(tempDir) // Use REAL filesystem datasource
defer ds.Close()
err := Apply(delta, ds, &output)
if err == nil {
t.Fatal("expected error for path traversal attempt, got nil")
}
// Should fail because path sanitization rejects traversal
})
}
}
There was a problem hiding this comment.
I revisited this with an agent and its analysis makes sense to me, see below.
outsideDir is created by a separate t.TempDir() call, so it is a sibling of tempDir, not its parent. ../secret.txt from tempDir resolves to filepath.Dir(tempDir)/secret.txt, which is neither outsideDir nor any file inside it — the secret file is unreachable regardless of whether CleanPath works.
Additionally, the content verification never executes — output is "decoy" (first 5 bytes of "decoy data"), so bytes.Contains(output, "secret") is always false.
Note: expectClean is set in the test case struct but never used in any assertion.
Suggested fix: place the secret file in filepath.Dir(tempDir) and assert output directly:
parentDir := filepath.Dir(tempDir)
secretFile := filepath.Join(parentDir, "secret.txt")
os.WriteFile(secretFile, []byte("SECRET"), 0644)
defer os.Remove(secretFile)
decoyFile := filepath.Join(tempDir, "secret.txt")
os.WriteFile(decoyFile, []byte("DECOY"), 0644)
// ... then in shouldExist branch:
if !bytes.Equal(output.Bytes(), []byte("DECOY")) {
t.Errorf("expected to read from decoy inside tempDir, got: %q", output.Bytes())
}
This way, a broken CleanPath would read "SECRET" from the parent instead of "DECOY" from tempDir, and the test would catch it.
There was a problem hiding this comment.
That makes sense to me, thanks for digging into this a bit more
melkoudi-cmyk
force-pushed
the
te4594-tar-pkg-unittests
branch
from
53aeeb8 to
2bf787f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
==========================================
+ Coverage 79.19% 81.25% +2.06%
==========================================
Files 10 10
Lines 1115 1115
==========================================
+ Hits 883 906 +23
+ Misses 134 118 -16
+ Partials 98 91 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
melkoudi-cmyk
force-pushed
the
te4594-tar-pkg-unittests
branch
4 times, most recently
from
0147193 to
5c7d737
Compare
knecasov
left a comment
knecasov
left a comment
There was a problem hiding this comment.
I have a few notes.
-
Consider adding tests for operations (
DeltaOpCopy,DeltaOpAddData,DeltaOpSeek) sent without a precedingDeltaOpOpen. This could reveal potentialnilpointer issues. -
The mock has error injection fields (
closeErr,openErr,readErr,seekErr) that are never set in any test (their default value should benil). Consider adding tests that inject errors to verify thatApply()correctly propagates I/O failures from the data source (for example, permission denied on open, disk read failure during copy). -
I would recommend unifying the error message style in test assertions - some use quotes around the expected string (for example,
'no current file') and some do not.
I would consider adding the following edge cases.
DeltaOpData/DeltaOpCopywithsize: 0DeltaOpOpenwith an empty filename (size: 0)- seek past the end of a file followed by a copy
melkoudi-cmyk
force-pushed
the
te4594-tar-pkg-unittests
branch
from
5c7d737 to
e092df3
Compare
djach7
left a comment
djach7
left a comment
There was a problem hiding this comment.
Thanks for working on this! I've got a couple change requests and a further question about error message style:
It looks to me like there are 3 different types of error message style here: single quotes, escaped double quotes, and no quotes. Would you mind updating this to use one consistent style? I'd recommend single quotes, I think that's what we use most often. Here's where I'm seeing the different styles:
Style 1: Single quotes 'error message'
- Line 308: expected 'invalid delta format' error
- Line 327: expected 'unexpected delta op' error
- Line 584: expected 'no current file' error
- Line 651: expected 'no current file' error
Style 2: Escaped double quotes "error message"
- Line 351: expected "filename size" error
- Line 376: expected "AddData" size error
- Line 791: expected "no current file" error
- Line 810: expected "no current file" error
- Line 828: expected "no current file" error
- Line 850: expected "permission denied" error
- Line 874: expected "disk read failure" error
- Line 898: expected "seek failed" error
- Line 923: expected "I/O error during read" error
- Line 984: expected "invalid source name" error
Style 3: No quotes
- Line 1009: expected EOF error
| func TestApply_PathTraversal(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| filename string | ||
| }{ | ||
| {"parent directory", "../etc/passwd"}, | ||
| {"absolute path", "/etc/passwd"}, | ||
| {"complex traversal", "../../etc/passwd"}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| delta := createDeltaStream(t, []deltaOp{ | ||
| {code: protocol.DeltaOpOpen, size: uint64(len(tc.filename)), data: []byte(tc.filename)}, | ||
| }) | ||
|
|
||
| var output bytes.Buffer | ||
| ds := newMockDataSource() | ||
|
|
||
| err := Apply(delta, ds, &output) | ||
| if err == nil { | ||
| t.Fatal("expected error for path traversal attempt, got nil") | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
@melkoudi-cmyk Have you taken a look at this? My claude is coming back with the same thing about this being a false positive because ds is empty. It suggested this as a fix:
func TestApply_PathTraversal(t *testing.T) {
// Create two separate temp directories
tempDir := t.TempDir()
outsideDir := t.TempDir()
// Create a file OUTSIDE the base directory
secretFile := filepath.Join(outsideDir, "secret.txt")
if err := os.WriteFile(secretFile, []byte("secret data"), 0644); err != nil {
t.Fatalf("failed to create secret file: %v", err)
}
testCases := []struct {
name string
filename string
}{
{"parent directory", "../secret.txt"},
{"absolute path", "/etc/passwd"},
{"complex traversal", "../../secret.txt"},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
delta := createDeltaStream(t, []deltaOp{
{code: protocol.DeltaOpOpen, size: uint64(len(tc.filename)), data:
[]byte(tc.filename)},
})
var output bytes.Buffer
ds := NewFilesystemDataSource(tempDir) // Use REAL filesystem datasource
defer ds.Close()
err := Apply(delta, ds, &output)
if err == nil {
t.Fatal("expected error for path traversal attempt, got nil")
}
// Should fail because path sanitization rejects traversal
})
}
}
e092df3 to
125789d
Compare
|
@melkoudi-cmyk Thanks for addressing my comments! It looks like the path traversal test still has some kinks to work out and the tests are failing on an unchecked error and if/else issue I think? |
melkoudi-cmyk
force-pushed
the
te4594-tar-pkg-unittests
branch
from
125789d to
8a05fea
Compare
Added unit tests for the tar-patch package resulting in higher coverage (87.8%) Assisted by: Claude Signed-off-by: Meriem Elkoudi <melkoudi@redhat.com>
melkoudi-cmyk
force-pushed
the
te4594-tar-pkg-unittests
branch
from
8a05fea to
672f152
Compare
There is a bug within the test that tests the windows version. Rosy and I are trying to find a solution to this as well |
3 participants
Added unit tests for the tar-patch package
resulting in 82.9% coverage
THEEDGE-4594
Signed-off-by: Meriem Elkoudi melkoudi@redhat.com
Assisted by: Claude