Never write fetch's temp file into an existing file (closes #115) #118
@@ -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