Harden memory.sync git transport against ext::/fd:: command execution (#51)#60
Draft
TYRMars wants to merge 1 commit into
Draft
Harden memory.sync git transport against ext::/fd:: command execution (#51)#60TYRMars wants to merge 1 commit into
TYRMars wants to merge 1 commit into
Conversation
Pin GIT_ALLOW_PROTOCOL on every git invocation in memory_sync's run_git, mirroring memory_include. memory.sync_setup already validates remote_url via validate_git_url, but memory.sync's push/pull act on an already-stored remote.origin.url that never re-passes validation — an out-of-band remote set to an ext::/fd:: transport could still execute arbitrary commands. Exporting GIT_ALLOW_PROTOCOL refuses those protocols at the git layer as defence in depth. Adds a regression test that configures an ext:: remote directly (bypassing the validator) and asserts the smuggled command never runs. Fixes #51 https://claude.ai/code/session_01UxZiFGdjUnrqLitT3S92uC
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
2 participants
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.
Summary
Closes #51 — arbitrary command execution risk via git's
ext::/fd::remote-helper transports in thememory.sync_setup/memory.synccodepath.What was already in place
The primary validation the issue asks for is already present:
memory_sync.rscallscrate::memory_include::validate_git_url(remote_url)at the top ofMemorySyncSetupTool::invoke(added in ae7832a, the #37 fix). That rejectsext::/fd::/ any<transport>::form, leading-, and control chars before the URL reachesgit remote add.The remaining gap this PR closes
memory_sync.rshas its ownrun_githelper, and unlikememory_include.rs's it did not exportGIT_ALLOW_PROTOCOL. That matters because:memory.sync's push / pull / fetch operate on an already-storedremote.origin.urlthat never re-passesvalidate_git_url. A remote set out-of-band (manualgit remote add, a repo cloned with a hostile config, or any future validation bypass) could still fireext::/fd::.This PR pins
GIT_ALLOW_PROTOCOL=https:ssh:git:fileon every git invocation inmemory_sync, as defence in depth — mirroringmemory_include::run_git. The shared constant is promoted topub(crate)so there's a single source of truth.Changes
memory_include.rs:GIT_ALLOW_PROTOCOL→pub(crate).memory_sync.rs:run_gitnow exportsGIT_ALLOW_PROTOCOL.run_git_refuses_ext_transport_configured_out_of_band: configures anext::remote directly (bypassing the validator), runsgit push, and asserts the smuggled command never executes.Testing
cargo test -p harness-tools memory_sync::/memory_include::— all pass (commit-signing disabled in the sandbox; the 4git commit-based tests are unrelated and fail only because this environment forces SSH commit signing against a stub server).cargo clippy -p harness-tools --all-targets -- -D warnings— clean.Note:
file://is intentionally kept allowed (it's a safe local transport git handles natively); the issue listed it as a "reject" candidate but the existingvalidate_git_urlcontract deliberately permits it, so this PR does not change that.https://claude.ai/code/session_01UxZiFGdjUnrqLitT3S92uC
Generated by Claude Code