Skip to content

fix(marketplace): copy an in-tree symlink instead of refusing it - #257

Closed
smarchetti wants to merge 1 commit into
spxrogers:mainfrom
smarchetti:fix/relative-fetcher-in-tree-symlinks
Closed

fix(marketplace): copy an in-tree symlink instead of refusing it#257
smarchetti wants to merge 1 commit into
spxrogers:mainfrom
smarchetti:fix/relative-fetcher-in-tree-symlinks

Conversation

@smarchetti

Copy link
Copy Markdown

The asymmetry

RelativeFetcher's copyDir refuses every symlink inside a marketplace tree, while GitFetcher's rejectEscapingSymlinks deliberately permits one that resolves in-tree. The same repository is therefore registrable as github: but refused as a local path.

fetch_relative.go already documents the in-tree policy for a symlinked source path — "a symlink is fine as long as it RESOLVES inside the root (mirroring the git fetcher's in-tree-symlink policy)" — and TestRelativeFetcher_FollowsBenignSymlinkSource pins it as "legitimate per the git fetcher's in-tree-symlink policy". This PR extends that same, already-accepted principle from the source path to entries discovered inside the tree.

Why it bites hardest on a private marketplace

A private repo cannot be fetched as github: at all: the git fetcher builds git.CloneOptions{URL: rawURL} with no Auth, and go-git derives HTTP basic auth only from URL userinfo — no credential helper, no GITHUB_TOKEN, no .netrc. So a local clone registered as a path is the only route in. A repo that keeps one component tree and links the per-agent views at it (.claude/skills/x -> ../../.agents/skills/x, six such links in the repo I hit this on) had no route left at all.

Repro, against a real repo with six in-tree symlinks

Before — agentsync 0.14.0:

✗ ERROR  fetch marketplace <clone>: relative fetcher: copy <clone> → <cache>:
         relative fetcher: <clone>/.claude/skills/next-best-practices is a symlink
         (refusing — marketplace trees must contain only regular files and directories)

After, same clone:

✅ added marketplace tpg

with all six linked skill directories materialised in the cache, contents intact, and zero symlinks left in the cache.

The change

copyDir resolves a link and then judges it, instead of refusing outright:

link behaviour
in-tree dereferenced and copied (dir or file)
escapes the tree refused, unchanged — the hole this guard exists to close, since copyFile's os.Open follows links
unresolvable (dangling, ELOOP) refused, failing closed as the git fetcher does
points at its own ancestor refused — dereferencing would recurse. No counterpart in the git fetcher, which preserves links rather than following them, so it never faces the case

Dereferencing rather than recreating the link leaves the cache with no symlinks at all, so nothing reading it later can be redirected by one, and an absolute in-tree link needs no rewriting to stay valid under the new root.

copyDir takes the tree root explicitly, since the boundary must not shift as the walk descends. extractSubdir passes the extracted subdir rather than the clone root: the subdir is all that survives the swap, so a link reaching into a discarded part of the clone would dangle.

TestRelativeFetcher_RejectsSymlinkEntry — the original leak regression — is an escaping link and passes unchanged. New tests cover the in-tree dir link (relative), the in-tree file link (absolute), the symlink-free-cache property, and all three refusals.

Verification

scripts/test-in-container.sh — all gates green (vet, build, go test -race ./..., e2e, BDD, smoke).

Two unrelated papercuts hit on the way there, both macOS-only, happy to send a separate PR if useful:

  1. build_image() fails under /bin/bash 3.2 (macOS's default) with build_args[@]: unbound variableset -u plus an empty-array expansion on the docker path. Works under bash 5.
  2. test-in-container.sh -- <cmd> runs bash -lc, and the login shell drops /usr/local/go/bin from PATH, so -- go test ... fails with go: command not found.

Follow-ups, not in this PR

Two changes would remove the local-clone workaround entirely rather than just unbreak it. Both touch more than a fetcher guard, so I'd rather hear your preference before writing either:

  1. Accept ssh:// and scp-style remotes in parseMarketplaceSource. Small: checkURLScheme already allows ssh, and a scp-style URL never reaches url.Parse (no ://), so only the parser and deriveMarketplaceSlug need to change. go-git's ssh transport authenticates from the ssh-agent via DefaultAuthBuilder with no config.
  2. Resolve HTTPS credentials via git credential fill and pass http.BasicAuth to go-git. This is the one that covers a SAML-SSO org, where an unauthorized ssh key is rejected outright but the credential helper holds a working token.

🤖 Generated with Claude Code

RelativeFetcher's copyDir refused every symlink inside a marketplace tree,
while GitFetcher's rejectEscapingSymlinks permits one that resolves in-tree.
The same repository was therefore registrable as `github:` but refused as a
local path:

    relative fetcher: <mp>/.claude/skills/next-best-practices is a symlink
    (refusing — marketplace trees must contain only regular files and
    directories)

That asymmetry bites hardest on a PRIVATE marketplace. The git fetcher builds
go-git CloneOptions with no Auth, and go-git derives HTTP basic auth only from
URL userinfo — no credential helper, no GITHUB_TOKEN, no .netrc — so a private
repo cannot be fetched as `github:` at all, and a local clone registered as a
path is the only route. A repo that keeps one component tree and links the
per-agent views at it (.claude/skills/x -> .agents/skills/x) had no route left.

copyDir now resolves a link and judges it, rather than refusing outright:

  - in-tree           -> dereferenced and copied (dir or file)
  - escapes the tree  -> refused, unchanged; this is the hole the guard exists
                         to close, since copyFile's os.Open follows links
  - unresolvable      -> refused, failing closed as the git fetcher does
  - own ancestor      -> refused; dereferencing would recurse. No counterpart
                         in the git fetcher, which preserves links rather than
                         following them, so it never faces the case

Dereferencing rather than recreating the link leaves the cache with no symlinks
at all, so nothing reading it later can be redirected by one, and an absolute
in-tree link needs no rewriting to stay valid under the new root.

copyDir takes the tree root explicitly, since the boundary must not shift as
the walk descends. extractSubdir passes the extracted subdir, not the clone
root: the subdir is all that survives the swap, so a link reaching into a
discarded part of the clone would dangle.

TestRelativeFetcher_RejectsSymlinkEntry (the original leak regression) is an
escaping link and still passes unchanged.
@smarchetti

Copy link
Copy Markdown
Author

Closing — I've stopped using agentsync, so I'm no longer in a position to maintain or validate this. Leaving the analysis here in case it's useful to anyone who picks it up.

The asymmetry is real: RelativeFetcher's copyDir refuses every symlink inside a marketplace tree, while GitFetcher's rejectEscapingSymlinks deliberately permits one that resolves in-tree. So a repo using in-tree symlinks (.claude/skills/x -> ../../.agents/skills/x) is registrable as github: but refused as a local path. That mattered because a private repo can only be reached as a local path today — the git fetcher passes no Auth to go-git, which derives HTTP basic auth solely from URL userinfo and consults no credential helper.

The branch has the fix and tests if anyone wants to pick it up: escaping, dangling, and self-ancestor links stay refused; in-tree links are dereferenced so the cache ends up symlink-free. Full container gate was green.

Also flagged in the description: two macOS-only papercuts in scripts/test-in-container.sh (bash 3.2 empty-array expansion under set -u, and -- <cmd> losing go from PATH via bash -lc). Those are independent of this change and still worth fixing.

@smarchetti smarchetti closed this Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant