Never write fetch's temp file into an existing file (closes #115)
check / check (push) Failing after 27s

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
the name reappears in between. 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.

Model: opus-5-5
This commit is contained in:
2026-10-03 14:28:43 +00:00
parent 7e601929c8
commit 8686091f2d
3 changed files with 54 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
+17 -5
View File
@@ -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)
}
+34
View File
@@ -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")
}