Repository navigation
Protect shared agent sources during kit installation - #1
MGRAFF2006 wants to merge 3 commits into
Conversation
MGRAFF2006
left a comment
There was a problem hiding this comment.
Independent review of head 90879af16f15d357931d96865a3a3c4e0514cb79 against base 838ea35c60f44ce968eba8e57d5f7d20d396d2b5.
P1 — Canonicalize the repository root before comparing it with a resolved destination parent (scripts/install-agent-kit.sh:5,20-22). repo_root uses logical pwd, whereas the new guard uses physical realpath -m. Running through a symlink to the checkout therefore compares different path spellings for the same repository, bypassing the guard. In a temporary fixture with alias -> repo and home/.claude/skills -> repo/agent/skills, invoking bash alias/scripts/install-agent-kit.sh exits 0, moves the real agent/skills/example directory into the home backup, and replaces the source with a dangling self-referential link. This reproduces the precise destructive behavior this PR aims to prevent. Resolve the repository root physically, and exercise installation through a checkout alias in the regression check.
Minimal reproduction (all mutations confined to a temporary fixture):
import os, shutil, subprocess, tempfile
from pathlib import Path
source = Path('scripts/install-agent-kit.sh').resolve()
with tempfile.TemporaryDirectory() as tmp:
root = Path(tmp)
repo, home, alias = root/'repo', root/'home', root/'alias'
(repo/'scripts').mkdir(parents=True)
(repo/'agent/skills/example').mkdir(parents=True)
home.mkdir()
shutil.copy2(source, repo/'scripts/install-agent-kit.sh')
(repo/'agent/GLOBAL.md').write_text('shared instructions\n')
(repo/'agent/skills/example/SKILL.md').write_text('shared skill\n')
alias.symlink_to(repo, target_is_directory=True)
(home/'.claude').mkdir()
(home/'.claude/skills').symlink_to(repo/'agent/skills', target_is_directory=True)
env = dict(os.environ, AGENT_KIT_HOME=str(home),
CODEX_HOME=str(home/'.codex'), XDG_CONFIG_HOME=str(home/'.config'))
result = subprocess.run(['bash', str(alias/'scripts/install-agent-kit.sh')], env=env)
print('exit:', result.returncode)
print('source is dangling:', (repo/'agent/skills/example').is_symlink()
and not (repo/'agent/skills/example').exists())P2 — Validate the backup destination before creating or writing it (scripts/install-agent-kit.sh:29-36). The new guard validates the original destination being retired, but the actual backup destination is never checked. With home/.local/share/agent-kit-backups -> repo/agent and an existing home/AGENTS.md, a normal physical-checkout invocation still exits 0 and moves the machine-local instructions into repo/agent/<timestamp>/AGENTS.md. mkdir, mktemp, and mv therefore write into the shared checkout despite the protection, and may sync local instruction/skill contents that were supposed to stay local. This is an adjacent preexisting hole, not a newly introduced regression; it matters to the stated boundary against destinations inside the shared repo. Reject a backup root whose resolved parent/tree falls in the checkout before creating it or moving a file.
Validation: python3 scripts/check-agent-kit.py, shellcheck scripts/install-agent-kit.sh, and git diff --check all passed. Additional temporary fixtures verified nested relative Claude parent aliases, shared custom and relative CODEX_HOME/XDG_CONFIG_HOME, and Cursor duplicate retirement are rejected with source files preserved. A similarly named sibling directory is correctly allowed. No live home installation, model calls, remote-machine mutation, or repository-source edits were performed. This is bounded installer/path review, not a claim about unrelated project behavior or protection from concurrent symlink replacement.
MGRAFF2006
left a comment
There was a problem hiding this comment.
Independent follow-up review of exact head 988cf6a43263249ffa7fa6d90a20ff02cddfe10b.
The original P1 (symlinked checkout alias) and P2 (shared backup-root symlink) are fixed. I independently reran both probes: each exits 1, preserves original skill/local instructions, and creates no backup content in the checkout. Ordinary absolute and relative custom Codex/XDG roots, including a similarly named repo-sibling, still install successfully and preserve existing custom instructions in a backup. The expanded project check, shellcheck, and whitespace check pass.
P2 — Validate or normalize the final backup path, not only its base (scripts/install-agent-kit.sh:34-37). Relative custom roots are accepted, but relative="external$dst" embeds their ../ segments in the backup path. The new base guard succeeds and later mkdir/mv traverse out of that base into an entirely different destination, including the shared checkout. In the fixture below the custom Codex directory resolves outside the checkout (root/a/repo/agent/local), but its existing instructions are backed up into root/repo/agent/local/AGENTS.md, inside the checkout; installation exits 0. Fewer parent segments also escape the named backup directory, making the printed backup location incomplete. Normalize external paths before constructing their backup names and/or validate the resolved final backup destination before creating directories or moving files.
Reproduction, limited to a temporary fixture:
import os, shutil, subprocess, tempfile
from pathlib import Path
source = Path('scripts/install-agent-kit.sh').resolve()
with tempfile.TemporaryDirectory() as tmp:
root = Path(tmp)
repo, home = root/'repo', root/'home'
cwd = root/'a/b/c/d/e/f/g/h'
(repo/'scripts').mkdir(parents=True)
(repo/'agent/skills/example').mkdir(parents=True)
home.mkdir()
cwd.mkdir(parents=True)
shutil.copy2(source, repo/'scripts/install-agent-kit.sh')
(repo/'agent/GLOBAL.md').write_text('shared\n')
(repo/'agent/skills/example/SKILL.md').write_text('skill\n')
codex = '../' * 7 + 'repo/agent/local'
(cwd/codex).mkdir(parents=True)
(cwd/codex/'AGENTS.md').write_text('machine-local instructions\n')
env = dict(os.environ, AGENT_KIT_HOME=str(home), CODEX_HOME=codex,
XDG_CONFIG_HOME=str(home/'.config'))
result = subprocess.run(['bash', str(repo/'scripts/install-agent-kit.sh')],
env=env, cwd=cwd)
print('exit:', result.returncode)
print('backup inside checkout:', (repo/'agent/local/AGENTS.md').read_text())Validation performed without live-home installation or edits to the author's worktree. Scope remains installer mutation/backup/retirement and adversarial path aliases; this review does not assert unrelated project correctness or concurrent-symlink race protection.
MGRAFF2006
left a comment
There was a problem hiding this comment.
Independent follow-up review of exact head 042de63f2b8b8d115949e944e40386567522d1f7 against base 838ea35c60f44ce968eba8e57d5f7d20d396d2b5.
No remaining actionable findings within the reviewed installer/path scope. The relative external-root backup escape from round 2 is fixed: startup canonicalization removes traversal from backup names before they are constructed. I independently reran that deep-relative CODEX_HOME reproduction and confirmed the original local instructions remain within the named home backup tree, the live custom directory gets its intended link, and the shared checkout is unchanged.
Also reran the original symlinked-checkout and shared-backup-root failures: each correctly refuses with the original content preserved. Temporary controls covered a symlinked backup parent, nested relative Claude parent aliases, legacy Cursor retirement through shared parents, custom Codex/XDG roots inside the checkout, similarly prefixed sibling directories, ordinary absolute/relative custom roots, deep relative XDG plus agent-home roots, and aliases of the checkout/home/custom roots. All 13 controls passed. Successful installations were repeated to verify idempotency without additional backups; existing Codex and OpenCode instructions were found intact inside the named backup directory.
python3 scripts/check-agent-kit.py, shellcheck scripts/install-agent-kit.sh, and the complete PR whitespace check passed. Reviewed the complete installer mutation, retirement, and backup flow plus all three PR revisions. No live-home installation, author-worktree edits, model calls, or remote-machine changes occurred.
This is a bounded review of the installer and its regression checks, not a guarantee that unrelated project behavior is defect-free or that concurrent hostile symlink replacement is prevented.
When Claude or a legacy Codex skills directory points into the shared config checkout, installing the agent kit currently moves the source skill into a home backup and leaves a dangling link in the repository. Refuse replacements, duplicate retirement, and backup creation when their destination resolves into the shared checkout. Resolve the checkout physically so invoking through a symlink cannot bypass the guard.
Test plan:
python3 scripts/check-agent-kit.pypasses, including existing backups and idempotency checks.shellcheck scripts/install-agent-kit.shandgit diff --checkpass.No live home setup was changed.