Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Directory-link traversal can include the still-being-written output archive, while non-UTF-8 entries are silently omitted and some link-resolution failures are misreported as missing targets.
3 open issues
What changed in this PR
This PR tightens bundle creation to produce regular files and directories, supporting the bundle contract described in #863 and complementing the lexical confinement in #866.
Changes:
- Replaces tar’s directory traversal with a sorted walk that dereferences in-project links and rejects escaping links, dangling links, directory cycles, and unsupported entries.
- Rechecks plugin file paths after the build.
- Adds five Unix integration tests and documents the behavior change.
| File | Description |
|---|---|
crates/icp-project/src/operations/bundle.rs |
Adds confined directory traversal, link diagnostics, and post-build file validation. |
crates/icp-cli/tests/bundle_tests.rs |
Tests in-project link copying and rejection of unsafe links. |
CHANGELOG.md |
Documents the new bundle restrictions and symlink handling. |
Review effort: Balanced
| path: entry.clone(), | ||
| })?; | ||
| if meta.is_dir() { | ||
| self.walk(&entry, &entry_archive_path, root, open)?; |
There was a problem hiding this comment.
Right, and that was new with this PR: tar's walk recorded the link and never reached the archive, while dereferencing does. Fixed in 4b3b82e: validate_output_path now hands back the canonical output, the walk carries it, and an entry that resolves to it is refused with OutputReachedThroughLink, named by the path it was reached by (assets/shared/bundle.tar.gz). bundle_rejects_output_reached_through_symlink is the regression test, with exactly the layout you describe.
| path: PathBuf::from(archive_path), | ||
| })?; | ||
| open.push(real.clone()); | ||
| for entry in crate::fs::read_dir(&real)? { |
There was a problem hiding this comment.
Agreed. crate::fs::read_dir skipping such a name is the right call for the glob machinery, where it lists candidates, and the wrong one here, where a skipped entry is a file silently missing from the bundle. In 4b3b82e the walk lists directories itself and refuses a name that is not UTF-8 with NameNotUtf8, naming the directory and the name lossily, since the archive could not carry it anyway (all paths here are UTF-8). bundle_rejects_a_name_that_is_not_utf8 covers it on Linux.
| if let Ok(target) = std::fs::read_link(path) { | ||
| return DanglingLinkSnafu { | ||
| path: path.to_path_buf(), | ||
| target: target.display().to_string(), | ||
| } | ||
| .fail(); | ||
| } |
There was a problem hiding this comment.
Agreed. In 4b3b82e a link is called dangling only when canonicalizing fails with NotFound and read_link succeeds; a loop or an unreadable target keeps its own io::Error under ResolveArchiveEntry. bundle_rejects_symlink_loop checks that a self -> self link is reported as unresolvable and not as missing.
c3193e2 to
4eb6da5
Compare
A symlink inside a plugin's `files:` or `dirs:` directory was recorded as a link entry with its target unchecked, so a link to a file outside the project reached that file on whatever machine unpacked the bundle. A `files:` entry that was itself a symlink to a file outside the project was read straight into the bundle: it was validated lexically before the build, when it need not have existed, and the read followed the link. The archive walk is now the bundler's own rather than tar's. Every entry met is canonicalized and must land under the canonical project directory: one that does is appended as what it points at, so a project may link to its own files freely, and one that does not fails the bundle, naming the link and where it leads. A dangling link, a link back to a directory being copied, and anything that is neither a file nor a directory fail the same way. A `files:` entry is held to the project again once the build has run and it exists. The walk appends entries in sorted order, which makes the archive reproducible for a given tree where tar's own walk left the order to the filesystem. This is the bundling side of the rule that `files::Confined` (#866) relies on: a bundle holds no symlinks, a consumer refuses any archive entry that is not a regular file or directory (#863), and so the tree a confined loader reads has nothing in it that leads elsewhere. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…y name Three holes in the archive walk, from review: - The output path is checked against the copied directories by its spelling, which a symlink inside one of them gets around: with `assets/shared -> ../shared` and `--output shared/bundle.tar.gz`, the walk met the archive it was writing and copied a partial copy of it into itself. The canonical output now travels with the walk, and an entry that resolves to it is refused, named by the path it was reached by. - `crate::fs::read_dir` skips a name that is not UTF-8, which for a bundle meant a file silently left out where tar's walk had carried it. The walk lists directories itself and refuses such a name, since the archive cannot carry it either way. - A link was called dangling whenever canonicalizing it failed and `read_link` succeeded, which also covers a link that loops or leads somewhere unreadable. Only a `NotFound` is dangling now; the rest keep their own cause. Entries met under a link are listed where the directory really is but named by the path they were reached by, so every message says what the user wrote rather than where it led. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4b3b82e to
4e0bc00
Compare
…stale doc comment The new `## Experimental` heading sat above the existing Intel macOS chore entry, which made that general change read as experimental. The heading and the bundle entry now follow it, as in v1.8.0. `validate_output_path` kept its old doc comment above the new one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A symlink inside a plugin's
files:ordirs:directory was recorded in the bundle as a link entry with its target unchecked, so a link to a file outside the project reached that file on whatever machine unpacked the bundle. Afiles:entry that was itself a link to a file outside the project was read straight into the bundle, since it is checked lexically before the build, when it need not exist yet.The archive walk is now the bundler's own rather than tar's
append_dir_all. Every entry met is canonicalized and must land under the canonical project directory. One that does is appended as what it points at, so a project may link to its own files freely. One that does not fails the bundle, naming the link and where it leads. A dangling link, a link that loops, a link back to a directory being copied, an entry that resolves to the archive being written, a name that is not UTF-8, and anything that is neither a file nor a directory fail the same way. Afiles:entry is held to the project once more after the build, when it exists. Entries go in sorted order, so the same tree gives the same archive.This is the bundling side of the rule #866 states. The user's own project may link wherever it likes. A bundle holds only regular files and directories, in-project links dereferenced and out-of-project links refused. A consumer of an untrusted bundle refuses anything else on the way in, after which the lexical
Confinedis complete.Seven unix integration tests: an in-project file and directory link copied as what they point at, an outward link, a dangling link, a cycle, a loop, the output reached through a link, and a symlinked
files:entry.Ref: #863, #866
🤖 Generated with Claude Code