Refuse fetch writes through a symlink in the destination (closes #86)
check / check (push) Successful in 1m10s
check / check (push) Successful in 1m10s
sanitizePath checks manifest paths only as text, so a symlink already inside the destination directory could send fetch's writes outside it. checkNoSymlinks now looks at each existing part of a path with os.Lstat and refuses the path if any part is a symlink, wherever it points. fetch runs it immediately before each write: creating the parent directories, creating the temp file, and renaming it into place. The new test puts such a symlink at each of those three places, and once inside a plain directory, and checks that the fetch fails and nothing outside changes. The G304 comment now states what holds. A symlink swapped in between a check and its write is not caught; os.Root closes that once the Go version is raised. Model: opus-5-5
This commit was merged in pull request #114.
This commit is contained in:
@@ -440,3 +440,52 @@ func TestFetchProgress(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, content, downloaded)
|
||||
}
|
||||
|
||||
// TestFetchRefusesSymlinks runs fetch into a destination directory that
|
||||
// holds a symlink pointing outside it, in each of the three places fetch
|
||||
// writes: a parent directory, the temp file, and the file itself, which
|
||||
// the temp file is renamed onto; and once as a directory inside a plain
|
||||
// directory. The fetch must fail and nothing outside may change.
|
||||
//
|
||||
//nolint:paralleltest // changes the process-global working directory
|
||||
func TestFetchRefusesSymlinks(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
entry string // the manifest's only file
|
||||
link string // symlink placed in the destination directory
|
||||
target string // what link points to, relative to the outside directory
|
||||
}{
|
||||
{"parent directory", "sub/deeper/file.txt", "sub", "."},
|
||||
{"directory inside a plain directory", "docs/data/passwd", "docs/data", "."},
|
||||
{"temp file", testFileTxt, ".file.txt.tmp", "new.txt"},
|
||||
{"file", testFileTxt, testFileTxt, "new.txt"},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
content := []byte("fetched")
|
||||
sourceFs := afero.NewMemMapFs()
|
||||
require.NoError(t, sourceFs.MkdirAll(filepath.Dir("/"+tt.entry), 0o755))
|
||||
require.NoError(t, afero.WriteFile(sourceFs, "/"+tt.entry, content, 0o644))
|
||||
|
||||
server := httptest.NewServer(fetchTestHandler(
|
||||
scanToManifest(t, sourceFs), map[string][]byte{tt.entry: content}))
|
||||
defer server.Close()
|
||||
|
||||
outside := t.TempDir()
|
||||
|
||||
chdirTemp(t)
|
||||
require.NoError(t, os.MkdirAll(filepath.Dir(tt.link), 0o750))
|
||||
require.NoError(t, os.Symlink(filepath.Join(outside, tt.target), tt.link))
|
||||
|
||||
opts := testOpts([]string{testApp, "fetch", "-q", server.URL}, afero.NewOsFs())
|
||||
assert.Equal(t, 1, runCLI(opts))
|
||||
assert.Contains(t, testStderr(t, opts), "failed to download "+tt.entry+
|
||||
": symlink in path not allowed: "+tt.link)
|
||||
|
||||
written, err := os.ReadDir(outside)
|
||||
require.NoError(t, err)
|
||||
assert.Empty(t, written, "fetch wrote outside the destination")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user