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/fetch.go b/internal/cli/fetch.go index 7983e97..f6871ea 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -489,12 +489,24 @@ 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. + err = os.Remove(tmpPath) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("failed to remove old temp file: %w", err) + } + + // Create the temp file with os.Create's mode, but fail if anything + // has appeared at tmpPath since the removal (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 + // G302: the mode keeps fetched files readable by group and other, + // like dirPerms. 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.OpenFile( //nolint:gosec // G302, G304: see comment above + tmpPath, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o666) 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..21cabdd 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -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, "fetch", "-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") +}