feat(sync-plugin): forbid symlinks in plugin dirs/files#627
Merged
Conversation
Declared `dirs`/`files` entries are resolved on the host before the WASI sandbox boundary, so the lexical "relative, no `..`" check was not enough: an entry that is a symlink — or traverses a symlinked parent component — would let a preopen or a host read resolve to a target outside the canister directory. Add `first_symlink_component`, a shared helper that walks each component of a declared path under the base directory and rejects the entry if any prefix is a symlink. Apply it to `dirs` (runtime preopen) and `files` (host read), with new `SymlinkDir`/`SymlinkFile` error variants. Symlinks inside a preopen that escape it remain handled by cap-std at runtime. Symlinks are forbidden outright for now; the restriction can be relaxed later if a safe use case emerges. Update the concept/reference/design docs, which previously implied all symlink escapes were rejected. Tests: unit tests for the helper, a fixture-gated runtime test for the SymlinkDir wiring, and two end-to-end tests asserting deploy fails when a declared dir/file points outside the project via a symlink. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Recorded under the Unreleased > Experimental subsection, per the changelog convention for the (experimental) sync-plugin feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens sync-plugin path handling by forbidding symlinks in declared dirs/files, preventing host-side resolution from escaping the canister directory before the WASI sandbox boundary.
Changes:
- Added
first_symlink_component(base, rel)helper and reused it acrossicp-sync-pluginruntime (dirs) andicpCLI sync (files). - Rejected symlinked declared paths with new error variants (
RunPluginError::SymlinkDir,PluginError::SymlinkFile). - Updated documentation and added unit/runtime/integration tests covering final and intermediate symlink cases.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/reference/configuration.md | Documents the new “no symlinks in declared dirs/files” rule. |
| docs/concepts/sync-plugins.md | Clarifies runtime sandbox behavior vs. declared-path symlink rejection. |
| crates/icp/src/canister/sync/plugin.rs | Adds symlink rejection for host-side reads of declared files. |
| crates/icp-sync-plugin/src/runtime.rs | Adds symlink rejection prior to preopening declared dirs; includes a fixture-gated test. |
| crates/icp-sync-plugin/src/path.rs | Introduces shared helper to detect the first symlink component under a base dir + unit tests. |
| crates/icp-sync-plugin/src/lib.rs | Exposes the new helper from the crate API. |
| crates/icp-sync-plugin/DESIGN.md | Documents the declared-path symlink ban and rationale. |
| crates/icp-sync-plugin/Cargo.toml | Adds camino-tempfile as a dev-dependency for new tests. |
| crates/icp-cli/tests/sync_tests.rs | Adds E2E tests ensuring symlinked declared dirs/files are rejected. |
| Cargo.lock | Updates lockfile for the new dev dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review on #627: - Harden the lexical check: a Windows drive-relative path like `C:foo` carries a `Prefix` component but `is_absolute()` returns false, so the old check let it through and `join` would discard the base directory. Replace the ad-hoc `is_absolute() + ..` check with a shared `escapes_base` that rejects `ParentDir`, `RootDir`, and `Prefix(_)`, mirroring the bundler's escape checks. Used for both `dirs` and `files`. - `first_symlink_component` now returns the offending sub-path relative to the base (e.g. `link` or `link/inner`) instead of the fully joined host path, so the error no longer leaks the absolute on-disk location. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the host-side reading of declared `files` from the CLI's `sync/plugin.rs` into `run_plugin`, alongside the existing `dirs` preopen. `run_plugin` now takes `files: Vec<String>` (manifest-relative names) instead of pre-read `(name, content)` pairs and opens them itself from `base_dir`. This keeps all filesystem access and path-safety logic in one place: the `escapes_base` and `first_symlink_component` helpers are now crate-private (no longer re-exported), and the file-path error variants (`UnsafeFile`, `SymlinkFile`, `ReadFile`) live on `RunPluginError`. The CLI just forwards the manifest's `dirs`/`files` strings. Behavior is unchanged: the same checks apply to `files` as before, now surfaced via `PluginError::Run`. Add runtime-level tests for the moved logic (missing file -> ReadFile, symlinked file -> SymlinkFile) and update DESIGN.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `escapes_base` assertions lived in the `cfg(all(test, unix))` module, so the Windows-specific `Prefix` rejection (`C:foo`) — the whole reason that branch exists — was never compiled or run. CI runs on Windows, so split the tests: - A cross-platform `escapes_base_tests` module: shared assertions, plus a `#[cfg(windows)]` test asserting drive-relative/absolute/UNC prefixes escape, and a `#[cfg(unix)]` test documenting that `C:foo` is a plain filename there. - The symlink tests stay unix-only (they create symlinks) as `symlink_tests`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
adamspofford-dfinity
approved these changes
Jun 26, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Declared
dirs/filesin apluginsync step are resolved on the host before the WASI sandbox boundary, so the old lexical check (relative, no..) wasn't enough — a symlinked entry, a symlinked parent, or a Windows drive-relative path (C:foo, not "absolute") could escape the canister directory.Per a team decision, symlinks are now forbidden outright in plugin
dirs/fileswhile the feature is young; the restriction can be relaxed later. This PR also closes the related drive-relative escape and moves alldirs/filesfilesystem access into the sandbox crate.What changed
Two crate-private helpers in
icp-sync-plugin(path.rs), applied uniformly todirs(before preopen) andfiles(before read):escapes_base— rejects.., filesystem root, and Windows drive-prefix (C:foo) components, mirroringbundle.rs:861.first_symlink_component— rejects an entry that is, or traverses, a symlink; returns the offending sub-path relative to base so errors don't leak absolute paths.run_pluginnow reads the declared files itself (files: Vec<String>instead of pre-read pairs), so the CLI just forwards manifest strings. Symlinks inside a preopen are still rejected by cap-std at runtime, unchanged.Tests & docs
#[cfg(windows)]test for the drive/UNC-prefix case (runs on Windows CI).symlinked_dir/symlinked_file/read_fileerrors) and e2e tests (deployfails on a symlinkeddirs/files); existing plugin happy-paths still pass.sync-plugins.md/configuration.md/DESIGN.md, plus a changelog entry underUnreleased > Experimental.🤖 Generated with Claude Code