diff --git a/Cargo.lock b/Cargo.lock index 21467a5..130e283 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2554,7 +2554,7 @@ dependencies = [ [[package]] name = "kit" -version = "0.1.119" +version = "0.1.120" dependencies = [ "a2a-protocol-client", "a2a-protocol-server", diff --git a/Cargo.toml b/Cargo.toml index 60387c1..0651894 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "kit" -version = "0.1.119" +version = "0.1.120" edition = "2024" rust-version = "1.94.0" publish = false diff --git a/Dockerfile b/Dockerfile index 3d9cf78..874ecf9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -68,7 +68,7 @@ LABEL org.opencontainers.image.title="Kit" \ org.opencontainers.image.revision="${REVISION}" RUN apt-get update \ - && apt-get install -y --no-install-recommends ca-certificates \ + && apt-get install -y --no-install-recommends ca-certificates git \ && rm -rf /var/lib/apt/lists/* \ && groupadd --gid 1000 kit \ && useradd --uid 1000 --gid 1000 --create-home --no-log-init --shell /bin/sh kit \ @@ -99,7 +99,7 @@ LABEL org.opencontainers.image.title="Kit" \ org.opencontainers.image.revision="${REVISION}" RUN apt-get update \ - && apt-get install -y --no-install-recommends ca-certificates \ + && apt-get install -y --no-install-recommends ca-certificates git \ && rm -rf /var/lib/apt/lists/* \ && groupadd --gid 1000 kit \ && useradd --uid 1000 --gid 1000 --create-home --no-log-init --shell /bin/sh kit \ @@ -131,7 +131,7 @@ LABEL org.opencontainers.image.title="Kit" \ org.opencontainers.image.version="${VERSION}" \ org.opencontainers.image.revision="${REVISION}" -RUN apk add --no-cache ca-certificates \ +RUN apk add --no-cache ca-certificates git \ && addgroup -S -g 1000 kit \ && adduser -S -D -u 1000 -G kit -h /home/kit -s /bin/sh kit \ && mkdir -p /workspace \ diff --git a/README.md b/README.md index 7e4a4ac..56bfa93 100644 --- a/README.md +++ b/README.md @@ -192,7 +192,7 @@ See [MCP](docs/user/mcp.md). ### Agent Plugins and Agent Skills -Kit loads validated [Agent Plugin](docs/user/agent-plugins.md) packages from a local directory or a SHA-256-pinned archive. Kit supports ZIP, tar.gz, tar, and GitHub tag archives. +Kit loads validated [Agent Plugin](docs/user/agent-plugins.md) packages from a local directory, a SHA-256-pinned archive, or an HTTPS Git repository pinned to a full commit or validated tag. Kit supports ZIP, tar.gz, tar, and GitHub tag archives. Plugin skills become available in the `skill` catalog. Kit starts plugin `stdio` and `streamable-http` MCP servers without an `mcp.json` file. @@ -208,6 +208,12 @@ source = "archive" url = "https://github.com/speakeasy-api/gram-plugin/archive/refs/tags/v1.2.0.tar.gz" sha256 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" subdir = "plugin" + +[plugins.git-review] +source = "git" +url = "https://plugins.example.com/review.git" +rev = "main" +subdir = "agent-plugin" ``` diff --git a/docs/user/agent-plugins.md b/docs/user/agent-plugins.md index 89aa011..7e0c42c 100644 --- a/docs/user/agent-plugins.md +++ b/docs/user/agent-plugins.md @@ -1,6 +1,6 @@ # Agent Plugins -Kit can load Agent Plugin packages from a local directory or a checksum-pinned online archive. Source resolution happens at startup. Kit uses `agentkit-plugins` to validate the resolved package, exposes its valid Agent Skills through the existing `skill` tool, and registers its supported MCP servers. A plugin-only configuration works without `--mcp-config` or `mcp_config`. +Kit can load Agent Plugin packages from a local directory, a checksum-pinned online archive, or a Git repository. Source resolution happens at startup. Kit uses `agentkit-plugins` to validate the resolved package, exposes its valid Agent Skills through the existing `skill` tool, and registers its supported MCP servers. A plugin-only configuration works without `--mcp-config` or `mcp_config`. ## Configure a source @@ -16,16 +16,26 @@ source = "archive" url = "https://github.com/owner/repo/archive/refs/tags/v1.2.0.tar.gz" sha256 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" subdir = "optional/plugin/path" + +[plugins.git-plugin] +source = "git" +url = "https://plugins.example.com/marketplace/opaque-id.git" +rev = "main" +subdir = "agent-plugins/example" ``` Aliases must contain 1–64 lowercase ASCII letters, digits, or single hyphens, with an alphanumeric first and last character. Duplicate plugin manifest names are an error. A relative `path` is resolved against Kit's working directory, not the configuration directory. An absolute path is used directly. Local packages are validated on every startup and remain mutable local content. -For an `archive`, provide the final archive URL. Kit does not clone Git repositories, translate forge URLs, select branches or tags, or automatically update plugins. `sha256` is mandatory and identifies the exact downloaded bytes. HTTPS is required, except that explicit loopback HTTP URLs are accepted for local testing. Redirects from HTTPS must remain HTTPS. URL credentials and fragments are rejected. +For an `archive`, provide the final archive URL. Kit does not translate forge URLs or automatically update archive sources. `sha256` is mandatory and identifies the exact downloaded bytes. HTTPS is required, except that explicit loopback HTTP URLs are accepted for local testing. Redirects from HTTPS must remain HTTPS. URL credentials and fragments are rejected. Kit recognizes ZIP, gzip-compressed tar, and plain tar by content. Archives may contain `plugin.json` at the extraction root or one top-level directory, as forge-generated archives commonly do. `subdir`, when present, is applied below that selected base. Archive paths must be contained relative paths; links, special files, duplicate normalized paths, and extraction-limit violations are rejected. Executable mode bits are not preserved in this release. +For `git`, `url` is required, while `rev` and `subdir` are optional. When `rev` is omitted, Kit fetches the remote's `HEAD`, which selects its default branch. `rev` can be an exact 40-hex SHA-1 commit ID or a safe Git ref name such as `main`, `v1.2.0`, `refs/heads/main`, or `refs/tags/v1.2.0`. Git options, refspecs, revision expressions, control characters, and malformed ref names are rejected. Kit fetches the selected name into a private Kit-controlled ref, resolves it to a full commit ID, and uses that immutable ID for archive validation and the cache. A full commit is the reproducible choice; omitted or named revisions are fetched again on every startup and can select a new commit when the remote ref moves. The URL must be an absolute HTTPS URL without user information, a query, or a fragment. Local, SCP-like, SSH, `git`, file, and external-helper transports are rejected. + +Kit invokes the installed `git` executable without a shell. It preserves normal system and user Git configuration so configured noninteractive HTTPS credential helpers can authenticate private repositories, but it disables Git terminal and configured askpass prompts, sets standard GUI credential-helper controls to noninteractive, and rejects credentials in the configured URL. Before network access, Kit verifies that `url.*.insteadOf` configuration did not rewrite the validated origin. System and uncommitted attribute files are disabled with controlled empty files; committed `.gitattributes` remains effective. Git diagnostics and configured URLs are not included in errors or written to temporary files. The selected portable `subdir` is archived with literal path semantics. Kit does not check out a worktree or run repository hooks, filters, Git LFS, or submodules; symlinks and submodules in the selected tree are rejected. + ## Plugin MCP servers Kit supports the Agent Plugin `stdio` and `streamable-http` transports. The deprecated `sse` transport is not supported: Kit skips each SSE server and writes a diagnostic containing the plugin alias and server name to stderr. Other valid servers in that plugin remain available. @@ -54,7 +64,15 @@ Archive content is downloaded, checked against `sha256`, validated, and extracte ~/.kit/plugin-cache/ ``` -The cache is local state protected by the permissions of `~/.kit`; it is not a sandbox or a publisher-identity check. Remove a damaged cache entry to force a verified download. The configured checksum proves archive-byte integrity, not who published those bytes. +Git packages are fetched into an isolated bare staging repository, validated, and atomically published under: + +```text +~/.kit/plugin-cache/git-v1//-/repo +``` + +A validated full-commit cache entry can be reused without network access. Tags are still resolved remotely on every startup before a matching resolved-commit cache entry is reused. Concurrent publishers can duplicate fetch work, but publication remains atomic: losers validate the completed winner and attempt to remove their own staging directories. Kit also removes only exactly named staging directories under the relevant cache key when they are at least 24 hours old; it does not age out published cache entries. Git commands have a 120-second timeout, hard-bounded pipe output, backoff-based live object-store checks, and a final 256 MiB object-store validation. Git archives stream directly into bounded hardened tar extraction. + +The cache is local state protected by the permissions of `~/.kit`; it is not a sandbox or a publisher-identity check. Remove a damaged cache entry to force a verified download. The configured archive checksum proves archive-byte integrity, while a Git commit identifies repository content; neither proves who published it. Resolution or package-validation failures stop startup. Non-fatal package diagnostics are written to stderr with the plugin alias. Supported validated MCP declarations are registered and begin connecting in the background at startup; unsupported SSE declarations produce the skip diagnostic described above. diff --git a/docs/user/getting-started-and-configuration.md b/docs/user/getting-started-and-configuration.md index d543dae..f0915b9 100644 --- a/docs/user/getting-started-and-configuration.md +++ b/docs/user/getting-started-and-configuration.md @@ -189,6 +189,12 @@ path = "./plugins/local-plugin" source = "archive" url = "https://example.com/plugin.tar.gz" sha256 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + +[plugins.git-plugin] +source = "git" +url = "https://plugins.example.com/repository.git" +rev = "main" +subdir = "agent-plugins/example" ``` `root`, `provider`, `model`, and credential settings apply to all four runtime commands. Subagent model aliases and explicit-override allowlists are scoped by fully qualified harness under `[subagent.harnesses."acp.name"]`. Omitting `allow_model_overrides` permits all explicit model selections accepted by that harness; an empty list disables explicit model overrides. This policy does not restrict the harness's inherited or default model. @@ -251,7 +257,7 @@ If startup reports `could not load AGENTS.md context`, inspect the `AGENTS.md` f ## Agent Skills -Kit discovers [Agent Skills](https://agentskills.io/) recursively from `/.agents/skills` and `~/.agents/skills`. Validated [Agent Plugins](agent-plugins.md) can add exact skill directories and supported MCP servers from local packages or checksum-pinned archives. Collision precedence for skills is project skills, user skills, then plugins in lexical alias order. Project skills therefore override user and plugin skills with the same name. Each skill lives in a directory containing `SKILL.md`; its frontmatter must include a `name` using lowercase letters, digits, and hyphens that matches the directory name and a non-empty `description`. +Kit discovers [Agent Skills](https://agentskills.io/) recursively from `/.agents/skills` and `~/.agents/skills`. Validated [Agent Plugins](agent-plugins.md) can add exact skill directories and supported MCP servers from local packages, checksum-pinned archives, or HTTPS Git sources pinned to a full commit or validated tag. Collision precedence for skills is project skills, user skills, then plugins in lexical alias order. Project skills therefore override user and plugin skills with the same name. Each skill lives in a directory containing `SKILL.md`; its frontmatter must include a `name` using lowercase letters, digits, and hyphens that matches the directory name and a non-empty `description`. ```markdown --- diff --git a/docs/user/security-limits-and-troubleshooting.md b/docs/user/security-limits-and-troubleshooting.md index ad81f89..f9f23c0 100644 --- a/docs/user/security-limits-and-troubleshooting.md +++ b/docs/user/security-limits-and-troubleshooting.md @@ -63,6 +63,8 @@ Persisted session transcripts are append-only JSONL records under `~/.kit/sessio Kit can expose credentials through the authority of its process even when it does not print them. Shell commands and ACP harnesses normally inherit the process environment. Local MCP servers, including plugin stdio servers, are executable code. Plugin stdio servers receive absolute `PLUGIN_ROOT` and `PLUGIN_DATA` paths and can read or modify anything allowed by the Kit process. Remote MCP tools receive invoked arguments, and explicit MCP configuration may contain a plain `bearerToken`. Keep config files and environment variables private, use narrowly scoped accounts, and do not ask the model to echo tokens for diagnosis. +Git plugin sources use the installed system Git and its normal credential-helper configuration for private HTTPS repositories. Inline URL credentials, URL queries, and URL fragments are rejected. Kit disables Git terminal and configured askpass prompts, sets standard GUI credential-helper controls to noninteractive, and disables inherited `GIT_*` shaping and trace settings, hooks, templates, redirects, non-HTTPS transports, uncommitted or system attribute files, submodules, and maintenance for source resolution. It rejects configured URL rewrites before network access and does not expose Git stderr or the configured URL in errors or temporary files. Committed `.gitattributes` remains effective. Credential helpers still run with Kit's process authority and custom helpers can ignore noninteractive conventions, so configure only trusted helpers and narrowly scoped credentials. Git package validation does not sandbox code that the validated plugin later declares as an MCP server. + OpenAI and MCP use one credential backend selected with `--credential-store` or `credential_store`. The default is `memory`. `kit auth login openai` uses PKCE, state, and nonce on fixed loopback callback ports 1455 and 1457 and verifies RS256 tokens against OpenAI's pinned JWKS endpoint. Standalone OpenAI login rejects `memory`; select persistent `keychain` or `file` storage and use the same selection for runtime commands. Token values are redacted from diagnostics and zeroized where practical. The synchronization lock file contains no credentials. `kit auth logout openai` revokes before deletion and retains the credential when revocation fails; `--local-only` deliberately skips revocation and should be used only when remote revocation cannot be completed. The shared backend choices are: @@ -85,7 +87,9 @@ Use `kit tui --help` for credential-store options. If nested Kit agents need the The following are fixed runtime limits, not configurable policy controls: -- Shell timeout: 120 seconds by default; accepted values are 1 through 3600 seconds. Timeout reports `shell command timed out`. Shell stdout and stderr remain complete inside compose and fail if either stream exceeds the 64 MiB internal safety limit. Final compose results from 8 KiB through the 64 MiB result limit spill at the model-context boundary, which receives a bounded head-and-tail preview and artifact path. +- Shell timeout: 120 seconds by default; accepted values are 1 through 3600 seconds. Timeout reports `shell command timed out`. Shell stdout and stderr remain complete inside compose and fail if either stream exceeds the 64 MiB internal safety limit. +- Shell and Git timeout or output-limit cleanup targets the spawned process tree. On Unix, Kit starts the direct child in a separate process group and terminates that group; a descendant that deliberately creates a new session or process group can escape this cleanup. On Windows, Kit makes a best-effort `taskkill /PID /T /F` request, which is not a guarantee that every descendant stops. On other platforms, only direct-child termination is available. Always inspect for partial side effects after interruption or failure. +- Git plugin source commands have a fixed 120-second per-command timeout and hard-bounded stdout and stderr pipes. Fetches use backoff-based live object-store checks and a final 256 MiB validation. Archive output streams directly into extraction; selected content also uses the archive entry, per-file, and expanded-size limits. Final compose results from 8 KiB through the 64 MiB result limit spill at the model-context boundary, which receives a bounded head-and-tail preview and artifact path. - Subagents: nesting depth is two and at most 120 live subagent sessions are retained per main session. Errors include `subagent depth limit (2) reached` and `live subagent session limit (120) reached`. Reuse completed sessions or release unneeded ones with `close` instead of creating unbounded children. - ACP children: startup handshake and `session/fork` waits are 30 seconds. Cancellation allows 5 seconds to settle before Kit tears down the child. Captured ACP updates are limited to 64 updates and 64 KiB; the returned `updates.truncated` flag reports loss. - MCP: background server initialization uses a 20-second connection timeout. Tool calls have a 60-second deadline by default; `timeout_seconds` can override it with a value from 1 through 3600 for a call expected to take longer. OAuth authorization expires after 10 minutes. `tool_search` returns at most 5 tools globally across all servers and caps the serialized response at 32 KiB; search with a configured server name, specific product term, or tool keywords. Use the exact query `mcp` (case-insensitive) for a compact configured-server status list; `total_servers`, `returned_servers`, and `truncated` report any tail entries omitted by the same cap. @@ -98,7 +102,7 @@ Provider context windows, model token limits, child-agent turn limits, remote ra ### Kit does not start or the TUI exits before opening a session -1. Run `kit --version` and `kit --help` to verify the installed binary and command syntax. +1. Run `kit --version` and `kit --help` to verify the installed binary and command syntax. For a Git plugin error, confirm that `git` is installed, the HTTPS credential helper can authenticate noninteractively, and the configured commit or ref exists (or that the remote has a default branch when `rev` is omitted); Kit intentionally omits remote Git diagnostics because they can contain secrets. 2. If the diagnostic says `could not open working directory` or `working directory is not a directory`, verify that `--root` exists, is a directory, and is accessible to the Kit process. 3. If A2A binding fails or the port is taken, omit the fixed address to get an available loopback port, choose another loopback port, or use `kit acp` when HTTP is unnecessary. 4. If the failure mentions OpenAI subscription credentials, run status and login with the same persistent `--credential-store keychain` or `--credential-store file --credential-dir ...` used by the runtime; standalone login rejects `memory`. Ensure loopback port 1455 or 1457 is available. Retry without pasting secrets into the prompt. diff --git a/macos/Config/Version.xcconfig b/macos/Config/Version.xcconfig index 06af2c6..5049afe 100644 --- a/macos/Config/Version.xcconfig +++ b/macos/Config/Version.xcconfig @@ -1,2 +1,2 @@ // Generated by scripts/generate-macos-project.sh from Cargo.toml. -KIT_VERSION = 0.1.119 +KIT_VERSION = 0.1.120 diff --git a/src/lib.rs b/src/lib.rs index 61511f3..eb58bcd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,6 +7,7 @@ pub mod docs; pub mod events; mod fatal; pub mod plugins; +pub(crate) mod process_tree; pub mod protocols; pub mod provider; pub mod runtime; diff --git a/src/main.rs b/src/main.rs index babbffe..c245c6b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1485,11 +1485,16 @@ source = "archive" url = "https://example.com/plugin.tar.gz" sha256 = "{}" subdir = "packages/plugin" + +[plugins.git-plugin] +source = "git" +url = "https://example.com/repo" +future_option = true "#, "ab".repeat(32) )) .unwrap(); - assert_eq!(configured.plugins.len(), 2); + assert_eq!(configured.plugins.len(), 3); assert!(matches!( configured.plugins["local-plugin"], kit::plugins::PluginConfig::Path { .. } @@ -1498,10 +1503,14 @@ subdir = "packages/plugin" configured.plugins["remote-plugin"], kit::plugins::PluginConfig::Archive { .. } )); + assert!(matches!( + configured.plugins["git-plugin"], + kit::plugins::PluginConfig::Git { rev: None, .. } + )); assert!(Config::default().plugins.is_empty()); for invalid in [ - "[plugins.bad]\nsource = 'git'\nurl = 'https://example.com/repo'", + "[plugins.bad]\nsource = 'git'\nrev = 'main'", "[plugins.bad]\nsource = 'archive'\nurl = 'https://example.com/plugin.zip'", ] { assert!(toml::from_str::(invalid).is_err(), "{invalid}"); diff --git a/src/plugins.rs b/src/plugins.rs index b4ad990..1ce1dd9 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -1,9 +1,14 @@ use std::{ collections::{BTreeMap, BTreeSet}, - fs::{self, OpenOptions}, + env, + ffi::{OsStr, OsString}, + fs::{self, File, OpenOptions}, io::{self, Cursor, Read, Write}, path::{Component, Path, PathBuf}, - time::Duration, + process::{Command, Stdio}, + sync::mpsc, + thread, + time::{Duration, Instant, SystemTime}, }; use agentkit_plugins::{AgentPlugin, PluginMcpServer}; @@ -12,11 +17,26 @@ use serde::Deserialize; use sha2::{Digest, Sha256}; use url::{Host, Url}; +use crate::process_tree::{isolate_process_tree, terminate_process_tree_with_pid}; + +#[cfg(unix)] +use std::os::unix::fs::DirBuilderExt; + const MAX_DOWNLOAD_BYTES: u64 = 64 * 1024 * 1024; const MAX_ARCHIVE_ENTRIES: usize = 10_000; const MAX_FILE_BYTES: u64 = 64 * 1024 * 1024; const MAX_EXPANDED_BYTES: u64 = 256 * 1024 * 1024; const MAX_TAR_STREAM_BYTES: u64 = MAX_EXPANDED_BYTES + 16 * 1024 * 1024; +const MAX_GIT_TREE_BYTES: u64 = 16 * 1024 * 1024; +const MAX_GIT_METADATA_BYTES: u64 = 16 * 1024 * 1024; +const MAX_GIT_DIAGNOSTIC_BYTES: u64 = 64 * 1024; +const MAX_GIT_OBJECT_ENTRIES: usize = MAX_ARCHIVE_ENTRIES * 10; +const GIT_COMMAND_TIMEOUT: Duration = Duration::from_secs(120); +const GIT_OBJECT_SCAN_INITIAL_DELAY: Duration = Duration::from_millis(100); +const GIT_OBJECT_SCAN_MAX_DELAY: Duration = Duration::from_secs(1); +const STALE_STAGING_AGE: Duration = Duration::from_secs(24 * 60 * 60); +const GIT_CACHE_VERSION: &str = "git-v1"; +const GIT_PRIVATE_FETCH_REF: &str = "refs/kit/plugin-source"; struct LimitedReader { inner: R, @@ -31,7 +51,9 @@ impl Read for LimitedReader { "plugin tar stream exceeds expanded size limit", )); } - let limit = usize::try_from(self.remaining.min(buffer.len() as u64)).unwrap(); + let limit = usize::try_from(self.remaining.min(buffer.len() as u64)).map_err(|_| { + io::Error::new(io::ErrorKind::InvalidData, "plugin read limit is too large") + })?; let read = self.inner.read(&mut buffer[..limit])?; self.remaining -= read as u64; Ok(read) @@ -49,6 +71,11 @@ pub enum PluginConfig { sha256: String, subdir: Option, }, + Git { + url: String, + rev: Option, + subdir: Option, + }, } #[derive(Clone, Debug)] @@ -101,6 +128,9 @@ fn resolve_blocking( sha256, subdir, } => resolve_archive(url, sha256, subdir.as_deref(), cache_root)?, + PluginConfig::Git { url, rev, subdir } => { + resolve_git(url, rev.as_deref(), subdir.as_deref(), cache_root)? + } }; let plugin = AgentPlugin::load(&root).map_err(|error| { format!( @@ -186,254 +216,1464 @@ fn resolve_path(path: &Path, runtime_root: &Path) -> Result { .map_err(|error| format!("could not resolve plugin path {}: {error}", path.display())) } -fn resolve_archive( - value: &str, - expected_digest: &str, - subdir: Option<&Path>, - cache_root: &Path, -) -> Result { - let url = Url::parse(value).map_err(|error| format!("invalid plugin archive URL: {error}"))?; - validate_download_url(&url)?; - let expected = parse_sha256(expected_digest)?; - let subdir = subdir.map(validate_relative_path).transpose()?; - let digest = expected_digest.to_ascii_lowercase(); - fs::create_dir_all(cache_root).map_err(|error| { - format!( - "could not create plugin cache {}: {error}", - cache_root.display() - ) - })?; - let destination = cache_root.join(&digest); - match fs::symlink_metadata(&destination) { - Ok(metadata) if !metadata.file_type().is_dir() => { - return Err(format!( - "plugin cache entry is not a directory: {}", - destination.display() - )); - } - Ok(_) => {} - Err(error) if error.kind() == io::ErrorKind::NotFound => {} - Err(error) => return Err(format!("could not inspect plugin cache entry: {error}")), - } - if !destination.exists() { - let mut random = [0u8; 8]; - getrandom::fill(&mut random).map_err(|error| error.to_string())?; - let suffix = random - .iter() - .map(|byte| format!("{byte:02x}")) - .collect::(); - let staging = cache_root.join(format!(".{digest}.staging-{suffix}")); - fs::create_dir(&staging) - .map_err(|error| format!("could not create plugin staging directory: {error}"))?; - let result = (|| { - let bytes = download(&url)?; - let actual = Sha256::digest(&bytes); - if actual.as_slice() != expected { - return Err(format!("SHA-256 mismatch for plugin archive {url}")); - } - extract_archive(&bytes, &staging)?; - let candidate = select_package_root(&staging, subdir.as_deref())?; - AgentPlugin::load(&candidate).map_err(|error| { - format!( - "invalid plugin archive package at {}: {error}", - candidate.display() - ) - })?; - match fs::rename(&staging, &destination) { - Ok(()) => Ok(()), - Err(_error) if destination.is_dir() => Ok(()), - Err(error) => Err(format!("could not publish plugin archive: {error}")), - } - })(); - if staging.exists() { - let _ = fs::remove_dir_all(&staging); +#[derive(Clone, Debug, PartialEq, Eq)] +enum GitRevision { + Commit(String), + Ref(String), + DefaultBranch, +} + +#[derive(Clone, Copy)] +enum GitProtocol { + Https, + #[cfg(test)] + Local, +} + +#[derive(Debug, PartialEq, Eq)] +enum GitFailure { + Unavailable(io::ErrorKind), + Timeout, + OutputLimit, + ObjectStoreLimit, + ObjectStoreInspection(io::ErrorKind), + Archive(String), + Exit(Option), +} + +struct GitRunRequest<'a> { + cwd: &'a Path, + args: &'a [OsString], + config: &'a [(OsString, OsString)], + stdout_limit: u64, + stderr_limit: u64, + object_store_limit: Option<&'a Path>, +} + +trait GitRunner: Sync { + fn run(&self, request: GitRunRequest<'_>) -> Result, GitFailure>; + + fn archive(&self, request: GitRunRequest<'_>, destination: &Path) -> Result<(), GitFailure>; +} + +struct SystemGitRunner { + timeout: Duration, +} + +impl Default for SystemGitRunner { + fn default() -> Self { + Self { + timeout: GIT_COMMAND_TIMEOUT, } - result?; } - select_package_root(&destination, subdir.as_deref()) } -fn validate_download_url(url: &Url) -> Result<(), String> { - if !url.username().is_empty() || url.password().is_some() || url.fragment().is_some() { - return Err("plugin archive URL must not contain credentials or a fragment".into()); - } - let loopback = match url.host() { - Some(Host::Domain(host)) => host == "localhost", - Some(Host::Ipv4(address)) => address.is_loopback(), - Some(Host::Ipv6(address)) => address.is_loopback(), - None => false, - }; - if url.scheme() != "https" && !(url.scheme() == "http" && loopback) { - return Err("plugin archive URL must use HTTPS (or loopback HTTP)".into()); - } - Ok(()) +enum GitStdout { + Capture(u64), + Archive { destination: PathBuf, limit: u64 }, } -fn parse_sha256(value: &str) -> Result<[u8; 32], String> { - if value.len() != 64 || !value.bytes().all(|byte| byte.is_ascii_hexdigit()) { - return Err("plugin archive sha256 must contain exactly 64 hexadecimal characters".into()); - } - let mut digest = [0u8; 32]; - for (index, pair) in value.as_bytes().chunks_exact(2).enumerate() { - let text = std::str::from_utf8(pair).expect("hexadecimal is UTF-8"); - digest[index] = u8::from_str_radix(text, 16).expect("hexadecimal pair parses"); - } - Ok(digest) +enum GitPipeEvent { + Stdout(Result, GitFailure>), + Stderr(Result<(), GitFailure>), } -fn download(url: &Url) -> Result, String> { - let allow_loopback_http = url.scheme() == "http"; - let client = reqwest::blocking::Client::builder() - .connect_timeout(Duration::from_secs(20)) - .timeout(Duration::from_secs(120)) - .redirect(reqwest::redirect::Policy::custom(move |attempt| { - if attempt.previous().len() >= 10 { - return attempt.error("too many plugin archive redirects"); +impl SystemGitRunner { + fn execute( + &self, + request: GitRunRequest<'_>, + stdout_mode: GitStdout, + ) -> Result, GitFailure> { + let mut command = Command::new("git"); + command + .args(request.args) + .current_dir(request.cwd) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + for (name, _) in env::vars_os() { + if name + .to_string_lossy() + .to_ascii_uppercase() + .starts_with("GIT_") + { + command.env_remove(name); } - let target = attempt.url(); - if validate_download_url(target).is_ok() - && (target.scheme() == "https" || allow_loopback_http) + } + command.env("GIT_CONFIG_COUNT", request.config.len().to_string()); + for (index, (key, value)) in request.config.iter().enumerate() { + command + .env(format!("GIT_CONFIG_KEY_{index}"), key) + .env(format!("GIT_CONFIG_VALUE_{index}"), value); + } + command + .env_remove("SSH_ASKPASS") + .env("GIT_TERMINAL_PROMPT", "0") + .env("GCM_INTERACTIVE", "Never") + .env("GIT_LITERAL_PATHSPECS", "1") + .env("GIT_ATTR_NOSYSTEM", "1") + .env("LC_ALL", "C"); + isolate_process_tree(&mut command); + + let mut child = command + .spawn() + .map_err(|error| GitFailure::Unavailable(error.kind()))?; + let child_pid = child.id(); + let stdout = child.stdout.take().ok_or_else(|| { + terminate_process_tree_with_pid(&mut child, child_pid); + GitFailure::Unavailable(io::ErrorKind::BrokenPipe) + })?; + let stderr = child.stderr.take().ok_or_else(|| { + terminate_process_tree_with_pid(&mut child, child_pid); + GitFailure::Unavailable(io::ErrorKind::BrokenPipe) + })?; + let (sender, receiver) = mpsc::channel(); + let stdout_sender = sender.clone(); + let stdout_thread = thread::spawn(move || { + let result = match stdout_mode { + GitStdout::Capture(limit) => read_bounded(stdout, limit), + GitStdout::Archive { destination, limit } => { + let result = extract_tar( + LimitedReader { + inner: stdout, + remaining: limit, + }, + &destination, + ); + match result { + Ok(()) => Ok(Vec::new()), + Err(error) + if error.contains("plugin tar stream exceeds expanded size limit") => + { + Err(GitFailure::OutputLimit) + } + Err(error) => Err(GitFailure::Archive(error)), + } + } + }; + let _ = stdout_sender.send(GitPipeEvent::Stdout(result)); + }); + let stderr_limit = request.stderr_limit; + let stderr_thread = thread::spawn(move || { + let result = drain_bounded(stderr, stderr_limit); + let _ = sender.send(GitPipeEvent::Stderr(result)); + }); + + let started = Instant::now(); + let mut next_object_scan = started + GIT_OBJECT_SCAN_INITIAL_DELAY; + let mut object_scan_delay = GIT_OBJECT_SCAN_INITIAL_DELAY; + let mut process_done = false; + let mut status = None; + let mut stdout_result = None; + let mut stdout_done = false; + let mut stderr_done = false; + let mut failure = None; + let mut tree_terminated = false; + + while !process_done || !stdout_done || !stderr_done { + if !stdout_done || !stderr_done { + match receiver.recv_timeout(Duration::from_millis(10)) { + Ok(GitPipeEvent::Stdout(result)) => { + stdout_done = true; + match result { + Ok(bytes) => stdout_result = Some(bytes), + Err(error) => { + failure.get_or_insert(error); + terminate_process_tree_with_pid(&mut child, child_pid); + tree_terminated = true; + process_done = true; + } + } + } + Ok(GitPipeEvent::Stderr(result)) => { + stderr_done = true; + if let Err(error) = result { + failure.get_or_insert(error); + terminate_process_tree_with_pid(&mut child, child_pid); + tree_terminated = true; + process_done = true; + } + } + Err(mpsc::RecvTimeoutError::Timeout) => {} + Err(mpsc::RecvTimeoutError::Disconnected) => { + failure.get_or_insert(GitFailure::Unavailable(io::ErrorKind::BrokenPipe)); + break; + } + } + } + + let now = Instant::now(); + if now.duration_since(started) >= self.timeout { + failure.get_or_insert(GitFailure::Timeout); + terminate_process_tree_with_pid(&mut child, child_pid); + tree_terminated = true; + break; + } + + if failure.is_some() { + if !tree_terminated { + terminate_process_tree_with_pid(&mut child, child_pid); + tree_terminated = true; + } + process_done = true; + continue; + } + if process_done { + continue; + } + + if let Some(path) = request.object_store_limit + && now >= next_object_scan { - attempt.follow() - } else { - attempt.error("plugin archive redirect is not secure") + match inspect_git_object_store(path) { + Ok(()) => {} + Err(error) => { + failure = Some(error); + terminate_process_tree_with_pid(&mut child, child_pid); + tree_terminated = true; + process_done = true; + continue; + } + } + object_scan_delay = (object_scan_delay * 2).min(GIT_OBJECT_SCAN_MAX_DELAY); + next_object_scan = now + object_scan_delay; } - })) - .build() - .map_err(|error| format!("could not create plugin HTTP client: {error}"))?; - let response = client - .get(url.clone()) - .send() - .and_then(reqwest::blocking::Response::error_for_status) - .map_err(|error| format!("could not download plugin archive {url}: {error}"))?; - if response - .content_length() - .is_some_and(|length| length > MAX_DOWNLOAD_BYTES) - { - return Err("plugin archive exceeds the 64 MiB download limit".into()); - } - let mut bytes = Vec::new(); - response - .take(MAX_DOWNLOAD_BYTES + 1) - .read_to_end(&mut bytes) - .map_err(|error| format!("could not read plugin archive: {error}"))?; - if bytes.len() as u64 > MAX_DOWNLOAD_BYTES { - return Err("plugin archive exceeds the 64 MiB download limit".into()); + + match child.try_wait() { + Ok(Some(value)) => { + status = Some(value); + // A successful Git parent may leave helpers holding the pipes. + terminate_process_tree_with_pid(&mut child, child_pid); + tree_terminated = true; + process_done = true; + } + Ok(None) => {} + Err(error) => { + failure = Some(GitFailure::Unavailable(error.kind())); + terminate_process_tree_with_pid(&mut child, child_pid); + tree_terminated = true; + process_done = true; + } + } + } + + if failure.is_some() && !tree_terminated { + terminate_process_tree_with_pid(&mut child, child_pid); + } + let stdout_joined = stdout_thread.join().is_ok(); + let stderr_joined = stderr_thread.join().is_ok(); + if !stdout_joined || !stderr_joined { + failure.get_or_insert(GitFailure::Timeout); + } + if let Some(path) = request.object_store_limit + && failure.is_none() + && let Err(error) = inspect_git_object_store(path) + { + failure = Some(error); + } + if let Some(error) = failure { + return Err(error); + } + let status = status.ok_or(GitFailure::Unavailable(io::ErrorKind::BrokenPipe))?; + if !status.success() { + return Err(GitFailure::Exit(status.code())); + } + stdout_result.ok_or(GitFailure::Unavailable(io::ErrorKind::BrokenPipe)) } - Ok(bytes) } -fn extract_archive(bytes: &[u8], destination: &Path) -> Result<(), String> { - if bytes.starts_with(b"PK\x03\x04") { - extract_zip(bytes, destination) - } else if bytes.starts_with(&[0x1f, 0x8b]) { - extract_tar( - LimitedReader { - inner: GzDecoder::new(Cursor::new(bytes)), - remaining: MAX_TAR_STREAM_BYTES, - }, - destination, - ) - } else { - extract_tar( - LimitedReader { - inner: Cursor::new(bytes), - remaining: MAX_TAR_STREAM_BYTES, +impl GitRunner for SystemGitRunner { + fn run(&self, request: GitRunRequest<'_>) -> Result, GitFailure> { + let limit = request.stdout_limit; + self.execute(request, GitStdout::Capture(limit)) + } + + fn archive(&self, request: GitRunRequest<'_>, destination: &Path) -> Result<(), GitFailure> { + let limit = request.stdout_limit; + self.execute( + request, + GitStdout::Archive { + destination: destination.to_path_buf(), + limit, }, - destination, ) + .map(|_| ()) } } -fn extract_zip(bytes: &[u8], destination: &Path) -> Result<(), String> { - let mut archive = zip::ZipArchive::new(Cursor::new(bytes)) - .map_err(|error| format!("invalid ZIP plugin archive: {error}"))?; - if archive.len() > MAX_ARCHIVE_ENTRIES { - return Err("plugin archive contains too many entries".into()); +fn read_bounded(mut reader: impl Read, limit: u64) -> Result, GitFailure> { + let mut bytes = Vec::new(); + reader + .by_ref() + .take(limit.saturating_add(1)) + .read_to_end(&mut bytes) + .map_err(|error| GitFailure::Unavailable(error.kind()))?; + if bytes.len() as u64 > limit { + return Err(GitFailure::OutputLimit); } - let mut paths = BTreeSet::new(); - let mut expanded = 0u64; - for index in 0..archive.len() { - let mut entry = archive.by_index(index).map_err(|error| error.to_string())?; - let path = validate_archive_path(Path::new(entry.name()))?; - if !paths.insert(path.clone()) { - return Err(format!("duplicate plugin archive path {}", path.display())); + Ok(bytes) +} + +fn drain_bounded(mut reader: impl Read, limit: u64) -> Result<(), GitFailure> { + let mut buffer = [0u8; 8192]; + let mut total = 0u64; + loop { + let read = reader + .read(&mut buffer) + .map_err(|error| GitFailure::Unavailable(error.kind()))?; + if read == 0 { + return Ok(()); } - let output = destination.join(&path); - let mode = entry.unix_mode().unwrap_or(0); - let file_type = mode & 0o170000; - if entry.is_dir() { - fs::create_dir_all(&output).map_err(|error| error.to_string())?; - } else if file_type == 0 || file_type == 0o100000 { - expanded = expanded - .checked_add(entry.size()) - .ok_or("plugin archive expanded size overflow")?; - if entry.size() > MAX_FILE_BYTES || expanded > MAX_EXPANDED_BYTES { - return Err("plugin archive exceeds expanded size limits".into()); - } - let size = entry.size(); - write_entry(&mut entry, &output, size)?; - } else { - return Err(format!("unsupported ZIP entry type at {}", path.display())); + total = total.saturating_add(read as u64); + if total > limit { + return Err(GitFailure::OutputLimit); } } +} + +fn inspect_git_object_store(path: &Path) -> Result<(), GitFailure> { + let bytes = git_object_store_size(path) + .map_err(|error| GitFailure::ObjectStoreInspection(error.kind()))?; + if bytes > MAX_EXPANDED_BYTES { + return Err(GitFailure::ObjectStoreLimit); + } Ok(()) } -fn extract_tar(reader: impl Read, destination: &Path) -> Result<(), String> { - let mut archive = tar::Archive::new(reader); - let mut paths = BTreeSet::new(); - let mut count = 0usize; - let mut expanded = 0u64; - for entry in archive - .entries() - .map_err(|error| format!("invalid tar archive: {error}"))? - { - let mut entry = entry.map_err(|error| format!("invalid tar entry: {error}"))?; - count += 1; - if count > MAX_ARCHIVE_ENTRIES { - return Err("plugin archive contains too many entries".into()); - } - let kind = entry.header().entry_type(); - if kind == tar::EntryType::XGlobalHeader { - if entry.size() > MAX_FILE_BYTES { - return Err("plugin archive metadata exceeds size limits".into()); +fn enforce_git_staging_metadata(git_dir: &Path, remote: &OsStr) -> Result<(), String> { + let remote = remote + .to_str() + .ok_or("plugin Git URL must be valid Unicode")? + .as_bytes(); + if remote.is_empty() { + return Err("plugin Git remote must not be empty".into()); + } + + let mut pending = vec![git_dir.to_path_buf()]; + let mut entries = 0usize; + let mut bytes = 0u64; + while let Some(directory) = pending.pop() { + let contents = fs::read_dir(&directory) + .map_err(|_| "could not inspect Git plugin staging metadata")?; + for entry in contents { + let entry = entry.map_err(|_| "could not inspect Git plugin staging metadata")?; + let name = entry.file_name(); + if name == OsStr::new("FETCH_HEAD") { + return Err("Git plugin fetch wrote forbidden FETCH_HEAD metadata".into()); } - continue; - } - let path = validate_archive_path(&entry.path().map_err(|error| error.to_string())?)?; - if !paths.insert(path.clone()) { - return Err(format!("duplicate plugin archive path {}", path.display())); - } - let output = destination.join(&path); - if kind.is_dir() { - fs::create_dir_all(&output).map_err(|error| error.to_string())?; - } else if kind.is_file() { - let size = entry.size(); - expanded = expanded - .checked_add(size) - .ok_or("plugin archive expanded size overflow")?; - if size > MAX_FILE_BYTES || expanded > MAX_EXPANDED_BYTES { - return Err("plugin archive exceeds expanded size limits".into()); + if directory == git_dir && name == OsStr::new("objects") { + continue; + } + entries += 1; + if entries > MAX_GIT_OBJECT_ENTRIES { + return Err("Git plugin staging metadata contains too many entries".into()); + } + let path = entry.path(); + let metadata = fs::symlink_metadata(&path) + .map_err(|_| "could not inspect Git plugin staging metadata")?; + if metadata.file_type().is_dir() { + pending.push(path); + continue; + } + if !metadata.file_type().is_file() { + return Err("Git plugin staging metadata contains an unsupported entry".into()); + } + let remaining = MAX_GIT_METADATA_BYTES.saturating_sub(bytes); + let mut contents = Vec::new(); + File::open(path) + .and_then(|file| file.take(remaining + 1).read_to_end(&mut contents)) + .map_err(|_| "could not read Git plugin staging metadata")?; + bytes = bytes + .checked_add(contents.len() as u64) + .ok_or("Git plugin staging metadata size overflow")?; + if bytes > MAX_GIT_METADATA_BYTES { + return Err("Git plugin staging metadata exceeds its size limit".into()); + } + if contents + .windows(remote.len()) + .any(|candidate| candidate == remote) + { + return Err("Git plugin staging metadata contains the configured remote".into()); } - write_entry(&mut entry, &output, size)?; - } else { - return Err(format!("unsupported tar entry type at {}", path.display())); } } Ok(()) } -fn validate_archive_path(path: &Path) -> Result { - if path.as_os_str().is_empty() || path.to_string_lossy().contains('\\') { - return Err("plugin archive contains an invalid path".into()); +fn create_private_directory(path: &Path) -> io::Result<()> { + let mut builder = fs::DirBuilder::new(); + #[cfg(unix)] + builder.mode(0o700); + builder.create(path) +} + +fn random_suffix() -> Result { + let mut random = [0u8; 8]; + getrandom::fill(&mut random).map_err(|error| error.to_string())?; + Ok(random.iter().map(|byte| format!("{byte:02x}")).collect()) +} + +struct StagingDirectory { + path: PathBuf, +} + +impl StagingDirectory { + fn create(parent: &Path, prefix: &str) -> Result { + cleanup_stale_staging(parent, prefix, STALE_STAGING_AGE)?; + let path = parent.join(format!("{prefix}{}", random_suffix()?)); + create_private_directory(&path) + .map_err(|error| format!("could not create plugin staging directory: {error}"))?; + Ok(Self { path }) } - let mut normalized = PathBuf::new(); - for component in path.components() { - match component { - Component::Normal(value) => normalized.push(value), - Component::CurDir => {} - Component::ParentDir | Component::RootDir | Component::Prefix(_) => { + + fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for StagingDirectory { + fn drop(&mut self) { + if fs::symlink_metadata(&self.path).is_ok_and(|metadata| metadata.file_type().is_dir()) { + let _ = fs::remove_dir_all(&self.path); + } + } +} + +fn cleanup_stale_staging(parent: &Path, prefix: &str, age: Duration) -> Result<(), String> { + let entries = fs::read_dir(parent) + .map_err(|error| format!("could not inspect plugin staging directories: {error}"))?; + let now = SystemTime::now(); + for entry in entries { + let entry = entry + .map_err(|error| format!("could not inspect plugin staging directory: {error}"))?; + let name = entry.file_name(); + let Some(suffix) = name.to_str().and_then(|name| name.strip_prefix(prefix)) else { + continue; + }; + if suffix.len() != 16 + || !suffix + .bytes() + .all(|byte| byte.is_ascii_digit() || matches!(byte, b'a'..=b'f')) + { + continue; + } + let path = entry.path(); + let metadata = match fs::symlink_metadata(&path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == io::ErrorKind::NotFound => continue, + Err(error) => { + return Err(format!( + "could not inspect stale plugin staging directory: {error}" + )); + } + }; + if !metadata.file_type().is_dir() + || metadata + .modified() + .ok() + .and_then(|modified| now.duration_since(modified).ok()) + .is_none_or(|elapsed| elapsed < age) + { + continue; + } + fs::remove_dir_all(path) + .map_err(|error| format!("could not remove stale plugin staging directory: {error}"))?; + } + Ok(()) +} + +fn resolve_git( + value: &str, + rev: Option<&str>, + subdir: Option<&str>, + cache_root: &Path, +) -> Result { + let url = validate_git_url(value)?; + let revision = rev + .map(validate_git_revision) + .transpose()? + .unwrap_or(GitRevision::DefaultBranch); + let subdir = subdir.map(validate_git_subdir).transpose()?; + let source = url.as_str(); + let source_key = sha256_text(source); + resolve_git_source( + OsStr::new(source), + &source_key, + &revision, + subdir.as_deref(), + cache_root, + GitProtocol::Https, + &SystemGitRunner::default(), + ) +} + +#[cfg(test)] +fn resolve_git_local( + repository: &Path, + rev: &str, + subdir: Option<&Path>, + cache_root: &Path, + runner: &dyn GitRunner, +) -> Result { + resolve_git_local_revision( + repository, + validate_git_revision(rev)?, + subdir, + cache_root, + runner, + ) +} + +#[cfg(test)] +fn resolve_git_local_revision( + repository: &Path, + revision: GitRevision, + subdir: Option<&Path>, + cache_root: &Path, + runner: &dyn GitRunner, +) -> Result { + let repository = repository + .canonicalize() + .map_err(|error| format!("could not resolve test Git repository: {error}"))?; + let subdir = subdir + .map(|path| { + path.to_str() + .ok_or_else(|| "plugin Git subdir must be valid Unicode".to_string()) + .and_then(validate_git_subdir) + }) + .transpose()?; + let source_key = sha256_text(&repository.to_string_lossy()); + resolve_git_source( + repository.as_os_str(), + &source_key, + &revision, + subdir.as_deref(), + cache_root, + GitProtocol::Local, + runner, + ) +} + +fn validate_git_url(value: &str) -> Result { + let url = Url::parse(value).map_err(|_| "invalid plugin Git URL".to_string())?; + if url.scheme() != "https" + || url.host().is_none() + || !url.username().is_empty() + || url.password().is_some() + || url.query().is_some() + || url.fragment().is_some() + { + return Err( + "plugin Git URL must be an absolute HTTPS URL without credentials, query, or fragment" + .into(), + ); + } + Ok(url) +} + +fn validate_git_revision(value: &str) -> Result { + if value.len() == 40 && value.bytes().all(|byte| byte.is_ascii_hexdigit()) { + return Ok(GitRevision::Commit(value.to_ascii_lowercase())); + } + if value == "HEAD" { + return Ok(GitRevision::DefaultBranch); + } + if !valid_git_ref_name(value) { + return Err("invalid plugin Git revision name".into()); + } + Ok(GitRevision::Ref(value.to_owned())) +} + +fn valid_git_ref_name(value: &str) -> bool { + if value.is_empty() + || value.len() > 1024 + || value == "@" + || value.starts_with(['/', '-', '+']) + || value.ends_with('/') + || value.ends_with('.') + || value.contains("//") + || value.contains("..") + || value.contains("@{") + || value.bytes().any(|byte| { + byte <= b' ' + || byte == 0x7f + || matches!(byte, b'~' | b'^' | b':' | b'?' | b'*' | b'[' | b'\\') + }) + { + return false; + } + value.split('/').all(|component| { + !component.is_empty() && !component.starts_with('.') && !component.ends_with(".lock") + }) +} + +fn validate_git_subdir(raw: &str) -> Result { + if raw.is_empty() + || raw.starts_with('/') + || raw.ends_with('/') + || raw.contains('\\') + || raw + .split('/') + .any(|component| component.is_empty() || component == "." || component == "..") + { + return Err("plugin Git subdir must be a canonical contained relative path".into()); + } + + let mut normalized = PathBuf::new(); + for value in raw.split('/') { + let upper = value + .split('.') + .next() + .unwrap_or(value) + .to_ascii_uppercase(); + let reserved = matches!( + upper.as_str(), + "CON" | "PRN" | "AUX" | "NUL" | "COM¹" | "COM²" | "COM³" | "LPT¹" | "LPT²" | "LPT³" + ) || (upper.len() == 4 + && (upper.starts_with("COM") || upper.starts_with("LPT")) + && matches!(upper.as_bytes()[3], b'1'..=b'9')); + if value.is_empty() + || value.ends_with(['.', ' ']) + || value.chars().any(|character| { + character.is_control() + || matches!(character, '<' | '>' | ':' | '"' | '|' | '?' | '*' | '\\') + }) + || reserved + { + return Err("plugin Git subdir is not portable".into()); + } + normalized.push(value); + } + Ok(normalized) +} + +fn sha256_text(value: &str) -> String { + Sha256::digest(value.as_bytes()) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} + +fn hardened_git_args( + protocol: GitProtocol, + hooks: &Path, + attributes: &Path, + args: &[&OsStr], +) -> Vec { + let mut output = vec![ + OsString::from("-c"), + OsString::from("protocol.allow=never"), + OsString::from("-c"), + OsString::from("protocol.https.allow=always"), + OsString::from("-c"), + OsString::from("protocol.file.allow=never"), + OsString::from("-c"), + OsString::from("protocol.ext.allow=never"), + OsString::from("-c"), + OsString::from("http.followRedirects=false"), + OsString::from("-c"), + OsString::from("http.sslVerify=true"), + OsString::from("-c"), + OsString::from("fetch.recurseSubmodules=false"), + OsString::from("-c"), + OsString::from("submodule.recurse=false"), + OsString::from("-c"), + OsString::from("maintenance.auto=false"), + OsString::from("-c"), + OsString::from("gc.auto=0"), + OsString::from("-c"), + OsString::from("core.logAllRefUpdates=false"), + OsString::from("-c"), + OsString::from("fetch.writeCommitGraph=false"), + OsString::from("-c"), + OsString::from("fetch.fsckObjects=true"), + OsString::from("-c"), + OsString::from("transfer.fsckObjects=true"), + OsString::from("-c"), + OsString::from(format!("core.hooksPath={}", hooks.display())), + OsString::from("-c"), + OsString::from(format!("init.templateDir={}", hooks.display())), + OsString::from("-c"), + OsString::from(format!("core.attributesFile={}", attributes.display())), + ]; + #[cfg(test)] + if matches!(protocol, GitProtocol::Local) { + output.extend([ + OsString::from("-c"), + OsString::from("protocol.file.allow=always"), + ]); + } + #[cfg(not(test))] + let _ = protocol; + output.extend(args.iter().map(|value| (*value).to_os_string())); + output +} + +fn hardened_git_config(remote: Option<&str>) -> Vec<(OsString, OsString)> { + let mut config = vec![ + (OsString::from("core.askPass"), OsString::new()), + ( + OsString::from("credential.interactive"), + OsString::from("false"), + ), + ]; + if let Some(remote) = remote { + config.extend([ + ( + OsString::from(format!("http.{remote}.followRedirects")), + OsString::from("false"), + ), + ( + OsString::from(format!("http.{remote}.sslVerify")), + OsString::from("true"), + ), + ]); + } + config +} + +struct GitCommandContext<'a> { + runner: &'a dyn GitRunner, + protocol: GitProtocol, + hooks: &'a Path, + attributes: &'a Path, + remote: Option<&'a str>, +} + +impl GitCommandContext<'_> { + fn run( + &self, + operation: &'static str, + cwd: &Path, + args: &[&OsStr], + stdout_limit: u64, + object_store_limit: Option<&Path>, + ) -> Result, String> { + let args = hardened_git_args(self.protocol, self.hooks, self.attributes, args); + let config = hardened_git_config(self.remote); + self.runner + .run(GitRunRequest { + cwd, + args: &args, + config: &config, + stdout_limit, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit, + }) + .map_err(|failure| describe_git_failure(operation, failure)) + } + + fn archive( + &self, + operation: &'static str, + cwd: &Path, + args: &[&OsStr], + destination: &Path, + ) -> Result<(), String> { + let args = hardened_git_args(self.protocol, self.hooks, self.attributes, args); + let config = hardened_git_config(self.remote); + self.runner + .archive( + GitRunRequest { + cwd, + args: &args, + config: &config, + stdout_limit: MAX_TAR_STREAM_BYTES, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit: None, + }, + destination, + ) + .map_err(|failure| describe_git_failure(operation, failure)) + } +} + +fn describe_git_failure(operation: &str, failure: GitFailure) -> String { + match failure { + GitFailure::Unavailable(kind) => { + format!("could not run Git for plugin {operation}: {kind:?}") + } + GitFailure::Timeout => format!("Git plugin {operation} timed out"), + GitFailure::OutputLimit => { + format!("Git plugin {operation} exceeded its output limit") + } + GitFailure::ObjectStoreLimit => { + "Git plugin fetch exceeded its object-store size limit".into() + } + GitFailure::ObjectStoreInspection(kind) => { + format!("could not inspect Git plugin object store: {kind:?}") + } + GitFailure::Archive(error) => format!("invalid Git plugin archive: {error}"), + GitFailure::Exit(code) => match code { + Some(code) => format!("Git plugin {operation} failed with status {code}"), + None => format!("Git plugin {operation} was terminated"), + }, + } +} + +fn resolve_git_source( + remote: &OsStr, + source_key: &str, + revision: &GitRevision, + subdir: Option<&Path>, + cache_root: &Path, + protocol: GitProtocol, + runner: &dyn GitRunner, +) -> Result { + let subdir_text = subdir.map(path_to_git_string).transpose()?; + let subdir_key = sha256_text(subdir_text.as_deref().unwrap_or(".")); + let source_root = cache_root.join(GIT_CACHE_VERSION).join(source_key); + fs::create_dir_all(&source_root) + .map_err(|error| format!("could not create Git plugin cache: {error}"))?; + + if let GitRevision::Commit(oid) = revision { + let destination = git_cache_destination(&source_root, oid, &subdir_key); + if let Ok(root) = + validate_git_cache_entry(&destination, source_key, oid, &subdir_key, subdir) + { + return Ok(root); + } + } + + let remote_config = match protocol { + GitProtocol::Https => Some( + remote + .to_str() + .ok_or("plugin Git URL must be valid Unicode")?, + ), + #[cfg(test)] + GitProtocol::Local => None, + }; + let staging_guard = StagingDirectory::create(&source_root, ".staging-")?; + let staging = staging_guard.path(); + let hooks = staging.join("hooks"); + let attributes = staging.join("attributes"); + let git_dir = staging.join("git"); + let repository = staging.join("repo"); + fs::create_dir(&hooks).map_err(|error| error.to_string())?; + fs::create_dir(&git_dir).map_err(|error| error.to_string())?; + OpenOptions::new() + .write(true) + .create_new(true) + .open(&attributes) + .map_err(|error| format!("could not create controlled Git attributes file: {error}"))?; + let git = GitCommandContext { + runner, + protocol, + hooks: &hooks, + attributes: &attributes, + remote: remote_config, + }; + + verify_git_effective_url(&git, staging, remote)?; + + git.run( + "repository initialization", + &git_dir, + &[ + OsStr::new("init"), + OsStr::new("--bare"), + OsStr::new("--object-format=sha1"), + OsStr::new("--"), + OsStr::new("."), + ], + MAX_GIT_DIAGNOSTIC_BYTES, + None, + )?; + fs::create_dir_all(git_dir.join("info")) + .map_err(|error| format!("could not create Git info directory: {error}"))?; + OpenOptions::new() + .write(true) + .create_new(true) + .open(git_dir.join("info/attributes")) + .map_err(|error| format!("could not create empty Git info attributes: {error}"))?; + verify_git_effective_url(&git, &git_dir, remote)?; + let fetch_revision = match revision { + GitRevision::Commit(oid) | GitRevision::Ref(oid) => oid.as_str(), + GitRevision::DefaultBranch => "HEAD", + }; + let fetch_refspec = format!("{fetch_revision}:{GIT_PRIVATE_FETCH_REF}"); + git.run( + "fetch", + &git_dir, + &[ + OsStr::new("fetch"), + OsStr::new("--force"), + OsStr::new("--depth=1"), + OsStr::new("--no-tags"), + OsStr::new("--no-recurse-submodules"), + OsStr::new("--no-write-fetch-head"), + OsStr::new("--"), + remote, + OsStr::new(&fetch_refspec), + ], + MAX_GIT_DIAGNOSTIC_BYTES, + Some(&git_dir.join("objects")), + )?; + enforce_git_staging_metadata(&git_dir, remote)?; + let private_commit = format!("{GIT_PRIVATE_FETCH_REF}^{{commit}}"); + let fetched = git.run( + "commit verification", + &git_dir, + &[ + OsStr::new("rev-parse"), + OsStr::new("--verify"), + OsStr::new("--end-of-options"), + OsStr::new(&private_commit), + ], + 128, + None, + )?; + let fetched = parse_git_oid(&fetched).ok_or("Git plugin revision is not a commit")?; + if let GitRevision::Commit(expected) = revision + && fetched != *expected + { + return Err("Git plugin fetch returned a different commit".into()); + } + enforce_git_object_store_limit(&git_dir.join("objects"))?; + + let destination = git_cache_destination(&source_root, &fetched, &subdir_key); + if let Ok(root) = + validate_git_cache_entry(&destination, source_key, &fetched, &subdir_key, subdir) + { + return Ok(root); + } + + let oid = OsStr::new(&fetched); + let mut tree_args = vec![ + OsStr::new("ls-tree"), + OsStr::new("-r"), + OsStr::new("-l"), + OsStr::new("-z"), + oid, + ]; + if let Some(path) = subdir_text.as_deref() { + tree_args.extend([OsStr::new("--"), OsStr::new(path)]); + } + let tree = git.run( + "tree inspection", + &git_dir, + &tree_args, + MAX_GIT_TREE_BYTES, + None, + )?; + validate_git_tree(&tree)?; + + let mut archive_args = vec![OsStr::new("archive"), OsStr::new("--format=tar"), oid]; + if let Some(path) = subdir_text.as_deref() { + archive_args.extend([OsStr::new("--"), OsStr::new(path)]); + } + fs::create_dir(&repository).map_err(|error| error.to_string())?; + git.archive("archive", &git_dir, &archive_args, &repository)?; + let candidate = select_git_package_root(&repository, subdir)?; + AgentPlugin::load(&candidate).map_err(|error| { + format!( + "invalid Git plugin package at {}: {error}", + candidate.display() + ) + })?; + enforce_git_staging_metadata(&git_dir, remote)?; + + fs::remove_dir_all(&git_dir).map_err(|error| error.to_string())?; + fs::remove_dir_all(&hooks).map_err(|error| error.to_string())?; + fs::remove_file(&attributes).map_err(|error| error.to_string())?; + write_git_cache_marker(staging, source_key, &fetched, &subdir_key)?; + match fs::rename(staging, &destination) { + Ok(()) => {} + Err(_) + if validate_git_cache_entry( + &destination, + source_key, + &fetched, + &subdir_key, + subdir, + ) + .is_ok() => {} + Err(error) => return Err(format!("could not publish Git plugin cache: {error}")), + } + validate_git_cache_entry(&destination, source_key, &fetched, &subdir_key, subdir) +} + +fn verify_git_effective_url( + git: &GitCommandContext<'_>, + cwd: &Path, + remote: &OsStr, +) -> Result<(), String> { + let expected = remote + .to_str() + .ok_or("plugin Git URL must be valid Unicode")?; + let output = git.run( + "URL verification", + cwd, + &[ + OsStr::new("ls-remote"), + OsStr::new("--get-url"), + OsStr::new("--"), + remote, + ], + MAX_GIT_DIAGNOSTIC_BYTES, + None, + )?; + let effective = output.strip_suffix(b"\n").unwrap_or(&output); + let effective = effective.strip_suffix(b"\r").unwrap_or(effective); + if effective != expected.as_bytes() { + return Err("plugin Git URL is rewritten by Git configuration".into()); + } + Ok(()) +} + +fn parse_git_oid(output: &[u8]) -> Option { + let value = std::str::from_utf8(output).ok()?.trim(); + (value.len() == 40 && value.bytes().all(|byte| byte.is_ascii_hexdigit())) + .then(|| value.to_ascii_lowercase()) +} + +fn path_to_git_string(path: &Path) -> Result { + path.components() + .map(|component| match component { + Component::Normal(value) => value + .to_str() + .map(str::to_owned) + .ok_or_else(|| "plugin Git subdir must be valid Unicode".to_string()), + _ => Err("plugin Git subdir must be a contained relative path".into()), + }) + .collect::, _>>() + .map(|components| components.join("/")) +} + +fn validate_git_tree(output: &[u8]) -> Result<(), String> { + let mut count = 0usize; + let mut total = 0u64; + for record in output + .split(|byte| *byte == 0) + .filter(|record| !record.is_empty()) + { + count += 1; + if count > MAX_ARCHIVE_ENTRIES { + return Err("Git plugin contains too many entries".into()); + } + let tab = record + .iter() + .position(|byte| *byte == b'\t') + .ok_or("invalid Git tree response")?; + let metadata = + std::str::from_utf8(&record[..tab]).map_err(|_| "invalid Git tree response")?; + let fields = metadata.split_whitespace().collect::>(); + if fields.len() != 4 { + return Err("invalid Git tree response".into()); + } + if fields[0] == "120000" { + return Err("Git plugin must not contain symbolic links".into()); + } + if fields[0] == "160000" || fields[1] == "commit" { + return Err("Git plugin must not contain submodules".into()); + } + if fields[1] != "blob" { + return Err("Git plugin contains an unsupported object".into()); + } + let size = fields[3] + .parse::() + .map_err(|_| "invalid Git object size")?; + total = total + .checked_add(size) + .ok_or("Git plugin object size overflow")?; + if size > MAX_FILE_BYTES || total > MAX_EXPANDED_BYTES { + return Err("Git plugin exceeds object size limits".into()); + } + } + Ok(()) +} + +fn git_object_store_size(objects: &Path) -> io::Result { + git_object_store_size_with_limit(objects, MAX_GIT_OBJECT_ENTRIES) +} + +fn git_object_store_size_with_limit(objects: &Path, entry_limit: usize) -> io::Result { + if !fs::symlink_metadata(objects)?.file_type().is_dir() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "Git object store is not a directory", + )); + } + let mut pending = vec![objects.to_path_buf()]; + let mut entries = 0usize; + let mut bytes = 0u64; + while let Some(directory) = pending.pop() { + let contents = fs::read_dir(&directory)?; + for entry in contents { + let entry = entry?; + entries += 1; + if entries > entry_limit { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "Git object store contains too many entries", + )); + } + let metadata = fs::symlink_metadata(entry.path())?; + if metadata.file_type().is_dir() { + pending.push(entry.path()); + } else if metadata.file_type().is_file() { + bytes = bytes.checked_add(metadata.len()).ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "Git object store size overflow") + })?; + if bytes > MAX_EXPANDED_BYTES { + return Ok(bytes); + } + } else { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "Git object store contains an unsupported entry", + )); + } + } + } + Ok(bytes) +} + +fn enforce_git_object_store_limit(objects: &Path) -> Result<(), String> { + let bytes = + git_object_store_size(objects).map_err(|_| "could not inspect Git plugin object store")?; + if bytes > MAX_EXPANDED_BYTES { + return Err("Git plugin object store exceeds its size limit".into()); + } + Ok(()) +} + +fn git_cache_destination(source_root: &Path, oid: &str, subdir_key: &str) -> PathBuf { + source_root.join(format!("{oid}-{subdir_key}")) +} + +fn git_cache_marker(source_key: &str, oid: &str, subdir_key: &str) -> String { + format!("{GIT_CACHE_VERSION}\n{source_key}\n{oid}\n{subdir_key}\n") +} + +fn write_git_cache_marker( + directory: &Path, + source_key: &str, + oid: &str, + subdir_key: &str, +) -> Result<(), String> { + let mut marker = OpenOptions::new() + .write(true) + .create_new(true) + .open(directory.join("complete")) + .map_err(|error| format!("could not create Git plugin cache marker: {error}"))?; + marker + .write_all(git_cache_marker(source_key, oid, subdir_key).as_bytes()) + .and_then(|()| marker.sync_all()) + .map_err(|error| format!("could not write Git plugin cache marker: {error}")) +} + +fn validate_git_cache_entry( + directory: &Path, + source_key: &str, + oid: &str, + subdir_key: &str, + subdir: Option<&Path>, +) -> Result { + let metadata = fs::symlink_metadata(directory) + .map_err(|_| "Git plugin cache entry is missing".to_string())?; + if !metadata.file_type().is_dir() { + return Err("Git plugin cache entry is not a directory".into()); + } + let entries = fs::read_dir(directory) + .map_err(|_| "could not inspect Git plugin cache entry")? + .collect::, _>>() + .map_err(|_| "could not inspect Git plugin cache entry")?; + if entries.len() != 2 + || !entries.iter().any(|entry| entry.file_name() == "repo") + || !entries.iter().any(|entry| entry.file_name() == "complete") + { + return Err("Git plugin cache entry contains unexpected files".into()); + } + let repository_path = directory.join("repo"); + let repository = fs::symlink_metadata(&repository_path) + .map_err(|_| "Git plugin cache repository is missing".to_string())?; + if !repository.file_type().is_dir() { + return Err("Git plugin cache repository is not a directory".into()); + } + let marker_path = directory.join("complete"); + if !fs::symlink_metadata(&marker_path).is_ok_and(|metadata| metadata.file_type().is_file()) { + return Err("Git plugin cache entry is incomplete".into()); + } + let expected = git_cache_marker(source_key, oid, subdir_key); + let mut marker = Vec::new(); + File::open(marker_path) + .and_then(|file| { + file.take(expected.len() as u64 + 1) + .read_to_end(&mut marker) + }) + .map_err(|_| "could not read Git plugin cache marker")?; + if marker != expected.as_bytes() { + return Err("Git plugin cache entry is incomplete".into()); + } + let root = select_git_package_root(&repository_path, subdir)?; + AgentPlugin::load(&root) + .map_err(|error| format!("invalid cached Git plugin package: {error}"))?; + Ok(root) +} + +fn select_git_package_root(repository: &Path, subdir: Option<&Path>) -> Result { + let canonical_repository = repository + .canonicalize() + .map_err(|error| format!("could not resolve Git plugin cache: {error}"))?; + let selected = subdir.map_or_else(|| repository.to_path_buf(), |path| repository.join(path)); + let selected = selected + .canonicalize() + .map_err(|error| format!("could not resolve Git plugin subdir: {error}"))?; + if !selected.is_dir() || !selected.starts_with(&canonical_repository) { + return Err("plugin Git subdir escapes the repository".into()); + } + Ok(selected) +} + +fn resolve_archive( + value: &str, + expected_digest: &str, + subdir: Option<&Path>, + cache_root: &Path, +) -> Result { + let url = Url::parse(value).map_err(|error| format!("invalid plugin archive URL: {error}"))?; + validate_download_url(&url)?; + let expected = parse_sha256(expected_digest)?; + let subdir = subdir.map(validate_relative_path).transpose()?; + let digest = expected_digest.to_ascii_lowercase(); + fs::create_dir_all(cache_root).map_err(|error| { + format!( + "could not create plugin cache {}: {error}", + cache_root.display() + ) + })?; + let destination = cache_root.join(&digest); + match fs::symlink_metadata(&destination) { + Ok(metadata) if !metadata.file_type().is_dir() => { + return Err(format!( + "plugin cache entry is not a directory: {}", + destination.display() + )); + } + Ok(_) => return validate_archive_cache_entry(&destination, subdir.as_deref()), + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(format!("could not inspect plugin cache entry: {error}")), + } + + let prefix = format!(".{digest}.staging-"); + let staging_guard = StagingDirectory::create(cache_root, &prefix)?; + let staging = staging_guard.path(); + let bytes = download(&url)?; + let actual = Sha256::digest(&bytes); + if actual.as_slice() != expected { + return Err(format!("SHA-256 mismatch for plugin archive {url}")); + } + extract_archive(&bytes, staging)?; + let candidate = select_package_root(staging, subdir.as_deref())?; + AgentPlugin::load(&candidate).map_err(|error| { + format!( + "invalid plugin archive package at {}: {error}", + candidate.display() + ) + })?; + match fs::rename(staging, &destination) { + Ok(()) => {} + Err(_) if validate_archive_cache_entry(&destination, subdir.as_deref()).is_ok() => {} + Err(error) => return Err(format!("could not publish plugin archive: {error}")), + } + validate_archive_cache_entry(&destination, subdir.as_deref()) +} + +fn validate_archive_cache_entry( + destination: &Path, + subdir: Option<&Path>, +) -> Result { + let root = select_package_root(destination, subdir)?; + AgentPlugin::load(&root) + .map_err(|error| format!("invalid cached plugin archive package: {error}"))?; + Ok(root) +} + +fn validate_download_url(url: &Url) -> Result<(), String> { + if !url.username().is_empty() || url.password().is_some() || url.fragment().is_some() { + return Err("plugin archive URL must not contain credentials or a fragment".into()); + } + let loopback = match url.host() { + Some(Host::Domain(host)) => host == "localhost", + Some(Host::Ipv4(address)) => address.is_loopback(), + Some(Host::Ipv6(address)) => address.is_loopback(), + None => false, + }; + if url.scheme() != "https" && !(url.scheme() == "http" && loopback) { + return Err("plugin archive URL must use HTTPS (or loopback HTTP)".into()); + } + Ok(()) +} + +fn parse_sha256(value: &str) -> Result<[u8; 32], String> { + if value.len() != 64 || !value.bytes().all(|byte| byte.is_ascii_hexdigit()) { + return Err("plugin archive sha256 must contain exactly 64 hexadecimal characters".into()); + } + let mut digest = [0u8; 32]; + for (index, pair) in value.as_bytes().chunks_exact(2).enumerate() { + let text = std::str::from_utf8(pair) + .map_err(|error| format!("could not parse plugin archive sha256: {error}"))?; + digest[index] = u8::from_str_radix(text, 16) + .map_err(|error| format!("could not parse plugin archive sha256: {error}"))?; + } + Ok(digest) +} + +fn download(url: &Url) -> Result, String> { + let allow_loopback_http = url.scheme() == "http"; + let client = reqwest::blocking::Client::builder() + .connect_timeout(Duration::from_secs(20)) + .timeout(Duration::from_secs(120)) + .redirect(reqwest::redirect::Policy::custom(move |attempt| { + if attempt.previous().len() >= 10 { + return attempt.error("too many plugin archive redirects"); + } + let target = attempt.url(); + if validate_download_url(target).is_ok() + && (target.scheme() == "https" || allow_loopback_http) + { + attempt.follow() + } else { + attempt.error("plugin archive redirect is not secure") + } + })) + .build() + .map_err(|error| format!("could not create plugin HTTP client: {error}"))?; + let response = client + .get(url.clone()) + .send() + .and_then(reqwest::blocking::Response::error_for_status) + .map_err(|error| format!("could not download plugin archive {url}: {error}"))?; + if response + .content_length() + .is_some_and(|length| length > MAX_DOWNLOAD_BYTES) + { + return Err("plugin archive exceeds the 64 MiB download limit".into()); + } + let mut bytes = Vec::new(); + response + .take(MAX_DOWNLOAD_BYTES + 1) + .read_to_end(&mut bytes) + .map_err(|error| format!("could not read plugin archive: {error}"))?; + if bytes.len() as u64 > MAX_DOWNLOAD_BYTES { + return Err("plugin archive exceeds the 64 MiB download limit".into()); + } + Ok(bytes) +} + +fn extract_archive(bytes: &[u8], destination: &Path) -> Result<(), String> { + if bytes.starts_with(b"PK\x03\x04") { + extract_zip(bytes, destination) + } else if bytes.starts_with(&[0x1f, 0x8b]) { + extract_tar( + LimitedReader { + inner: GzDecoder::new(Cursor::new(bytes)), + remaining: MAX_TAR_STREAM_BYTES, + }, + destination, + ) + } else { + extract_tar( + LimitedReader { + inner: Cursor::new(bytes), + remaining: MAX_TAR_STREAM_BYTES, + }, + destination, + ) + } +} + +fn extract_zip(bytes: &[u8], destination: &Path) -> Result<(), String> { + let mut archive = zip::ZipArchive::new(Cursor::new(bytes)) + .map_err(|error| format!("invalid ZIP plugin archive: {error}"))?; + if archive.len() > MAX_ARCHIVE_ENTRIES { + return Err("plugin archive contains too many entries".into()); + } + let mut paths = BTreeSet::new(); + let mut expanded = 0u64; + for index in 0..archive.len() { + let mut entry = archive.by_index(index).map_err(|error| error.to_string())?; + let path = validate_archive_path(Path::new(entry.name()))?; + if !paths.insert(path.clone()) { + return Err(format!("duplicate plugin archive path {}", path.display())); + } + let output = destination.join(&path); + let mode = entry.unix_mode().unwrap_or(0); + let file_type = mode & 0o170000; + if entry.is_dir() { + fs::create_dir_all(&output).map_err(|error| error.to_string())?; + } else if file_type == 0 || file_type == 0o100000 { + expanded = expanded + .checked_add(entry.size()) + .ok_or("plugin archive expanded size overflow")?; + if entry.size() > MAX_FILE_BYTES || expanded > MAX_EXPANDED_BYTES { + return Err("plugin archive exceeds expanded size limits".into()); + } + let size = entry.size(); + write_entry(&mut entry, &output, size)?; + } else { + return Err(format!("unsupported ZIP entry type at {}", path.display())); + } + } + Ok(()) +} + +fn extract_tar(reader: impl Read, destination: &Path) -> Result<(), String> { + let mut archive = tar::Archive::new(reader); + let mut paths = BTreeSet::new(); + let mut count = 0usize; + let mut expanded = 0u64; + for entry in archive + .entries() + .map_err(|error| format!("invalid tar archive: {error}"))? + { + let mut entry = entry.map_err(|error| format!("invalid tar entry: {error}"))?; + count += 1; + if count > MAX_ARCHIVE_ENTRIES { + return Err("plugin archive contains too many entries".into()); + } + let kind = entry.header().entry_type(); + if kind == tar::EntryType::XGlobalHeader { + if entry.size() > MAX_FILE_BYTES { + return Err("plugin archive metadata exceeds size limits".into()); + } + continue; + } + let path = validate_archive_path(&entry.path().map_err(|error| error.to_string())?)?; + if !paths.insert(path.clone()) { + return Err(format!("duplicate plugin archive path {}", path.display())); + } + let output = destination.join(&path); + if kind.is_dir() { + fs::create_dir_all(&output).map_err(|error| error.to_string())?; + } else if kind.is_file() { + let size = entry.size(); + expanded = expanded + .checked_add(size) + .ok_or("plugin archive expanded size overflow")?; + if size > MAX_FILE_BYTES || expanded > MAX_EXPANDED_BYTES { + return Err("plugin archive exceeds expanded size limits".into()); + } + write_entry(&mut entry, &output, size)?; + } else { + return Err(format!("unsupported tar entry type at {}", path.display())); + } + } + Ok(()) +} + +fn validate_archive_path(path: &Path) -> Result { + if path.as_os_str().is_empty() || path.to_string_lossy().contains('\\') { + return Err("plugin archive contains an invalid path".into()); + } + let mut normalized = PathBuf::new(); + for component in path.components() { + match component { + Component::Normal(value) => normalized.push(value), + Component::CurDir => {} + Component::ParentDir | Component::RootDir | Component::Prefix(_) => { return Err(format!( "plugin archive path escapes extraction root: {}", path.display() @@ -501,41 +1741,188 @@ fn select_package_root(extraction: &Path, subdir: Option<&Path>) -> Result String { + Sha256::digest(bytes) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() + } + + fn serve_once(body: Vec) -> (String, thread::JoinHandle<()>) { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + let handle = thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = [0u8; 1024]; + let _ = stream.read(&mut request).unwrap(); + write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ) + .unwrap(); + stream.write_all(&body).unwrap(); + }); + (format!("http://{address}/plugin.tar"), handle) + } + + struct TestRepository { + directory: tempfile::TempDir, + } + + impl TestRepository { + fn new() -> Self { + let directory = tempfile::tempdir().unwrap(); + let repository = Self { directory }; + repository.git(&["init", "--quiet"]); + repository.git(&["config", "user.name", "Kit Test"]); + repository.git(&["config", "user.email", "kit@example.invalid"]); + repository + } + + fn path(&self) -> &Path { + self.directory.path() + } + + fn command(&self, args: &[&str]) -> Command { + let hooks = self.path().join(".test-hooks"); + fs::create_dir_all(&hooks).unwrap(); + let mut command = Command::new("git"); + command + .arg("-c") + .arg(format!("core.hooksPath={}", hooks.display())) + .args(args) + .current_dir(self.path()) + .env("GIT_TERMINAL_PROMPT", "0"); + command + } + + fn git(&self, args: &[&str]) -> String { + let output = self.command(args).output().unwrap(); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout).unwrap().trim().to_owned() + } + + fn git_input(&self, args: &[&str], input: &[u8]) -> String { + let mut command = self.command(args); + command + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + let mut child = command.spawn().unwrap(); + child.stdin.take().unwrap().write_all(input).unwrap(); + let output = child.wait_with_output().unwrap(); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout).unwrap().trim().to_owned() + } + + fn commit_file(&self, path: &str, body: &[u8], message: &str) -> String { + let path = self.path().join(path); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, body).unwrap(); + self.git(&["add", "--all"]); + self.git(&["commit", "--quiet", "-m", message]); + self.git(&["rev-parse", "HEAD"]) + } + } + + #[derive(Default)] + struct TimeoutRunner; + + struct OutputLimitRunner; + + struct RewrittenUrlRunner; + + struct RecordingRunner { + inner: SystemGitRunner, + calls: std::sync::Mutex>>, + } + + impl Default for RecordingRunner { + fn default() -> Self { + Self { + inner: SystemGitRunner::default(), + calls: std::sync::Mutex::new(Vec::new()), + } + } + } + + impl GitRunner for TimeoutRunner { + fn run(&self, _request: GitRunRequest<'_>) -> Result, GitFailure> { + Err(GitFailure::Timeout) + } + + fn archive( + &self, + _request: GitRunRequest<'_>, + _destination: &Path, + ) -> Result<(), GitFailure> { + Err(GitFailure::Timeout) + } + } -#[cfg(test)] -mod tests { - use std::net::TcpListener; - use std::thread; + impl GitRunner for OutputLimitRunner { + fn run(&self, _request: GitRunRequest<'_>) -> Result, GitFailure> { + Err(GitFailure::OutputLimit) + } - use super::*; + fn archive( + &self, + _request: GitRunRequest<'_>, + _destination: &Path, + ) -> Result<(), GitFailure> { + Err(GitFailure::OutputLimit) + } + } - const MANIFEST: &str = r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"test-plugin"}"#; + impl GitRunner for RewrittenUrlRunner { + fn run(&self, _request: GitRunRequest<'_>) -> Result, GitFailure> { + Ok(b"https://rewritten.example/repository.git\n".to_vec()) + } - fn sha256_hex(bytes: &[u8]) -> String { - Sha256::digest(bytes) - .iter() - .map(|byte| format!("{byte:02x}")) - .collect() + fn archive( + &self, + _request: GitRunRequest<'_>, + _destination: &Path, + ) -> Result<(), GitFailure> { + Err(GitFailure::Unavailable(io::ErrorKind::Unsupported)) + } } - fn serve_once(body: Vec) -> (String, thread::JoinHandle<()>) { - let listener = TcpListener::bind("127.0.0.1:0").unwrap(); - let address = listener.local_addr().unwrap(); - let handle = thread::spawn(move || { - let (mut stream, _) = listener.accept().unwrap(); - let mut request = [0u8; 1024]; - let _ = stream.read(&mut request).unwrap(); - write!( - stream, - "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", - body.len() - ) - .unwrap(); - stream.write_all(&body).unwrap(); - }); - (format!("http://{address}/plugin.tar"), handle) + impl GitRunner for RecordingRunner { + fn run(&self, request: GitRunRequest<'_>) -> Result, GitFailure> { + self.calls.lock().unwrap().push(request.args.to_vec()); + self.inner.run(request) + } + + fn archive( + &self, + request: GitRunRequest<'_>, + destination: &Path, + ) -> Result<(), GitFailure> { + self.calls.lock().unwrap().push(request.args.to_vec()); + self.inner.archive(request, destination) + } } #[test] @@ -548,6 +1935,15 @@ mod tests { )) .unwrap(); assert!(matches!(archive, PluginConfig::Archive { .. })); + let git: PluginConfig = toml::from_str( + "source = 'git'\nurl = 'https://plugins.example/repository.git'\nrev = 'v1'\nsubdir = 'plugins/example'", + ) + .unwrap(); + assert!(matches!(git, PluginConfig::Git { rev: Some(_), .. })); + let git_default: PluginConfig = + toml::from_str("source = 'git'\nurl = 'https://plugins.example/repository.git'") + .unwrap(); + assert!(matches!(git_default, PluginConfig::Git { rev: None, .. })); assert!(matches!( toml::from_str::("source = 'path'\npath = '.'\nfuture_option = true"), Ok(PluginConfig::Path { .. }) @@ -565,6 +1961,858 @@ mod tests { assert!(validate_archive_path(Path::new("windows\\escape")).is_err()); } + #[test] + fn validates_git_urls_revisions_and_portable_subdirs() { + assert!(validate_git_url("https://plugins.example/repository.git").is_ok()); + assert!(validate_git_url("https://plugins.example/repository=x.git").is_ok()); + for invalid in [ + "http://plugins.example/repository.git", + "/repository.git", + "https://user:secret@plugins.example/repository.git", + "https://plugins.example/repository.git?token=secret", + "https://plugins.example/repository.git#main", + ] { + let error = validate_git_url(invalid).unwrap_err(); + assert!(!error.contains("secret")); + } + assert_eq!( + validate_git_revision(&"A1".repeat(20)).unwrap(), + GitRevision::Commit("a1".repeat(20)) + ); + assert_eq!( + validate_git_revision("v1.2.3").unwrap(), + GitRevision::Ref("v1.2.3".into()) + ); + assert_eq!( + validate_git_revision("abc123").unwrap(), + GitRevision::Ref("abc123".into()) + ); + assert_eq!( + validate_git_revision("refs/tags/abc123").unwrap(), + GitRevision::Ref("refs/tags/abc123".into()) + ); + assert_eq!( + validate_git_revision("HEAD").unwrap(), + GitRevision::DefaultBranch + ); + assert_eq!( + validate_git_revision("main").unwrap(), + GitRevision::Ref("main".into()) + ); + assert_eq!( + validate_git_revision("refs/heads/main").unwrap(), + GitRevision::Ref("refs/heads/main".into()) + ); + assert_eq!( + validate_git_revision("refs/tags/releases/v1").unwrap(), + GitRevision::Ref("refs/tags/releases/v1".into()) + ); + for invalid in [ + "+refs/tags/v1:refs/tags/pwned", + "-v1", + "v1^{}", + "v1..v2", + "refs/tags/.hidden", + "refs/tags/a.lock", + ] { + assert!( + validate_git_revision(invalid).is_err(), + "accepted {invalid}" + ); + } + let args = hardened_git_args( + GitProtocol::Https, + Path::new("hooks"), + Path::new("attributes"), + &[], + ); + let config = hardened_git_config(Some("https://plugins.example/repository=x.git")); + assert!(config.contains(&( + OsString::from("http.https://plugins.example/repository=x.git.sslVerify"), + OsString::from("true") + ))); + assert!(config.contains(&( + OsString::from("http.https://plugins.example/repository=x.git.followRedirects"), + OsString::from("false") + ))); + assert!(config.contains(&(OsString::from("core.askPass"), OsString::new()))); + assert!(config.contains(&( + OsString::from("credential.interactive"), + OsString::from("false") + ))); + assert!(args.contains(&OsString::from("core.attributesFile=attributes"))); + assert!( + !args + .iter() + .any(|arg| arg.to_string_lossy().starts_with("credential.helper=")) + ); + + assert!(validate_git_subdir("plugins/reviewer").is_ok()); + for invalid in [ + "../escape", + "./plugin", + "plugin/./nested", + "plugin//nested", + "plugin/nested/", + "CON/file", + "COM¹/file", + "COM²/file", + "COM³/file", + "LPT¹/file", + "LPT²/file", + "LPT³.txt/file", + "trailing.", + "a\\b", + " :(glob)", + ] { + assert!(validate_git_subdir(invalid).is_err(), "accepted {invalid}"); + } + } + + #[test] + fn system_git_runner_applies_url_config_with_equals_in_path() { + let directory = tempfile::tempdir().unwrap(); + let remote = "https://plugins.example/repository=x.git"; + let query = format!("http.{remote}.sslVerify"); + let command_args = [ + OsStr::new("config"), + OsStr::new("--get"), + OsStr::new(&query), + ]; + let args = hardened_git_args( + GitProtocol::Https, + directory.path(), + directory.path(), + &command_args, + ); + let config = hardened_git_config(Some(remote)); + let output = SystemGitRunner { + timeout: Duration::from_secs(5), + } + .run(GitRunRequest { + cwd: directory.path(), + args: &args, + config: &config, + stdout_limit: MAX_GIT_DIAGNOSTIC_BYTES, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit: None, + }) + .unwrap(); + + assert_eq!(String::from_utf8(output).unwrap().trim(), "true"); + } + + #[cfg(unix)] + #[test] + fn system_git_runner_disables_configured_askpass_probe() { + use std::os::unix::fs::PermissionsExt as _; + + let repository = TestRepository::new(); + let marker = repository.path().join("askpass-called"); + let probe = repository.path().join("askpass-probe.sh"); + fs::write( + &probe, + format!( + "#!/bin/sh\nprintf called > '{}'\nprintf 'secret\n'\n", + marker.display() + ), + ) + .unwrap(); + fs::set_permissions(&probe, fs::Permissions::from_mode(0o700)).unwrap(); + repository.git(&["config", "core.askPass", probe.to_str().unwrap()]); + + repository.git_input( + &["credential", "fill"], + b"protocol=https\nhost=example.invalid\nusername=kit\n\n", + ); + assert!(marker.is_file(), "askpass test probe was not executable"); + fs::remove_file(&marker).unwrap(); + + let alias = OsString::from( + "alias.askpass-probe=!printf 'protocol=https\nhost=example.invalid\nusername=kit\n\n' | git credential fill", + ); + let args = vec![OsString::from("-c"), alias, OsString::from("askpass-probe")]; + let config = hardened_git_config(None); + let result = SystemGitRunner { + timeout: Duration::from_secs(5), + } + .run(GitRunRequest { + cwd: repository.path(), + args: &args, + config: &config, + stdout_limit: MAX_GIT_DIAGNOSTIC_BYTES, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit: None, + }); + + assert!(matches!(result, Err(GitFailure::Exit(_)))); + assert!( + !marker.exists(), + "configured core.askPass escaped SystemGitRunner controls" + ); + } + + #[test] + fn system_git_runner_hard_bounds_pipes_and_fails_closed_on_object_scan() { + let directory = tempfile::tempdir().unwrap(); + let runner = SystemGitRunner { + timeout: Duration::from_secs(5), + }; + + let version_args = vec![OsString::from("--version")]; + assert_eq!( + runner.run(GitRunRequest { + cwd: directory.path(), + args: &version_args, + config: &[], + stdout_limit: 1, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit: None, + }), + Err(GitFailure::OutputLimit) + ); + + let invalid_args = vec![OsString::from("definitely-not-a-git-command")]; + assert_eq!( + runner.run(GitRunRequest { + cwd: directory.path(), + args: &invalid_args, + config: &[], + stdout_limit: MAX_GIT_DIAGNOSTIC_BYTES, + stderr_limit: 1, + object_store_limit: None, + }), + Err(GitFailure::OutputLimit) + ); + + let repository = TestRepository::new(); + repository.commit_file("plugin.json", MANIFEST.as_bytes(), "plugin"); + let archive_destination = tempfile::tempdir().unwrap(); + let archive_args = vec![ + OsString::from("archive"), + OsString::from("--format=tar"), + OsString::from("HEAD"), + ]; + assert_eq!( + runner.archive( + GitRunRequest { + cwd: repository.path(), + args: &archive_args, + config: &[], + stdout_limit: 64, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit: None, + }, + archive_destination.path(), + ), + Err(GitFailure::OutputLimit) + ); + + let counted_objects = directory.path().join("counted-objects"); + fs::create_dir(&counted_objects).unwrap(); + fs::write(counted_objects.join("one"), b"one").unwrap(); + fs::write(counted_objects.join("two"), b"two").unwrap(); + assert_eq!( + git_object_store_size_with_limit(&counted_objects, 1) + .unwrap_err() + .kind(), + io::ErrorKind::InvalidData + ); + + let missing_objects = directory.path().join("missing-objects"); + assert!(matches!( + runner.run(GitRunRequest { + cwd: directory.path(), + args: &version_args, + config: &[], + stdout_limit: MAX_GIT_DIAGNOSTIC_BYTES, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit: Some(&missing_objects), + }), + Err(GitFailure::ObjectStoreInspection(io::ErrorKind::NotFound)) + )); + assert!(fs::read_dir(directory.path()).unwrap().all(|entry| { + !entry + .unwrap() + .file_name() + .to_string_lossy() + .starts_with(".git-stderr") + })); + } + + #[cfg(unix)] + #[test] + fn system_git_runner_timeout_terminates_descendants_and_joins_pipes() { + let directory = tempfile::tempdir().unwrap(); + let pid_file = directory.path().join("descendant.pid"); + let alias = OsString::from(format!( + "alias.spawn=!sleep 30 & echo $! > {}; wait", + pid_file.display() + )); + let args = vec![OsString::from("-c"), alias, OsString::from("spawn")]; + let runner = SystemGitRunner { + timeout: Duration::from_millis(300), + }; + assert_eq!( + runner.run(GitRunRequest { + cwd: directory.path(), + args: &args, + config: &[], + stdout_limit: MAX_GIT_DIAGNOSTIC_BYTES, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit: None, + }), + Err(GitFailure::Timeout) + ); + + let pid = fs::read_to_string(pid_file) + .unwrap() + .trim() + .parse::() + .unwrap(); + let mut exists = true; + for _ in 0..100 { + let result = unsafe { libc::kill(pid, 0) }; + exists = result == 0 || io::Error::last_os_error().raw_os_error() != Some(libc::ESRCH); + if !exists { + break; + } + thread::sleep(Duration::from_millis(10)); + } + assert!(!exists, "Git descendant {pid} survived runner cleanup"); + } + + #[cfg(unix)] + #[test] + fn system_git_runner_times_out_during_archive_extraction_and_joins_threads() { + let directory = tempfile::tempdir().unwrap(); + let mut partial_tar = tar_with_file("plugin.json", b"{}"); + partial_tar.truncate(1024); + let partial_tar_path = directory.path().join("partial.tar"); + fs::write(&partial_tar_path, partial_tar).unwrap(); + let alias = OsString::from(format!( + "alias.slow=!cat {}; sleep 30", + partial_tar_path.display() + )); + let args = vec![OsString::from("-c"), alias, OsString::from("slow")]; + let destination = tempfile::tempdir().unwrap(); + let runner = SystemGitRunner { + timeout: Duration::from_millis(300), + }; + let started = Instant::now(); + assert_eq!( + runner.archive( + GitRunRequest { + cwd: directory.path(), + args: &args, + config: &[], + stdout_limit: MAX_TAR_STREAM_BYTES, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit: None, + }, + destination.path(), + ), + Err(GitFailure::Timeout) + ); + assert!(started.elapsed() < Duration::from_secs(5)); + assert_eq!( + fs::read(destination.path().join("plugin.json")).unwrap(), + b"{}" + ); + } + + #[cfg(unix)] + #[test] + fn system_git_runner_fails_closed_when_live_object_store_becomes_invalid() { + let directory = tempfile::tempdir().unwrap(); + let objects = directory.path().join("objects"); + fs::create_dir(&objects).unwrap(); + let objects_for_thread = objects.clone(); + let invalidator = thread::spawn(move || { + thread::sleep(Duration::from_millis(150)); + fs::remove_dir_all(&objects_for_thread).unwrap(); + fs::write(objects_for_thread, b"not a directory").unwrap(); + }); + let args = vec![ + OsString::from("-c"), + OsString::from("alias.wait=!sleep 30"), + OsString::from("wait"), + ]; + let runner = SystemGitRunner { + timeout: Duration::from_secs(5), + }; + let started = Instant::now(); + let result = runner.run(GitRunRequest { + cwd: directory.path(), + args: &args, + config: &[], + stdout_limit: MAX_GIT_DIAGNOSTIC_BYTES, + stderr_limit: MAX_GIT_DIAGNOSTIC_BYTES, + object_store_limit: Some(&objects), + }); + invalidator.join().unwrap(); + assert_eq!( + result, + Err(GitFailure::ObjectStoreInspection( + io::ErrorKind::InvalidData + )) + ); + assert!(started.elapsed() < Duration::from_secs(4)); + } + + #[test] + fn rejects_remote_bytes_and_fetch_head_in_non_object_git_metadata() { + let directory = tempfile::tempdir().unwrap(); + let git_dir = directory.path(); + fs::create_dir(git_dir.join("objects")).unwrap(); + let remote = OsStr::new("https://plugins.example/private.git"); + fs::write( + git_dir.join("objects/ignored"), + b"https://plugins.example/private.git", + ) + .unwrap(); + enforce_git_staging_metadata(git_dir, remote).unwrap(); + + fs::write( + git_dir.join("config-leak"), + b"https://plugins.example/private.git", + ) + .unwrap(); + assert!( + enforce_git_staging_metadata(git_dir, remote) + .unwrap_err() + .contains("configured remote") + ); + fs::remove_file(git_dir.join("config-leak")).unwrap(); + + fs::write(git_dir.join("FETCH_HEAD"), b"forbidden").unwrap(); + assert!( + enforce_git_staging_metadata(git_dir, remote) + .unwrap_err() + .contains("FETCH_HEAD") + ); + } + + #[test] + fn rejects_rewritten_effective_url_before_network_and_cleans_staging() { + let cache = tempfile::tempdir().unwrap(); + let remote = "https://plugins.example/repository.git"; + let source_key = sha256_text(remote); + let error = resolve_git_source( + OsStr::new(remote), + &source_key, + &GitRevision::Commit("01".repeat(20)), + None, + cache.path(), + GitProtocol::Https, + &RewrittenUrlRunner, + ) + .unwrap_err(); + assert!(error.contains("rewritten")); + assert!( + fs::read_dir(cache.path().join(GIT_CACHE_VERSION).join(source_key)) + .unwrap() + .all(|entry| !entry + .unwrap() + .file_name() + .to_string_lossy() + .starts_with(".staging-")) + ); + } + + #[test] + fn staging_guard_removes_current_and_safely_named_stale_directories() { + let parent = tempfile::tempdir().unwrap(); + let stale = parent.path().join(".staging-0000000000000000"); + let unrelated = parent.path().join(".staging-not-owned"); + fs::create_dir(&stale).unwrap(); + fs::create_dir(&unrelated).unwrap(); + cleanup_stale_staging(parent.path(), ".staging-", Duration::ZERO).unwrap(); + assert!(!stale.exists()); + assert!(unrelated.exists()); + + let active_path; + { + let staging = StagingDirectory::create(parent.path(), ".staging-").unwrap(); + active_path = staging.path().to_path_buf(); + assert!(active_path.is_dir()); + } + assert!(!active_path.exists()); + } + + #[test] + fn resolves_default_branch_and_main_to_commits() { + let repository = TestRepository::new(); + let first = repository.commit_file("plugin.json", MANIFEST.as_bytes(), "plugin"); + repository.git(&["branch", "-M", "main"]); + let cache = tempfile::tempdir().unwrap(); + + let default_root = resolve_git_local_revision( + repository.path(), + GitRevision::DefaultBranch, + None, + cache.path(), + &SystemGitRunner::default(), + ) + .unwrap(); + assert!( + default_root + .parent() + .unwrap() + .file_name() + .unwrap() + .to_string_lossy() + .starts_with(&first) + ); + + let second = repository.commit_file("version.txt", b"second", "second"); + let main_root = resolve_git_local( + repository.path(), + "main", + None, + cache.path(), + &SystemGitRunner::default(), + ) + .unwrap(); + assert!( + main_root + .parent() + .unwrap() + .file_name() + .unwrap() + .to_string_lossy() + .starts_with(&second) + ); + assert_eq!(fs::read(main_root.join("version.txt")).unwrap(), b"second"); + } + + #[test] + fn resolves_git_commit_and_reuses_it_offline() { + let repository = TestRepository::new(); + let commit = repository.commit_file("plugin.json", MANIFEST.as_bytes(), "plugin"); + let cache = tempfile::tempdir().unwrap(); + let root = resolve_git_local( + repository.path(), + &commit, + None, + cache.path(), + &SystemGitRunner::default(), + ) + .unwrap(); + assert_eq!( + fs::read_to_string(root.join("plugin.json")).unwrap(), + MANIFEST + ); + + // A full commit cache hit does not invoke the now-unavailable runner. + let root_again = resolve_git_local( + repository.path(), + &commit, + None, + cache.path(), + &TimeoutRunner, + ) + .unwrap(); + assert_eq!(root_again, root); + + fs::write(root.parent().unwrap().join("unexpected"), b"corrupt").unwrap(); + let error = resolve_git_local( + repository.path(), + &commit, + None, + cache.path(), + &TimeoutRunner, + ) + .unwrap_err(); + assert!(error.contains("timed out")); + assert!( + fs::read_dir(root.parent().unwrap().parent().unwrap()) + .unwrap() + .all(|entry| !entry + .unwrap() + .file_name() + .to_string_lossy() + .starts_with(".staging-")) + ); + } + + #[test] + fn forces_sha1_and_preserves_only_committed_git_attributes() { + let repository = TestRepository::new(); + repository.commit_file( + ".gitattributes", + b"secret.txt export-ignore\n", + "attributes", + ); + repository.commit_file("plugin.json", MANIFEST.as_bytes(), "plugin"); + let commit = repository.commit_file("secret.txt", b"secret", "secret"); + let cache = tempfile::tempdir().unwrap(); + let runner = RecordingRunner::default(); + let root = + resolve_git_local(repository.path(), &commit, None, cache.path(), &runner).unwrap(); + assert!(root.join(".gitattributes").is_file()); + assert!(!root.join("secret.txt").exists()); + + let calls = runner.calls.lock().unwrap(); + assert!( + calls + .iter() + .flatten() + .any(|arg| arg == "--object-format=sha1") + ); + assert_eq!( + calls + .iter() + .filter(|args| args.iter().any(|arg| arg == "--get-url")) + .count(), + 2 + ); + assert!(calls.iter().all(|args| { + args.iter() + .any(|arg| arg.to_string_lossy().starts_with("core.attributesFile=")) + })); + let fetch = calls + .iter() + .find(|args| args.iter().any(|arg| arg == "--no-write-fetch-head")) + .expect("fetch must suppress FETCH_HEAD"); + assert!( + fetch + .iter() + .any(|arg| { arg == &OsString::from(format!("{commit}:{GIT_PRIVATE_FETCH_REF}")) }) + ); + assert!(calls.iter().any(|args| { + args.iter() + .any(|arg| arg == &OsString::from(format!("{GIT_PRIVATE_FETCH_REF}^{{commit}}"))) + })); + assert!( + calls + .iter() + .flatten() + .all(|arg| arg != OsStr::new("FETCH_HEAD^{commit}")) + ); + } + + #[test] + fn resolves_fresh_lightweight_and_annotated_tags() { + let repository = TestRepository::new(); + let first = repository.commit_file("plugin.json", MANIFEST.as_bytes(), "first"); + repository.git(&["tag", "stable", &first]); + repository.git(&["tag", "-a", "annotated", "-m", "release", &first]); + let cache = tempfile::tempdir().unwrap(); + let first_root = resolve_git_local( + repository.path(), + "stable", + None, + cache.path(), + &SystemGitRunner::default(), + ) + .unwrap(); + resolve_git_local( + repository.path(), + "refs/tags/annotated", + None, + cache.path(), + &SystemGitRunner::default(), + ) + .unwrap(); + + let second = repository.commit_file("version.txt", b"second", "second"); + repository.git(&["tag", "--force", "stable", &second]); + let second_root = resolve_git_local( + repository.path(), + "refs/tags/stable", + None, + cache.path(), + &SystemGitRunner::default(), + ) + .unwrap(); + assert_ne!(first_root, second_root); + assert_eq!( + fs::read(second_root.join("version.txt")).unwrap(), + b"second" + ); + } + + #[test] + fn resolves_git_subdir_with_literal_pathspecs() { + let repository = TestRepository::new(); + repository.commit_file("plugin.json", MANIFEST.as_bytes(), "root plugin"); + let commit = repository.commit_file( + "packages/reviewer/plugin.json", + MANIFEST.as_bytes(), + "nested plugin", + ); + let cache = tempfile::tempdir().unwrap(); + let root_package = resolve_git_local( + repository.path(), + &commit, + None, + cache.path(), + &SystemGitRunner::default(), + ) + .unwrap(); + let nested_package = resolve_git_local( + repository.path(), + &commit, + Some(Path::new("packages/reviewer")), + cache.path(), + &SystemGitRunner::default(), + ) + .unwrap(); + assert!(root_package.join("plugin.json").is_file()); + assert!(nested_package.join("plugin.json").is_file()); + assert_ne!(root_package, nested_package); + } + + #[test] + fn rejects_noncommit_tags_symlinks_and_submodules() { + let noncommit = TestRepository::new(); + noncommit.commit_file("plugin.json", MANIFEST.as_bytes(), "plugin"); + let blob = noncommit.git(&["hash-object", "plugin.json"]); + noncommit.git(&["tag", "blob", &blob]); + let cache = tempfile::tempdir().unwrap(); + let error = resolve_git_local( + noncommit.path(), + "blob", + None, + cache.path(), + &SystemGitRunner::default(), + ) + .unwrap_err(); + assert!(error.contains("not a commit") || error.contains("commit verification")); + + let symlink = TestRepository::new(); + symlink.commit_file("plugin.json", MANIFEST.as_bytes(), "plugin"); + let link_blob = symlink.git_input(&["hash-object", "-w", "--stdin"], b"target"); + symlink.git(&[ + "update-index", + "--add", + "--cacheinfo", + "120000", + &link_blob, + "link", + ]); + symlink.git(&["commit", "--quiet", "-m", "symlink"]); + let symlink_commit = symlink.git(&["rev-parse", "HEAD"]); + let error = resolve_git_local( + symlink.path(), + &symlink_commit, + None, + tempfile::tempdir().unwrap().path(), + &SystemGitRunner::default(), + ) + .unwrap_err(); + assert!(error.contains("symbolic links")); + + let submodule = TestRepository::new(); + let parent = submodule.commit_file("plugin.json", MANIFEST.as_bytes(), "plugin"); + submodule.git(&[ + "update-index", + "--add", + "--cacheinfo", + "160000", + &parent, + "nested", + ]); + submodule.git(&["commit", "--quiet", "-m", "submodule"]); + let submodule_commit = submodule.git(&["rev-parse", "HEAD"]); + let error = resolve_git_local( + submodule.path(), + &submodule_commit, + None, + tempfile::tempdir().unwrap().path(), + &SystemGitRunner::default(), + ) + .unwrap_err(); + assert!(error.contains("submodules")); + } + + #[test] + fn enforces_git_object_size_limits() { + let too_large = format!( + "100644 blob {} {}\tlarge.bin\0", + "01".repeat(20), + MAX_FILE_BYTES + 1 + ); + assert!( + validate_git_tree(too_large.as_bytes()) + .unwrap_err() + .contains("size limits") + ); + + let store = tempfile::tempdir().unwrap(); + fs::create_dir(store.path().join("pack")).unwrap(); + File::create(store.path().join("pack/oversized.pack")) + .unwrap() + .set_len(MAX_EXPANDED_BYTES + 1) + .unwrap(); + assert!( + enforce_git_object_store_limit(store.path()) + .unwrap_err() + .contains("size limit") + ); + } + + #[test] + fn concurrent_git_publishers_validate_the_winner() { + let repository = TestRepository::new(); + let commit = repository.commit_file("plugin.json", MANIFEST.as_bytes(), "plugin"); + let repository = repository.path().to_path_buf(); + let cache = tempfile::tempdir().unwrap(); + let cache_path = cache.path().to_path_buf(); + let barrier = std::sync::Arc::new(std::sync::Barrier::new(2)); + let handles = (0..2) + .map(|_| { + let repository = repository.clone(); + let commit = commit.clone(); + let cache_path = cache_path.clone(); + let barrier = barrier.clone(); + thread::spawn(move || { + barrier.wait(); + resolve_git_local( + &repository, + &commit, + None, + &cache_path, + &SystemGitRunner::default(), + ) + .unwrap() + }) + }) + .collect::>(); + let roots = handles + .into_iter() + .map(|handle| handle.join().unwrap()) + .collect::>(); + assert_eq!(roots[0], roots[1]); + assert!(roots[0].join("plugin.json").is_file()); + } + + #[test] + fn git_timeout_and_output_errors_redact_the_remote() { + let url = validate_git_url("https://plugins.example/private/opaque-secret.git").unwrap(); + let source_key = sha256_text(url.as_str()); + for (runner, expected) in [ + (&TimeoutRunner as &dyn GitRunner, "timed out"), + (&OutputLimitRunner as &dyn GitRunner, "output limit"), + ] { + let cache = tempfile::tempdir().unwrap(); + let error = resolve_git_source( + OsStr::new(url.as_str()), + &source_key, + &GitRevision::Ref("refs/tags/stable".into()), + None, + cache.path(), + GitProtocol::Https, + runner, + ) + .unwrap_err(); + assert!(error.contains(expected)); + assert!(!error.contains("opaque-secret")); + assert!(!error.contains(url.as_str())); + } + } + fn tar_with_file(path: &str, body: &[u8]) -> Vec { let mut bytes = Vec::new(); { diff --git a/src/process_tree.rs b/src/process_tree.rs new file mode 100644 index 0000000..16fa0ba --- /dev/null +++ b/src/process_tree.rs @@ -0,0 +1,80 @@ +use std::process::{Child, Command}; + +/// Starts a synchronous child as the leader of an isolated process tree. +pub(crate) fn isolate_process_tree(command: &mut Command) { + #[cfg(unix)] + { + use std::os::unix::process::CommandExt as _; + + command.process_group(0); + } +} + +/// Terminates a synchronous child and the process tree rooted at it. +/// +/// New callers should capture the PID immediately after spawn and use +/// [`terminate_process_tree_with_pid`] so cleanup can still target descendants +/// after the direct child exits. +#[allow(dead_code)] +pub(crate) fn terminate_process_tree(child: &mut Child) { + let pid = child.id(); + terminate_process_tree_with_pid(child, pid); +} + +/// Terminates a synchronous child using the PID captured immediately after spawn. +pub(crate) fn terminate_process_tree_with_pid(child: &mut Child, pid: u32) { + kill_process_tree(pid); + let _ = child.kill(); + let _ = child.wait(); +} + +/// Starts an asynchronous child as the leader of an isolated process tree. +pub(crate) fn isolate_tokio_process_tree(command: &mut tokio::process::Command) { + isolate_process_tree(command.as_std_mut()); +} + +/// Terminates an asynchronous child and the process tree rooted at it. +pub(crate) async fn terminate_tokio_process_tree(child: &mut tokio::process::Child, pid: u32) { + kill_tokio_process_tree(pid).await; + let _ = child.kill().await; +} + +#[cfg(unix)] +fn kill_process_tree(pid: u32) { + if let Ok(pid) = i32::try_from(pid) { + // Children are created as process-group leaders, so a negative PID + // addresses the complete group. + unsafe { + libc::kill(-pid, libc::SIGKILL); + } + } +} + +#[cfg(windows)] +fn kill_process_tree(pid: u32) { + let _ = Command::new("taskkill").args(taskkill_args(pid)).status(); +} + +#[cfg(not(any(unix, windows)))] +fn kill_process_tree(_pid: u32) {} + +#[cfg(unix)] +async fn kill_tokio_process_tree(pid: u32) { + kill_process_tree(pid); +} + +#[cfg(windows)] +async fn kill_tokio_process_tree(pid: u32) { + let _ = tokio::process::Command::new("taskkill") + .args(taskkill_args(pid)) + .status() + .await; +} + +#[cfg(not(any(unix, windows)))] +async fn kill_tokio_process_tree(_pid: u32) {} + +#[cfg(windows)] +fn taskkill_args(pid: u32) -> [String; 4] { + ["/PID".into(), pid.to_string(), "/T".into(), "/F".into()] +} diff --git a/src/tools/shell.rs b/src/tools/shell.rs index 0bf6e5b..5452a7b 100644 --- a/src/tools/shell.rs +++ b/src/tools/shell.rs @@ -9,6 +9,8 @@ use serde::Deserialize; use serde_json::json; use tokio::{io::AsyncReadExt, process::Command}; +use crate::process_tree::{isolate_tokio_process_tree, terminate_tokio_process_tree}; + const MAX_INTERNAL_OUTPUT_BYTES: usize = 64 * 1024 * 1024; #[derive(Clone)] @@ -83,10 +85,13 @@ impl Tool for ShellTool { .stdout(Stdio::piped()) .stderr(Stdio::piped()) .kill_on_drop(true); - isolate_process_tree(&mut command); + isolate_tokio_process_tree(&mut command); let mut child = command .spawn() .map_err(|error| ToolError::ExecutionFailed(error.to_string()))?; + let pid = child + .id() + .ok_or_else(|| ToolError::Internal("spawned shell did not have a process ID".into()))?; let stdout = child .stdout .take() @@ -97,21 +102,13 @@ impl Tool for ShellTool { .ok_or_else(|| ToolError::Internal("shell stderr was not piped".into()))?; let mut stdout_task = tokio::spawn(read_output(stdout)); let mut stderr_task = tokio::spawn(read_output(stderr)); - let execution = async { - let status = child - .wait() - .await - .map_err(|error| ToolError::ExecutionFailed(error.to_string()))?; - let stdout = (&mut stdout_task) - .await - .map_err(|error| ToolError::Internal(error.to_string()))? - .map_err(|error| ToolError::ExecutionFailed(error.to_string()))?; - let stderr = (&mut stderr_task) - .await - .map_err(|error| ToolError::Internal(error.to_string()))? - .map_err(|error| ToolError::ExecutionFailed(error.to_string()))?; - Ok::<_, ToolError>((status, stdout, stderr)) - }; + let mut stdout_finished = false; + let mut stderr_finished = false; + let mut status = None; + let mut stdout = None; + let mut stderr = None; + let timeout = tokio::time::sleep(Duration::from_secs(input.timeout_seconds)); + tokio::pin!(timeout); // A cancelled turn must not wait out the command: the loop awaits this // invocation, so an uncooperative tool keeps the whole turn alive until // the timeout, however long the caller asked for. @@ -121,26 +118,98 @@ impl Tool for ShellTool { None => std::future::pending().await, } }; - let finished = tokio::select! { - result = tokio::time::timeout(Duration::from_secs(input.timeout_seconds), execution) => Some(result), - () = interrupted => None, - }; - // The command's futures are dropped with the select, so the child is - // ours to kill again. - let (status, stdout, stderr) = match finished { - Some(Ok(result)) => result?, - outcome => { - kill_process_tree(&mut child).await; - stdout_task.abort(); - stderr_task.abort(); - let _ = stdout_task.await; - let _ = stderr_task.await; - return Err(match outcome { - Some(_) => ToolError::ExecutionFailed("shell command timed out".into()), - None => ToolError::Cancelled, - }); + tokio::pin!(interrupted); + + while status.is_none() || stdout.is_none() || stderr.is_none() { + let event = tokio::select! { + result = child.wait(), if status.is_none() => ShellEvent::Wait(result), + result = &mut stdout_task, if !stdout_finished => ShellEvent::Stdout(result), + result = &mut stderr_task, if !stderr_finished => ShellEvent::Stderr(result), + () = &mut timeout => ShellEvent::Timeout, + () = &mut interrupted => ShellEvent::Cancelled, + }; + match event { + ShellEvent::Wait(Ok(exit_status)) => status = Some(exit_status), + ShellEvent::Wait(Err(error)) => { + terminate_shell( + &mut child, + pid, + &mut stdout_task, + stdout_finished, + &mut stderr_task, + stderr_finished, + ) + .await; + return Err(ToolError::ExecutionFailed(error.to_string())); + } + ShellEvent::Stdout(result) => { + stdout_finished = true; + match output_task_result(result) { + Ok(output) => stdout = Some(output), + Err(error) => { + terminate_shell( + &mut child, + pid, + &mut stdout_task, + stdout_finished, + &mut stderr_task, + stderr_finished, + ) + .await; + return Err(error); + } + } + } + ShellEvent::Stderr(result) => { + stderr_finished = true; + match output_task_result(result) { + Ok(output) => stderr = Some(output), + Err(error) => { + terminate_shell( + &mut child, + pid, + &mut stdout_task, + stdout_finished, + &mut stderr_task, + stderr_finished, + ) + .await; + return Err(error); + } + } + } + ShellEvent::Timeout => { + terminate_shell( + &mut child, + pid, + &mut stdout_task, + stdout_finished, + &mut stderr_task, + stderr_finished, + ) + .await; + return Err(ToolError::ExecutionFailed("shell command timed out".into())); + } + ShellEvent::Cancelled => { + terminate_shell( + &mut child, + pid, + &mut stdout_task, + stdout_finished, + &mut stderr_task, + stderr_finished, + ) + .await; + return Err(ToolError::Cancelled); + } } - }; + } + let status = + status.ok_or_else(|| ToolError::Internal("shell status was not collected".into()))?; + let stdout = + stdout.ok_or_else(|| ToolError::Internal("shell stdout was not collected".into()))?; + let stderr = + stderr.ok_or_else(|| ToolError::Internal("shell stderr was not collected".into()))?; let output = json!({ "exit_code": status.code(), "success": status.success(), @@ -154,35 +223,42 @@ impl Tool for ShellTool { } } -#[cfg(unix)] -fn isolate_process_tree(command: &mut Command) { - use std::os::unix::process::CommandExt as _; - // A shell is only the group leader; cancelling it must also stop pipelines, - // sleeps, and other descendants it launched. - command.as_std_mut().process_group(0); +type OutputTask = tokio::task::JoinHandle>; + +enum ShellEvent { + Wait(std::io::Result), + Stdout(Result, tokio::task::JoinError>), + Stderr(Result, tokio::task::JoinError>), + Timeout, + Cancelled, } -#[cfg(windows)] -fn isolate_process_tree(_command: &mut Command) {} - -async fn kill_process_tree(child: &mut tokio::process::Child) { - #[cfg(unix)] - if let Some(pid) = child.id() - && let Ok(pid) = i32::try_from(pid) - { - // The child was created as its own process-group leader. - unsafe { - libc::kill(-pid, libc::SIGKILL); - } - } - #[cfg(windows)] - if let Some(pid) = child.id() { - let _ = tokio::process::Command::new("taskkill") - .args(["/PID", &pid.to_string(), "/T", "/F"]) - .status() - .await; +fn output_task_result( + result: Result, tokio::task::JoinError>, +) -> Result { + result + .map_err(|error| ToolError::Internal(error.to_string()))? + .map_err(|error| ToolError::ExecutionFailed(error.to_string())) +} + +async fn terminate_shell( + child: &mut tokio::process::Child, + pid: u32, + stdout_task: &mut OutputTask, + stdout_finished: bool, + stderr_task: &mut OutputTask, + stderr_finished: bool, +) { + terminate_tokio_process_tree(child, pid).await; + abort_output_task(stdout_task, stdout_finished).await; + abort_output_task(stderr_task, stderr_finished).await; +} + +async fn abort_output_task(task: &mut OutputTask, finished: bool) { + if !finished { + task.abort(); + let _ = task.await; } - let _ = child.kill().await; } async fn read_output(mut reader: impl tokio::io::AsyncRead + Unpin) -> std::io::Result { @@ -224,11 +300,68 @@ const fn default_timeout() -> u64 { #[cfg(all(test, unix))] mod tests { - use std::time::Duration; + use std::{sync::Arc, time::Duration}; + use agentkit_core::{MetadataMap, SessionId, TurnId}; + use agentkit_tools_core::{ + AllowAllPermissions, OwnedToolContext, Tool as _, ToolError, ToolRequest, + }; + use serde_json::json; use tokio::io::AsyncWriteExt as _; - use super::{isolate_process_tree, kill_process_tree, read_output, shell_command}; + use super::{MAX_INTERNAL_OUTPUT_BYTES, ShellTool, read_output, shell_command}; + use crate::process_tree::{isolate_tokio_process_tree, terminate_tokio_process_tree}; + + async fn invoke_shell( + root: &std::path::Path, + command: &str, + timeout_seconds: u64, + ) -> Result<(), ToolError> { + let tool = ShellTool::new(root.to_path_buf()); + let context = OwnedToolContext { + session_id: SessionId::new("session"), + turn_id: TurnId::new("turn"), + metadata: MetadataMap::new(), + permissions: Arc::new(AllowAllPermissions), + resources: Arc::new(()), + cancellation: None, + execution_scope: None, + approved_request: None, + }; + let request = ToolRequest::new( + "call", + "shell", + json!({ + "command": command, + "timeout_seconds": timeout_seconds, + }), + "session", + "turn", + ); + tool.invoke(request, &mut context.borrowed()) + .await + .map(|_| ()) + } + + fn assert_output_limit(error: ToolError) { + let ToolError::ExecutionFailed(message) = error else { + panic!("unexpected shell error: {error}"); + }; + assert_eq!( + message, + format!("shell output exceeds {MAX_INTERNAL_OUTPUT_BYTES} bytes") + ); + } + + async fn assert_process_exited(pid: i32) { + for _ in 0..100 { + if unsafe { libc::kill(pid, 0) } != 0 { + return; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + panic!("process {pid} survived shell termination"); + } #[tokio::test] async fn oversized_output_stays_complete() { @@ -253,6 +386,59 @@ mod tests { assert_eq!(captured, "small output"); } + #[tokio::test] + async fn stdout_output_limit_terminates_shell() { + let directory = tempfile::tempdir().unwrap(); + let error = tokio::time::timeout( + Duration::from_secs(10), + invoke_shell(directory.path(), "yes stdout", 5), + ) + .await + .expect("stdout output limit did not stop the shell") + .unwrap_err(); + + assert_output_limit(error); + } + + #[tokio::test] + async fn stderr_output_limit_terminates_shell() { + let directory = tempfile::tempdir().unwrap(); + let error = tokio::time::timeout( + Duration::from_secs(10), + invoke_shell(directory.path(), "yes stderr >&2", 5), + ) + .await + .expect("stderr output limit did not stop the shell") + .unwrap_err(); + + assert_output_limit(error); + } + + #[tokio::test] + async fn parent_exit_with_descendant_pipe_times_out_and_kills_descendant() { + let directory = tempfile::tempdir().unwrap(); + let pid_file = directory.path().join("descendant.pid"); + let command = format!("sleep 30 & echo $! > {}", pid_file.display()); + let error = tokio::time::timeout( + Duration::from_secs(5), + invoke_shell(directory.path(), &command, 1), + ) + .await + .expect("descendant pipe kept the shell invocation alive") + .unwrap_err(); + let ToolError::ExecutionFailed(message) = error else { + panic!("unexpected shell error: {error}"); + }; + assert_eq!(message, "shell command timed out"); + let descendant = std::fs::read_to_string(pid_file) + .unwrap() + .trim() + .parse() + .unwrap(); + + assert_process_exited(descendant).await; + } + #[tokio::test] async fn cancellation_kills_shell_descendants() { let directory = tempfile::tempdir().unwrap(); @@ -261,8 +447,9 @@ mod tests { "sleep 30 & echo $! > {}; wait", pid_file.display() )); - isolate_process_tree(&mut command); + isolate_tokio_process_tree(&mut command); let mut child = command.spawn().unwrap(); + let pid = child.id().unwrap(); let descendant = tokio::time::timeout(Duration::from_secs(1), async { loop { if let Ok(contents) = std::fs::read_to_string(&pid_file) @@ -276,14 +463,8 @@ mod tests { .await .expect("shell did not report its descendant PID"); - kill_process_tree(&mut child).await; + terminate_tokio_process_tree(&mut child, pid).await; - for _ in 0..100 { - if unsafe { libc::kill(descendant, 0) } != 0 { - return; - } - tokio::time::sleep(Duration::from_millis(10)).await; - } - panic!("shell descendant {descendant} survived cancellation"); + assert_process_exited(descendant).await; } }