fetch: sanitizePath is purely lexical and does not prevent symlink escape #86

New Issue

clawbot · 2026-08-09T04:16:00+02:00

clawbot commented

2026-08-09 04:16:00 +02:00

Split out of the PR #59 review (finding B8), which found the problem but
correctly scoped the fix out of a lint-cleanup PR.

Problem

sanitizePath in internal/cli/fetch.go is the only guard on paths taken
from a manifest before they are used to create directories and files under
the fetch destination. It performs three purely lexical checks:

reject empty paths
reject filepath.IsAbs
filepath.Clean, then reject a leading ../ (and re-check IsAbs)

Nothing resolves symlinks. If the destination tree already contains a
symlinked directory - say ./data is a symlink to /etc - then a manifest
entry data/passwd passes every check and is written through the symlink,
outside the destination tree. Both os.MkdirAll and the os.Create of the
temp file in downloadFile follow the symlink.

PR #59 narrowed the suppression on the os.Create line to G304 and replaced
its previous, overstated justification with one that states the limitation
explicitly, and moved the sanitizePath call inside downloadFile so the
lexical invariant is enforced locally rather than one function away. Neither
change closes this gap; they only stop misdescribing it.

Definition of done

A manifest entry cannot cause a write outside the fetch destination
directory, even when the destination tree contains a symlink that a
previous fetch (or anything else) placed there.
The chosen mechanism is applied to every write site in fetch: the
os.MkdirAll for parent directories, the temp file os.Create, and the
final rename.
A test constructs a destination directory containing a symlinked
subdirectory pointing outside the tree, fetches a manifest with an entry
under that subdirectory, and asserts the write is refused and nothing
outside the destination is touched.
The //nolint:gosec // G304: ... comment in downloadFile is updated to
describe whatever guarantee then actually holds, or removed if it is no
longer needed.

Notes

Candidate approaches worth weighing before implementing: resolving the
destination root once with filepath.EvalSymlinks and verifying every
resolved write path stays under it; refusing to traverse a symlinked
component at all; or os.OpenRoot / openat2-style rooted file access.
Picking among these is part of the work, not a decided outcome.

Related: the mode of directories created by fetch is 0o755 on purpose
(fetched trees are meant to be readable by other uids); do not "fix" this
by tightening permissions.

Split out of the PR #59 review (finding B8), which found the problem but correctly scoped the fix out of a lint-cleanup PR. ## Problem `sanitizePath` in `internal/cli/fetch.go` is the only guard on paths taken from a manifest before they are used to create directories and files under the fetch destination. It performs three purely lexical checks: - reject empty paths - reject `filepath.IsAbs` - `filepath.Clean`, then reject a leading `../` (and re-check `IsAbs`) Nothing resolves symlinks. If the destination tree already contains a symlinked directory - say `./data` is a symlink to `/etc` - then a manifest entry `data/passwd` passes every check and is written through the symlink, outside the destination tree. Both `os.MkdirAll` and the `os.Create` of the temp file in `downloadFile` follow the symlink. PR #59 narrowed the suppression on the `os.Create` line to G304 and replaced its previous, overstated justification with one that states the limitation explicitly, and moved the `sanitizePath` call inside `downloadFile` so the lexical invariant is enforced locally rather than one function away. Neither change closes this gap; they only stop misdescribing it. ## Definition of done - A manifest entry cannot cause a write outside the fetch destination directory, even when the destination tree contains a symlink that a previous fetch (or anything else) placed there. - The chosen mechanism is applied to every write site in `fetch`: the `os.MkdirAll` for parent directories, the temp file `os.Create`, and the final rename. - A test constructs a destination directory containing a symlinked subdirectory pointing outside the tree, fetches a manifest with an entry under that subdirectory, and asserts the write is refused and nothing outside the destination is touched. - The `//nolint:gosec // G304: ...` comment in `downloadFile` is updated to describe whatever guarantee then actually holds, or removed if it is no longer needed. ## Notes Candidate approaches worth weighing before implementing: resolving the destination root once with `filepath.EvalSymlinks` and verifying every resolved write path stays under it; refusing to traverse a symlinked component at all; or `os.OpenRoot` / `openat2`-style rooted file access. Picking among these is part of the work, not a decided outcome. Related: the mode of directories created by `fetch` is `0o755` on purpose (fetched trees are meant to be readable by other uids); do not "fix" this by tightening permissions.

clawbot referenced this issue from a commit

2026-08-09 04:19:59 +02:00

Update golangci-lint to v2.12.2 with canonical config (closes #60)

clawbot referenced this issue

2026-08-09 04:21:38 +02:00

Update golangci-lint to v2.12.2 with canonical config (closes #60) #59

clawbot referenced this issue

2026-08-09 04:22:38 +02:00

Update golangci-lint to v2.12.2 with canonical config (closes #60) #59

clawbot added this to the 1.0.0 milestone 2026-08-09 04:23:17 +02:00

clawbot referenced this issue

2026-08-09 04:31:57 +02:00