Never write fetch's temp file into an existing file (closes #115) #118

Merged
clawbot merged 1 commits from issue-115-fetch-tmp-hardlink into next 2026-10-03 16:58:36 +02:00
5 changed files with 56 additions and 5 deletions
+3
View File
@@ -24,6 +24,9 @@ only thing left of the `chore/align-repo-policies` branch is the list below.
# Completed Steps
- 2026-10-03: `fetch` removes whatever sits at a file's temp name and then
creates the temp file only if that name is free, so a hard link left there
cannot make it write into a file outside the destination directory (#115)
- 2026-10-03: `fetch` refuses any manifest path that runs through a symlink
already in the destination directory, checked before each of its writes
(directories, temp file, rename), so such a symlink cannot send a write
+1 -1
View File
@@ -126,7 +126,7 @@ func TestHelpCommand(t *testing.T) {
stdout := testStdout(t, opts)
assert.Contains(t, stdout, cmdGenerate)
assert.Contains(t, stdout, cmdCheck)
assert.Contains(t, stdout, "fetch")
assert.Contains(t, stdout, cmdFetch)
}
func TestGenerateCommand(t *testing.T) {
+15 -2
View File
@@ -36,6 +36,11 @@ const (
// traversal bit for group and other must stay set.
dirPerms os.FileMode = 0o755
// filePerms is the permission mode, before the umask, for downloaded
// files. It is the mode os.Create uses; like dirPerms, it keeps group
// and other read access.
filePerms os.FileMode = 0o666
// Bitrate unit thresholds in bits per second.
bpsPerGbps = 1e9
bpsPerMbps = 1e6
@@ -489,12 +494,20 @@ func downloadFile(
return err
}
// Create temp file.
// Remove whatever is at tmpPath, such as a leftover from an
// interrupted run, rather than write into it: it may be a hard link
// to a file outside the target directory, and removing a hard link
// removes only this name. If the removal fails, O_EXCL below makes
// the create fail.
_ = os.Remove(tmpPath)
// Create the temp file only if nothing is at tmpPath (O_EXCL).
//
// G304: tmpPath is a relative path that sanitizePath keeps inside the
// target directory as text, and checkNoSymlinks just found no symlink
// in it.
out, err := os.Create(tmpPath) //nolint:gosec // G304: see comment above
out, err := os.OpenFile( //nolint:gosec // G304: see comment above
tmpPath, os.O_RDWR|os.O_CREATE|os.O_EXCL, filePerms)
if err != nil {
return fmt.Errorf("failed to create temp file: %w", err)
}
+35 -1
View File
@@ -478,7 +478,7 @@ func TestFetchRefusesSymlinks(t *testing.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())
opts := testOpts([]string{testApp, cmdFetch, "-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)
@@ -489,3 +489,37 @@ func TestFetchRefusesSymlinks(t *testing.T) {
})
}
}
// TestFetchReplacesHardLinkAtTempName runs fetch into a destination
// directory that holds, at the temp file's name, a hard link to a file
// outside it. To fetch that is an ordinary leftover from an interrupted
// earlier run: it must replace it and succeed, and the outside file must
// not change.
//
//nolint:paralleltest // changes the process-global working directory
func TestFetchReplacesHardLinkAtTempName(t *testing.T) {
content := []byte("fetched")
sourceFs := afero.NewMemMapFs()
require.NoError(t, afero.WriteFile(sourceFs, "/"+testFileTxt, content, 0o644))
server := httptest.NewServer(fetchTestHandler(
scanToManifest(t, sourceFs), map[string][]byte{testFileTxt: content}))
defer server.Close()
outsideFile := filepath.Join(t.TempDir(), "secret.txt")
require.NoError(t, os.WriteFile(outsideFile, []byte("outside"), 0o600))
chdirTemp(t)
require.NoError(t, os.Link(outsideFile, ".file.txt.tmp"))
opts := testOpts([]string{testApp, cmdFetch, "-q", server.URL}, afero.NewOsFs())
require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
fetched, err := os.ReadFile(testFileTxt)
require.NoError(t, err)
assert.Equal(t, content, fetched)
outside, err := os.ReadFile(outsideFile) //nolint:gosec // test-controlled path
require.NoError(t, err)
assert.Equal(t, "outside", string(outside), "fetch wrote outside the destination")
}
+2 -1
View File
@@ -18,6 +18,7 @@ const (
cmdGenerate = "generate"
cmdCheck = "check"
cmdExport = "export"
cmdFetch = "fetch"
flagProgress = "progress"
@@ -300,7 +301,7 @@ func (mfa *CLIApp) listCommand() *cli.Command {
func (mfa *CLIApp) fetchCommand() *cli.Command {
return &cli.Command{
Name: "fetch",
Name: cmdFetch,
Usage: "fetch manifest and referenced files",
Action: func(c *cli.Context) error {
mfa.setVerbosity(c)