From 2cb7262934f1514bcadf1916d6ef375cf409589d Mon Sep 17 00:00:00 2001 From: archae0pteryx Date: Wed, 28 Oct 2026 07:09:41 -0700 Subject: [PATCH 1/2] Restrict git transports in every child skillz runs but a launched agent, and refuse GitHub shorthands that are not owner/repo. --- crates/skillz-harness/src/clone.rs | 90 +++++++++++++++++++++++++++++ crates/skillz-harness/src/runner.rs | 88 +++++++++++++++++++++++++++- 2 files changed, 175 insertions(+), 3 deletions(-) diff --git a/crates/skillz-harness/src/clone.rs b/crates/skillz-harness/src/clone.rs index 9615a9f..e3dd2a1 100644 --- a/crates/skillz-harness/src/clone.rs +++ b/crates/skillz-harness/src/clone.rs @@ -17,6 +17,24 @@ fn check_source(source: &str) -> anyhow::Result<()> { Ok(()) } +/// A GitHub shorthand is exactly `/`: anything else (`../x`, `~/x`, `a/b/c`) is +/// not a repository name and must not reach git. +fn check_repo_name(source: &str) -> anyhow::Result<()> { + let is_url = source.contains("://") || source.starts_with("git@"); + let is_name = |part: &str| { + !part.is_empty() && part != "." && part != ".." && !part.starts_with(['-', '~']) + }; + let is_owner_and_repo = source + .split_once('/') + .is_some_and(|(owner, repo)| is_name(owner) && is_name(repo) && !repo.contains('/')); + anyhow::ensure!( + is_url || (is_owner_and_repo && !source.contains('\\')), + "invalid repository name: {}", + redact_text(source) + ); + Ok(()) +} + /// Resolve a local directory or inspect a temporary clone. The callback receives the stable /// install source as well as the fetched tree, so temporary paths are never persisted. pub fn with_source( @@ -40,6 +58,7 @@ pub fn with_source( Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} Err(e) => return Err(e).with_context(|| format!("reading source {}", redact_text(source))), } + check_repo_name(source)?; let temporary = tempfile::Builder::new() .prefix("skillz-install-") .tempdir()?; @@ -147,6 +166,77 @@ mod source_tests { assert!(runner.calls().is_empty()); } + #[test] + fn anything_but_owner_slash_repo_is_refused_and_runs_nothing() { + let fx = project_with(|_| {}); + let runner = FakeRunner::new(|_| {}); + let sources = [ + "../x", + "a/../b", + "./a/b", + "~/x", + "~o/r", + "o/~r", + "o/-r", + "o/r/extra", + "o", + "o//r", + "/o/r", + "o\\r", + "o/r\\x", + "o\\x/r", + "o/", + "/r", + "./o", + ]; + for source in sources { + let res = with_source(source, &fx.env.project, &runner, |_, _| Ok(())); + assert!(res.is_err(), "{source}"); + } + assert!(runner.calls().is_empty()); + } + + #[test] + fn owner_slash_repo_and_urls_are_cloned_unchanged() { + let fx = project_with(|_| {}); + let runner = FakeRunner::new(|_| {}); + let sources = ["o/r", "https://x.dev/o/r.git", "git@x.dev:o/r.git"]; + for source in sources { + with_source(source, &fx.env.project, &runner, |_, _| Ok(())).unwrap(); + } + let cloned: Vec = runner + .calls() + .iter() + .map(|(_, args, _)| args[4].clone()) + .collect(); + assert_eq!( + cloned, + [ + "https://github.com/o/r.git", + "https://x.dev/o/r.git", + "git@x.dev:o/r.git" + ] + ); + } + + #[test] + fn a_local_directory_at_a_deep_relative_path_is_used_in_place() { + let directory = tempfile::tempdir().unwrap(); + let deep = directory.path().join("a/b/c"); + std::fs::create_dir_all(&deep).unwrap(); + let runner = FakeRunner::new(|_| {}); + let mut used = None; + + with_source("a/b/c", directory.path(), &runner, |tree, _| { + used = Some(tree.to_path_buf()); + Ok(()) + }) + .unwrap(); + + assert_eq!(used, Some(deep.canonicalize().unwrap())); + assert!(runner.calls().is_empty()); + } + #[test] fn clone_failure_is_err_and_skips_install() { let fx = project_with(|_| {}); diff --git a/crates/skillz-harness/src/runner.rs b/crates/skillz-harness/src/runner.rs index 92ee20e..deee193 100644 --- a/crates/skillz-harness/src/runner.rs +++ b/crates/skillz-harness/src/runner.rs @@ -55,6 +55,10 @@ impl CommandRunner for NoCommands { } } +/// Transports git may use in every child skillz runs except a launched agent; blocks `ext::` and +/// other remote helpers whatever the user's `protocol.*.allow` says. +pub const GIT_ALLOW_PROTOCOL: (&str, &str) = ("GIT_ALLOW_PROTOCOL", "http:https:ssh:git:file"); + /// Spawns real processes in `cwd` with stdin and stdout inherited (npx and claude print /// prompts and errors to stdout) and stderr captured for the error message. `envs` are set on /// every child, so a config dir skillz was pointed at (`CLAUDE_CONFIG_DIR`, `CODEX_HOME`) @@ -73,6 +77,17 @@ impl SystemRunner { .current_dir(cwd); command } + + fn command_restricting_git_transports( + &self, + program: &str, + args: &[String], + cwd: &Path, + ) -> Command { + let mut command = self.command(program, args, cwd); + command.env(GIT_ALLOW_PROTOCOL.0, GIT_ALLOW_PROTOCOL.1); + command + } } impl CommandRunner for SystemRunner { @@ -95,7 +110,7 @@ impl CommandRunner for SystemRunner { fn run(&self, program: &str, args: &[String], cwd: &Path) -> anyhow::Result { let out = self - .command(program, args, cwd) + .command_restricting_git_transports(program, args, cwd) .stdin(Stdio::inherit()) .stdout(Stdio::inherit()) .output() @@ -108,7 +123,7 @@ impl CommandRunner for SystemRunner { fn run_quiet(&self, program: &str, args: &[String], cwd: &Path) -> anyhow::Result { let out = self - .command(program, args, cwd) + .command_restricting_git_transports(program, args, cwd) .stdin(Stdio::null()) .output() .with_context(|| format!("failed to run `{program}` in {}", cwd.display()))?; @@ -126,7 +141,7 @@ impl CommandRunner for SystemRunner { fn capture(&self, program: &str, args: &[String], cwd: &Path) -> anyhow::Result { let out = self - .command(program, args, cwd) + .command_restricting_git_transports(program, args, cwd) .stdin(Stdio::null()) .output() .with_context(|| format!("failed to run `{program}` in {}", cwd.display()))?; @@ -249,6 +264,73 @@ mod tests { let res = SystemRunner::default().run("skillz-definitely-not-a-program", &[], dir.path()); assert!(res.is_err()); } + + fn plain_and_redirected_runners() -> Vec> { + let redirected = SystemRunner::default() + .with_envs(vec![("SKILLZ_KEEP", "/keep".into())]) + .unwrap(); + vec![Box::new(SystemRunner::default()), redirected] + } + + fn sh_args(script: String) -> [String; 2] { + ["-c".into(), script] + } + + #[test] + fn captured_children_may_only_use_the_allowed_git_transports() { + let dir = tempfile::tempdir().unwrap(); + let args = sh_args("printf %s \"$GIT_ALLOW_PROTOCOL\"".into()); + + for runner in plain_and_redirected_runners() { + let seen = runner.capture("sh", &args, dir.path()).unwrap(); + assert_eq!(seen, "http:https:ssh:git:file"); + } + } + + #[test] + fn run_and_run_quiet_children_may_only_use_the_allowed_git_transports() { + let dir = tempfile::tempdir().unwrap(); + let args = sh_args(format!( + "test \"$GIT_ALLOW_PROTOCOL\" = {}", + GIT_ALLOW_PROTOCOL.1 + )); + + for runner in plain_and_redirected_runners() { + assert!(runner.run("sh", &args, dir.path()).unwrap().success); + assert!(runner.run_quiet("sh", &args, dir.path()).unwrap().success); + } + } + + #[test] + fn a_launched_agent_does_not_get_the_git_transport_restriction() { + let dir = tempfile::tempdir().unwrap(); + let args = sh_args(format!( + "test \"$GIT_ALLOW_PROTOCOL\" != {}", + GIT_ALLOW_PROTOCOL.1 + )); + + for runner in plain_and_redirected_runners() { + assert_eq!(runner.launch("sh", &args, dir.path()).unwrap(), 0); + } + } + + #[test] + fn the_transport_restriction_overrides_the_users_git_config() { + let dir = tempfile::tempdir().unwrap(); + let args = [ + "-c", + "protocol.allow=always", + "ls-remote", + "ext::sh -c touch% marker", + ] + .map(String::from); + + let out = SystemRunner::default().run_quiet("git", &args, dir.path()); + + assert!(!out.unwrap().success); + assert!(!dir.path().join("marker").exists()); + } + #[test] fn child_redirects_preserve_other_configured_environment_variables() { let directory = tempfile::tempdir().unwrap(); From fb1cced13362485a6f6f94be3be7287f6dcd1f40 Mon Sep 17 00:00:00 2001 From: archae0pteryx Date: Wed, 28 Oct 2026 07:26:57 -0700 Subject: [PATCH 2/2] Make the launch test independent of the parent GIT_ALLOW_PROTOCOL by passing a sentinel value through with_envs. --- crates/skillz-harness/src/runner.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/skillz-harness/src/runner.rs b/crates/skillz-harness/src/runner.rs index deee193..83fc339 100644 --- a/crates/skillz-harness/src/runner.rs +++ b/crates/skillz-harness/src/runner.rs @@ -302,16 +302,14 @@ mod tests { } #[test] - fn a_launched_agent_does_not_get_the_git_transport_restriction() { + fn a_launched_agent_keeps_its_own_git_allow_protocol() { let dir = tempfile::tempdir().unwrap(); - let args = sh_args(format!( - "test \"$GIT_ALLOW_PROTOCOL\" != {}", - GIT_ALLOW_PROTOCOL.1 - )); + let args = sh_args("test \"$GIT_ALLOW_PROTOCOL\" = sentinel".into()); + let runner = SystemRunner::default() + .with_envs(vec![(GIT_ALLOW_PROTOCOL.0, "sentinel".into())]) + .unwrap(); - for runner in plain_and_redirected_runners() { - assert_eq!(runner.launch("sh", &args, dir.path()).unwrap(), 0); - } + assert_eq!(runner.launch("sh", &args, dir.path()).unwrap(), 0); } #[test]