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)