Never write fetch's temp file into an existing file (closes #115)
check / check (push) Successful in 1m3s
check / check (push) Successful in 1m3s
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
This commit is contained in:
@@ -24,6 +24,9 @@ only thing left of the `chore/align-repo-policies` branch is the list below.
|
|||||||
|
|
||||||
# Completed Steps
|
# 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
|
- 2026-10-03: `fetch` refuses any manifest path that runs through a symlink
|
||||||
already in the destination directory, checked before each of its writes
|
already in the destination directory, checked before each of its writes
|
||||||
(directories, temp file, rename), so such a symlink cannot send a write
|
(directories, temp file, rename), so such a symlink cannot send a write
|
||||||
|
|||||||
@@ -126,7 +126,7 @@ func TestHelpCommand(t *testing.T) {
|
|||||||
stdout := testStdout(t, opts)
|
stdout := testStdout(t, opts)
|
||||||
assert.Contains(t, stdout, cmdGenerate)
|
assert.Contains(t, stdout, cmdGenerate)
|
||||||
assert.Contains(t, stdout, cmdCheck)
|
assert.Contains(t, stdout, cmdCheck)
|
||||||
assert.Contains(t, stdout, "fetch")
|
assert.Contains(t, stdout, cmdFetch)
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestGenerateCommand(t *testing.T) {
|
func TestGenerateCommand(t *testing.T) {
|
||||||
|
|||||||
+15
-2
@@ -36,6 +36,11 @@ const (
|
|||||||
// traversal bit for group and other must stay set.
|
// traversal bit for group and other must stay set.
|
||||||
dirPerms os.FileMode = 0o755
|
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.
|
// Bitrate unit thresholds in bits per second.
|
||||||
bpsPerGbps = 1e9
|
bpsPerGbps = 1e9
|
||||||
bpsPerMbps = 1e6
|
bpsPerMbps = 1e6
|
||||||
@@ -489,12 +494,20 @@ func downloadFile(
|
|||||||
return err
|
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
|
// G304: tmpPath is a relative path that sanitizePath keeps inside the
|
||||||
// target directory as text, and checkNoSymlinks just found no symlink
|
// target directory as text, and checkNoSymlinks just found no symlink
|
||||||
// in it.
|
// 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 {
|
if err != nil {
|
||||||
return fmt.Errorf("failed to create temp file: %w", err)
|
return fmt.Errorf("failed to create temp file: %w", err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -478,7 +478,7 @@ func TestFetchRefusesSymlinks(t *testing.T) {
|
|||||||
require.NoError(t, os.MkdirAll(filepath.Dir(tt.link), 0o750))
|
require.NoError(t, os.MkdirAll(filepath.Dir(tt.link), 0o750))
|
||||||
require.NoError(t, os.Symlink(filepath.Join(outside, tt.target), tt.link))
|
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.Equal(t, 1, runCLI(opts))
|
||||||
assert.Contains(t, testStderr(t, opts), "failed to download "+tt.entry+
|
assert.Contains(t, testStderr(t, opts), "failed to download "+tt.entry+
|
||||||
": symlink in path not allowed: "+tt.link)
|
": 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")
|
||||||
|
}
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ const (
|
|||||||
cmdGenerate = "generate"
|
cmdGenerate = "generate"
|
||||||
cmdCheck = "check"
|
cmdCheck = "check"
|
||||||
cmdExport = "export"
|
cmdExport = "export"
|
||||||
|
cmdFetch = "fetch"
|
||||||
|
|
||||||
flagProgress = "progress"
|
flagProgress = "progress"
|
||||||
|
|
||||||
@@ -300,7 +301,7 @@ func (mfa *CLIApp) listCommand() *cli.Command {
|
|||||||
|
|
||||||
func (mfa *CLIApp) fetchCommand() *cli.Command {
|
func (mfa *CLIApp) fetchCommand() *cli.Command {
|
||||||
return &cli.Command{
|
return &cli.Command{
|
||||||
Name: "fetch",
|
Name: cmdFetch,
|
||||||
Usage: "fetch manifest and referenced files",
|
Usage: "fetch manifest and referenced files",
|
||||||
Action: func(c *cli.Context) error {
|
Action: func(c *cli.Context) error {
|
||||||
mfa.setVerbosity(c)
|
mfa.setVerbosity(c)
|
||||||
|
|||||||
Reference in New Issue
Block a user