diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 3b3c5039..c2be4da3 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -26,5 +26,13 @@ jobs: if: runner.os == 'Linux' run: cargo fmt --check + # The secret-scan confinement tests skip without a working bubblewrap. + - name: Install bubblewrap + if: runner.os == 'Linux' + run: | + sudo apt-get update + sudo apt-get install -y bubblewrap apparmor-profiles + sudo apparmor_parser -r /usr/share/apparmor/extra-profiles/bwrap-userns-restrict + - name: Run tests run: cargo test --locked diff --git a/docs/agent-profiles.md b/docs/agent-profiles.md index 29a7fd06..1653beb4 100644 --- a/docs/agent-profiles.md +++ b/docs/agent-profiles.md @@ -165,15 +165,17 @@ allow_tools = ["search_issues", "get_issue"] The HTTP rule controls credential injection at the endpoint. The MCP rule controls which tools the agent may call. Omit `allow_tools` or leave it empty to deny all tools by default. Use `allow_tools = ["*"]` to allow all tools (a matching `deny_tools` still takes precedence). -## Dependency Hooks +## API Hooks -If your profile enables authenticated hooks (e.g., dependency checking), grant capability explicitly: +Hooks that call the Stashbase API (dependency checking, secret scanning) must be granted explicitly: ```toml -allow_hooks = ["dependency_check"] +allow_hooks = ["dependency_check", "secret_scan"] ``` -The child receives only a scoped local broker token. Hooks are disabled by default. +The child receives only a scoped local broker token, never your API key. Hooks are disabled by default. + +`secret_scan` lets the git hooks from `stashbase scan install` run inside the sandbox. The hook sends only a token to the Agent Proxy, which runs `stashbase scan staged` (or `unpushed`) on the host, in the run's working directory, with your key. Because the agent can edit the repo's scan config, these scans ignore its `match` and `output-dir` settings; `excluded-files` and `ignored-secrets` still apply. The host-side scan is confined, so a symlink or a `.git` redirect planted by the agent can't point it at your other files. On Linux it sees only system files plus the run's working tree, git directories and the CLI binary. On macOS it can't read file contents anywhere users' data lives (home directories, `/Volumes`, `/tmp`, `/var/folders`, `/opt`) except those same run paths; system files stay readable, and file names and sizes stay visible because path lookup needs them. The profile's `deny_read` entries stay denied on both. This uses Seatbelt on macOS and bubblewrap on Linux; where neither is available (including Windows), a profile with `secret_scan` refuses to start. Restricted scans also stop at 1 MiB per changed file, 32 MiB in total, 10,000 files or 1,000 commits; larger changes have to be committed from outside the sandbox. The hook needs `curl` in the sandbox (the default Docker image has it). Hooks installed before this existed need `stashbase scan install` once more to pick up the sandbox-aware block. Without `secret_scan`, a scan hook inside the sandbox fails with a message pointing here. ## Audit Logs and Session Revocation diff --git a/docs/sandboxing.md b/docs/sandboxing.md index fc399807..512ececd 100644 --- a/docs/sandboxing.md +++ b/docs/sandboxing.md @@ -111,6 +111,10 @@ stashbase agent run --profile coding --docker-image node:22-alpine -- claude Your global `git config user.name` and `user.email` (if configured on the host) are forwarded into the container as `GIT_AUTHOR_NAME`, `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME`, and `GIT_COMMITTER_EMAIL`. This is the one piece of host configuration deliberately forwarded despite the filesystem allow-list, since it's authorship metadata, not a credential — without it, `git commit` inside the sandbox fails with no identity configured. It does not grant push access: `git push` (or any other authenticated git operation) still needs a real credential, wired through `[secrets]` like `GITHUB_TOKEN`, or run from outside the sandbox. Raw SSH keys are never forwarded. A profile that explicitly sets one of these four env vars itself takes precedence over the forwarded host value. +### Secret scan hooks + +Git hooks installed with `stashbase scan install` work in the sandbox when the profile sets `allow_hooks = ["secret_scan"]`. The sandbox has neither the Stashbase CLI nor your API key, so the hook asks the Agent Proxy to run the scan on the host against the same working directory, with `curl` and a per-run token. Findings come back to the agent, and the commit or push is blocked, exactly as outside the sandbox. The scan itself runs confined (Seatbelt on macOS, bubblewrap on Linux): outside system files, it can read only the run's working tree, its git directories and the CLI binary, never your other files that the agent points it at through symlinks or `.git` redirects; `secret_scan` is unavailable on Windows for that reason. Like any git hook, it is skipped by `git commit --no-verify` — a safety net, not an enforcement boundary. See [Agent Profiles](agent-profiles.md#api-hooks). + ### Notifications, herdr and cmux Agent notifications work in the sandbox with no setup. Your terminal's identity (`TERM`, `COLORTERM`, `TERM_PROGRAM`, `TERM_PROGRAM_VERSION`, `LC_TERMINAL`) is forwarded into the container, so Claude Code and Codex send the same notifications they would outside it (OSC 9/777/99 or the bell) when a turn finishes or they need input. Ghostty, iTerm2, kitty, [cmux](https://cmux.com), [herdr](https://herdr.dev), tmux and other terminals and multiplexers pick them up as usual. Only the terminal's name and version are forwarded; nothing else about your terminal or session reaches the container. diff --git a/src/api/client.rs b/src/api/client.rs index 2d638262..7f4187f8 100644 --- a/src/api/client.rs +++ b/src/api/client.rs @@ -19,7 +19,7 @@ use crate::models::api_client::{ use crate::{REQUEST_ABORTED, REQUEST_TIMEOUT_SECS}; const DEFAULT_API_URL: &str = "https://api.stashbase.dev"; -const API_URL_ENV_VAR: &str = "STASHBASE_API_URL"; +pub(crate) const API_URL_ENV_VAR: &str = "STASHBASE_API_URL"; const BUILD_TIME_API_URL: Option<&str> = option_env!("STASHBASE_API_URL"); pub const CLI_USER_AGENT: &str = concat!("stashbase/cli/", env!("CARGO_PKG_VERSION")); diff --git a/src/handlers/agent/init.rs b/src/handlers/agent/init.rs index b9dc027a..05998008 100644 --- a/src/handlers/agent/init.rs +++ b/src/handlers/agent/init.rs @@ -14,8 +14,8 @@ const PROFILE_TEMPLATE: &str = r#"# Stashbase Agent Proxy profile. # Add only destinations the agent genuinely needs to contact. egress_hosts = [] -# Enable installed dependency hooks for this profile. -# allow_hooks = ["dependency_check"] +# Enable installed dependency and secret-scan hooks for this profile. +# allow_hooks = ["dependency_check", "secret_scan"] # Optional: allow local test servers and Unix-socket IPC (macOS only). # Linux keeps loopback available for the embedded proxy; this setting has no effect there. @@ -126,7 +126,9 @@ mod tests { fn template_starts_closed_and_includes_a_generic_rule() { assert!(PROFILE_TEMPLATE.contains("egress_hosts = []")); assert!(PROFILE_TEMPLATE.contains("allow_network_listeners = true")); - assert!(PROFILE_TEMPLATE.contains("# allow_hooks = [\"dependency_check\"]")); + assert!( + PROFILE_TEMPLATE.contains("# allow_hooks = [\"dependency_check\", \"secret_scan\"]") + ); assert!(PROFILE_TEMPLATE.contains("[secrets.SECRET_NAME]")); assert!(PROFILE_TEMPLATE.contains("[[secrets.SECRET_NAME.rules]]")); } diff --git a/src/handlers/agent/validate.rs b/src/handlers/agent/validate.rs index 44f6e6de..73d531f1 100644 --- a/src/handlers/agent/validate.rs +++ b/src/handlers/agent/validate.rs @@ -696,7 +696,7 @@ fn validate_hook_capabilities(profile: &AgentProfile) -> Vec { let unsupported = profile .allow_hooks .iter() - .filter(|hook| hook.as_str() != "dependency_check") + .filter(|hook| !matches!(hook.as_str(), "dependency_check" | "secret_scan")) .collect::>(); if !unsupported.is_empty() { return unsupported @@ -709,6 +709,11 @@ fn validate_hook_capabilities(profile: &AgentProfile) -> Vec { }) .collect(); } + if profile.allow_hooks.iter().any(|hook| hook == "secret_scan") { + if let Some(reason) = crate::handlers::run::scan_sandbox::unavailable_reason() { + return vec![fail("Hook capability", format!("{reason}."))]; + } + } vec![ok( "Hook capabilities", if profile.allow_hooks.is_empty() { @@ -1132,6 +1137,32 @@ mod tests { .contains("Unsupported hook capability 'anything_else'")); } + #[test] + fn secret_scan_is_a_known_hook() { + let profile = AgentProfile { + file: None, + egress_hosts: None, + allow_network_listeners: false, + deny_hosts: None, + filesystem: Default::default(), + sandbox: Default::default(), + workspace: Default::default(), + mcp_servers: HashMap::new(), + secrets: HashMap::new().into(), + personal_credentials: HashMap::new(), + policy_tests: Vec::new(), + allow_hooks: vec!["dependency_check".to_owned(), "secret_scan".to_owned()], + }; + + let checks = validate_hook_capabilities(&profile); + + let confinable = crate::handlers::run::scan_sandbox::unavailable_reason().is_none(); + assert_eq!( + checks.iter().all(|check| check.status != Status::Fail), + confinable + ); + } + #[test] fn docker_backend_profile_gets_a_docker_runtime_check_not_native_ones() { // A Docker-backend profile never touches Seatbelt/systemd-run/ diff --git a/src/handlers/entry/root.rs b/src/handlers/entry/root.rs index ad779ca8..ad976c27 100644 --- a/src/handlers/entry/root.rs +++ b/src/handlers/entry/root.rs @@ -878,11 +878,8 @@ pub async fn handle_cli(args: Cli) -> Exit { } } - let dependency_hooks_requested = profile - .allow_hooks - .iter() - .any(|hook| hook == "dependency_check"); - let dependency_hooks = dependency_hooks_enabled(&profile, &api_key); + let requested_hooks = profile.requested_hooks(); + let hooks = enabled_hooks(&profile, &api_key); if !silent { eprintln!("Network sandbox: enabled"); if profile.sandbox.backend @@ -891,17 +888,10 @@ pub async fn handle_cli(args: Cli) -> Exit { eprintln!("Sandbox backend: Docker (container-isolated)"); } print_agent_egress_warnings(&profile); - eprintln!( - "API hook broker: {}", - if dependency_hooks { - "enabled (dependency_check)" - } else { - "disabled" - } - ); - if dependency_hooks_requested && !dependency_hooks { + eprintln!("API hook broker: {}", hooks.label()); + if requested_hooks.any() && !hooks.any() { eprintln!( - "Warning: dependency_check is configured but no API key is available; dependency checks are disabled." + "Warning: allow_hooks is configured but no API key is available; API hooks are disabled." ); } } @@ -1314,7 +1304,7 @@ pub async fn handle_cli(args: Cli) -> Exit { ); let result = handle_remote_agent_run( api_key.clone(), - dependency_hooks, + hooks, command, policy, crate::handlers::run::proxy::RemoteProxyConfig { proxy_url, session: remote_session.clone(), placeholders, child_env, protocol, ca_file: remote_ca_file, routing }, @@ -1362,7 +1352,7 @@ pub async fn handle_cli(args: Cli) -> Exit { set_comments: Vec::new(), print_secrets: None, no_print_secrets: true, - dependency_hooks, + hooks, local_session, config_file: None, file: profile.file, @@ -1422,7 +1412,7 @@ pub async fn handle_cli(args: Cli) -> Exit { json_format: raw_output, silent, scope: run_cmd.scope, - dependency_hooks: false, + hooks: crate::models::agent::EnabledHooks::default(), local_session: None, }; @@ -1590,12 +1580,15 @@ fn uses_local_dependency_hook_broker(entity_type: &EntityType) -> bool { ) } -fn dependency_hooks_enabled(profile: &crate::models::agent::AgentProfile, api_key: &str) -> bool { - !api_key.is_empty() - && profile - .allow_hooks - .iter() - .any(|hook| hook == "dependency_check") +fn enabled_hooks( + profile: &crate::models::agent::AgentProfile, + api_key: &str, +) -> crate::models::agent::EnabledHooks { + if api_key.is_empty() { + crate::models::agent::EnabledHooks::default() + } else { + profile.requested_hooks() + } } fn uses_local_dependency_hook_broker_mode(entity_type: &EntityType, mode: Option<&str>) -> bool { @@ -2379,12 +2372,11 @@ fn spawn_remote_session_rotation( mod tests { use super::{ audit_binding_sources, codex_mcp_binding_header_overrides, configured_host_matches, - dependency_hooks_enabled, directory_profile_git_warning, - ensure_replacement_session_is_compatible, exit_for, infer_remote_agent_type, - is_bare_agent_hook, remote_bindings, remote_codex_command_with_mcp_binding_headers, - remote_session_rotation_delay_for, remote_session_started_message, - remote_session_transport_identity, remote_source_env_names, secret_child_name, - summarize_audit_events, uses_local_dependency_hook_broker_mode, + directory_profile_git_warning, enabled_hooks, ensure_replacement_session_is_compatible, + exit_for, infer_remote_agent_type, is_bare_agent_hook, remote_bindings, + remote_codex_command_with_mcp_binding_headers, remote_session_rotation_delay_for, + remote_session_started_message, remote_session_transport_identity, remote_source_env_names, + secret_child_name, summarize_audit_events, uses_local_dependency_hook_broker_mode, }; use crate::api::remote_proxy::{RemoteBinding, RemoteBindingSource}; use crate::cmd::root::Cli; @@ -2488,8 +2480,33 @@ mod tests { "allow_hooks": ["dependency_check"] })) .unwrap(); - assert!(!dependency_hooks_enabled(&profile, "")); - assert!(dependency_hooks_enabled(&profile, "key")); + assert!(!enabled_hooks(&profile, "").dependency_check); + assert!(enabled_hooks(&profile, "key").dependency_check); + } + + #[test] + fn enabled_hooks_reads_both_hook_kinds_and_needs_an_api_key() { + let profile_with = |hooks: &[&str]| -> AgentProfile { + serde_json::from_value(serde_json::json!({ + "file": null, + "egress_hosts": null, + "deny_hosts": null, + "allow_hooks": hooks + })) + .unwrap() + }; + + let hooks = enabled_hooks(&profile_with(&["secret_scan"]), "key"); + assert!(hooks.secret_scan && !hooks.dependency_check); + assert_eq!(hooks.label(), "enabled (secret_scan)"); + assert_eq!( + enabled_hooks(&profile_with(&["dependency_check", "secret_scan"]), "key").label(), + "enabled (dependency_check, secret_scan)" + ); + assert_eq!( + enabled_hooks(&profile_with(&["secret_scan"]), "").label(), + "disabled" + ); } #[test] diff --git a/src/handlers/run/docker_sandbox.rs b/src/handlers/run/docker_sandbox.rs index 99b59161..107007e6 100644 --- a/src/handlers/run/docker_sandbox.rs +++ b/src/handlers/run/docker_sandbox.rs @@ -512,6 +512,7 @@ const PROXY_URL_ENV_KEYS: &[&str] = &[ "http_proxy", "https_proxy", crate::api::dependencies::HOOK_BROKER_URL_ENV, + crate::handlers::run::proxy::SCAN_BROKER_URL_ENV, ]; /// Rewrites the proxy's child-process env vars so the container reaches the @@ -1694,6 +1695,23 @@ mod tests { assert_eq!(rewritten["STASHBASE_GH_TOKEN"], "127.0.0.1"); } + #[test] + fn rewrite_proxy_urls_rewrites_the_scan_broker_url() { + let mut env_vars = std::collections::HashMap::new(); + env_vars.insert( + crate::handlers::run::proxy::SCAN_BROKER_URL_ENV.to_owned(), + "http://127.0.0.1:9999/__stashbase/scan".to_owned(), + ); + + let rewritten = + rewrite_proxy_urls_for_container(&env_vars, "127.0.0.1", "host.docker.internal"); + + assert_eq!( + rewritten[crate::handlers::run::proxy::SCAN_BROKER_URL_ENV], + "http://host.docker.internal:9999/__stashbase/scan" + ); + } + #[test] fn rewrite_proxy_urls_is_a_no_op_when_hosts_match() { let mut env_vars = std::collections::HashMap::new(); diff --git a/src/handlers/run/entry.rs b/src/handlers/run/entry.rs index 955704aa..42cc9760 100644 --- a/src/handlers/run/entry.rs +++ b/src/handlers/run/entry.rs @@ -380,11 +380,63 @@ fn finish_run_worktree(worktree: &super::worktree::RunWorktree, docker: bool, si } } +/// What the proxy's hook broker serves for this run, or `None` when no hook +/// is enabled or there is no API key to call Stashbase with. Fails when +/// `secret_scan` is enabled but its host-side scan can't be confined here. +fn hook_broker_config( + hooks: crate::models::agent::EnabledHooks, + api_key: Option, + workdir: &Path, + denied_read: &[String], +) -> anyhow::Result> { + let Some(api_key) = api_key.filter(|key| !key.is_empty()) else { + return Ok(None); + }; + if !hooks.any() { + return Ok(None); + } + // The scan runs the host's own CLI in the directory the agent works in. + let secret_scan = if hooks.secret_scan { + if let Some(reason) = super::scan_sandbox::unavailable_reason() { + anyhow::bail!(reason); + } + let exe = std::env::current_exe().map_err(|error| { + anyhow::anyhow!("secret_scan cannot locate the stashbase binary: {error}") + })?; + Some(super::proxy::SecretScanConfig { + workdir: workdir.to_owned(), + isolation: super::proxy::ScanIsolation::Confined( + super::scan_sandbox::ScanConfinement::for_run(workdir, &exe, denied_read), + ), + timeout: std::time::Duration::from_secs(120), + }) + } else { + None + }; + Ok(Some(super::proxy::HookBrokerConfig { + api_key, + dependency_check: hooks.dependency_check, + secret_scan, + })) +} + +/// Checked before any sandbox setup, so a refusal leaves nothing to clean up. +fn ensure_secret_scan_can_be_confined( + hooks: crate::models::agent::EnabledHooks, +) -> anyhow::Result<()> { + if hooks.secret_scan { + if let Some(reason) = super::scan_sandbox::unavailable_reason() { + anyhow::bail!(reason); + } + } + Ok(()) +} + /// Runs an agent through the localhost relay while credentials stay in the /// control-plane's short-lived remote agent-proxy session. pub async fn handle_remote_agent_run( api_key: String, - hooks_enabled: bool, + hooks: crate::models::agent::EnabledHooks, command: Vec, policy: super::proxy::ProxyPolicy, remote: super::proxy::RemoteProxyConfig, @@ -395,6 +447,7 @@ pub async fn handle_remote_agent_run( source_env_names: Vec, silent: bool, ) -> anyhow::Result<()> { + ensure_secret_scan_can_be_confined(hooks)?; let cmd = command.first().context("no command provided")?.clone(); let args = command.into_iter().skip(1).collect(); let denied_read_paths = policy.denied_read_paths.clone(); @@ -442,13 +495,18 @@ pub async fn handle_remote_agent_run( )?; (None, String::new()) }; + let scan_workdir = match run_worktree.workdir() { + Some(workdir) => workdir.to_owned(), + None => std::env::current_dir()?, + }; + let remote_hooks = hook_broker_config(hooks, Some(api_key), &scan_workdir, &denied_read_paths)?; let proxy_start_result = if let Some(network) = &docker_network { super::proxy::Proxy::start_remote_with_hook_and_bind_host( remote, policy, audit_log, proxy_port, - hooks_enabled.then_some(api_key), + remote_hooks, &super::docker_sandbox::proxy_bind_host(network), ) .await @@ -458,7 +516,7 @@ pub async fn handle_remote_agent_run( policy, audit_log, proxy_port, - hooks_enabled.then_some(api_key), + remote_hooks, ) .await }; @@ -620,7 +678,7 @@ pub struct HandleRunArgs { pub json_format: bool, pub silent: bool, pub scope: Option, - pub dependency_hooks: bool, + pub hooks: crate::models::agent::EnabledHooks, pub local_session: Option, } @@ -655,7 +713,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { json_format, silent, scope, - dependency_hooks, + hooks, local_session, } = args; @@ -715,8 +773,10 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { false, silent, json_format, - false, - None, + // No secrets to fetch, but the hook broker still calls Stashbase + // on the host with the parent's key. + hooks, + Some(api_key.clone()), local_session, ) .await; @@ -1166,7 +1226,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { is_from_file, silent, json_format, - dependency_hooks, + hooks, Some(api_key.clone()), local_session, ) @@ -1209,7 +1269,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { false, silent, json_format, - dependency_hooks, + hooks, Some(api_key.clone()), local_session, ) @@ -1369,7 +1429,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { is_from_file, silent, json_format, - dependency_hooks, + hooks, Some(api_key.clone()), local_session, ) @@ -1404,7 +1464,7 @@ pub async fn handle_load_env_run(args: HandleRunArgs) -> anyhow::Result<()> { is_from_file, silent, json_format, - dependency_hooks, + hooks, Some(api_key.clone()), local_session, ) @@ -1517,10 +1577,11 @@ async fn handle_run( is_from_file: bool, silent: bool, json_format: bool, - dependency_hooks: bool, + hooks: crate::models::agent::EnabledHooks, hook_api_key: Option, local_session: Option, ) -> anyhow::Result<()> { + ensure_secret_scan_can_be_confined(hooks)?; apply_secret_bindings(&mut secrets, secret_bindings); let secrets_hash_map = if secret_bindings.is_empty() { env::expand_and_inject_env(&mut secrets) @@ -1701,13 +1762,19 @@ async fn handle_run( )?; (None, String::new()) }; + let scan_workdir = match run_worktree.workdir() { + Some(workdir) => workdir.to_owned(), + None => std::env::current_dir()?, + }; + let local_hooks = + hook_broker_config(hooks, hook_api_key, &scan_workdir, &denied_read_paths)?; let proxy_start_result = if let Some(network) = &docker_network { super::proxy::Proxy::start_with_hook_and_bind_host( secrets_hash_map, proxy_policy.unwrap_or_else(super::proxy::ProxyPolicy::permissive), audit_log, proxy_port, - dependency_hooks.then_some(hook_api_key).flatten(), + local_hooks, &super::docker_sandbox::proxy_bind_host(network), ) .await @@ -1717,7 +1784,7 @@ async fn handle_run( proxy_policy.unwrap_or_else(super::proxy::ProxyPolicy::permissive), audit_log, proxy_port, - dependency_hooks.then_some(hook_api_key).flatten(), + local_hooks, ) .await }; @@ -1981,11 +2048,11 @@ fn missing_secret_labels( mod tests { use super::{ apply_secret_bindings, can_prompt_for_image_build, declined_image_build_error, - load_run_secrets_from_file, loaded_message, loading_message, + hook_broker_config, load_run_secrets_from_file, loaded_message, loading_message, merge_remote_and_local_secrets, missing_image_build_command, missing_image_error, missing_secret_labels, needs_remote_fetch, prepare_local_run_secrets, shell_quote, }; - use crate::models::secrets::SecretWithoutComment; + use crate::models::{agent::EnabledHooks, secrets::SecretWithoutComment}; use std::{ collections::HashMap, fs, @@ -1993,6 +2060,64 @@ mod tests { time::{SystemTime, UNIX_EPOCH}, }; + #[test] + fn hook_broker_config_scans_the_run_workdir() { + let workdir = std::path::Path::new("/repo/.stashbase/worktrees/feat"); + let scan_only = EnabledHooks { + dependency_check: false, + secret_scan: true, + }; + + let result = hook_broker_config(scan_only, Some("k".to_owned()), workdir, &[]); + + if let Some(reason) = crate::handlers::run::scan_sandbox::unavailable_reason() { + // Refused up front rather than run unconfined. + let Err(error) = result else { + panic!("secret_scan ran without confinement"); + }; + assert_eq!(error.to_string(), reason); + return; + } + let config = result.unwrap().unwrap(); + assert!(!config.dependency_check); + let scan = config.secret_scan.unwrap(); + assert_eq!(scan.workdir, workdir); + assert_eq!(scan.timeout, std::time::Duration::from_secs(120)); + assert!(matches!( + scan.isolation, + crate::handlers::run::proxy::ScanIsolation::Confined(_) + )); + } + + #[test] + fn hook_broker_config_needs_a_hook_and_an_api_key() { + let workdir = std::path::Path::new("/repo"); + let both = EnabledHooks { + dependency_check: true, + secret_scan: true, + }; + + assert!( + hook_broker_config(EnabledHooks::default(), Some("k".to_owned()), workdir, &[]) + .unwrap() + .is_none() + ); + assert!(hook_broker_config(both, None, workdir, &[]) + .unwrap() + .is_none()); + assert!(hook_broker_config(both, Some(String::new()), workdir, &[]) + .unwrap() + .is_none()); + let deps_only = EnabledHooks { + dependency_check: true, + secret_scan: false, + }; + let config = hook_broker_config(deps_only, Some("k".to_owned()), workdir, &[]) + .unwrap() + .unwrap(); + assert!(config.dependency_check && config.secret_scan.is_none()); + } + #[test] fn missing_default_image_points_to_agent_docker_build() { let source = crate::handlers::run::docker_sandbox::AgentImageSource::Default; diff --git a/src/handlers/run/mod.rs b/src/handlers/run/mod.rs index c1ab0a0b..0c0a9069 100644 --- a/src/handlers/run/mod.rs +++ b/src/handlers/run/mod.rs @@ -4,6 +4,7 @@ pub mod format; pub mod fs_rules; pub mod proxy; pub mod routing; +pub mod scan_sandbox; pub mod subprocess; pub mod trust; pub mod worktree; diff --git a/src/handlers/run/proxy.rs b/src/handlers/run/proxy.rs index b2fea6bf..0371273b 100644 --- a/src/handlers/run/proxy.rs +++ b/src/handlers/run/proxy.rs @@ -84,6 +84,75 @@ const AUDIT_LOG_RETENTION: Duration = Duration::from_secs(30 * 24 * 60 * 60); const AUDIT_LOG_MAX_FILES: usize = 1_000; const MCP_INSPECTION_HEADER: &str = "x-stashbase-mcp-inspection"; const DEPENDENCY_HOOK_PATH: &str = "/__stashbase/dependency-check"; +const SECRET_SCAN_HOOK_PATH: &str = "/__stashbase/scan"; +/// Where a sandboxed git hook asks the proxy to run `stashbase scan` on the host. +pub const SCAN_BROKER_URL_ENV: &str = "STASHBASE_SCAN_BROKER_URL"; +const SECRET_SCAN_BODY_LIMIT: usize = 1024 * 1024; + +/// Authenticated hooks the proxy serves on the parent's behalf, so the child +/// never holds the Stashbase API key. +pub struct HookBrokerConfig { + pub api_key: String, + pub dependency_check: bool, + /// Enables the secret-scan route. + pub secret_scan: Option, +} + +pub struct SecretScanConfig { + /// The run's working directory, scanned on the host. + pub workdir: PathBuf, + pub timeout: Duration, + pub isolation: ScanIsolation, +} + +pub enum ScanIsolation { + /// The scan reads an agent-controlled repository, so it runs with roughly + /// the agent's own view of the host filesystem. + Confined(super::scan_sandbox::ScanConfinement), + /// Runs `exe` directly, for tests of the route itself. + #[cfg(test)] + Unconfined { exe: PathBuf }, +} + +/// An empty home for one scan, so it reads none of the user's dotfiles +/// (git's global config, the CLI's own config) and can still write. +struct ScratchHome(PathBuf); + +impl ScratchHome { + fn create() -> std::io::Result { + let path = std::env::temp_dir().join(format!("stashbase-scan-home-{}", Uuid::new_v4())); + std::fs::create_dir_all(&path)?; + Ok(Self(path)) + } +} + +impl Drop for ScratchHome { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +struct HookBroker { + token: String, + client: reqwest::Client, + api_key: String, + dependency_check: bool, + secret_scan: Option, + // One scan at a time: concurrent hooks would race on the same index. + scan_lock: tokio::sync::Mutex<()>, +} + +impl HookBroker { + fn authorized(&self, request: &Request) -> bool { + request.method() == Method::POST + && request.uri().query().is_none() + && request + .headers() + .get("authorization") + .and_then(|value| value.to_str().ok()) + .is_some_and(|value| value == format!("Bearer {}", self.token)) + } +} /// One metadata-only event emitted by the local proxy audit log. #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, Hash)] @@ -1014,8 +1083,7 @@ struct ProxyState { connections: Arc, remote: Option, mcp_inspection_token: String, - dependency_hook_token: Option, - dependency_hook_client: Option, + hook_broker: Option>, revocation_path: Arc>>, } @@ -1117,7 +1185,7 @@ impl Proxy { policy: ProxyPolicy, audit_log: Option, proxy_port: Option, - api_key: Option, + hooks: Option, ) -> Result { Self::start_inner( secrets, @@ -1125,7 +1193,7 @@ impl Proxy { audit_log, proxy_port, None, - api_key, + hooks, true, "127.0.0.1", ) @@ -1141,11 +1209,11 @@ impl Proxy { policy: ProxyPolicy, audit_log: Option, proxy_port: Option, - api_key: Option, + hooks: Option, bind_host: &str, ) -> Result { Self::start_inner( - secrets, policy, audit_log, proxy_port, None, api_key, true, bind_host, + secrets, policy, audit_log, proxy_port, None, hooks, true, bind_host, ) .await } @@ -1175,7 +1243,7 @@ impl Proxy { policy: ProxyPolicy, audit_log: Option, proxy_port: Option, - api_key: Option, + hooks: Option, ) -> Result { let placeholders = remote.placeholders.clone(); Self::start_inner( @@ -1184,7 +1252,7 @@ impl Proxy { audit_log, proxy_port, Some(remote), - api_key, + hooks, true, "127.0.0.1", ) @@ -1198,7 +1266,7 @@ impl Proxy { policy: ProxyPolicy, audit_log: Option, proxy_port: Option, - api_key: Option, + hooks: Option, bind_host: &str, ) -> Result { let placeholders = remote.placeholders.clone(); @@ -1208,7 +1276,7 @@ impl Proxy { audit_log, proxy_port, Some(remote), - api_key, + hooks, true, bind_host, ) @@ -1221,7 +1289,7 @@ impl Proxy { audit_log: Option, proxy_port: Option, remote: Option, - hook_api_key: Option, + hooks: Option, hook_mode_set: bool, bind_host: &str, ) -> Result { @@ -1333,16 +1401,22 @@ impl Proxy { connections: connections.clone(), remote, mcp_inspection_token: Uuid::new_v4().to_string(), - dependency_hook_token: hook_api_key.as_ref().map(|_| Uuid::new_v4().to_string()), - dependency_hook_client: hook_api_key.as_ref().map(|api_key| { - reqwest::Client::builder() - .no_proxy() - .default_headers(reqwest::header::HeaderMap::from_iter([( - reqwest::header::AUTHORIZATION, - format!("Bearer {api_key}").parse().unwrap(), - )])) - .build() - .expect("dependency hook client must build") + hook_broker: hooks.map(|hooks| { + Arc::new(HookBroker { + token: Uuid::new_v4().to_string(), + client: reqwest::Client::builder() + .no_proxy() + .default_headers(reqwest::header::HeaderMap::from_iter([( + reqwest::header::AUTHORIZATION, + format!("Bearer {}", hooks.api_key).parse().unwrap(), + )])) + .build() + .expect("hook broker client must build"), + api_key: hooks.api_key, + dependency_check: hooks.dependency_check, + secret_scan: hooks.secret_scan, + scan_lock: tokio::sync::Mutex::new(()), + }) }), revocation_path: revocation_path.clone(), }; @@ -1386,21 +1460,29 @@ impl Proxy { if hook_mode_set { child_env.insert( crate::api::dependencies::HOOK_MODE_ENV.to_owned(), - if hook_api_key.is_some() { + if state.hook_broker.is_some() { "broker" } else { "disabled" } .to_owned(), ); - if let (Some(token), Some(_)) = (&state.dependency_hook_token, &hook_api_key) { - child_env.insert( - crate::api::dependencies::HOOK_BROKER_URL_ENV.to_owned(), - format!("http://{address}{DEPENDENCY_HOOK_PATH}"), - ); + if let Some(broker) = &state.hook_broker { + if broker.dependency_check { + child_env.insert( + crate::api::dependencies::HOOK_BROKER_URL_ENV.to_owned(), + format!("http://{address}{DEPENDENCY_HOOK_PATH}"), + ); + } + if broker.secret_scan.is_some() { + child_env.insert( + SCAN_BROKER_URL_ENV.to_owned(), + format!("http://{address}{SECRET_SCAN_HOOK_PATH}"), + ); + } child_env.insert( crate::api::dependencies::HOOK_BROKER_TOKEN_ENV.to_owned(), - token.clone(), + broker.token.clone(), ); } } @@ -1826,6 +1908,14 @@ fn proxy_request( if request.uri().path() == DEPENDENCY_HOOK_PATH { return Ok(handle_dependency_hook(request, &state).await); } + if let Some(mode) = request + .uri() + .path() + .strip_prefix(SECRET_SCAN_HOOK_PATH) + .map(str::to_owned) + { + return Ok(handle_secret_scan_hook(request, &mode, &state, started).await); + } let request_id = new_local_request_id(); let host = request_host(&request, connect_authority.as_deref()); @@ -2350,29 +2440,18 @@ async fn handle_dependency_hook( request: Request, state: &ProxyState, ) -> Response { - let authorized = request.method() == Method::POST - && request.uri().query().is_none() - && state.dependency_hook_token.as_ref().is_some_and(|token| { - request - .headers() - .get("authorization") - .and_then(|value| value.to_str().ok()) - .is_some_and(|value| value == format!("Bearer {token}")) - }); - if !authorized { - return proxy_error_response( - StatusCode::FORBIDDEN, - "proxy.dependency_hook_not_allowed", - "Dependency hook route is not enabled for this run", - ); - } - let Some(client) = &state.dependency_hook_client else { + let Some(broker) = state + .hook_broker + .as_ref() + .filter(|broker| broker.dependency_check && broker.authorized(&request)) + else { return proxy_error_response( StatusCode::FORBIDDEN, "proxy.dependency_hook_not_allowed", "Dependency hook route is not enabled for this run", ); }; + let client = &broker.client; let body = match request.into_body().collect().await { Ok(body) => body.to_bytes(), Err(_) => { @@ -2421,6 +2500,163 @@ async fn handle_dependency_hook( builder.body(full_body(body)).unwrap() } +/// Keeps at most `limit` bytes of `reader`, discarding the rest as it +/// arrives. The scan's output size is agent-controlled, and draining (rather +/// than stopping) keeps the child from blocking on a full pipe. +async fn read_capped(mut reader: impl tokio::io::AsyncRead + Unpin, limit: usize) -> Vec { + use tokio::io::AsyncReadExt; + + let mut kept = Vec::new(); + let _ = (&mut reader) + .take(limit as u64) + .read_to_end(&mut kept) + .await; + let _ = tokio::io::copy(&mut reader, &mut tokio::io::sink()).await; + kept +} + +/// Runs the host's own `stashbase scan` for a sandboxed git hook. The hook +/// sends only the mode; what gets scanned, with which key and which config +/// options, is decided here. +async fn handle_secret_scan_hook( + request: Request, + mode: &str, + state: &ProxyState, + started: Instant, +) -> Response { + let request_id = new_local_request_id(); + let Some((broker, scan)) = state + .hook_broker + .as_ref() + .filter(|broker| broker.authorized(&request)) + .and_then(|broker| broker.secret_scan.as_ref().map(|scan| (broker, scan))) + else { + return proxy_error_response( + StatusCode::FORBIDDEN, + "proxy.secret_scan_not_allowed", + "Secret scan hook is not enabled for this run; add \"secret_scan\" to allow_hooks", + ); + }; + let mode = match mode { + "/staged" => "staged", + "/unpushed" => "unpushed", + _ => { + return proxy_error_response( + StatusCode::NOT_FOUND, + "proxy.secret_scan_unknown_mode", + "Unknown scan mode; expected staged or unpushed", + ) + } + }; + + let _guard = broker.scan_lock.lock().await; + let scan_failed = || { + proxy_error_response( + StatusCode::BAD_GATEWAY, + "proxy.secret_scan_failed", + "Secret scan could not be started", + ) + }; + let Ok(home) = ScratchHome::create() else { + return scan_failed(); + }; + let scan_args = ["scan", mode, "--json", "--silent"]; + let (program, args) = match &scan.isolation { + ScanIsolation::Confined(confinement) => match confinement.wrap(&scan_args, &home.0) { + Ok(command) => command, + Err(_) => return scan_failed(), + }, + #[cfg(test)] + ScanIsolation::Unconfined { exe } => ( + exe.to_string_lossy().into_owned(), + scan_args.iter().map(|arg| (*arg).to_owned()).collect(), + ), + }; + let mut command = tokio::process::Command::new(program); + command + .args(args) + .current_dir(&scan.workdir) + .env("HOME", &home.0) + // The API this run uses, whether it came from the environment or was + // built in, so the scan never falls back to a different default. + .env( + crate::api::client::API_URL_ENV_VAR, + crate::api::client::get_api_url(), + ) + .env("TMPDIR", &home.0) + .env_remove("XDG_CONFIG_HOME") + .env_remove("XDG_CACHE_HOME") + .env_remove("XDG_DATA_HOME") + .env_remove("XDG_STATE_HOME") + .env("STASHBASE_API_KEY", &broker.api_key) + .env(crate::models::scans::SCAN_RESTRICTED_ENV, "1") + // Keeps the scan's telemetry off, like everything else in the session. + .env("STASHBASE_SANDBOX", "1") + .env_remove(crate::api::dependencies::HOOK_MODE_ENV) + .env_remove(crate::api::dependencies::HOOK_BROKER_URL_ENV) + .env_remove(crate::api::dependencies::HOOK_BROKER_TOKEN_ENV) + .env_remove(SCAN_BROKER_URL_ENV) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .kill_on_drop(true); + + let run = async move { + let mut child = command.spawn()?; + let stdout = child.stdout.take().expect("stdout is piped"); + let stderr = child.stderr.take().expect("stderr is piped"); + let (stdout, stderr, status) = tokio::join!( + read_capped(stdout, SECRET_SCAN_BODY_LIMIT), + read_capped(stderr, SECRET_SCAN_BODY_LIMIT), + child.wait(), + ); + status.map(|status| (status, stdout, stderr)) + }; + // On timeout `run` is dropped with the child, which `kill_on_drop` ends. + let response = match tokio::time::timeout(scan.timeout, run).await { + Err(_) => proxy_error_response( + StatusCode::GATEWAY_TIMEOUT, + "proxy.secret_scan_timeout", + "Secret scan did not finish in time", + ), + Ok(Err(_)) => proxy_error_response( + StatusCode::BAD_GATEWAY, + "proxy.secret_scan_failed", + "Secret scan could not be started", + ), + Ok(Ok((exit, stdout, stderr))) => { + let status = match exit.code() { + Some(0) => StatusCode::OK, + Some(1) => StatusCode::UNPROCESSABLE_ENTITY, + _ => StatusCode::BAD_GATEWAY, + }; + let mut body = stdout; + body.extend_from_slice(&stderr); + if status == StatusCode::BAD_GATEWAY { + // A scan killed before printing anything (e.g. by the + // confinement) would otherwise leave an empty body. + body.extend_from_slice(format!("\nstashbase scan exited with {exit}\n").as_bytes()); + } + body.truncate(SECRET_SCAN_BODY_LIMIT); + Response::builder() + .status(status) + .header("content-type", "text/plain; charset=utf-8") + .body(full_body(Bytes::from(body))) + .unwrap() + } + }; + state.record_audit_with_request( + &request_id, + "secret_scan_hook", + None, + Some(&Method::POST), + None, + Some(response.status()), + Some(started.elapsed()), + ); + response +} + /// Standard remote-proxy requests are built per request so a new connection /// always observes the latest rotated token. Existing response streams retain /// the client and session that opened them. @@ -5270,7 +5506,11 @@ mod tests { ProxyPolicy::permissive(), None, None, - Some("parent-api-key".to_owned()), + Some(HookBrokerConfig { + api_key: "parent-api-key".to_owned(), + dependency_check: true, + secret_scan: None, + }), ) .await .unwrap(); @@ -5300,6 +5540,261 @@ mod tests { disabled.stop().await; } + #[cfg(unix)] + fn scan_test_dir() -> PathBuf { + let dir = std::env::temp_dir().join(format!("stashbase-scan-hook-{}", Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + dir.canonicalize().unwrap() + } + + #[cfg(unix)] + fn fake_scan_exe(dir: &std::path::Path, exit_code: i32, sleep_secs: u32) -> PathBuf { + let path = dir.join("fake-stashbase"); + std::fs::write( + &path, + format!( + "#!/bin/sh\nsleep {sleep_secs}\n\ + echo \"args=$*\"\necho \"cwd=$(pwd -P)\"\n\ + echo \"key=$STASHBASE_API_KEY\"\necho \"api_url=$STASHBASE_API_URL\"\necho \"restricted=$STASHBASE_SCAN_RESTRICTED\"\n\ + echo \"hook_token=${{STASHBASE_HOOK_BROKER_TOKEN:-unset}}\"\n\ + echo 'finding on stderr' >&2\nexit {exit_code}\n" + ), + ) + .unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); + path + } + + #[cfg(unix)] + async fn start_scan_proxy(exe: PathBuf, workdir: PathBuf, timeout: Duration) -> Proxy { + Proxy::start_with_hook( + HashMap::new(), + ProxyPolicy::permissive(), + None, + None, + Some(HookBrokerConfig { + api_key: "parent-api-key".to_owned(), + dependency_check: false, + secret_scan: Some(SecretScanConfig { + workdir, + timeout, + isolation: ScanIsolation::Unconfined { exe }, + }), + }), + ) + .await + .unwrap() + } + + fn hook_token(proxy: &Proxy) -> String { + proxy.child_env()[crate::api::dependencies::HOOK_BROKER_TOKEN_ENV].clone() + } + + async fn post_to(url: String, token: Option<&str>) -> (u16, String) { + let client = reqwest::Client::builder().no_proxy().build().unwrap(); + let mut request = client.post(url); + if let Some(token) = token { + request = request.bearer_auth(token); + } + let response = request.send().await.unwrap(); + (response.status().as_u16(), response.text().await.unwrap()) + } + + #[cfg(unix)] + async fn post_scan(proxy: &Proxy, mode: &str, token: Option<&str>) -> (u16, String) { + let url = format!("{}/{mode}", proxy.child_env()[SCAN_BROKER_URL_ENV]); + post_to(url, token).await + } + + #[cfg(unix)] + #[tokio::test] + async fn secret_scan_hook_runs_host_scan_in_workdir_with_parent_key_restricted() { + let dir = scan_test_dir(); + let proxy = start_scan_proxy( + fake_scan_exe(&dir, 0, 0), + dir.clone(), + Duration::from_secs(10), + ) + .await; + + let (status, body) = post_scan(&proxy, "staged", Some(&hook_token(&proxy))).await; + + assert_eq!(status, 200, "{body}"); + assert!(body.contains("args=scan staged --json --silent"), "{body}"); + assert!(body.contains(&format!("cwd={}", dir.display())), "{body}"); + assert!(body.contains("key=parent-api-key"), "{body}"); + assert!( + body.contains(&format!("api_url={}", crate::api::client::get_api_url())), + "{body}" + ); + assert!(body.contains("restricted=1"), "{body}"); + assert!(body.contains("hook_token=unset"), "{body}"); + assert!(body.contains("finding on stderr"), "{body}"); + assert!(!proxy.child_env().contains_key("STASHBASE_API_KEY")); + assert!(!proxy + .child_env() + .contains_key(crate::api::dependencies::HOOK_BROKER_URL_ENV)); + proxy.stop().await; + let _ = std::fs::remove_dir_all(dir); + } + + #[cfg(unix)] + #[tokio::test] + async fn secret_scan_hook_maps_findings_to_422_and_supports_unpushed() { + let dir = scan_test_dir(); + let proxy = start_scan_proxy( + fake_scan_exe(&dir, 1, 0), + dir.clone(), + Duration::from_secs(10), + ) + .await; + + let (status, body) = post_scan(&proxy, "unpushed", Some(&hook_token(&proxy))).await; + + assert_eq!(status, 422, "{body}"); + assert!( + body.contains("args=scan unpushed --json --silent"), + "{body}" + ); + proxy.stop().await; + let _ = std::fs::remove_dir_all(dir); + } + + #[cfg(unix)] + #[tokio::test] + async fn secret_scan_hook_rejects_bad_token_and_unknown_mode() { + let dir = scan_test_dir(); + let proxy = start_scan_proxy( + fake_scan_exe(&dir, 0, 0), + dir.clone(), + Duration::from_secs(10), + ) + .await; + let token = hook_token(&proxy); + + assert_eq!(post_scan(&proxy, "staged", None).await.0, 403); + assert_eq!(post_scan(&proxy, "staged", Some("wrong")).await.0, 403); + assert_eq!(post_scan(&proxy, "changes", Some(&token)).await.0, 404); + proxy.stop().await; + let _ = std::fs::remove_dir_all(dir); + } + + #[tokio::test] + async fn secret_scan_route_is_not_served_when_only_dependency_check_is_allowed() { + let proxy = Proxy::start_with_hook( + HashMap::new(), + ProxyPolicy::permissive(), + None, + None, + Some(HookBrokerConfig { + api_key: "parent-api-key".to_owned(), + dependency_check: true, + secret_scan: None, + }), + ) + .await + .unwrap(); + assert!(!proxy.child_env().contains_key(SCAN_BROKER_URL_ENV)); + let url = proxy.child_env()[crate::api::dependencies::HOOK_BROKER_URL_ENV].replace( + DEPENDENCY_HOOK_PATH, + &format!("{SECRET_SCAN_HOOK_PATH}/staged"), + ); + + let (status, _) = post_to(url, Some(&hook_token(&proxy))).await; + + assert_eq!(status, 403); + proxy.stop().await; + } + + #[cfg(unix)] + #[tokio::test] + async fn dependency_route_is_not_served_when_only_secret_scan_is_allowed() { + let dir = scan_test_dir(); + let proxy = start_scan_proxy( + fake_scan_exe(&dir, 0, 0), + dir.clone(), + Duration::from_secs(10), + ) + .await; + let url = proxy.child_env()[SCAN_BROKER_URL_ENV] + .replace(SECRET_SCAN_HOOK_PATH, DEPENDENCY_HOOK_PATH); + + let (status, _) = post_to(url, Some(&hook_token(&proxy))).await; + + assert_eq!(status, 403); + proxy.stop().await; + let _ = std::fs::remove_dir_all(dir); + } + + #[cfg(unix)] + #[tokio::test] + async fn secret_scan_hook_times_out_with_504() { + let dir = scan_test_dir(); + let proxy = start_scan_proxy( + fake_scan_exe(&dir, 0, 5), + dir.clone(), + Duration::from_millis(300), + ) + .await; + + let (status, body) = post_scan(&proxy, "staged", Some(&hook_token(&proxy))).await; + + assert_eq!(status, 504, "{body}"); + assert!(body.contains("proxy.secret_scan_timeout"), "{body}"); + proxy.stop().await; + let _ = std::fs::remove_dir_all(dir); + } + + #[cfg(unix)] + #[tokio::test] + async fn secret_scan_hook_caps_huge_output_and_still_finishes() { + let dir = scan_test_dir(); + let exe = dir.join("loud-stashbase"); + std::fs::write( + &exe, + "#!/bin/sh\nhead -c 5242880 /dev/zero | tr '\\0' a\nhead -c 5242880 /dev/zero | tr '\\0' b >&2\nexit 1\n", + ) + .unwrap(); + std::fs::set_permissions(&exe, std::fs::Permissions::from_mode(0o755)).unwrap(); + let proxy = start_scan_proxy(exe, dir.clone(), Duration::from_secs(10)).await; + + let (status, body) = post_scan(&proxy, "staged", Some(&hook_token(&proxy))).await; + + // 422 rather than 504: the excess was drained, so the scan exited. + assert_eq!(status, 422); + assert!(body.len() <= SECRET_SCAN_BODY_LIMIT, "{}", body.len()); + assert!(body.starts_with('a')); + proxy.stop().await; + let _ = std::fs::remove_dir_all(dir); + } + + #[cfg(unix)] + #[tokio::test] + async fn secret_scan_hooks_are_serialized() { + let dir = scan_test_dir(); + let proxy = start_scan_proxy( + fake_scan_exe(&dir, 0, 1), + dir.clone(), + Duration::from_secs(10), + ) + .await; + let token = hook_token(&proxy); + + let started = std::time::Instant::now(); + let (a, b) = tokio::join!( + post_scan(&proxy, "staged", Some(&token)), + post_scan(&proxy, "staged", Some(&token)), + ); + + assert_eq!((a.0, b.0), (200, 200)); + assert!( + started.elapsed() >= Duration::from_secs(2), + "scans ran concurrently" + ); + proxy.stop().await; + let _ = std::fs::remove_dir_all(dir); + } + #[tokio::test] async fn child_environment_uses_the_binding_name_for_its_default_placeholder() { let proxy = Proxy::start( @@ -5500,8 +5995,7 @@ mod tests { connections: Arc::new(ActiveConnections::default()), remote: None, mcp_inspection_token: "inspection-token".to_owned(), - dependency_hook_token: None, - dependency_hook_client: None, + hook_broker: None, revocation_path: Arc::new(RwLock::new(None)), }; diff --git a/src/handlers/run/scan_sandbox.rs b/src/handlers/run/scan_sandbox.rs new file mode 100644 index 00000000..1db5fedb --- /dev/null +++ b/src/handlers/run/scan_sandbox.rs @@ -0,0 +1,446 @@ +//! Confines the host-side secret scan the Agent Proxy runs for a sandboxed +//! agent. The scan reads a repository the agent controls, so a symlink, a +//! `gitdir:` file, `objects/info/alternates` or `include.path` could +//! otherwise point it at any file on the host. On Linux the scan's root is +//! built from nothing but system files and the run's paths. On macOS, where +//! the loader needs too many system paths to list reliably, file contents +//! under every location that holds users' data (homes, volumes, temporary +//! directories, `/opt`) are denied except the run's working tree, its git +//! directories, the CLI binary and a scratch home. On both, the profile's +//! `deny_read` entries stay denied. + +use anyhow::Result; +use std::path::{Path, PathBuf}; + +pub struct ScanConfinement { + workdir: PathBuf, + git_dirs: Vec, + exe: PathBuf, + denied_read: Vec, +} + +impl ScanConfinement { + /// Captures the run's git directories now, before the agent can point + /// `.git` anywhere else. + pub fn for_run(workdir: &Path, exe: &Path, denied_read: &[String]) -> Self { + let workdir = canonical(workdir); + let mut git_dirs = Vec::new(); + if let Ok(repo) = git2::Repository::discover(&workdir) { + for dir in [repo.path(), repo.commondir()] { + let dir = canonical(dir); + if !dir.starts_with(&workdir) && !git_dirs.contains(&dir) { + git_dirs.push(dir); + } + } + } + Self { + workdir, + git_dirs, + exe: canonical(exe), + denied_read: denied_read.to_vec(), + } + } + + /// The program and arguments that run `exe args` confined, with `home` + /// (an empty, writable directory) standing in for the user's home and + /// temporary directory. + pub fn wrap(&self, args: &[&str], home: &Path) -> Result<(String, Vec)> { + platform::wrap(self, args, home) + } + + #[cfg_attr(not(any(target_os = "macos", target_os = "linux")), allow(dead_code))] + fn readable(&self, home: &Path) -> Vec { + let mut paths = vec![self.workdir.clone(), self.exe.clone(), canonical(home)]; + paths.extend(self.git_dirs.iter().cloned()); + paths + } +} + +/// Why secret scans can't be confined on this machine, if they can't. +pub fn unavailable_reason() -> Option { + platform::unavailable_reason() +} + +fn canonical(path: &Path) -> PathBuf { + path.canonicalize().unwrap_or_else(|_| path.to_owned()) +} + +#[cfg(target_os = "macos")] +mod platform { + use super::ScanConfinement; + use crate::handlers::run::subprocess::{denied_file_rules, escape_sbpl_path}; + use anyhow::Result; + use std::path::Path; + + /// Where users' files live on macOS. Everything else on the system + /// (libraries, frameworks, `/etc`) stays readable, so the CLI loads + /// normally; these are denied except for the run's own paths. + const USER_DATA_ROOTS: &[&str] = &[ + "/Users", + "/Volumes", + "/private/tmp", + "/private/var/folders", + "/private/var/root", + "/opt", + ]; + + pub fn unavailable_reason() -> Option { + None + } + + pub fn wrap( + confinement: &ScanConfinement, + args: &[&str], + home: &Path, + ) -> Result<(String, Vec)> { + let mut command = vec!["-p".to_owned(), profile(confinement, home)?]; + command.push(confinement.exe.to_string_lossy().into_owned()); + command.extend(args.iter().map(|arg| (*arg).to_owned())); + Ok(("/usr/bin/sandbox-exec".to_owned(), command)) + } + + pub(super) fn profile(confinement: &ScanConfinement, home: &Path) -> Result { + let quote = |path: &Path| format!("\"{}\"", escape_sbpl_path(&path.to_string_lossy())); + // Metadata (stat, realpath, libgit2's repository lookup) stays + // readable everywhere. One deny with exclusions, since a later allow + // does not override an earlier deny for the same operation. + let roots = USER_DATA_ROOTS + .iter() + .map(|root| format!("(subpath {})", quote(Path::new(root)))) + .collect::>() + .join(" "); + let allowed = confinement + .readable(home) + .into_iter() + .map(|path| { + let filter = if path.is_file() { "literal" } else { "subpath" }; + format!("({filter} {})", quote(&path)) + }) + .collect::>() + .join(" "); + let mut rules = vec![ + "(version 1)".to_owned(), + "(allow default)".to_owned(), + format!( + "(deny file-read-data file-read-xattr (require-all (require-any {roots}) (require-not (require-any {allowed}))))" + ), + ]; + // Last, so the profile's own denials win inside the working tree. + rules.push(denied_file_rules( + &confinement.denied_read, + &[], + &confinement.workdir, + )?); + Ok(rules.join("\n")) + } +} + +#[cfg(target_os = "linux")] +mod platform { + use super::ScanConfinement; + use crate::handlers::run::{docker_sandbox::empty_shadow_file_path, fs_rules}; + use anyhow::Result; + use std::path::{Path, PathBuf}; + + /// Top-level directories that are usually symlinks into `/usr`. + const ROOT_LINKS: &[&str] = &["/bin", "/sbin", "/lib", "/lib32", "/lib64", "/libx32"]; + + /// What the CLI needs from `/etc` and `/run`: DNS, TLS trust roots, user + /// lookup, time zone and the dynamic linker cache. + const SYSTEM_FILES: &[&str] = &[ + "/etc/resolv.conf", + "/etc/hosts", + "/etc/nsswitch.conf", + "/etc/host.conf", + "/etc/gai.conf", + "/etc/ssl", + "/etc/ca-certificates", + "/etc/pki", + "/etc/passwd", + "/etc/group", + "/etc/localtime", + "/etc/ld.so.cache", + "/etc/ld.so.conf", + "/etc/ld.so.conf.d", + "/run/systemd/resolve", + ]; + + fn executable() -> Option<&'static str> { + ["bwrap", "bubblewrap"].into_iter().find(|name| { + std::env::var_os("PATH").is_some_and(|path| { + std::env::split_paths(&path).any(|directory| directory.join(name).is_file()) + }) + }) + } + + pub fn unavailable_reason() -> Option { + let Some(executable) = executable() else { + return Some( + "secret_scan needs bubblewrap (`bwrap`) on Linux to confine the host-side scan" + .to_owned(), + ); + }; + let probe = std::process::Command::new(executable) + .args([ + "--die-with-parent", + "--ro-bind", + "/", + "/", + "--tmpfs", + "/tmp", + "--", + "/bin/true", + ]) + .output(); + match probe { + Ok(output) if output.status.success() => None, + Ok(output) => Some(format!( + "secret_scan needs a working bubblewrap to confine the host-side scan: {}", + String::from_utf8_lossy(&output.stderr).trim() + )), + Err(error) => Some(format!( + "secret_scan needs a working bubblewrap to confine the host-side scan: {error}" + )), + } + } + + pub fn wrap( + confinement: &ScanConfinement, + args: &[&str], + home: &Path, + ) -> Result<(String, Vec)> { + let executable = executable() + .ok_or_else(|| anyhow::anyhow!("bubblewrap is not available"))? + .to_owned(); + let mut command = bwrap_args(confinement, home).map_err(anyhow::Error::msg)?; + command.push("--".to_owned()); + command.push(confinement.exe.to_string_lossy().into_owned()); + command.extend(args.iter().map(|arg| (*arg).to_owned())); + Ok((executable, command)) + } + + /// Builds the scan's root from nothing: unlike the agent's own Linux + /// sandbox, `/` is not bound, so only what is listed here exists. + pub(super) fn bwrap_args( + confinement: &ScanConfinement, + home: &Path, + ) -> Result, String> { + let text = |path: &Path| path.to_string_lossy().into_owned(); + let mut args = vec![ + "--die-with-parent".to_owned(), + "--proc".to_owned(), + "/proc".to_owned(), + "--dev".to_owned(), + "/dev".to_owned(), + "--tmpfs".to_owned(), + "/tmp".to_owned(), + "--ro-bind".to_owned(), + "/usr".to_owned(), + "/usr".to_owned(), + ]; + for link in ROOT_LINKS { + let path = Path::new(link); + match std::fs::read_link(path) { + Ok(target) => args.extend(["--symlink".to_owned(), text(&target), text(path)]), + Err(_) if path.is_dir() => { + args.extend(["--ro-bind".to_owned(), text(path), text(path)]) + } + Err(_) => {} + } + } + for file in SYSTEM_FILES { + args.extend([ + "--ro-bind-try".to_owned(), + (*file).to_owned(), + (*file).to_owned(), + ]); + } + let home = super::canonical(home); + for path in confinement.readable(&home) { + let mode = if path == home { "--bind" } else { "--ro-bind" }; + args.extend([mode.to_owned(), text(&path), text(&path)]); + } + for path in + fs_rules::expand_policy_entries(&confinement.denied_read, &confinement.workdir, None)? + { + if PathBuf::from(&path).is_dir() { + args.extend(["--tmpfs".to_owned(), path]); + } else { + args.extend(["--ro-bind".to_owned(), empty_shadow_file_path()?, path]); + } + } + args.extend(["--chdir".to_owned(), text(&confinement.workdir)]); + Ok(args) + } +} + +#[cfg(not(any(target_os = "macos", target_os = "linux")))] +mod platform { + use super::ScanConfinement; + use anyhow::Result; + use std::path::Path; + + pub fn unavailable_reason() -> Option { + Some("secret_scan is supported only on macOS and Linux, where the host-side scan can be confined".to_owned()) + } + + pub fn wrap(_: &ScanConfinement, _: &[&str], _: &Path) -> Result<(String, Vec)> { + anyhow::bail!(unavailable_reason().unwrap_or_default()) + } +} + +#[cfg(all(test, any(target_os = "macos", target_os = "linux")))] +mod tests { + use super::*; + use std::fs; + + struct Layout { + root: PathBuf, + workdir: PathBuf, + home: PathBuf, + outside: PathBuf, + } + + impl Drop for Layout { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.root); + let _ = fs::remove_dir_all(&self.home); + } + } + + /// A repo under `base`, with a secret next to it (outside the worktree) + /// and a symlink from the worktree to that secret. + fn layout(base: &Path) -> Option { + let root = base.join(format!( + ".stashbase-scan-sandbox-test-{}", + uuid::Uuid::new_v4() + )); + let workdir = root.join("repo"); + let home = + std::env::temp_dir().join(format!("stashbase-scan-home-{}", uuid::Uuid::new_v4())); + fs::create_dir_all(&workdir).ok()?; + fs::create_dir_all(&home).ok()?; + git2::Repository::init(&workdir).ok()?; + fs::write(workdir.join("inside.txt"), "inside").ok()?; + let outside = root.join("outside-secret.txt"); + fs::write(&outside, "outside").ok()?; + std::os::unix::fs::symlink(&outside, workdir.join("link.txt")).ok()?; + Some(Layout { + root: canonical(&root), + workdir: canonical(&workdir), + home, + outside: canonical(&outside), + }) + } + + fn run_cat(confinement: &ScanConfinement, home: &Path, file: &Path) -> std::process::Output { + let (program, args) = confinement.wrap(&[&file.to_string_lossy()], home).unwrap(); + std::process::Command::new(program) + .args(args) + .current_dir(&confinement.workdir) + .output() + .unwrap() + } + + /// Skips locally when this machine can't confine the scan, but fails in + /// CI, where a skip would hide that the confinement never ran. + fn confinement_available() -> bool { + match unavailable_reason() { + None => true, + Some(reason) if std::env::var_os("CI").is_some() => { + panic!("scan confinement must run in CI: {reason}") + } + Some(reason) => { + eprintln!("skipping: {reason}"); + false + } + } + } + + /// `cat` stands in for the CLI binary: the confinement is about which + /// files the process can read, not what it does with them. + fn assert_reads_only_the_worktree(base: &Path) { + let Some(layout) = layout(base) else { + eprintln!( + "skipping: cannot create a test layout under {}", + base.display() + ); + return; + }; + let cat = canonical(Path::new("/bin/cat")); + let confinement = ScanConfinement::for_run(&layout.workdir, &cat, &[]); + + let inside = run_cat( + &confinement, + &layout.home, + &layout.workdir.join("inside.txt"), + ); + let via_link = run_cat(&confinement, &layout.home, &layout.workdir.join("link.txt")); + let direct = run_cat(&confinement, &layout.home, &layout.outside); + + assert_eq!(String::from_utf8_lossy(&inside.stdout), "inside"); + assert!( + !String::from_utf8_lossy(&via_link.stdout).contains("outside"), + "followed a symlink out of the worktree under {}", + base.display() + ); + assert!( + !String::from_utf8_lossy(&direct.stdout).contains("outside"), + "read a host file outside the worktree under {}", + base.display() + ); + } + + #[test] + fn confined_scan_cannot_read_outside_the_worktree_under_home() { + if !confinement_available() { + return; + } + if let Some(home) = std::env::var_os("HOME") { + assert_reads_only_the_worktree(Path::new(&home)); + } + } + + #[test] + fn confined_scan_cannot_read_outside_the_worktree_under_tmp() { + if !confinement_available() { + return; + } + assert_reads_only_the_worktree(Path::new("/tmp")); + assert_reads_only_the_worktree(&std::env::temp_dir()); + } + + #[test] + fn confined_scan_keeps_profile_deny_read_inside_the_worktree() { + if !confinement_available() { + return; + } + let Some(layout) = layout(&std::env::temp_dir()) else { + return; + }; + fs::write(layout.workdir.join(".env"), "SECRET=1").unwrap(); + let cat = canonical(Path::new("/bin/cat")); + let confinement = ScanConfinement::for_run(&layout.workdir, &cat, &[".env".to_owned()]); + + let denied = run_cat(&confinement, &layout.home, &layout.workdir.join(".env")); + + assert!( + !String::from_utf8_lossy(&denied.stdout).contains("SECRET"), + "read a deny_read file" + ); + } + + #[test] + fn for_run_captures_git_dirs_outside_the_worktree() { + let Some(layout) = layout(&std::env::temp_dir()) else { + return; + }; + let cat = canonical(Path::new("/bin/cat")); + + let confinement = ScanConfinement::for_run(&layout.workdir, &cat, &[]); + + // A plain repo's `.git` is inside the worktree, so nothing extra. + assert!(confinement.git_dirs.is_empty()); + assert_eq!(confinement.workdir, layout.workdir); + } +} diff --git a/src/handlers/run/subprocess.rs b/src/handlers/run/subprocess.rs index 29642799..eb985919 100644 --- a/src/handlers/run/subprocess.rs +++ b/src/handlers/run/subprocess.rs @@ -831,7 +831,7 @@ fn codex_workspace_rules(boundary: CodexSandboxBoundary, current_dir: &Path) -> } #[cfg(target_os = "macos")] -fn denied_file_rules( +pub(crate) fn denied_file_rules( deny_read: &[String], deny_write: &[String], current_dir: &Path, @@ -967,7 +967,7 @@ fn filesystem_denial_from_line( } #[cfg(target_os = "macos")] -fn escape_sbpl_path(path: &str) -> String { +pub(crate) fn escape_sbpl_path(path: &str) -> String { path.replace('\\', "\\\\").replace('"', "\\\"") } diff --git a/src/handlers/scans/commits.rs b/src/handlers/scans/commits.rs index 09ab5363..572a5029 100644 --- a/src/handlers/scans/commits.rs +++ b/src/handlers/scans/commits.rs @@ -11,7 +11,7 @@ use crate::{ scans::{ CommitChanges, CommitsScanResponse, DiffHunk, DiffProcessingState, FileHunks, IgnoredSecretsPayload, MatchConfigPayload, ProjectContextConfigPayload, - ScanCommitChangesPayload, ScanConfig, ScanOutputJson, + ScanCommitChangesPayload, ScanConfig, ScanOutputJson, SCAN_RESTRICTED_ENV, }, validation::{InputValidationError, ScanInputValidationError}, }, @@ -21,7 +21,8 @@ use crate::{ default_scan_exclude_patterns, file_content_equals, filter_new_findings, get_file_matches, get_latest_scan_file, is_binary_file, is_valid_sha256_hash, load_baseline_results, process_diff_line, save_scan_results, should_exclude_file, - update_findings_with_file_matches, SCAN_CONTEXT_LINES, SCAN_IGNORE_LINE_COMMENT, + update_findings_with_file_matches, RestrictedScanBudget, SCAN_CONTEXT_LINES, + SCAN_IGNORE_LINE_COMMENT, }, spinner::new_spinner, validation::validate_project_identifier, @@ -84,6 +85,11 @@ pub async fn handle_scan_unpushed_commit_hunks( }, None => ScanConfig::default(), }; + let config = if std::env::var(SCAN_RESTRICTED_ENV).as_deref() == Ok("1") { + config.restricted_for_broker() + } else { + config + }; let exclude = default_scan_exclude_patterns() .into_iter() @@ -782,6 +788,7 @@ pub fn get_unpushed_commit_hunks( let mut all_commit_changes = Vec::new(); let mut current = local_commit; + let mut budget = RestrictedScanBudget::for_current_scan(); // Check all commits that would be pushed (new commits since merge base) loop { @@ -806,6 +813,10 @@ pub fn get_unpushed_commit_hunks( let mut diff_opts = git2::DiffOptions::new(); diff_opts.context_lines(context_lines as u32); diff_opts.show_binary(false); + if let Some(budget) = budget.as_mut() { + budget.charge_commit()?; + RestrictedScanBudget::limit_diff_options(&mut diff_opts); + } let diff = repo .diff_tree_to_tree( @@ -816,6 +827,9 @@ pub fn get_unpushed_commit_hunks( .map_err(|e| ScanInputValidationError::GitDiffGeneration { message: e.message().to_string(), })?; + if let Some(budget) = budget.as_mut() { + budget.charge_diff(&repo, &diff)?; + } let state = Rc::new(RefCell::new(DiffProcessingState::new())); let ignore_line_comment = ignore_line_comment.to_string(); diff --git a/src/handlers/scans/config.rs b/src/handlers/scans/config.rs index 91010a9a..a24f4403 100644 --- a/src/handlers/scans/config.rs +++ b/src/handlers/scans/config.rs @@ -175,12 +175,62 @@ pub fn resolve_scan_config_path(config_file_path: Option) -> Option Option { + let default_config = workdir.join(DEFAULT_SCAN_CONFIG_PATH); + let usable = if restricted { + fs::symlink_metadata(&default_config).is_ok_and(|metadata| metadata.is_file()) } else { - None + default_config.exists() + }; + usable.then_some(default_config) +} + +#[cfg(all(test, unix))] +mod restricted_config_tests { + use super::{default_config_in, DEFAULT_SCAN_CONFIG_PATH}; + use std::fs; + + fn temp_dir() -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!( + "stashbase-scan-config-{}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + fs::create_dir_all(&dir).unwrap(); + dir + } + + #[test] + fn restricted_scan_ignores_a_symlinked_config() { + let dir = temp_dir(); + let outside = dir.join("outside-secret"); + fs::write(&outside, "excluded-files: []\n").unwrap(); + let workdir = dir.join("repo"); + fs::create_dir_all(&workdir).unwrap(); + std::os::unix::fs::symlink(&outside, workdir.join(DEFAULT_SCAN_CONFIG_PATH)).unwrap(); + + assert!(default_config_in(&workdir, true).is_none()); + assert!(default_config_in(&workdir, false).is_some()); + let _ = fs::remove_dir_all(dir); + } + + #[test] + fn restricted_scan_uses_a_regular_config() { + let dir = temp_dir(); + fs::write(dir.join(DEFAULT_SCAN_CONFIG_PATH), "excluded-files: []\n").unwrap(); + + assert_eq!( + default_config_in(&dir, true), + Some(dir.join(DEFAULT_SCAN_CONFIG_PATH)) + ); + let _ = fs::remove_dir_all(dir); } } diff --git a/src/handlers/scans/files.rs b/src/handlers/scans/files.rs index 43526aed..7d66b0da 100644 --- a/src/handlers/scans/files.rs +++ b/src/handlers/scans/files.rs @@ -6,7 +6,7 @@ use crate::{ scans::{ DiffHunk, DiffProcessingState, FileChangesScanResponse, FileHunks, IgnoredSecretsPayload, MatchConfigPayload, ProjectContextConfigPayload, ScanConfig, - ScanFileChangesPayload, ScanOutputJson, + ScanFileChangesPayload, ScanOutputJson, SCAN_RESTRICTED_ENV, }, validation::{InputValidationError, ScanInputValidationError}, }, @@ -16,7 +16,8 @@ use crate::{ default_scan_exclude_patterns, file_content_equals, filter_new_findings, get_file_matches, get_latest_scan_file, is_binary_file, is_valid_sha256_hash, load_baseline_results, process_diff_line, save_scan_results, should_exclude_file, - update_findings_with_file_matches, SCAN_CONTEXT_LINES, SCAN_IGNORE_LINE_COMMENT, + update_findings_with_file_matches, RestrictedScanBudget, SCAN_CONTEXT_LINES, + SCAN_IGNORE_LINE_COMMENT, }, spinner::new_spinner, validation::validate_project_identifier, @@ -145,6 +146,11 @@ async fn handle_scan_file_hunks( }, None => ScanConfig::default(), }; + let config = if std::env::var(SCAN_RESTRICTED_ENV).as_deref() == Ok("1") { + config.restricted_for_broker() + } else { + config + }; let exclude = default_scan_exclude_patterns() .into_iter() @@ -777,9 +783,13 @@ fn get_file_hunks( } }; + let mut budget = RestrictedScanBudget::for_current_scan(); let mut diff_opts = git2::DiffOptions::new(); diff_opts.context_lines(context_lines as u32); diff_opts.show_binary(false); + if budget.is_some() { + RestrictedScanBudget::limit_diff_options(&mut diff_opts); + } if matches!(mode, FileScanMode::Changed) { diff_opts.include_untracked(true); diff_opts.recurse_untracked_dirs(true); @@ -796,6 +806,9 @@ fn get_file_hunks( .map_err(|e| ScanInputValidationError::GitDiffGeneration { message: e.message().to_string(), })?; + if let Some(budget) = budget.as_mut() { + budget.charge_diff(&repo, &diff)?; + } let state = Rc::new(RefCell::new(DiffProcessingState::new())); diff --git a/src/handlers/scans/install.rs b/src/handlers/scans/install.rs index d52acaaa..01fcbf8b 100644 --- a/src/handlers/scans/install.rs +++ b/src/handlers/scans/install.rs @@ -39,6 +39,38 @@ impl HookType { } } +/// Inside an agent sandbox the hook asks the Agent Proxy to run the scan on +/// the host, since the sandbox has neither the CLI nor an API key. +fn hook_block(hook_type: HookType) -> String { + format!( + r#"{start} +if [ -n "${{STASHBASE_SCAN_BROKER_URL:-}}" ]; then + command -v curl >/dev/null 2>&1 || {{ + echo "curl not found in the agent sandbox. Cannot run the Stashbase scan." + exit 1 + }} + curl -sS --fail-with-body --noproxy '*' -X POST \ + -H "Authorization: Bearer ${{STASHBASE_HOOK_BROKER_TOKEN:-}}" \ + "$STASHBASE_SCAN_BROKER_URL/{mode}" || exit 1 +elif [ "${{STASHBASE_SANDBOX:-}}" = "1" ]; then + echo "Stashbase scan hook is not enabled for this agent run. Add allow_hooks = [\"secret_scan\"] to the agent profile." + exit 1 +else + command -v stashbase >/dev/null 2>&1 || {{ + echo "stashbase CLI not found. Skipping scan." + exit 1 + }} + + stashbase scan {mode} --silent --json || exit 1 +fi +{end} +"#, + start = STASHBASE_SCAN_START_MARKER, + mode = hook_type.scan_mode(), + end = STASHBASE_SCAN_END_MARKER, + ) +} + pub fn install_scan_hook( hook_type: HookType, file_path: Option<&str>, @@ -61,12 +93,7 @@ pub fn install_scan_hook( ) })?; - let hook_block = format!( - "{start}\ncommand -v stashbase >/dev/null 2>&1 || {{\n echo \"stashbase CLI not found. Skipping scan.\"\n exit 1\n}}\n\nstashbase scan {mode} --silent --json || exit 1\n{end}\n", - start = STASHBASE_SCAN_START_MARKER, - mode = hook_type.scan_mode(), - end = STASHBASE_SCAN_END_MARKER, - ); + let hook_block = hook_block(hook_type); let mut was_already_installed = false; @@ -289,7 +316,7 @@ fn normalize_after_uninstall(content: String) -> String { #[cfg(test)] mod tests { - use super::{install_scan_hook, uninstall_scan_hook, HookType}; + use super::{hook_block, install_scan_hook, uninstall_scan_hook, HookType}; use once_cell::sync::Lazy; use std::{ env, fs, @@ -341,6 +368,118 @@ mod tests { assert!(status.success(), "git init failed"); } + #[cfg(unix)] + fn run_block(hook_type: HookType, env: &[(&str, &str)], with_curl: bool) -> (i32, String) { + run_script(hook_block(hook_type), env, with_curl) + } + + #[cfg(unix)] + fn run_script(script: String, env: &[(&str, &str)], with_curl: bool) -> (i32, String) { + use std::os::unix::fs::PermissionsExt; + + let bin = temp_dir().join("bin"); + fs::create_dir_all(&bin).expect("failed to create bin dir"); + let stub = |name: &str, body: &str| { + let path = bin.join(name); + fs::write(&path, format!("#!/bin/sh\n{body}\n")).expect("stub write failed"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).expect("chmod failed"); + }; + if with_curl { + stub("curl", "echo \"curl $*\"; exit ${FAKE_CURL_EXIT:-0}"); + } + stub("stashbase", "echo \"cli $*\""); + + // PATH holds only the stubs, so a real curl can never stand in for a + // missing one; everything else the block uses is a shell builtin. + let output = Command::new("/bin/sh") + .arg("-c") + .arg(script) + .env_clear() + .env("PATH", &bin) + .envs(env.iter().copied()) + .output() + .expect("failed to run hook block"); + let text = String::from_utf8_lossy(&output.stdout).into_owned() + + &String::from_utf8_lossy(&output.stderr); + (output.status.code().expect("hook was killed"), text) + } + + #[cfg(unix)] + const BROKER_ENV: [(&str, &str); 3] = [ + ("STASHBASE_SANDBOX", "1"), + ("STASHBASE_SCAN_BROKER_URL", "http://h:1/__stashbase/scan"), + ("STASHBASE_HOOK_BROKER_TOKEN", "t"), + ]; + + #[cfg(unix)] + #[test] + fn hook_uses_the_broker_inside_the_sandbox() { + let (code, out) = run_block(HookType::PrePush, &BROKER_ENV, true); + + assert_eq!(code, 0, "{out}"); + assert!( + out.contains("http://h:1/__stashbase/scan/unpushed"), + "{out}" + ); + assert!(out.contains("Authorization: Bearer t"), "{out}"); + assert!(!out.contains("cli "), "{out}"); + } + + #[cfg(unix)] + #[test] + fn hook_fails_when_the_broker_reports_findings() { + let mut env = BROKER_ENV.to_vec(); + env.push(("FAKE_CURL_EXIT", "22")); + + let (code, out) = run_block(HookType::PreCommit, &env, true); + + assert_eq!(code, 1, "{out}"); + assert!(out.contains("http://h:1/__stashbase/scan/staged"), "{out}"); + } + + #[cfg(unix)] + #[test] + fn hook_in_sandbox_without_broker_names_the_profile_fix() { + let (code, out) = run_block(HookType::PreCommit, &[("STASHBASE_SANDBOX", "1")], true); + + assert_eq!(code, 1, "{out}"); + assert!(out.contains("allow_hooks = [\"secret_scan\"]"), "{out}"); + assert!(!out.contains("stashbase CLI not found"), "{out}"); + } + + #[cfg(unix)] + #[test] + fn hook_in_sandbox_without_curl_says_so() { + let (code, out) = run_block(HookType::PreCommit, &BROKER_ENV, false); + + assert_eq!(code, 1, "{out}"); + assert!(out.contains("curl not found"), "{out}"); + } + + #[cfg(unix)] + #[test] + fn hook_works_inside_an_existing_set_u_hook() { + // `scan install` appends to existing hooks, which may use `set -u`. + let script = format!("set -u\n{}", hook_block(HookType::PreCommit)); + + let (code, out) = run_script(script.clone(), &[], true); + assert_eq!(code, 0, "{out}"); + assert!(out.contains("cli scan staged --silent --json"), "{out}"); + + let (code, out) = run_script(script, &BROKER_ENV, true); + assert_eq!(code, 0, "{out}"); + assert!(out.contains("http://h:1/__stashbase/scan/staged"), "{out}"); + } + + #[cfg(unix)] + #[test] + fn hook_outside_sandbox_runs_the_cli_as_before() { + let (code, out) = run_block(HookType::PreCommit, &[], true); + + assert_eq!(code, 0, "{out}"); + assert!(out.contains("cli scan staged --silent --json"), "{out}"); + } + #[test] fn creates_new_pre_commit_hook() { let _lock = test_lock(); diff --git a/src/models/agent.rs b/src/models/agent.rs index f3d5b834..55268de8 100644 --- a/src/models/agent.rs +++ b/src/models/agent.rs @@ -42,6 +42,48 @@ pub struct AgentProfile { pub allow_hooks: Vec, } +/// Authenticated hooks the run broker serves on the agent's behalf. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct EnabledHooks { + pub dependency_check: bool, + pub secret_scan: bool, +} + +impl EnabledHooks { + pub fn any(self) -> bool { + self.dependency_check || self.secret_scan + } + + /// The startup line's description, e.g. `enabled (dependency_check, secret_scan)`. + pub fn label(self) -> String { + let names = [ + (self.dependency_check, "dependency_check"), + (self.secret_scan, "secret_scan"), + ] + .into_iter() + .filter_map(|(enabled, name)| enabled.then_some(name)) + .collect::>(); + if names.is_empty() { + "disabled".to_owned() + } else { + format!("enabled ({})", names.join(", ")) + } + } +} + +impl AgentProfile { + /// The hooks `allow_hooks` asks for, before checking an API key exists. + pub fn requested_hooks(&self) -> EnabledHooks { + EnabledHooks { + dependency_check: self + .allow_hooks + .iter() + .any(|hook| hook == "dependency_check"), + secret_scan: self.allow_hooks.iter().any(|hook| hook == "secret_scan"), + } + } +} + /// An MCP endpoint rule. `hosts` and `paths` identify the endpoint, while /// `tools` limits the tools exposed through it. #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/src/models/scans.rs b/src/models/scans.rs index 4a5473ef..3ce1f805 100644 --- a/src/models/scans.rs +++ b/src/models/scans.rs @@ -62,7 +62,22 @@ impl Default for ScanConfig { } } +/// Set by the Agent Proxy on the host-side scan it runs for a sandboxed +/// agent's git hook. The scan config lives in the repo, which the agent can +/// edit, so options that reach beyond narrowing results are ignored. +pub const SCAN_RESTRICTED_ENV: &str = "STASHBASE_SCAN_RESTRICTED"; + impl ScanConfig { + /// Drops `match_config` (matching against stored secrets and reading + /// arbitrary host files) and `output_dir` (writing outside the repo). + pub fn restricted_for_broker(self) -> Self { + Self { + match_config: None, + output_dir: None, + ..self + } + } + pub fn load_from_file(config_path: &str) -> Result { let file = std::fs::File::open(config_path).map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { @@ -721,3 +736,38 @@ impl ScanOutputJson { Ok(pretty_json) } } + +#[cfg(test)] +mod restricted_tests { + use super::ScanConfig; + + #[test] + fn restricted_config_drops_agent_controlled_reach_but_keeps_narrowing() { + let yaml = r#" +excluded-files: ["fixtures/**"] +output-dir: "/tmp/anywhere" +ignored-secrets: + hashes: ["aa"] + regexes: ["^test_"] +match: + project: + identifier: "prod" + environments: ["production"] + files: ["~/.aws/credentials"] +"#; + let config: ScanConfig = serde_yaml::from_str(yaml).unwrap(); + assert!(config.match_config.is_some() && config.output_dir.is_some()); + + let restricted = config.restricted_for_broker(); + + assert!(restricted.match_config.is_none()); + assert!(restricted.output_dir.is_none()); + assert_eq!( + restricted.excluded_files, + Some(vec!["fixtures/**".to_owned()]) + ); + let ignored = restricted.ignored_secrets.unwrap(); + assert_eq!(ignored.hashes, Some(vec!["aa".to_owned()])); + assert_eq!(ignored.regexes, Some(vec!["^test_".to_owned()])); + } +} diff --git a/src/models/validation.rs b/src/models/validation.rs index 809f0c44..9afcdac2 100644 --- a/src/models/validation.rs +++ b/src/models/validation.rs @@ -194,6 +194,7 @@ pub enum ScanInputValidationError { ConfigFileParse { path: String, message: String }, InvalidIgnoreSecretRegex { regex: String, message: String }, InvalidIgnoreSecretHash { hash: String }, + RestrictedScanTooLarge { detail: String }, } #[derive(Debug, Serialize)] @@ -1141,6 +1142,13 @@ impl ScanInputValidationError { "Invalid ignore secret hash.", Some(Box::leak(format!("Hash: {}", hash).into_boxed_str())), ), + ScanInputValidationError::RestrictedScanTooLarge { detail } => ( + "Changes are too large for a sandboxed scan.", + Some(Box::leak( + format!("{detail}. Commit or push them from outside the agent sandbox.") + .into_boxed_str(), + )), + ), } } } diff --git a/src/utils/scans.rs b/src/utils/scans.rs index 7f1789f1..e6e96eb1 100644 --- a/src/utils/scans.rs +++ b/src/utils/scans.rs @@ -612,3 +612,201 @@ pub fn update_findings_with_file_matches( } } } + +pub const RESTRICTED_SCAN_MAX_FILE_BYTES: u64 = 1024 * 1024; +pub const RESTRICTED_SCAN_MAX_TOTAL_BYTES: u64 = 32 * 1024 * 1024; +pub const RESTRICTED_SCAN_MAX_FILES: usize = 10_000; +pub const RESTRICTED_SCAN_MAX_COMMITS: usize = 1_000; + +/// Caps what a restricted scan reads. The Agent Proxy runs it on the host for +/// a sandboxed agent, which controls the staged content and the history, so +/// without a cap it could make the host load arbitrarily large blobs. +pub struct RestrictedScanBudget { + remaining_bytes: u64, + remaining_files: usize, + remaining_commits: usize, +} + +impl RestrictedScanBudget { + pub fn new() -> Self { + Self { + remaining_bytes: RESTRICTED_SCAN_MAX_TOTAL_BYTES, + remaining_files: RESTRICTED_SCAN_MAX_FILES, + remaining_commits: RESTRICTED_SCAN_MAX_COMMITS, + } + } + + /// `Some` when this process is a restricted scan. + pub fn for_current_scan() -> Option { + (std::env::var(crate::models::scans::SCAN_RESTRICTED_ENV).as_deref() == Ok("1")) + .then(Self::new) + } + + /// Keeps libgit2 from loading a blob above the per-file cap, as a second + /// line behind `charge_diff`. + pub fn limit_diff_options(options: &mut git2::DiffOptions) { + options.max_size(RESTRICTED_SCAN_MAX_FILE_BYTES as i64); + } + + pub fn charge_commit(&mut self) -> Result<(), ScanInputValidationError> { + if self.remaining_commits == 0 { + return Err(too_large(format!( + "more than {RESTRICTED_SCAN_MAX_COMMITS} commits to scan" + ))); + } + self.remaining_commits -= 1; + Ok(()) + } + + /// Charges every changed file in `diff` before any content is loaded. + /// Sizes come from the object headers, which the agent cannot misstate + /// the way it can an index entry. + pub fn charge_diff( + &mut self, + repo: &git2::Repository, + diff: &git2::Diff, + ) -> Result<(), ScanInputValidationError> { + let odb = repo + .odb() + .map_err(|e| ScanInputValidationError::GitDiffProcessing { + message: e.message().to_string(), + })?; + for delta in diff.deltas() { + if delta.status() == git2::Delta::Deleted { + continue; + } + if self.remaining_files == 0 { + return Err(too_large(format!( + "more than {RESTRICTED_SCAN_MAX_FILES} changed files" + ))); + } + self.remaining_files -= 1; + + let file = delta.new_file(); + if file.id().is_zero() { + continue; + } + let (size, _) = odb.read_header(file.id()).map_err(|e| { + ScanInputValidationError::GitDiffProcessing { + message: e.message().to_string(), + } + })?; + let size = size as u64; + if size > RESTRICTED_SCAN_MAX_FILE_BYTES { + let path = file + .path() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_default(); + return Err(too_large(format!("'{path}' is larger than 1 MiB"))); + } + if size > self.remaining_bytes { + return Err(too_large("the changes total more than 32 MiB".to_owned())); + } + self.remaining_bytes -= size; + } + Ok(()) + } +} + +fn too_large(detail: String) -> ScanInputValidationError { + ScanInputValidationError::RestrictedScanTooLarge { detail } +} + +#[cfg(test)] +mod restricted_budget_tests { + use super::*; + + fn temp_repo() -> (std::path::PathBuf, git2::Repository) { + let dir = + std::env::temp_dir().join(format!("stashbase-scan-budget-{}", uuid::Uuid::new_v4())); + let repo = git2::Repository::init(&dir).unwrap(); + (dir, repo) + } + + fn diff_adding<'repo>( + repo: &'repo git2::Repository, + files: &[(&str, usize)], + ) -> git2::Diff<'repo> { + let mut builder = repo.treebuilder(None).unwrap(); + for (name, size) in files { + let blob = repo.blob(&vec![b'a'; *size]).unwrap(); + builder.insert(name, blob, 0o100644).unwrap(); + } + let tree = repo.find_tree(builder.write().unwrap()).unwrap(); + repo.diff_tree_to_tree(None, Some(&tree), None).unwrap() + } + + #[test] + fn budget_accepts_ordinary_changes() { + let (dir, repo) = temp_repo(); + let diff = diff_adding(&repo, &[("a.txt", 10), ("b.txt", 1024)]); + + assert!(RestrictedScanBudget::new() + .charge_diff(&repo, &diff) + .is_ok()); + let _ = fs::remove_dir_all(dir); + } + + #[test] + fn budget_rejects_a_file_over_the_per_file_cap() { + let (dir, repo) = temp_repo(); + let size = RESTRICTED_SCAN_MAX_FILE_BYTES as usize + 1; + let diff = diff_adding(&repo, &[("big.txt", size)]); + + let error = RestrictedScanBudget::new() + .charge_diff(&repo, &diff) + .unwrap_err(); + + assert!( + matches!(&error, ScanInputValidationError::RestrictedScanTooLarge { detail } if detail.contains("big.txt")), + "{error:?}" + ); + let _ = fs::remove_dir_all(dir); + } + + #[test] + fn budget_rejects_changes_over_the_total_cap_across_diffs() { + let (dir, repo) = temp_repo(); + let per_file = RESTRICTED_SCAN_MAX_FILE_BYTES as usize; + let files = (0..20) + .map(|index| (format!("f{index}.txt"), per_file)) + .collect::>(); + let files = files + .iter() + .map(|(name, size)| (name.as_str(), *size)) + .collect::>(); + let (first, second) = files.split_at(16); + let mut budget = RestrictedScanBudget::new(); + + // 16 MiB and then 4 MiB fit; another 16 MiB, as in a later + // commit's diff, crosses the 32 MiB total. + assert!(budget + .charge_diff(&repo, &diff_adding(&repo, first)) + .is_ok()); + assert!(budget + .charge_diff(&repo, &diff_adding(&repo, second)) + .is_ok()); + let error = budget + .charge_diff(&repo, &diff_adding(&repo, first)) + .unwrap_err(); + + assert!( + matches!(&error, ScanInputValidationError::RestrictedScanTooLarge { detail } if detail.contains("32 MiB")), + "{error:?}" + ); + let _ = fs::remove_dir_all(dir); + } + + #[test] + fn budget_caps_the_number_of_commits() { + let mut budget = RestrictedScanBudget::new(); + for _ in 0..RESTRICTED_SCAN_MAX_COMMITS { + budget.charge_commit().unwrap(); + } + + assert!(matches!( + budget.charge_commit(), + Err(ScanInputValidationError::RestrictedScanTooLarge { .. }) + )); + } +}