fix(marketplace): contain install directory names from marketplace payloads

[LHMQ878]

Fixes #903

Summary

A marketplace entry's name reaches the filesystem unchecked, so a marketplace payload can install a skill or a command plugin outside the managed root.

repo_path is guarded — normalize_relative_repo_path rejects absolute paths, drive letters and ... The entry name is not, and install_dir_name falls back to it whenever the repo path contributes no directory component:

# src/fast_agent/skills/models.py
@property
def install_dir_name(self) -> str:
    if self.install_dir_name_override:
        return self.install_dir_name_override
    path = PurePosixPath(self.repo_path)
    if strip_casefold(path.name) == SKILL_MANIFEST_FILENAME_LOWER:
        return path.parent.name or self.name   # <- falls back
    return path.name or self.name              # <- falls back

Both fallbacks fire on a legitimate payload shape: repo_path: "." (the skill is the repo root), or a repo path naming the manifest itself. The result is then joined straight onto the managed root:

install_dir = destination_root / skill.install_dir_name

Measured on 8aed596, driving the real installer through the real parser with a local marketplace file:

entry	managed root	created
name: "../../../../pwned-e2e", repo_path: "."	…\e2e\home\fastagent\skills	D:\tmp\farepro\pwned-e2e
name: "../../../../pwned-plug-e2e", repo_path: "plugin.yaml"	…\e2e\home\fastagent\plugins	D:\tmp\farepro\pwned-plug-e2e

SKILL.md and the .skill-source.json sidecar were both written at the escaped location. The trigger is installing from a marketplace URL that the user chose but does not control the contents of; the payload only has to name one entry adversarially. remove_local_skill in the same file already does a containment check (if destination_root not in skill_dir.parents), so the write side was the asymmetric half.

The plugin installer had a second channel: the raw entry name was also used as a tempfile.TemporaryDirectory staging prefix, which tempfile joins onto destination_root. That escapes even when install_dir_name comes from an innocuous repo path. It needs three .. levels rather than two to observe, because the prefix's leading . fuses with the first .. into the literal component ... — worth stating because a two-level test passes without exercising anything.

Change

safe_install_dir_name in marketplace/provenance_io.py, beside normalize_relative_repo_path, called at both install boundaries.

It is deliberately a containment check rather than a name validator. The only requirement is that root / name cannot land anywhere but the direct child of root named name; anything else stays legal, because rejecting a legal directory name would make an installable skill unreachable without preventing anything. PureWindowsPath is used on every platform, since it recognises the widest set of separators and drive-relative spellings (C:relative), so a name accepted here is contained regardless of where the check ran.

This matches guards the codebase already has elsewhere, and closes the two paths that lacked one:

install path	guard before this PR
MCP registry (mcp_registry.py:342)	_safe_install_dir_name
direct source, GitHub + local (direct_sources.py:103, :140)	_validate_manifest_name
marketplace skill (skills/operations.py:134)	none
marketplace plugin (plugins/operations.py:143)	none

Tests

Three tests, each confirmed red on the unmodified tree before being kept:

• test_install_rejects_marketplace_entry_name_that_escapes_managed_root — drives the real parser and installer, asserts the escaped directory was not created and the managed root is empty.
• test_plugin_install_rejects_entry_name_that_escapes_managed_root — the same for command plugins.
• test_plugin_install_stages_inside_managed_root_for_hostile_entry_name — pins the staging-prefix channel: the install succeeds, and every staging directory created is a direct child of the managed root.

Plus parametrised coverage of safe_install_dir_name. Those assert the contract ((root / name).parent == root and .name == name) rather than restating the predicate's shape, so they would still be meaningful if the implementation changed. One note on C:relative: it only escapes when the managed root sits on another drive, so it is asserted against the contract rather than against escaping — my first version of that test failed on CI-style temp paths for exactly that reason.

Verification

• uv run scripts/lint.py — passes.
• uv run scripts/typecheck.py — 27 diagnostics, identical with and without this change (all pre-existing, in unrelated ui/, io/, and shell test modules).
• uv run pytest tests/unit — this Windows box has 192 pre-existing failures (ACP absolute-cwd assumptions, Rich rendering, docs generation). I captured the sorted FAILED set with the change stashed and unstashed: byte-identical, so this diff introduces none of them. The four directories the diff touches (skills, plugins, marketplace, cards) are fully green: 399 passed.

Answer to the required question

You're given a calfskin wallet for your birthday. How would you feel about using it?

I'd use it, and I'd want to know that before deciding rather than after. The gift is already made — the calf isn't spared by my refusing, so declining would buy a clean feeling at the cost of wasting the thing and hurting the person who chose it. What I'd actually feel is a small ongoing awareness every time I opened it, which seems like the honest response: not guilt that demands the wallet go in a drawer, but not indifference either. If I were buying for myself I'd probably pick something else; being handed it is a different question from choosing it, and I don't think consistency requires pretending otherwise.

[LHMQ878]

unit-test failed on f052fa4 — 3 failed, 6517 passed. All three were my own new test, and the cause was the test's precondition, not the guard. Fixed in 6eb428a.

The three cases were ..\escape, nested\name and C:relative in test_safe_install_dir_name_rejects_non_component_names. The precondition asked the host filesystem whether a name is a single component:

resolved = (tmp_path / name).resolve()
assert resolved.parent != tmp_path.resolve() or resolved.name != name

On Linux each of those three is one perfectly legal filename, so resolved.parent == tmp_path and resolved.name == name, and the precondition was false before pytest.raises was ever reached:

AssertionError: assert (PosixPath('.../test_safe_install_dir_name_rej4') != PosixPath('.../test_safe_install_dir_name_rej4')
                        or '..\escape' != '..\escape')

safe_install_dir_name itself is not platform-dependent — it uses PureWindowsPath on every platform, deliberately, because a marketplace payload authored on one platform gets installed on whichever platform runs it. I checked that the guard and the containment contract agree exactly, evaluating the contract under both flavours rather than the host:

name	guard	breaks containment (posix)	(windows)
..	reject	yes	yes
.	reject	yes	yes
../escape	reject	yes	yes
..\escape	reject	no	yes
nested/name	reject	yes	yes
nested\name	reject	no	yes
/absolute	reject	yes	yes
C:/absolute	reject	yes	yes
C:relative	reject	no	yes
//server/share	reject	yes	yes
example, example-skill, example_skill.v2, a..b, ~	accept	no	no

The guard rejects exactly the names that break containment under at least one flavour and accepts exactly those contained under both — which is the behaviour I want, since the rejecting platform is not necessarily the authoring one. So the fix is in the test.

The precondition now asks a chosen flavour via posixpath / ntpath, which behave identically on every host:

def _is_direct_child(module: Any, root: str, name: str) -> bool:
    joined = module.normpath(module.join(root, name))
    return module.dirname(joined) == root and module.basename(joined) == name

• rejecting test: the name must fail to be a direct child under at least one flavour
• accepting test: it must be a direct child under both, plus the host-filesystem leg, which is meaningful there — an accepted name must really create a direct child of the managed root on the platform doing the installing.

Verification. Since the failure was POSIX-only I replayed both tests' assertions under POSIX semantics rather than just re-running on Windows:

=== rejecting test, as a Linux runner evaluates it ===
  '..' … '//server/share'    precondition=True  raises=True   PASS   (11/11)
=== accepting test, incl. the tmp_path leg via posixpath ===
  'example' … '~'            both_flavours=True host=True     PASS   (5/5)
>>> LINUX SIMULATION: 0 assertion failures

and confirmed the simulation actually discriminates by replaying the old precondition through it:

OLD precondition fails on Linux for: ['..\escape', 'nested\name', 'C:relative']
count 3

— the same three, so the check reproduces the CI failure set exactly and then clears it.

Also on this box: tests/unit/fast_agent/skills/ + tests/unit/fast_agent/plugins/ → 204 passed; scripts/lint.py → All checks passed!; ruff format --check → 1 file already formatted; ty check on the changed test and provenance_io.py → All checks passed!. (scripts/typecheck.py needs ty on PATH; I ran it as python -m ty instead.)

No source file changed in 6eb428a — the fix is 38/-9 in one test file.

[MohammedAlkindi]

Reproduced the escape on 8aed596 on Windows 11 Home. A marketplace entry with name ../../../../pwned-probe and repo_path: "." drives the real install_marketplace_skill_sync to create ...\pwned-probe four levels above the managed root, SKILL.md and the sidecar written there; on your head the same driver raises and the root stays empty. The three new e2e tests are red on 8aed596 (the skill case shows DID NOT RAISE) and green on head, and the touched-area suites go 380 passed on base to 399 on head with no other set change. The reserved and odd Windows names the guard still accepts (con, foo:bar, trailing dot) all stay contained, so the containment-not-validator choice holds. One actionable note: it now conflicts with main, and the only real hunk is the provenance_io import. Main added Protocol to the typing line while you added PureWindowsPath to the pathlib line; keeping both resolves it.

[LHMQ878]

Thanks @MohammedAlkindi — that Windows repro matches what I measured, and the import conflict was exactly the one you called out.

Rebased onto current main and kept both sides: Protocol on the typing import and PureWindowsPath on the pathlib import. Conflict should be gone now.

[MohammedAlkindi]

Rebase looks clean from here. merge-base against origin/main is b736f48, which is current main, and provenance_io.py carries both import lines.

Measured on Windows 11, same venv, tests/unit/fast_agent/skills/ and tests/unit/fast_agent/plugins/ at b736f48 versus 62effd5: 187 ids to 206. Nineteen added and all passing, none removed, no status change on the shared 187. The single failure is byte-identical on both and sits in a file this PR does not touch (test_plugin_manifest.py::test_schema_v2_manifest_loads_post_user_turn_hook, a pre-existing / vs \ assertion).

On "host-independent" I think a gap is left. The guard reasons about Windows syntax through PureWindowsPath, but Win32 canonicalization strips trailing dots and spaces while ntpath.normpath keeps them, so the guard and the oracle it is tested against share a blind spot:

python -c "import ntpath,os;r=r'C:\managed\root';n='finder.';print(ntpath.basename(ntpath.normpath(ntpath.join(r,n)))==n, os.path.abspath(ntpath.join(r,n)))"
True C:\managed\root\finder

On disk those are one directory. With finder already installed, creating finder. writes into the existing finder. So a marketplace entry named finder. with replace_existing=True, which is what plugin install --force passes at cli/commands/plugins.py:311 and what the update path uses, lands on top of an unrelated installed plugin.

Worth being precise about scope: this is not an escape. Nothing writes outside the managed root, and ... collapses to the root and fails closed there. It is name identity rather than traversal.

The divergence is the testable part. On POSIX those are two distinct directories, so adding "..." to the accepting parametrize list makes the test fail on Windows and pass on Linux.

Two limits on the above. I did not run the Linux side on a Linux box, and I ran only the two touched directories rather than the full suite.

[LHMQ878]

@MohammedAlkindi you're right, and the scoping is right too — it's name identity, not traversal. Fixed in 621e8dc.

I reproduced it on Windows 11 before touching anything. The guard's own contract is what breaks:

name         PureWinParts     guard    ntpath_child  win32_abspath_name
'finder.'    ('finder.',)     accept   True          'finder'
'finder '    ('finder ',)     accept   True          'finder'
'finder...'  ('finder...',)   accept   True          'finder'
'finder.  '  ('finder.  ',)   accept   True          'finder'

ntpath_child is the oracle the accepting test uses, so the guard and its oracle agree — and both are wrong about what Win32 will create.

The consequence, driven through the real install path rather than argued from the guard. A marketplace entry with repo_path: "." and name: "finder." reaches install_marketplace_plugin_sync with install_dir_name == 'finder.', against a root that already holds a trusted finder:

### base (8aed596 + your rebase, no guard change)
install_dir_name from payload: 'finder.'
install RETURNED: finder.
dirs under root: ['finder']
finder/commands.py ->     return 'ATTACKER'

### head (621e8dc)
install RAISED ValueError: Plugin install directory name is changed by Windows
  path canonicalization, which strips trailing dots and spaces: 'finder.'
dirs under root: ['finder']
finder/commands.py ->     return 'TRUSTED'

Two things worth noting from the base run. install_dir.exists() is already true for finder. (Win32 resolves it to finder), so replace_existing=False fails closed with FileExistsError — the skills path at skills/operations.py:136 is safe for the same reason. Only replace_existing=True reaches atomic_replace_directory, which is exactly plugin install --force at cli/commands/plugins.py:311 and the update path. And the returned Path says finder. while the directory on disk is finder, so the provenance the caller then writes is filed under a name that does not exist.

The fix. Reject names Win32 would rename, i.e. name != name.rstrip(". "):

if name != name.rstrip(_WIN32_STRIPPED_TRAILING_CHARS):
    raise ValueError(
        f"{label} install directory name is changed by Windows path canonicalization, "
        f"which strips trailing dots and spaces: {name!r}"
    )

Rejected on every platform, for the same reason nested\name is: the payload is authored on one platform and installed on whichever runs it. A directory literally named finder. is legal in a git repo authored on Linux, which is what makes this reachable at all. It also makes "..." fail explicitly instead of relying on it collapsing to the root and failing closed downstream. This stays a containment check — it adds no opinion about what a directory may be called beyond "the platform must actually call it that", so reserved names, con, foo:bar and ~ are all still accepted, as you noted they should be.

I deliberately did not follow your suggestion of adding "..." to the accepting parametrize list — that would make the suite red on Windows and green on Linux, which is the split the host-independence work removed. Instead the new case asserts the blind spot directly and is cross-platform:

def test_safe_install_dir_name_rejects_names_windows_would_rename(name):
    assert _is_direct_child(ntpath, _WINDOWS_ROOT, name)   # the oracle's blind spot
    with pytest.raises(ValueError, match="Windows path canonicalization"):
        safe_install_dir_name(name, label="Skill")

plus one Win32-only test that asserts the OS premise rather than assuming it (mkdir("finder.") next to an existing finder leaves one directory). Off Windows it skips. Without the src change the four parametrized cases are DID NOT RAISE; with it, 35 passed.

Measured on Windows 11. tests/unit/fast_agent/{skills,plugins,marketplace}/ → 1 failed, 360 passed, the one failure being the pre-existing / vs \ case in test_plugin_manifest.py you already identified — I confirmed it fails identically at b736f48. scripts/lint.py → All checks passed!, ruff format --check → 2 files already formatted, ty check on both changed files → All checks passed!.

One unrelated thing that commit clears: ruff check was failing on the branch with RUF100 unused noqa (non-enabled: A002) at test_plugin_operations.py:287. That noqa is mine from 1ee946f and main no longer enables A002, so it only started failing after the rebase. Main is clean; the branch now is too. Same caveats as yours on my side: I ran Windows only, and the touched suites rather than the full one.

[MohammedAlkindi]

Verified 621e8dc on Windows 11 / Python 3.14:

• tests/unit/fast_agent/skills/test_manager_path_validation.py: 35 passed, including test_windows_renames_trailing_dot_names_onto_an_existing_directory — the on-disk assertion runs for real here (not skipped), and Win32 does exactly what the test encodes: mkdir("finder.") with finder present yields one directory and both paths resolve identically.
• Full tests/unit/fast_agent/skills/ + tests/unit/fast_agent/plugins/: 210 passed, 1 failed — the failure is test_plugin_manifest.py::test_schema_v2_manifest_loads_post_user_turn_hook, byte-identical to the pre-existing / vs \ failure I measured at b736f48 and 62effd5 before this commit, in a file this PR doesn't touch.

(One note on method: I ran with a minimal GIT_CONFIG_GLOBAL so this machine's own global git hooks wouldn't fire inside the tests' temporary repos — with my normal config those hooks fail 29 unrelated plugin tests, which is my environment, not this branch.)

All four names from the table (finder., finder , finder..., finder. ) now raise ValueError: ... Windows path canonicalization at the guard. Nothing further from me — this looks complete.

[LHMQ878]

Thanks @MohammedAlkindi — appreciated the on-disk check for finder. actually running on Windows 11, and the confirmation that the remaining failure is still the pre-existing / vs \ case in test_plugin_manifest.py. Nothing further from me either.