fetch: a hard link at the temp file's name lets fetch overwrite a file outside the destination #115

New Issue

2026-10-03T16:08:28+02:00

clawbot commented

2026-10-03 16:08:28 +02:00

Found by the reviewer of #114, which fixed the symlink case of the same problem (#86).

Problem

downloadFile in internal/cli/fetch.go writes each file first to a temp file next to it (.name.tmp, or name.tmp for a dotfile) and opens it with os.Create. os.Create opens an existing file and empties it. If a hard link to a file outside the destination already sits at that temp name, fetch empties that outside file and writes the downloaded bytes into it. The symlink check added in 114 does not see this, because a hard link is an ordinary file. os.Root would not close it either.

Definition of done

fetch never writes into a file that already exists at the temp name: it removes whatever is there first (which removes only that name, not the file a hard link points to), then creates the temp file so that the create fails if a file of that name appeared in between (os.O_CREATE|os.O_EXCL).
A test puts a hard link at the temp name pointing to a file outside the destination, runs mfer fetch against a test server, and checks that the outside file is unchanged.
The leftover temp file from an interrupted earlier run is still handled: removed and replaced, not an error.
make check passes; TODO.md updated in the same commit.

Implementation requirements

Standard library only; keep it to the temp-file open in downloadFile and its test.
Do not change the symlink checks from 114.
Commit title ends with (closes #N) for this issue's number.

Model: opus-5-5

Found by the reviewer of https://git.eeqj.de/sneak/mfer/pulls/114, which fixed the symlink case of the same problem (https://git.eeqj.de/sneak/mfer/issues/86). ## Problem `downloadFile` in `internal/cli/fetch.go` writes each file first to a temp file next to it (`.name.tmp`, or `name.tmp` for a dotfile) and opens it with `os.Create`. `os.Create` opens an existing file and empties it. If a hard link to a file outside the destination already sits at that temp name, `fetch` empties that outside file and writes the downloaded bytes into it. The symlink check added in 114 does not see this, because a hard link is an ordinary file. `os.Root` would not close it either. ## Definition of done - `fetch` never writes into a file that already exists at the temp name: it removes whatever is there first (which removes only that name, not the file a hard link points to), then creates the temp file so that the create fails if a file of that name appeared in between (`os.O_CREATE|os.O_EXCL`). - A test puts a hard link at the temp name pointing to a file outside the destination, runs `mfer fetch` against a test server, and checks that the outside file is unchanged. - The leftover temp file from an interrupted earlier run is still handled: removed and replaced, not an error. - `make check` passes; `TODO.md` updated in the same commit. ## Implementation requirements - Standard library only; keep it to the temp-file open in `downloadFile` and its test. - Do not change the symlink checks from 114. - Commit title ends with ` (closes #N)` for this issue's number. Model: opus-5-5

clawbot added this to the 1.0.0 milestone 2026-10-03 16:08:28 +02:00

clawbot added the critical label 2026-10-03 16:08:28 +02:00

clawbot commented

2026-10-03 16:08:32 +02:00

Labelled critical: anyone who runs mfer fetch into a directory holding a hard link at a temp file's name can have the file that link points to, outside the destination, overwritten with downloaded bytes.

Model: opus-5-5

Labelled critical: anyone who runs `mfer fetch` into a directory holding a hard link at a temp file's name can have the file that link points to, outside the destination, overwritten with downloaded bytes. Model: opus-5-5

clawbot referenced this issue

2026-10-03 16:09:09 +02:00

1.0.0 milestone #92

clawbot referenced this issue from a commit

2026-10-03 16:28:45 +02:00

Never write fetch's temp file into an existing file (closes #115)

clawbot referenced this issue from a commit

2026-10-03 16:31:52 +02:00

Never write fetch's temp file into an existing file (closes #115)

clawbot referenced a pull request that will close this issue

2026-10-03 16:33:20 +02:00

Never write fetch's temp file into an existing file (closes #115) #118

clawbot commented

2026-10-03 16:33:26 +02:00

Implemented in #118: fetch now removes whatever is at the temp name and creates the temp file only if the name is free, so a hard link there can no longer make it write outside the destination. One deviation is described in the PR: the command name fetch became the constant cmdFetch because lint required it.

Model: opus-5-5

Implemented in https://git.eeqj.de/sneak/mfer/pulls/118: `fetch` now removes whatever is at the temp name and creates the temp file only if the name is free, so a hard link there can no longer make it write outside the destination. One deviation is described in the PR: the command name `fetch` became the constant `cmdFetch` because lint required it. Model: opus-5-5

clawbot closed this issue

2026-10-03 16:58:36 +02:00

clawbot referenced this issue from a commit

2026-10-03 16:58:37 +02:00

Never write fetch's temp file into an existing file (closes #115)

clawbot referenced this issue

2026-10-03 18:25:15 +02:00

Review toward 1.0: identify and file all next steps #99

Sign in to join this conversation.