diff --git a/Cargo.lock b/Cargo.lock index 170e416..270c901 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -76,6 +76,16 @@ version = "1.0.103" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2a4385e2e34eb35d6b3efe798b9eb88096925d87726c0798709bf56d9ed84af3" +[[package]] +name = "assert-json-diff" +version = "2.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47e4f2b81832e72834d7518d8487a0396a28cc408186a2e8854c0f98011faf12" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "async-trait" version = "0.1.89" @@ -358,6 +368,7 @@ dependencies = [ "tokio", "tracing", "uuid", + "wiremock", ] [[package]] @@ -379,6 +390,24 @@ dependencies = [ "typenum", ] +[[package]] +name = "deadpool" +version = "0.12.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0be2b1d1d6ec8d846f05e137292d0b89133caf95ef33695424c09568bdd39b1b" +dependencies = [ + "deadpool-runtime", + "lazy_static", + "num_cpus", + "tokio", +] + +[[package]] +name = "deadpool-runtime" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "092966b41edc516079bdf31ec78a2e0588d1d0c08f78b91d8307215928642b2b" + [[package]] name = "deranged" version = "0.5.8" @@ -483,6 +512,21 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "futures" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b147ee9d1f6d097cef9ce628cd2ee62288d963e16fb287bd9286455b241382d" +dependencies = [ + "futures-channel", + "futures-core", + "futures-executor", + "futures-io", + "futures-sink", + "futures-task", + "futures-util", +] + [[package]] name = "futures-channel" version = "0.3.32" @@ -490,6 +534,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "07bbe89c50d7a535e539b8c17bc0b49bdb77747034daa8087407d655f3f7cc1d" dependencies = [ "futures-core", + "futures-sink", ] [[package]] @@ -498,6 +543,34 @@ version = "0.3.32" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7e3450815272ef58cec6d564423f6e755e25379b217b0bc688e295ba24df6b1d" +[[package]] +name = "futures-executor" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "baf29c38818342a3b26b5b923639e7b1f4a61fc5e76102d4b1981c6dc7a7579d" +dependencies = [ + "futures-core", + "futures-task", + "futures-util", +] + +[[package]] +name = "futures-io" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cecba35d7ad927e23624b22ad55235f2239cfa44fd10428eecbeba6d6a717718" + +[[package]] +name = "futures-macro" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e835b70203e41293343137df5c0664546da5745f82ec9b84d40be8336958447b" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "futures-sink" version = "0.3.32" @@ -516,8 +589,13 @@ version = "0.3.32" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "389ca41296e6190b48053de0321d02a77f32f8a5d2461dd38762c0593805c6d6" dependencies = [ + "futures-channel", "futures-core", + "futures-io", + "futures-macro", + "futures-sink", "futures-task", + "memchr", "pin-project-lite", "slab", ] @@ -598,6 +676,12 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" +[[package]] +name = "hermit-abi" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" + [[package]] name = "hex" version = "0.4.3" @@ -1069,6 +1153,16 @@ dependencies = [ "autocfg", ] +[[package]] +name = "num_cpus" +version = "1.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91df4bbde75afed763b708b7eee1e8e7651e02d97f6d5dd763e89367e957b23b" +dependencies = [ + "hermit-abi", + "libc", +] + [[package]] name = "once_cell" version = "1.21.4" @@ -1233,6 +1327,18 @@ dependencies = [ "bitflags", ] +[[package]] +name = "regex" +version = "1.12.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e10754a14b9137dd7b1e3e5b0493cc9171fdd105e0ab477f51b72e7f3ac0e276" +dependencies = [ + "aho-corasick", + "memchr", + "regex-automata", + "regex-syntax", +] + [[package]] name = "regex-automata" version = "0.4.14" @@ -2291,6 +2397,29 @@ dependencies = [ "memchr", ] +[[package]] +name = "wiremock" +version = "0.6.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08db1edfb05d9b3c1542e521aea074442088292f00b5f28e435c714a98f85031" +dependencies = [ + "assert-json-diff", + "base64", + "deadpool", + "futures", + "http", + "http-body-util", + "hyper", + "hyper-util", + "log", + "once_cell", + "regex", + "serde", + "serde_json", + "tokio", + "url", +] + [[package]] name = "wit-bindgen" version = "0.51.0" diff --git a/Cargo.toml b/Cargo.toml index 055beb6..78d7ca9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,3 +36,4 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] } uuid = { version = "1", features = ["v4"] } hex = "0.4" toml = "0.8" +wiremock = "0.6" diff --git a/crates/github/src/installation.rs b/crates/github/src/installation.rs index 803db0d..6f1b4f2 100644 --- a/crates/github/src/installation.rs +++ b/crates/github/src/installation.rs @@ -18,32 +18,83 @@ struct TokenResponse { token: String, } -/// Generates a fresh installation access token for the given installation ID. -/// Tokens expire after 1 hour; callers should cache and refresh. -pub async fn get_token(app_id: u64, private_key_pem: &str, installation_id: u64) -> Result { - get_token_with_base_url( +/// Authority role a scoped installation token is minted for (issue #4). +/// +/// Each role maps to the minimum GitHub App permission set for one phase of a +/// task's lifecycle, constrained to the single target repository. The agent +/// process only ever receives an `AgentGit` token; `Publication` is minted +/// after the result envelope has passed contract validation. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub enum TokenRole { + /// Adapter-held: resolve refs, drive Check Runs, post progress comments. + Orchestration, + /// Agent-held (`COVEN_GIT_TOKEN`): clone/fetch and push of the working + /// branch. No issues, pull-request, or checks authority. + AgentGit, + /// Adapter-held, minted post-validation: open the draft PR and post the + /// PR-opened comment. + Publication, +} + +impl TokenRole { + fn permissions(self) -> serde_json::Value { + match self { + TokenRole::Orchestration => serde_json::json!({ + "contents": "read", + "checks": "write", + "issues": "write", + "pull_requests": "read", + }), + TokenRole::AgentGit => serde_json::json!({ "contents": "write" }), + TokenRole::Publication => serde_json::json!({ + "contents": "read", + "issues": "write", + "pull_requests": "write", + }), + } + } +} + +/// Generates a fresh installation access token constrained to one repository +/// and one [`TokenRole`]'s permission set. Tokens expire after 1 hour. +pub async fn get_scoped_token( + app_id: u64, + private_key_pem: &str, + installation_id: u64, + repo_name: &str, + role: TokenRole, +) -> Result { + get_scoped_token_with_base_url( DEFAULT_API_BASE_URL, app_id, private_key_pem, installation_id, + repo_name, + role, ) .await } -pub async fn get_token_with_base_url( +pub async fn get_scoped_token_with_base_url( api_base_url: &str, app_id: u64, private_key_pem: &str, installation_id: u64, + repo_name: &str, + role: TokenRole, ) -> Result { - tracing::info!(installation_id, "generating installation access token"); + tracing::info!( + installation_id, + ?role, + "generating scoped installation access token" + ); let jwt = app_jwt(app_id, private_key_pem)?; let client = client()?; let response = send_json( &client, api_base_url, &jwt, - access_token_request(installation_id), + scoped_access_token_request(installation_id, repo_name, role), ) .await?; let body: TokenResponse = response.json().await?; @@ -70,11 +121,21 @@ fn jwt_claims(app_id: u64, now: u64) -> JwtClaims { } } -fn access_token_request(installation_id: u64) -> GitHubRequest { +fn scoped_access_token_request( + installation_id: u64, + repo_name: &str, + role: TokenRole, +) -> GitHubRequest { GitHubRequest { method: "POST", path: format!("/app/installations/{installation_id}/access_tokens"), - body: serde_json::json!({}), + // `repositories` takes bare repo names (not owner/name). Requesting a + // permission the App was not installed with fails with 422; the App + // manifest grants contents/checks/issues/pull_requests, all superset. + body: serde_json::json!({ + "repositories": [repo_name], + "permissions": role.permissions(), + }), } } @@ -82,14 +143,55 @@ fn access_token_request(installation_id: u64) -> GitHubRequest { mod tests { use super::*; + use serde_json::json; + #[test] - fn access_token_request_targets_installation_endpoint() { - let request = access_token_request(12345); + fn scoped_token_request_targets_installation_endpoint() { + let request = scoped_access_token_request(12345, "coven-code", TokenRole::Orchestration); assert_eq!(request.method, "POST"); assert_eq!(request.path, "/app/installations/12345/access_tokens"); } + #[test] + fn agent_git_scope_grants_contents_write_only() { + let request = scoped_access_token_request(1, "coven-code", TokenRole::AgentGit); + + assert_eq!(request.body["repositories"], json!(["coven-code"])); + assert_eq!(request.body["permissions"], json!({ "contents": "write" })); + } + + #[test] + fn orchestration_scope_cannot_write_contents_or_pulls() { + let request = scoped_access_token_request(1, "coven-code", TokenRole::Orchestration); + + assert_eq!(request.body["repositories"], json!(["coven-code"])); + assert_eq!( + request.body["permissions"], + json!({ + "contents": "read", + "checks": "write", + "issues": "write", + "pull_requests": "read", + }) + ); + } + + #[test] + fn publication_scope_grants_pr_write_without_checks_or_contents_write() { + let request = scoped_access_token_request(1, "coven-code", TokenRole::Publication); + + assert_eq!(request.body["repositories"], json!(["coven-code"])); + assert_eq!( + request.body["permissions"], + json!({ + "contents": "read", + "issues": "write", + "pull_requests": "write", + }) + ); + } + #[test] fn jwt_claims_use_app_id_as_issuer_and_short_expiry() { let now = 1_700_000_000; diff --git a/crates/worker/Cargo.toml b/crates/worker/Cargo.toml index 2696f6d..1d4a0dc 100644 --- a/crates/worker/Cargo.toml +++ b/crates/worker/Cargo.toml @@ -13,3 +13,6 @@ tracing.workspace = true uuid.workspace = true coven-github-api = { path = "../github" } coven-github-config = { path = "../config" } + +[dev-dependencies] +wiremock.workspace = true diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index a01e644..4cafc1e 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -7,12 +7,14 @@ use tokio::process::Command; use tracing::{error, info, warn}; use coven_github_api::{ - check_run, installation, pr, repo, tasks::TaskStore, ReviewEvidenceStatus, ReviewMode, - SessionResult, SessionStatus, Task, TaskKind, DEFAULT_API_BASE_URL, + check_run, installation, installation::TokenRole, pr, repo, tasks::TaskStore, + ReviewEvidenceStatus, ReviewMode, SessionResult, SessionStatus, Task, TaskKind, + DEFAULT_API_BASE_URL, }; use coven_github_config::{Config, FamiliarConfig}; pub mod brief; +pub mod redact; /// Base unit for exponential backoff between retry-safe coven-code attempts. /// Attempt `n` sleeps `RETRY_BACKOFF_BASE * 2^n` (so 2s, 4s, … in production). @@ -67,18 +69,21 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res // failed so it stays visible in Cave, then propagate. let prepared = async { let private_key = std::fs::read_to_string(&config.github.private_key_path)?; - let token = installation::get_token_with_base_url( - api_base_url, - config.github.app_id, - &private_key, - task.installation_id, - ) - .await?; + let minter = Minter::App { + api_base_url: api_base_url.to_string(), + app_id: config.github.app_id, + private_key, + installation_id: task.installation_id, + repo_name: task.repo_name.clone(), + }; + // Adapter-held orchestration authority: resolve refs, drive the Check + // Run, post progress comments. The agent never sees this token. + let orchestration = minter.mint(TokenRole::Orchestration).await?; // Resolve target refs and base branch from live GitHub state. Check Runs // must attach to an immutable commit SHA, and PRs must target the repo's // actual base branch rather than a hardcoded "main". - let targets = resolve_targets(api_base_url, &token, &task).await?; + let targets = resolve_targets(api_base_url, &orchestration, &task).await?; // Create Check Run against the resolved head SHA. From this point on the // Check Run MUST reach a terminal conclusion on every code path — a flaky @@ -88,7 +93,7 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res let details_url = cave_session_url(config, &task.id); let check_id = check_run::create_with_base_url( api_base_url, - &token, + &orchestration, &task.repo_owner, &task.repo_name, &targets.head_sha, @@ -96,11 +101,11 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res Some(details_url.as_str()), ) .await?; - Ok::<_, anyhow::Error>((token, targets, check_id)) + Ok::<_, anyhow::Error>((minter, orchestration, targets, check_id)) } .await; - let (token, targets, check_id) = match prepared { + let (minter, orchestration, targets, check_id) = match prepared { Ok(prepared) => prepared, Err(e) => { error!(task_id = %task.id, "pre-flight failed before check run: {e:#}"); @@ -124,7 +129,8 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res config, &task, familiar, - &token, + &minter, + &orchestration, api_base_url, &targets, &workspace, @@ -144,7 +150,7 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res .await; if let Err(e) = check_run::complete_with_base_url( api_base_url, - &token, + &orchestration, &task.repo_owner, &task.repo_name, check_id, @@ -162,13 +168,15 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res task_store.mark_failed(&task.id).await; if let Err(ce) = check_run::complete_with_base_url( api_base_url, - &token, + &orchestration, &task.repo_owner, &task.repo_name, check_id, check_run::CheckConclusion::Failure, "Error", - &format!("Task failed: {e}"), + // Error chains can embed response bodies; scrub token-shaped + // strings before they reach the published Check Run summary. + &redact::redact(&format!("Task failed: {e}"), &[&orchestration]), ) .await { @@ -186,6 +194,49 @@ struct Published { opened_pr: Option, } +/// Mints repo-scoped installation tokens for one task. Tests substitute +/// `Fixed` so publication paths run without GitHub App credentials. +pub(crate) enum Minter { + App { + api_base_url: String, + app_id: u64, + private_key: String, + installation_id: u64, + repo_name: String, + }, + #[cfg(test)] + Fixed(std::collections::HashMap), +} + +impl Minter { + async fn mint(&self, role: TokenRole) -> Result { + match self { + Minter::App { + api_base_url, + app_id, + private_key, + installation_id, + repo_name, + } => { + installation::get_scoped_token_with_base_url( + api_base_url, + *app_id, + private_key, + *installation_id, + repo_name, + role, + ) + .await + } + #[cfg(test)] + Minter::Fixed(tokens) => tokens + .get(&role) + .cloned() + .ok_or_else(|| anyhow::anyhow!("no fixed token for role {role:?}")), + } + } +} + /// Provisions the workspace, runs coven-code with the retry policy, and publishes /// the outcome (PR + comments). Returns `Err` only for failures that should mark /// the task — and complete the Check Run — as failed: workspace/brief I/O errors @@ -197,7 +248,8 @@ async fn run_and_publish( config: &Config, task: &Task, familiar: &FamiliarConfig, - token: &str, + minter: &Minter, + orchestration: &str, api_base_url: &str, targets: &ResolvedTargets, workspace: &Path, @@ -208,7 +260,14 @@ async fn run_and_publish( let brief = brief::build(task, familiar, workspace, &targets.default_branch); let brief_path = workspace.join("session-brief.json"); let result_path = workspace.join("result.json"); - tokio::fs::write(&brief_path, serde_json::to_string_pretty(&brief)?).await?; + let brief_json = serde_json::to_string_pretty(&brief)?; + // Belt-and-braces on top of the serialization guard test: refuse to hand + // the agent a brief that somehow embeds a live credential. + anyhow::ensure!( + !redact::contains_live_token(&brief_json, &[orchestration]), + "session brief contained a live token; refusing to write it" + ); + tokio::fs::write(&brief_path, brief_json).await?; // Best-effort "starting" comment — a flaky comment API call must not abort // the task or orphan the Check Run. @@ -216,7 +275,7 @@ async fn run_and_publish( let start_msg = starting_comment(config, familiar, &task.id); if let Err(e) = pr::post_comment_with_base_url( api_base_url, - token, + orchestration, &task.repo_owner, &task.repo_name, issue_number, @@ -231,7 +290,7 @@ async fn run_and_publish( // Best-effort progress transition; the check is completed regardless below. if let Err(e) = check_run::update_with_base_url( api_base_url, - token, + orchestration, &task.repo_owner, &task.repo_name, check_id, @@ -244,65 +303,58 @@ async fn run_and_publish( warn!(task_id = %task.id, "failed to mark check in progress: {e:#}"); } + // The agent's only credential: contents:write on the target repo, minted + // immediately before spawn and injected via COVEN_GIT_TOKEN (never JSON). + let agent_git = minter.mint(TokenRole::AgentGit).await?; + // Run coven-code. Only retry-safe failures (exit 2, timeout, signal) are // retried; exit 1 (gave up) and exit 3 (needs input) are terminal. - let result = run_session( + let mut result = run_session( config, &brief_path, &result_path, - token, + &agent_git, config.worker.max_retries, ) .await?; + // Scrub token values and token-shaped strings from the envelope before + // anything downstream persists or publishes it (task store, comments, + // PR body, Check Run output). + redact::sanitize_result(&mut result, &[orchestration, &agent_git]); + // Publish according to the terminal disposition of the result. let disp = disposition(&result); let mut opened_pr = None; if disp.open_pr { if let Some(branch) = &result.branch { - match pr::open_pull_request_with_base_url( - api_base_url, - token, - &task.repo_owner, - &task.repo_name, - branch, - &targets.base_ref, - &pr_title(&result, task), - &result.pr_body, - true, // draft - ) - .await - { - Ok(pr_num) => { - opened_pr = Some(pr_num); - if let Some(issue_number) = task_issue_number(&task.kind) { - let msg = pr_opened_comment(config, &task.id, pr_num); - if let Err(e) = pr::post_comment_with_base_url( - api_base_url, - token, - &task.repo_owner, - &task.repo_name, - issue_number, - &msg, - ) - .await - { - warn!(task_id = %task.id, "failed to post PR comment: {e:#}"); - } - } + // Write authority for publication is minted only now — after the + // envelope passed contract validation and sanitization (issue #4). + match minter.mint(TokenRole::Publication).await { + Ok(publication) => { + opened_pr = open_draft_pr( + config, + task, + api_base_url, + &publication, + targets, + &result, + branch, + ) + .await; } Err(e) => { - // The branch is already pushed; the PR just didn't open. - // Surface it rather than failing the whole task, so the work - // isn't lost from the user's view. - warn!(task_id = %task.id, "failed to open PR: {e:#}"); + warn!(task_id = %task.id, "failed to mint publication token: {e:#}"); if let Some(issue_number) = task_issue_number(&task.kind) { - let msg = format!( - "I pushed `{branch}` but could not open the PR automatically: {e}. Open the branch manually or check the App's pull-request permission." + let msg = redact::redact( + &format!( + "I pushed `{branch}` but could not obtain publication credentials to open the PR: {e}" + ), + &[orchestration], ); let _ = pr::post_comment_with_base_url( api_base_url, - token, + orchestration, &task.repo_owner, &task.repo_name, issue_number, @@ -321,7 +373,7 @@ async fn run_and_publish( let msg = format!("I need input before I can continue:\n\n{}", result.summary); if let Err(e) = pr::post_comment_with_base_url( api_base_url, - token, + orchestration, &task.repo_owner, &task.repo_name, issue_number, @@ -337,6 +389,76 @@ async fn run_and_publish( Ok(Published { result, opened_pr }) } +/// Opens the draft PR and posts the PR-opened comment with post-validation +/// publication authority. Best-effort: failures are surfaced on the issue +/// rather than failing the task, since the branch is already pushed. +async fn open_draft_pr( + config: &Config, + task: &Task, + api_base_url: &str, + publication: &str, + targets: &ResolvedTargets, + result: &SessionResult, + branch: &str, +) -> Option { + match pr::open_pull_request_with_base_url( + api_base_url, + publication, + &task.repo_owner, + &task.repo_name, + branch, + &targets.base_ref, + &pr_title(result, task), + &result.pr_body, + true, // draft + ) + .await + { + Ok(pr_num) => { + if let Some(issue_number) = task_issue_number(&task.kind) { + let msg = pr_opened_comment(config, &task.id, pr_num); + if let Err(e) = pr::post_comment_with_base_url( + api_base_url, + publication, + &task.repo_owner, + &task.repo_name, + issue_number, + &msg, + ) + .await + { + warn!(task_id = %task.id, "failed to post PR comment: {e:#}"); + } + } + Some(pr_num) + } + Err(e) => { + // The branch is already pushed; the PR just didn't open. Surface it + // rather than failing the whole task, so the work isn't lost from + // the user's view. + warn!(task_id = %task.id, "failed to open PR: {e:#}"); + if let Some(issue_number) = task_issue_number(&task.kind) { + let msg = redact::redact( + &format!( + "I pushed `{branch}` but could not open the PR automatically: {e}. Open the branch manually or check the App's pull-request permission." + ), + &[publication], + ); + let _ = pr::post_comment_with_base_url( + api_base_url, + publication, + &task.repo_owner, + &task.repo_name, + issue_number, + &msg, + ) + .await; + } + None + } + } +} + /// Terminal disposition of a completed session, derived purely from the result. /// /// This refines the coarse "success or failure" prose in the headless contract @@ -1334,3 +1456,188 @@ mod process_tests { let _ = fs::remove_dir_all(root); } } + +#[cfg(all(test, unix))] +mod publication_tests { + use super::*; + use coven_github_api::installation::TokenRole; + use coven_github_config::{FamiliarConfig, GitHubAppConfig, ServerConfig, WorkerConfig}; + use std::collections::HashMap; + use std::fs; + use std::os::unix::fs::PermissionsExt; + use std::path::PathBuf; + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + const ORCHESTRATION: &str = "ghs_orchestration0000000000000000000000"; + const AGENT_GIT: &str = "ghs_agentgit0000000000000000000000000000"; + const PUBLICATION: &str = "ghs_publication0000000000000000000000000"; + + fn fixed_minter() -> Minter { + Minter::Fixed(HashMap::from([ + (TokenRole::Orchestration, ORCHESTRATION.to_string()), + (TokenRole::AgentGit, AGENT_GIT.to_string()), + (TokenRole::Publication, PUBLICATION.to_string()), + ])) + } + + #[tokio::test] + async fn publication_uses_post_validation_token_and_leaks_nothing() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/repos/OpenCoven/demo/issues/42/comments")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 1}))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path("/repos/OpenCoven/demo/check-runs/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({}))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/repos/OpenCoven/demo/pulls")) + .respond_with( + ResponseTemplate::new(201).set_body_json(serde_json::json!({"number": 17})), + ) + .mount(&server) + .await; + + // Fake coven-code: records the git token it was handed, then emits a + // result that tries to leak that token through free-text fields. + let script = r#"#!/usr/bin/env bash +printf '%s' "$COVEN_GIT_TOKEN" > "$(dirname "$5")/seen-token" +cat > "$5" < Vec { + requests + .iter() + .filter(|r| r.url.path() == p && r.method.as_str() == m) + .map(|r| { + r.headers + .get("authorization") + .expect("authorization header present") + .to_str() + .expect("ascii header") + .to_string() + }) + .collect() + }; + assert_eq!( + auth_of("/repos/OpenCoven/demo/pulls", "POST"), + vec![format!("Bearer {PUBLICATION}")] + ); + assert_eq!( + auth_of("/repos/OpenCoven/demo/check-runs/7", "PATCH"), + vec![format!("Bearer {ORCHESTRATION}")] + ); + // Starting comment (orchestration), then PR-opened comment (publication). + assert_eq!( + auth_of("/repos/OpenCoven/demo/issues/42/comments", "POST"), + vec![ + format!("Bearer {ORCHESTRATION}"), + format!("Bearer {PUBLICATION}"), + ] + ); + + let _ = fs::remove_dir_all(root); + } +} diff --git a/crates/worker/src/redact.rs b/crates/worker/src/redact.rs new file mode 100644 index 0000000..205f6c9 --- /dev/null +++ b/crates/worker/src/redact.rs @@ -0,0 +1,241 @@ +//! Token-pattern scanning and redaction for agent-visible artifacts (issue #4). +//! +//! Every free-text field of the result envelope passes through here before the +//! adapter persists or publishes it (task store, comments, PR bodies, Check +//! Run output). Redaction is belt-and-braces on top of scoped tokens: a leaked +//! value has already been reduced to single-repo authority, but it must still +//! never reach a durable store or GitHub output. + +use coven_github_api::SessionResult; + +/// Replacement marker for redacted credentials. +pub const REDACTED: &str = "[redacted-token]"; + +/// GitHub token prefixes: installation, classic PAT, OAuth, user-to-server, +/// refresh, fine-grained PAT. +const PREFIXES: [&str; 6] = ["ghs_", "ghp_", "gho_", "ghu_", "ghr_", "github_pat_"]; + +/// Minimum credential-body length before a prefix hit is treated as a token. +/// Real GitHub credential bodies are 30+ chars; this avoids redacting prose +/// that merely mentions a prefix. +const MIN_TOKEN_BODY: usize = 20; + +fn token_char(c: char) -> bool { + c.is_ascii_alphanumeric() || c == '_' +} + +/// Replaces exact live token values, GitHub token patterns, and +/// `x-access-token:` URL credentials with [`REDACTED`]. +pub fn redact(text: &str, live_tokens: &[&str]) -> String { + let mut out = text.to_string(); + for token in live_tokens { + if !token.is_empty() { + out = out.replace(token, REDACTED); + } + } + // Credentialed clone URLs: scrub the whole userinfo segment. + out = redact_pattern( + &out, + "x-access-token:", + |c| c != '@' && c != '"' && !c.is_whitespace(), + 1, + ); + for prefix in PREFIXES { + out = redact_pattern(&out, prefix, token_char, MIN_TOKEN_BODY); + } + out +} + +/// True when `text` contains one of the adapter's live token values. +/// +/// Deliberately does NOT pattern-match: user-authored task content may +/// legitimately quote token-shaped strings (e.g. an issue reporting a leak), +/// and that must not veto writing the session brief. +pub fn contains_live_token(text: &str, live_tokens: &[&str]) -> bool { + live_tokens + .iter() + .any(|token| !token.is_empty() && text.contains(token)) +} + +/// Redacts every free-text field of a session result in place. +pub fn sanitize_result(result: &mut SessionResult, live_tokens: &[&str]) { + fix(&mut result.summary, live_tokens); + fix(&mut result.pr_body, live_tokens); + fix_opt(&mut result.branch, live_tokens); + for commit in &mut result.commits { + fix(&mut commit.sha, live_tokens); + fix(&mut commit.message, live_tokens); + } + for file in &mut result.files_changed { + fix(file, live_tokens); + } + let review = &mut result.review; + for file in review + .reviewed_files + .iter_mut() + .chain(review.supporting_files.iter_mut()) + { + fix(file, live_tokens); + } + for finding in &mut review.findings { + fix(&mut finding.file, live_tokens); + fix(&mut finding.title, live_tokens); + fix(&mut finding.body, live_tokens); + fix_opt(&mut finding.recommendation, live_tokens); + } + for test in &mut review.tests_run { + fix(&mut test.command, live_tokens); + fix_opt(&mut test.output_summary, live_tokens); + } + fix_opt(&mut review.no_findings_reason, live_tokens); + for limitation in &mut review.limitations { + fix(limitation, live_tokens); + } +} + +fn fix(text: &mut String, live_tokens: &[&str]) { + let redacted = redact(text, live_tokens); + if redacted != *text { + *text = redacted; + } +} + +fn fix_opt(text: &mut Option, live_tokens: &[&str]) { + if let Some(text) = text { + fix(text, live_tokens); + } +} + +/// Replaces every occurrence of `prefix` followed by at least `min_body` +/// consecutive `body_char` characters with [`REDACTED`]. +fn redact_pattern( + text: &str, + prefix: &str, + body_char: fn(char) -> bool, + min_body: usize, +) -> String { + let mut out = String::with_capacity(text.len()); + let mut rest = text; + while let Some(idx) = rest.find(prefix) { + let (head, tail) = rest.split_at(idx); + out.push_str(head); + let after = &tail[prefix.len()..]; + let body_len: usize = after + .chars() + .take_while(|&c| body_char(c)) + .map(char::len_utf8) + .sum(); + if body_len >= min_body { + out.push_str(REDACTED); + rest = &after[body_len..]; + } else { + out.push_str(prefix); + rest = after; + } + } + out.push_str(rest); + out +} + +#[cfg(test)] +mod tests { + use super::*; + use coven_github_api::{ + CommitInfo, ReviewEvidenceStatus, ReviewFinding, ReviewMode, ReviewResult, + ReviewSeverity, ReviewTestRun, ReviewTestStatus, SessionResult, SessionStatus, + HEADLESS_CONTRACT_VERSION, + }; + + #[test] + fn live_token_values_are_replaced() { + let out = redact("pushed with ghs_live123 ok", &["ghs_live123"]); + assert_eq!(out, format!("pushed with {REDACTED} ok")); + } + + #[test] + fn github_token_patterns_are_redacted_without_knowing_the_value() { + let text = "leaked ghs_AbCdEfGhIjKlMnOpQrStUvWxYz0123456789 in output"; + let out = redact(text, &[]); + assert_eq!(out, format!("leaked {REDACTED} in output")); + } + + #[test] + fn x_access_token_url_credentials_are_redacted() { + let text = "https://x-access-token:ghs_short@github.com/o/r.git"; + let out = redact(text, &[]); + assert_eq!(out, format!("https://{REDACTED}@github.com/o/r.git")); + } + + #[test] + fn fine_grained_pat_pattern_is_redacted() { + let out = redact("github_pat_11ABCDEFG0123456789abcdefghij tail", &[]); + assert_eq!(out, format!("{REDACTED} tail")); + } + + #[test] + fn short_prefix_lookalikes_are_left_alone() { + let text = "the ghs_ prefix and ghp_abc are not credentials"; + assert_eq!(redact(text, &[]), text); + } + + #[test] + fn contains_live_token_matches_exact_values_only() { + assert!(contains_live_token( + "brief with ghs_live123 inside", + &["ghs_live123"] + )); + // Pattern-shaped strings in user-authored text (e.g. an issue body + // quoting a leaked token) must NOT trip the live check — that would + // let issue content veto task execution. + assert!(!contains_live_token( + "quotes ghs_AbCdEfGhIjKlMnOpQrStUvWxYz0123456789", + &["ghs_other"] + )); + } + + #[test] + fn sanitize_result_scrubs_every_free_text_field() { + let tok = "ghs_liveTOKENliveTOKENliveTOKEN000"; + let poison = |field: &str| format!("{field} leaks {tok}"); + let mut result = SessionResult { + contract_version: HEADLESS_CONTRACT_VERSION.to_string(), + status: SessionStatus::Success, + branch: Some(poison("branch")), + commits: vec![CommitInfo { + sha: poison("sha"), + message: poison("message"), + }], + files_changed: vec![poison("file")], + summary: poison("summary"), + pr_body: poison("pr_body"), + review: ReviewResult { + mode: ReviewMode::PullRequest, + evidence_status: ReviewEvidenceStatus::Complete, + reviewed_files: vec![poison("reviewed")], + supporting_files: vec![poison("supporting")], + findings: vec![ReviewFinding { + severity: ReviewSeverity::Low, + file: poison("finding-file"), + line: None, + title: poison("finding-title"), + body: poison("finding-body"), + recommendation: Some(poison("finding-rec")), + }], + tests_run: vec![ReviewTestRun { + command: poison("command"), + status: ReviewTestStatus::Passed, + output_summary: Some(poison("output")), + }], + no_findings_reason: Some(poison("reason")), + limitations: vec![poison("limitation")], + }, + exit_reason: None, + }; + + sanitize_result(&mut result, &[tok]); + + let json = serde_json::to_string(&result).expect("result should serialize"); + assert!(!json.contains(tok), "sanitized result still leaked: {json}"); + assert!(json.contains(REDACTED)); + } +} diff --git a/docs/headless-contract.md b/docs/headless-contract.md index 3be3dd6..095d0cc 100644 --- a/docs/headless-contract.md +++ b/docs/headless-contract.md @@ -50,6 +50,11 @@ session brief, the result envelope, the clone URL, logs, or any durable artifact. The 1-hour token TTL is the adapter's concern; the runtime treats the token as opaque and valid for the session. +The adapter scopes this token to `contents: write` on the target repository +only (issue #4): it carries no issues, pull-request, or checks authority. This +narrows the credential without changing its shape, channel, or use, so it is a +non-breaking clarification within contract v2. + > **Drift note (supersedes `COVEN-GITHUB.md`):** earlier spec prose referenced > `GIT_ASKPASS` / `GIT_TOKEN` and an `auth.token` field embedded in the brief. > Those are **removed**. The brief is tokenless (issue #4) and the only git diff --git a/docs/security.md b/docs/security.md index 23401d6..af4b0ad 100644 --- a/docs/security.md +++ b/docs/security.md @@ -48,7 +48,7 @@ flowchart LR Worker rules: - Create one workspace per task. -- Pass only the installation token required for that repository. +- Pass the agent only a `contents:write` token scoped to that repository; orchestration and publication authority stay in the adapter. - Clean up workspaces after task completion or failure. - Enforce timeout and retry limits. - Persist failure states before cleanup. @@ -57,13 +57,22 @@ See [Container Isolation](container-isolation.md) for the production isolation t ## Token Handling -The session brief is **tokenless**: it carries read context only and never embeds an installation token or a credentialed `clone_url`. Git authentication is injected into the `coven-code` child process out-of-band through the `COVEN_GIT_TOKEN` environment variable, which is never written to `session-brief.json` or any durable artifact. GitHub write authority (comments, Check Runs, branches, PRs) stays with the adapter behind its publication gate; the agent emits a result envelope and the adapter publishes. A serialization test fails if the brief ever serializes an `auth`/`token` field or an `x-access-token` clone URL. +The session brief is **tokenless**: it carries read context only and never embeds an installation token or a credentialed `clone_url`. A serialization test fails if the brief ever serializes an `auth`/`token` field or an `x-access-token` clone URL, and the worker refuses to write a brief containing a live token value. + +Per task, the adapter mints **three separately-scoped installation tokens**, each constrained to the single target repository: + +| Role | Permissions | Held by | Used for | +|---|---|---|---| +| Orchestration | `contents:read`, `checks:write`, `issues:write`, `pull_requests:read` | adapter | ref resolution, Check Run lifecycle, progress and needs-input comments | +| Agent git | `contents:write` | `coven-code` child (via `COVEN_GIT_TOKEN` env, never JSON) | clone/fetch and pushing the working branch — the contract's only permitted agent-side GitHub write | +| Publication | `contents:read`, `issues:write`, `pull_requests:write` | adapter, minted **after** the result envelope passes contract validation | opening the draft PR and the PR-opened comment | + +Every free-text field of the result envelope is scanned and redacted (live token values, GitHub token patterns, `x-access-token` URLs) before it reaches the task store, comments, PR bodies, or Check Run output. Error strings embedded in published comments and Check Run summaries pass through the same redaction. Remaining hardening targets: -- Prefer `GIT_ASKPASS` / credential-helper injection over a plain env var so the token never appears in child process listings. -- Avoid writing installation tokens to durable task stores. -- Redact tokens in logs, Check Runs, issue comments, PR bodies, and task APIs. +- Contract v3: move the branch push into the adapter so the agent token can drop to read-only entirely (coordinated with coven-code). +- Prefer `GIT_ASKPASS` / credential-helper injection over a plain env var (a contract change; the env channel is locked in contract v2). - Refresh installation tokens through a single token manager with cache expiry. ## Tenant Data