Commit 827fff5
Fernando Pinho
security: reject path traversal and symlinks in copy + .skills.toml
Fixes four path-safety issues that, in combination, allowed a malicious
skills library or a crafted .skills.toml (e.g. mergeable via PR) to
exfiltrate arbitrary files through the round-trip (read on `add`, leak
on `push`) and to delete arbitrary directories outside the project or
library root on `pull` / `push` / `detect`. Reported privately on
2026-05-19 by firebaguette via the Umanio Discord; all four issues are
addressed below.
1. Symlink follow in fs_util::copy_dir_all. A symlink inside a skill
folder bypassed `entry.file_type().is_dir()`, fell into the file
branch, and was dereferenced by `fs::copy`. A subsequent `push`
would have published the symlink target's content to the (possibly
public) library. Symlinks are now hard-rejected by `copy_dir_all`
at both the top-level source and any descendant entry, and
`replace_folder_contents` refuses a symlinked destination so
`remove_dir_all` cannot be tricked.
2. Path traversal via `destination` and `source_path` in .skills.toml.
Both fields deserialized as PathBuf with zero validation. Because
Path::join lets an absolute right-hand side replace the base, a
`.skills.toml` entry like `destination = "/home/user/.ssh"` made
`cwd.join(...)` resolve outside the project and the downstream
`remove_dir_all` wipe arbitrary directories. `..` traversal was
equally unguarded. New `InstalledSkill::validate` runs at load time
and rejects absolute paths, parent traversal, and Windows-prefix
components for both fields. The same check is wired
(defense-in-depth) at every destructive call site via the new
`path_safety::safe_join` helper.
3. `detect --target` accepted `..` even though it rejected absolute.
Validation in `commands::detect::resolve_target` and the
interactive custom-path prompt now go through
`validate_relative_subpath`, rejecting any non-Normal/CurDir
component.
4. Fork-name validation accepted `.` and `..` literally. The
`validate_fork_name` helpers in push.rs and pull.rs only rejected
`/` and `\`, so a fork named `..` would have produced a Path::join
resolving to the parent directory. `.` and `..` are now explicit
rejections.
Threat-model note: the fix is purely lexical (component-level) plus an
explicit symlink check at copy time. No filesystem canonicalize calls
were added, avoiding TOCTOU windows and keeping the validation
pure-functional (AppError::Config -> exit code 2). 34 new unit tests
cover each rejection class and each attack scenario.1 parent 04ee4eb commit 827fff5
10 files changed
Lines changed: 714 additions & 20 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
9 | 22 | | |
10 | 23 | | |
11 | 24 | | |
| |||
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | | - | |
| 3 | + | |
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
38 | 41 | | |
39 | 42 | | |
40 | 43 | | |
| |||
0 commit comments