security(personas): vet the persona repo URL before cloning it (#521) - #656
Open
harrylettering wants to merge 1 commit into
Open
security(personas): vet the persona repo URL before cloning it (#521)#656harrylettering wants to merge 1 commit into
harrylettering wants to merge 1 commit into
Conversation
`install_from_git` handed the caller's string straight to `git clone`, and git reads parts of that string as instructions rather than as an address: `ext::sh -c ...` runs a command through the remote-helper mechanism, and a URL starting with `-` is parsed as an option. Both execute before any manifest is parsed, so before the consent screen the install flow is supposed to rely on. `validate_git_url` now refuses any `<helper>::` form, a leading dash, whitespace and control characters, and anything outside https / ssh / git / `user@host:path`. It runs at the process boundary in `git_clone` and at the logical entry in `clone_persona_repo`, so an injected clone is not a way around it. The clone command itself adds `--` to end option parsing and `-c protocol.ext.allow=never` as a second lock on the helper path. Fixes andrewyng#521 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HYvy9TcuTD4CZMVcdwwH4D
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PersonaRegistry.install_from_gitpassed the caller's string straight togit clone, and git reads parts of that string as instructions rather than as an address:ext::sh -c '<command>'runs<command>through git's remote-helper mechanism.protocol.ext.allowdefaults touser, so a direct clone is exactly the case it still permits.-is parsed as an option (--upload-pack=...).Both execute code before any manifest is parsed, so before the install-time consent screen the persona trust model relies on. Reported as #521.
Fix
validate_git_urlvets the address up front:<helper>::form (ext::,fd::, …)-https://,ssh://,git://, or the scp-likeuser@host:pathIt runs in two places:
git_clone(the process boundary) andclone_persona_repo(the logical entry), so a caller that injects its owncloneis not a way around the check and a refused URL never reaches the cache-directory naming either.The clone command gains two more locks:
--ends option parsing;protocol.ext.allow=neveris a second lock on the helper path./v1/personas/installalready surfaces the raisedValueErrorto the caller, so no server change was needed.Tests
Four new tests in
tests/test_persona_loading.py: six refused URLs, four accepted ones, the injected-clone case proving the vet happens before the clone, and the argv shape.tests/test_persona_loading.pypasses (22). The full suite shows the same 28 failures before and after this change on my machine (Slack relay, browser address guard, UI e2e — all network/Playwright dependent), with no new ones.Trade-off
Refusing every
::also refuses IPv6 literals such asssh://[::1]/repo. A loopback IPv6 host is not a plausible persona source, and the refusal names its reason, so this fails closed on purpose.Fixes #521
🤖 Generated with Claude Code
https://claude.ai/code/session_01HYvy9TcuTD4CZMVcdwwH4D