From fa97c4519c0624d6c05b5e5ff037988c4bd6f938 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sat, 3 Oct 2026 16:58:35 +0200 Subject: [PATCH] Never write fetch's temp file into an existing file (closes #115) downloadFile opened the temp file with os.Create, which opens and empties a file already at that name. If that file was a hard link to a file outside the destination directory, fetch overwrote the outside file. It now removes whatever is at the temp name, which removes only that name, and creates the temp file with O_EXCL, so the create fails if anything is still or again there. A leftover temp file from an interrupted run is still replaced. The new test puts a hard link at the temp name and checks that fetch succeeds and the outside file is unchanged. The command name is now the constant cmdFetch, like the other command names, because lint requires it once a third test uses it. Model: opus-5-5 --- TODO.md | 3 +++ internal/cli/entry_test.go | 2 +- internal/cli/fetch.go | 17 +++++++++++++++-- internal/cli/fetch_test.go | 36 +++++++++++++++++++++++++++++++++++- internal/cli/mfer.go | 3 ++- 5 files changed, 56 insertions(+), 5 deletions(-) diff --git a/TODO.md b/TODO.md index 50eb1ee..6e41a04 100644 --- a/TODO.md +++ b/TODO.md @@ -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 diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index b718a9f..332becd 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -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) { diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 7983e97..3a2c5de 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -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) } diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index 6aaa82d..3095475 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -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") +} diff --git a/internal/cli/mfer.go b/internal/cli/mfer.go index 970edf8..8c4e769 100644 --- a/internal/cli/mfer.go +++ b/internal/cli/mfer.go @@ -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)