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.