restoreFile and verifyRestoredFiles joined each stored path onto the target with no containment check. A .. segment or an absolute path escaped the target, and because symlinks are restored first with their target written verbatim, a symlink entry plus a child path through it wrote outside the tree; the follow-up Remove, Chmod and Chown escaped too. age decryption proves a snapshot is readable, not honest, and restore usually runs as root, so a forged snapshot was an arbitrary file write.
Both call sites now resolve through containedRestorePath. It rejects a stored path unless filepath.IsLocal accepts it once the leading separator is removed (the join drops that separator), barring .., absolute, and empty paths. It then Lstats each existing ancestor below the target and refuses to descend through a symlink. The target directory itself may be a symlink, and honest symlinks pointing outside the tree are still written verbatim.
Tests build a snapshot database with a ../ path, an absolute path with .. segments, and a symlink entry followed by a child through it; each makes restore fail and writes nothing outside the target. A separate test confirms an honest outside-pointing symlink still restores. All three traversal cases were verified to fail without the guard.
Judgement call: the ancestor-only symlink check (leaf excluded) relies on the files.path UNIQUE constraint, so a leaf cannot be both a symlink and a regular file.
Model: opus-4-8
Fixes https://git.eeqj.de/sneak/vaultik/issues/154 (found by the review in https://git.eeqj.de/sneak/vaultik/issues/73).
`restoreFile` and `verifyRestoredFiles` joined each stored path onto the target with no containment check. A `..` segment or an absolute path escaped the target, and because symlinks are restored first with their target written verbatim, a symlink entry plus a child path through it wrote outside the tree; the follow-up `Remove`, `Chmod` and `Chown` escaped too. age decryption proves a snapshot is readable, not honest, and restore usually runs as root, so a forged snapshot was an arbitrary file write.
Both call sites now resolve through `containedRestorePath`. It rejects a stored path unless `filepath.IsLocal` accepts it once the leading separator is removed (the join drops that separator), barring `..`, absolute, and empty paths. It then `Lstat`s each existing ancestor below the target and refuses to descend through a symlink. The target directory itself may be a symlink, and honest symlinks pointing outside the tree are still written verbatim.
Tests build a snapshot database with a `../` path, an absolute path with `..` segments, and a symlink entry followed by a child through it; each makes restore fail and writes nothing outside the target. A separate test confirms an honest outside-pointing symlink still restores. All three traversal cases were verified to fail without the guard.
Judgement call: the ancestor-only symlink check (leaf excluded) relies on the `files.path` UNIQUE constraint, so a leaf cannot be both a symlink and a regular file.
Model: opus-4-8
TODO.md ("# Completed Steps" section): the branch no longer merges cleanly onto the current next. After this branch was cut, next gained the issue #148 changelog entry (issue #148) as the first item under "# Completed Steps", and this PR adds the #154 entry at the same position, so a three-way merge into next produces a content conflict and the squash-merge is blocked. Acceptable: rebase the branch onto the current next head, keeping both entries (the #148 entry above the #154 entry), and re-push. No source change is needed — the restore-containment fix itself meets the definition of done (issue #154).
Model: opus-4-8
FAIL -- needs-rebase.
1. `TODO.md` ("# Completed Steps" section): the branch no longer merges cleanly onto the current `next`. After this branch was cut, `next` gained the issue #148 changelog entry ([issue #148](https://git.eeqj.de/sneak/vaultik/issues/148)) as the first item under "# Completed Steps", and this PR adds the #154 entry at the same position, so a three-way merge into `next` produces a content conflict and the squash-merge is blocked. Acceptable: rebase the branch onto the current `next` head, keeping both entries (the #148 entry above the #154 entry), and re-push. No source change is needed — the restore-containment fix itself meets the definition of done ([issue #154](https://git.eeqj.de/sneak/vaultik/issues/154)).
Model: opus-4-8
restoreFile and verifyRestoredFiles joined the stored path onto the
target with no containment check, so a ".." segment or an absolute path
escaped the target, and a restored symlink could redirect a later child
write anywhere on disk. Since age decryption proves a snapshot is
readable but not honest, and restore usually runs as root, a forged
snapshot became an arbitrary file write.
Both call sites now go through containedRestorePath: it rejects a stored
path unless filepath.IsLocal accepts it with the leading separator
removed (barring "..", absolute, and empty paths), then Lstats each
existing ancestor below the target and refuses to descend through a
symlink. The target directory itself may be a symlink, and honest
symlinks pointing outside the tree are still written verbatim.
Model: opus-4-8
Rebased onto current next. The only conflict was in TODO.md, resolved by keeping both Completed Steps entries — the issue #148 entry above the issue #154 entry. No source change. make check green (full test suite ran, lint 0 issues). Force-pushed with lease.
Model: opus-4-8
Rebased onto current `next`. The only conflict was in `TODO.md`, resolved by keeping both Completed Steps entries — the [issue #148](https://git.eeqj.de/sneak/vaultik/issues/148) entry above the [issue #154](https://git.eeqj.de/sneak/vaultik/issues/154) entry. No source change. `make check` green (full test suite ran, lint 0 issues). Force-pushed with lease.
Model: opus-4-8
clawbot
merged commit 76a6917a35 into next2026-09-22 11:01:02 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #154 (found by the review in #73).
restoreFileandverifyRestoredFilesjoined each stored path onto the target with no containment check. A..segment or an absolute path escaped the target, and because symlinks are restored first with their target written verbatim, a symlink entry plus a child path through it wrote outside the tree; the follow-upRemove,ChmodandChownescaped too. age decryption proves a snapshot is readable, not honest, and restore usually runs as root, so a forged snapshot was an arbitrary file write.Both call sites now resolve through
containedRestorePath. It rejects a stored path unlessfilepath.IsLocalaccepts it once the leading separator is removed (the join drops that separator), barring.., absolute, and empty paths. It thenLstats each existing ancestor below the target and refuses to descend through a symlink. The target directory itself may be a symlink, and honest symlinks pointing outside the tree are still written verbatim.Tests build a snapshot database with a
../path, an absolute path with..segments, and a symlink entry followed by a child through it; each makes restore fail and writes nothing outside the target. A separate test confirms an honest outside-pointing symlink still restores. All three traversal cases were verified to fail without the guard.Judgement call: the ancestor-only symlink check (leaf excluded) relies on the
files.pathUNIQUE constraint, so a leaf cannot be both a symlink and a regular file.Model: opus-4-8
FAIL -- needs-rebase.
TODO.md("# Completed Steps" section): the branch no longer merges cleanly onto the currentnext. After this branch was cut,nextgained the issue #148 changelog entry (issue #148) as the first item under "# Completed Steps", and this PR adds the #154 entry at the same position, so a three-way merge intonextproduces a content conflict and the squash-merge is blocked. Acceptable: rebase the branch onto the currentnexthead, keeping both entries (the #148 entry above the #154 entry), and re-push. No source change is needed — the restore-containment fix itself meets the definition of done (issue #154).Model: opus-4-8
3441eec48cto5707a03b9aRebased onto current
next. The only conflict was inTODO.md, resolved by keeping both Completed Steps entries — the issue #148 entry above the issue #154 entry. No source change.make checkgreen (full test suite ran, lint 0 issues). Force-pushed with lease.Model: opus-4-8