You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(bundle): hold a bundle to regular files and directories - #868
A symlink inside a plugin's files: or dirs: 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. A files: 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. A files: entry is held to the project once more after the build, when it exists, and to the same rules as an entry met on the walk: not the output, and a regular file. The output is resolved once it has been created, since a dangling link given as the output only then says where the bundle is written, and each entry is resolved from its directory's real path, so a chain of directory links longer than the host follows in one path is still copied. 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, and Confined refuses any symlink it is asked to pass through.
Twelve 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, an output that is itself a link into a copied directory, a name that is not UTF-8, a symlinked files: entry, a files: entry that links to the output or is a named pipe, and a chain of 46 directory links.
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.
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.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
A dangling output symlink can cause the directory walk to miss the archive’s real path and include its own partial output, requiring path re-resolution after opening and a regression test.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Direct files: inputs lack regular-file and output-path checks, allowing FIFO reads to hang indefinitely and the archive being written to be included as incorrect plugin data.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Reject direct inputs that resolve to the output file
The new output-file guard is only applied to directory entries. With files: { seed: config.txt }, config.txt -> bundle.tar.gz, and --output bundle.tar.gz, canonicalization succeeds after the output is created, and this read copies the still-being-written archive (potentially empty because it is buffered) into the plugin input. Reject src == canonical_output here too so this case fails instead of producing incorrect plugin data.
The direct files: path still reads non-regular files: an in-project FIFO (or a symlink to one) passes canonicalization and is classified into plugin_files because it is not a directory. HostFileSystem::read then calls std::fs::read, which blocks indefinitely when the FIFO has no writer, instead of returning the new UnsupportedEntry error. Check that the resolved source is a regular file before reading it, as the directory walk does, and cover this case with a regression test.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Active issues remain: nested directory symlinks can trigger ELOOP, and standalone file entries can bypass the output guard and bundle an empty or partial copy of the archive itself.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The output guards miss hard-link aliases, allowing the empty or partially written bundle to archive itself; both paths need file-identity checks and hard-link regression tests.
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>
…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>
…ames
The walk resolved a directory as an entry and again on entering it; the
resolved path is now handed down. list_dir returns names rather than paths
the caller took the names back out of. Doc comments and the changelog entry
say the same in fewer words.
Co-Authored-By: Claude <noreply@anthropic.com>
The canonical output was worked out before the build, when an output path
that is a dangling symlink resolves to its own spelling. Creating the file
follows the link, so the walk was told to refuse a path the bundle was not
written to, and one linked into a copied directory was read into itself
without end. The output is now resolved after it is created.
Co-Authored-By: Claude <noreply@anthropic.com>
…from where a link led
A files: entry that is itself a link to the output was read into the
bundle half-written, and one that is a named pipe was read forever; both
are now refused as they are on the walk. The walk resolved each entry by
the path it was reached by, following every link crossed so far again, so
a chain of directory links longer than the host follows in one path
failed; each entry is now resolved from the directory's real path.
Co-Authored-By: Claude <noreply@anthropic.com>
…irectory handle
`--untrusted` on build, deploy and sync limits the steps a project may run
to a pre-built module and a plugin, and reads everything through
`files::Confined` at the project root.
`Confined` opens the root once as a `cap_std::fs::Dir` and makes every read
and write beneath that handle, so the operating system refuses a path that
leaves the root in the same step that opens it (`openat2` on Linux). On top
of that it refuses any path through a symlink, wherever the link leads.
This is #866 and #871 on main, without #868, and with `Confined` built on
cap-std in place of a path walk followed by a plain open.
Co-Authored-By: Claude <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The canonical-path guard misses hard-link aliases of the open output file, allowing symlink traversal to include the archive while it is still being written.
…irectory handle (#873)
* feat(project): --untrusted, with the project read beneath a cap-std directory handle
`--untrusted` on build, deploy and sync limits the steps a project may run
to a pre-built module and a plugin, and reads everything through
`files::Confined` at the project root.
`Confined` opens the root once as a `cap_std::fs::Dir` and makes every read
and write beneath that handle, so the operating system refuses a path that
leaves the root in the same step that opens it (`openat2` on Linux). On top
of that it refuses any path through a symlink, wherever the link leads.
This is #866 and #871 on main, without #868, and with `Confined` built on
cap-std in place of a path walk followed by a plain open.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(untrusted): refuse a pipe before it is read, vet what a plugin is handed, take a recipe path from the working directory
- Confined refuses to read what is not a regular file, before the open: a
pipe nobody writes to would otherwise hold an args, env or recipe read
forever.
- vet holds each dirs: and files: entry of a plugin step to a directory, or
a regular file or directory, in the project. The plugin runtime opens
those itself.
- A relative file:// recipe path is joined to the working directory before
it is read, as it resolves for a trusted project, rather than to the
confined root.
- Say that .icp is the tool's own state and is not vetted, and that the
plugin runtime opens by path after the look.
- Wrap the host feature list, which had passed taplo's column width.
Co-Authored-By: Claude <noreply@anthropic.com>
* refactor(untrusted): leave .icp to the tool, unguarded
What is under .icp is the tool's own state, and whoever takes a project
from elsewhere discards any that came with it. So the id store, the
artifact store and a network's directory go back to what they are on main,
with no confined filesystem handed to them.
Co-Authored-By: Claude <noreply@anthropic.com>
* feat(untrusted): refuse a recipe, rather than read it and vet what it renders to
A recipe is build instructions read from a file or fetched from a URL
while the project loads. Under --untrusted the loader is now given
untrusted::NoRecipes, a Resolve that fetches nothing, so a project naming
a recipe fails to load, wherever the recipe is from.
RecipeFetcher goes back to what it is on main: it no longer reads a local
template through the project's filesystem, since it is never asked for one
for an untrusted project.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(untrusted): the project is where the command stands, and no root is searched for
Finding a workspace root means reading the manifests above the working
directory and following the dependencies they name, with the host's own
filesystem, before there is a root to confine those reads to. It also let
a project above that declares this one be adopted as the root, which
widened what the untrusted project could read to the whole of that
project.
Under --untrusted the root is now the working directory, or the one
--project-root-override names, and the locator is pinned to it. A
directory with no icp.yaml is refused, saying so.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(untrusted): say that --untrusted is run from the directory that holds icp.yaml
It takes that directory as the project root and does not travel up to a
project above. Said in the flag's help, the CLI reference, the project
model and project dependencies pages, the changelog and the module doc.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(untrusted): say in the flag's help that .icp is not checked and is to be deleted
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(untrusted): say that vet checks no containment and takes it from the filesystem it is given
vet asks the filesystem whether a path is a file or a directory and takes
its word, so Vetted::new with HostFileSystem accepts a path outside the
root or behind a symlink without an error. Say so on both.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, and to the same rules as an entry met on the walk: not the output, and a regular file. The output is resolved once it has been created, since a dangling link given as the output only then says where the bundle is written, and each entry is resolved from its directory's real path, so a chain of directory links longer than the host follows in one path is still copied. 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, and
Confinedrefuses any symlink it is asked to pass through.Twelve 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, an output that is itself a link into a copied directory, a name that is not UTF-8, a symlinked
files:entry, afiles:entry that links to the output or is a named pipe, and a chain of 46 directory links.Ref: #863, #866
🤖 Generated with Claude Code