diff --git a/README.md b/README.md index d7b57195..22794f56 100644 --- a/README.md +++ b/README.md @@ -776,7 +776,7 @@ A finding's title becomes the advisory summary. Its description preserves its wr `security-review`, or `review = true` under `[security]`, enables one guidance review after a Run's opening session or a Spec run's Spec review, before Delivery pushes and enters the Repair loop. A Spec run reviews the whole Spec branch against its Base branch once; its Tickets' Runs get none. `no-security-review` overrides the setting. Both words, with or without dashes, are accepted on commands that start Runs and follow dispatched Runs and Base fixes. Setup asks about the setting; it is off by default. -The session reads the relevant attack-class guidance and supporting code without delegated auditors or the full audit workflow. It fixes only findings reproduced by a failing proof-of-concept test, keeps that test and commits the fix. Unaddressed introduced findings appear under Security in the PR's Unaddressed findings. A Merge run, including a Spec run asked to merge its Spec PR, leaves the PR ready for review and fails with the findings named if any remain, or if the review was refused or incomplete. A Run or Spec run not asked to merge records the outcome and continues. Pre-existing vulnerability details stay out of the PR; private recording of old findings is tracked in [#552](https://github.com/JacobStephens2/thirdshift/issues/552). The separate Security audit retains its full audit workflow. +The session reads the relevant attack-class guidance and supporting code without delegated auditors or the full audit workflow. It fixes only findings reproduced by a failing proof-of-concept test, keeps that test and commits the fix. Unaddressed introduced findings appear under Security in the PR's Unaddressed findings. A Merge run, including a Spec run asked to merge its Spec PR, leaves the PR ready for review and fails with the findings named if any remain, or if the review was refused or incomplete. A Run or Spec run not asked to merge records the outcome and continues. An old finding, whose proof-of-concept also fails at the pinned merge base, is reported in a private file under the repository's logs (`security-reviews/`). thirdshift records it as a draft advisory, or a `security-finding`/`needs-triage` issue on a private repository, reusing existing fingerprints and preserving the Day shift's grades. New records include scored reproduction and fix-size metadata so a later Security run can take the fix when allowed. Its details stay out of the PR and it does not hold Self-merge; fixing belongs to a separate Security run. The separate Security audit retains its full audit workflow. ### Fencing diff --git a/prompts/security-review.md b/prompts/security-review.md index 21b87686..5d91e0f2 100644 --- a/prompts/security-review.md +++ b/prompts/security-review.md @@ -6,16 +6,19 @@ When enabled, runs one guidance review after a Run's opening session or a Spec r ``` Use the `thirdshift-security-audit` skill in guidance mode. -Review the change for on branch with `git diff ...HEAD`, including supporting code. +Review the change for on branch against Base branch with `git diff ...HEAD`, including supporting code. Read the relevant security attack-class guidance and the repository's SECURITY.md or threat model when present. Use one session. Do not delegate auditors or run the full six-phase audit, validators, coverage ledger or audit artifacts. Fix only a Security finding you can show with a failing proof-of-concept test; run it before fixing, keep it as a regression test, and rerun it afterward. Use harmless local payloads; touch no deployed site or real third-party service. Commit each fix. -Classify introduced findings against the merge base: a proof-of-concept that also fails there is pre-existing. Never include pre-existing vulnerability details in the pull request or final message, and do not fix those here. +Merge base commit: ``. +Classify introduced findings against this pinned merge base: run each proof-of-concept on HEAD and the untouched merge base; a test that also fails there is pre-existing. Never include pre-existing vulnerability details, titles, fingerprints, tests or private-record links in the pull request or final message, and do not fix those here. +Private report file: ``. +Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence and scoring rationale, severity: critical/high/medium/low/informational scored with the skill's likelihood-and-impact rubric, fix_size: single/spec judged from whether one session can hold the fix). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching and reproduced-outcome format so a later Security run can fix it when allowed; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run. Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session. If refused, incomplete, or missing a prerequisite, say so and do not claim completion. After a complete review, end your final message with exactly one line of the form: -Security review: {"unaddressed_count":0,"findings":[]} -Count only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. No old-finding details belong in it. +Security review: {"unaddressed_count":0,"findings":[],"pre_existing_count":0} +unaddressed_count counts only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. pre_existing_count is the number of entries in the private report. No old-finding details belong in this line. You run headless: nobody is watching, and ending your turn ends the session. Run tests and other long commands in the foreground, raising the command's timeout if needed. If a command is moved to the background, wait for that task by its own task id or output file, never by process names or patterns (`pgrep`, `ps | grep`, and the like): other sessions on this machine run the same commands. Never end your turn while a background task you depend on is still running: ending the turn kills it. Before ending your turn, stop every background task you no longer need, by its task id (with the `TaskStop` tool, if you have it): a task still running when your turn ends is taken as work you were waiting on. ``` diff --git a/site/prompts/index.html b/site/prompts/index.html index 8395e54a..1244d2a8 100644 --- a/site/prompts/index.html +++ b/site/prompts/index.html @@ -163,16 +163,19 @@

Security review

When enabled, runs one guidance review after a Run's opening session or a Spec run's Spec review, before Delivery pushes and enters the Repair loop. Ticket Runs get none; Base fixes get one too. Introduced findings, refusals or incomplete reviews hold Self-merge.

Sent by 3 · Y · Review

Use the `thirdshift-security-audit` skill in guidance mode.
-Review the change for <Issue URL> on branch <branch> with `git diff <base>...HEAD`, including supporting code.
+Review the change for <Issue URL> on branch <branch> against Base branch <base> with `git diff <merge base commit>...HEAD`, including supporting code.
 Read the relevant security attack-class guidance and the repository's SECURITY.md or threat model when present.
 Use one session. Do not delegate auditors or run the full six-phase audit, validators, coverage ledger or audit artifacts.
 Fix only a Security finding you can show with a failing proof-of-concept test; run it before fixing, keep it as a regression test, and rerun it afterward. Use harmless local payloads; touch no deployed site or real third-party service. Commit each fix.
-Classify introduced findings against the merge base: a proof-of-concept that also fails there is pre-existing. Never include pre-existing vulnerability details in the pull request or final message, and do not fix those here.
+Merge base commit: `<merge base commit>`.
+Classify introduced findings against this pinned merge base: run each proof-of-concept on HEAD and the untouched merge base; a test that also fails there is pre-existing. Never include pre-existing vulnerability details, titles, fingerprints, tests or private-record links in the pull request or final message, and do not fix those here.
+Private report file: `<private report file>`.
+Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence and scoring rationale, severity: critical/high/medium/low/informational scored with the skill's likelihood-and-impact rubric, fix_size: single/spec judged from whether one session can hold the fix). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching and reproduced-outcome format so a later Security run can fix it when allowed; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run.
 Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session.
 If refused, incomplete, or missing a prerequisite, say so and do not claim completion.
 After a complete review, end your final message with exactly one line of the form:
-Security review: {"unaddressed_count":0,"findings":[]}
-Count only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. No old-finding details belong in it.
+Security review: {"unaddressed_count":0,"findings":[],"pre_existing_count":0}
+unaddressed_count counts only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. pre_existing_count is the number of entries in the private report. No old-finding details belong in this line.
 
 You run headless: nobody is watching, and ending your turn ends the session. Run tests and other long commands in the foreground, raising the command's timeout if needed. If a command is moved to the background, wait for that task by its own task id or output file, never by process names or patterns (`pgrep`, `ps | grep`, and the like): other sessions on this machine run the same commands. Never end your turn while a background task you depend on is still running: ending the turn kills it. Before ending your turn, stop every background task you no longer need, by its task id (with the `TaskStop` tool, if you have it): a task still running when your turn ends is taken as work you were waiting on.
diff --git a/src/delivery.rs b/src/delivery.rs index 2194855b..534ad960 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -376,12 +376,7 @@ impl Outside for InWorktree<'_> { } fn security_review(&mut self, prompt: &str) -> Result { - let message = self.sessions.run_to_final_message( - crate::session::Purpose::Security, - "security-review", - prompt, - )?; - Review::from_final_message(message.as_deref()) + crate::security::review::run(self.sessions, self.worktree, self.issue, self.base, prompt) } fn record_security_review(&mut self, review: &SecurityOutcome) -> Result<()> { @@ -634,7 +629,7 @@ mod tests { } fn security_review(&mut self, _: &str) -> Result { Review::from_final_message(Some( - "Security review: {\"unaddressed_count\":0,\"findings\":[]}", + "Security review: {\"unaddressed_count\":0,\"findings\":[],\"pre_existing_count\":0}", )) } fn catch_up(&mut self) -> Result<()> { diff --git a/src/prompt.rs b/src/prompt.rs index 81c78823..be6e566e 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -316,19 +316,25 @@ pub fn security_audit( /// One guidance session on the Run's change, before Delivery pushes and /// enters the Repair loop. The separate Security audit remains a full audit. +pub const SECURITY_REVIEW_REPORT_FILE: &str = ""; +pub const SECURITY_REVIEW_MERGE_BASE: &str = ""; + pub fn security_review(issue: &IssueUrl, base: &str, branch: &str) -> String { format!( "Use the `thirdshift-security-audit` skill in guidance mode.\n\ - Review the change for {url} on branch {branch} with `git diff {base}...HEAD`, including supporting code.\n\ + Review the change for {url} on branch {branch} against Base branch {base} with `git diff {SECURITY_REVIEW_MERGE_BASE}...HEAD`, including supporting code.\n\ Read the relevant security attack-class guidance and the repository's SECURITY.md or threat model when present.\n\ Use one session. Do not delegate auditors or run the full six-phase audit, validators, coverage ledger or audit artifacts.\n\ Fix only a Security finding you can show with a failing proof-of-concept test; run it before fixing, keep it as a regression test, and rerun it afterward. Use harmless local payloads; touch no deployed site or real third-party service. Commit each fix.\n\ - Classify introduced findings against the merge base: a proof-of-concept that also fails there is pre-existing. Never include pre-existing vulnerability details in the pull request or final message, and do not fix those here.\n\ + Merge base commit: `{SECURITY_REVIEW_MERGE_BASE}`.\n\ + Classify introduced findings against this pinned merge base: run each proof-of-concept on HEAD and the untouched merge base; a test that also fails there is pre-existing. Never include pre-existing vulnerability details, titles, fingerprints, tests or private-record links in the pull request or final message, and do not fix those here.\n\ + Private report file: `{SECURITY_REVIEW_REPORT_FILE}`.\n\ + Write a JSON array to that file, even when empty ([]). Each entry is an old finding with exactly these fields: fingerprint (stable across reviews and audits, no newlines or backticks), title (one line), description (the private write-up and evidence), proof_of_concept (test: full test text, command: exact command run at both commits, head_exit_code: positive failing exit code, merge_base_exit_code: positive failing exit code, notes: reproduction evidence and scoring rationale, severity: critical/high/medium/low/informational scored with the skill's likelihood-and-impact rubric, fix_size: single/spec judged from whether one session can hold the fix). All text fields are nonempty. Do not commit or copy this report into the worktree. thirdshift records it privately with the Security run's fingerprint matching and reproduced-outcome format so a later Security run can fix it when allowed; existing records and the Day shift's grades are preserved. Fixing old findings belongs to a separate Security run.\n\ Update this branch's pull request: list each unaddressed introduced finding under Security in its Unaddressed findings, with evidence and why it is unaddressed. Preserve Standards and Spec entries and the rest of the body. Do not merge. Delivery pushes any new commits and marks the PR ready after this session.\n\ If refused, incomplete, or missing a prerequisite, say so and do not claim completion.\n\ After a complete review, end your final message with exactly one line of the form:\n\ - Security review: {{\"unaddressed_count\":0,\"findings\":[]}}\n\ - Count only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. No old-finding details belong in it.\n\n\ + Security review: {{\"unaddressed_count\":0,\"findings\":[],\"pre_existing_count\":0}}\n\ + unaddressed_count counts only unaddressed findings introduced by this change; findings is an array of their short titles, with one title per finding. pre_existing_count is the number of entries in the private report. No old-finding details belong in this line.\n\n\ {HEADLESS}", url = issue.url, ) diff --git a/src/prompts_page.rs b/src/prompts_page.rs index 3c21e202..37b29adf 100644 --- a/src/prompts_page.rs +++ b/src/prompts_page.rs @@ -60,7 +60,7 @@ const SESSION_ID: &str = ""; const PROMPT: &str = ""; const MODEL: &str = ""; const EFFORT: &str = ""; -const PLACEHOLDERS: [&str; 18] = [ +const PLACEHOLDERS: [&str; 20] = [ ISSUE_URL, SPEC_URL, NUMBER, @@ -79,6 +79,8 @@ const PLACEHOLDERS: [&str; 18] = [ MODEL, EFFORT, prompt::REVIEW_REPORTS_DIRECTORY, + prompt::SECURITY_REVIEW_REPORT_FILE, + prompt::SECURITY_REVIEW_MERGE_BASE, ]; /// Who sends a prompt. diff --git a/src/security/audit.rs b/src/security/audit.rs index 4f7565a6..539b52c5 100644 --- a/src/security/audit.rs +++ b/src/security/audit.rs @@ -219,7 +219,7 @@ fn node(args: &[&str]) -> Result<()> { } /// Manifest identity only: no affected or patched versions are inferred. -fn package_in(worktree: &Path) -> Package { +pub(super) fn package_in(worktree: &Path) -> Package { for (file, section, ecosystem) in [ ("Cargo.toml", "package", "rust"), ("pyproject.toml", "project", "pip"), diff --git a/src/security/review.rs b/src/security/review.rs index b888fa7d..9defe469 100644 --- a/src/security/review.rs +++ b/src/security/review.rs @@ -2,10 +2,151 @@ //! appear here; pre-existing vulnerability details stay out of public text. use anyhow::{Context, Result, bail}; -use serde::Deserialize; -use std::fmt; +use serde::{Deserialize, Serialize}; +use std::{fmt, fs}; +use super::reproduction::{FixSize, Outcome as ReproductionOutcome, Reproduction, Severity}; +use crate::github::{DraftAdvisory, GitHub}; use crate::harness::interpretation::SafeguardRefusal; +use crate::issue::IssueUrl; +use crate::session::{Purpose, Sessions}; +use crate::worktree::Worktree; +use crate::{logs, progress, prompt}; + +/// A fresh report outside the worktree, retained even when the session or +/// private recording fails. Only the introduced titles reach Delivery. +pub(crate) fn run( + sessions: &Sessions<'_>, + worktree: &Worktree, + issue: &IssueUrl, + base: &str, + text: &str, +) -> Result { + let merge_base = worktree.merge_base_commit(base)?; + let root = logs::root(&issue.repo()).join("security-reviews"); + fs::create_dir_all(&root)?; + let directory = tempfile::Builder::new() + .prefix(&format!("{}-{}-", issue.number, logs::stamp())) + .tempdir_in(root)? + .keep(); + let report = directory.join("pre-existing.json"); + let text = text + .replace( + prompt::SECURITY_REVIEW_REPORT_FILE, + &report.to_string_lossy(), + ) + .replace(prompt::SECURITY_REVIEW_MERGE_BASE, &merge_base); + progress::step(format!( + "Security review private report: {}", + report.display() + )); + let message = sessions.run_to_final_message(Purpose::Security, "security-review", &text)?; + // A session may change commits on the Issue branch, but it cannot hand + // us a replacement checkout to observe or publish findings from. + worktree.head()?; + let review = Review::from_final_message(message.as_deref())?; + let findings: Vec = serde_json::from_slice(&fs::read(report)?)?; + if findings.len() != review.pre_existing_count { + bail!("Security review's private report does not match its final count"); + } + // Validate the entire report before creating any record. Malformed report + // diagnostics are never passed into the public outcome. + let findings = findings + .into_iter() + .map(|finding| { + let reproduction = finding.reproduction()?; + Ok((finding, reproduction)) + }) + .collect::>>()?; + if !findings.is_empty() { + let github = GitHub::new(); + let repo = issue.repo_slug(); + let mut known = github.security_records(&repo)?; + let package = super::audit::package_in(worktree.path()); + for (finding, reproduction) in findings { + let draft = DraftAdvisory { + fingerprint: finding.fingerprint.clone(), + summary: finding.title.clone(), + description: reproduction.description(&format!( + "Found by thirdshift's Security review.\n\nFingerprint: `{}`\nAudited commit: `{merge_base}`\nReview issue: {}\n\n{}\n\n```json\n{}\n```\n", + finding.fingerprint, + issue.url, + finding.description, + serde_json::to_string_pretty(&finding)? + )), + package: package.clone(), + }; + // Reuse every record state and preserve the Day shift's grade. + if known.finding(&draft).is_none() { + let record = github.create_security_record(&repo, &known, &draft)?; + known.remember(record); + progress::step("recorded a pre-existing Security finding privately"); + } + } + } + Ok(review) +} + +#[derive(Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +struct OldFinding { + fingerprint: String, + title: String, + description: String, + proof_of_concept: ProofOfConcept, +} + +#[derive(Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +struct ProofOfConcept { + test: String, + command: String, + head_exit_code: i32, + merge_base_exit_code: i32, + notes: String, + severity: String, + fix_size: String, +} + +impl OldFinding { + fn reproduction(&self) -> Result { + let proof = &self.proof_of_concept; + if [ + &self.fingerprint, + &self.title, + &self.description, + &proof.test, + &proof.command, + &proof.notes, + ] + .iter() + .any(|text| text.trim().is_empty()) + || self.fingerprint.contains(['\r', '\n', '`']) + || self.title.contains(['\r', '\n']) + || proof.head_exit_code <= 0 + || proof.merge_base_exit_code <= 0 + { + bail!( + "Security review's private report lacks a valid old finding and failing proof-of-concept runs" + ); + } + let severity = Severity::parse(&proof.severity) + .context("Security review's private report has an invalid reproduced severity")?; + let size = match proof.fix_size.as_str() { + "single" => FixSize::Single, + "spec" => FixSize::Spec, + _ => bail!("Security review's private report has an invalid reproduced fix size"), + }; + Ok(Reproduction { + outcome: ReproductionOutcome::Reproduced { severity, size }, + notes: format!( + "Command: {}\nHEAD exit code: {}\nMerge base exit code: {}\n\n{}", + proof.command, proof.head_exit_code, proof.merge_base_exit_code, proof.notes + ), + test: proof.test.clone(), + }) + } +} #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum Ask { @@ -73,6 +214,7 @@ impl std::error::Error for Hold {} pub(crate) struct Review { unaddressed_count: usize, pub findings: Vec, + pre_existing_count: usize, } impl Review { diff --git a/src/worktree.rs b/src/worktree.rs index 3f0dea0f..610face7 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -204,6 +204,14 @@ impl Worktree { .ordinary(&self.launch, |operation| operation.head()) } + /// Observe the merge base against the published Base branch only while + /// this is still the acquired checkout on the selected Issue branch. + pub fn merge_base_commit(&self, base: &str) -> Result { + self.checkout.ordinary(&self.launch, |operation| { + operation.run(&["merge-base", &format!("refs/remotes/origin/{base}"), "HEAD"]) + }) + } + /// Fetch and sample `origin/`, then merge that commit into the /// Issue branch. A clean outcome includes the selected commit for CI /// comparison, including when the merge had nothing to do. diff --git a/tests/security_review.rs b/tests/security_review.rs index 7b5d118a..1fc324d0 100644 --- a/tests/security_review.rs +++ b/tests/security_review.rs @@ -1,5 +1,6 @@ mod support; +use serde_json::json; use support::Scenario; const OPENS_PR: &str = r#" @@ -9,7 +10,272 @@ git commit -q -m 'Add feature' gh pr create --base main --head issue-7 --title 'Add feature' --body 'Closes #7' "#; -const CLEAN: &str = r#"printf '%s\n' 'Security review: {"unaddressed_count":0,"findings":[]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#; +fn old_finding() -> serde_json::Value { + json!({ + "fingerprint": "old-input-bound", + "title": "Old unchecked bound", + "description": "Private old vulnerability evidence.", + "proof_of_concept": { + "test": "assert_bounded_input();", + "command": "cargo test bounded_input", + "head_exit_code": 101, + "merge_base_exit_code": 101, + "notes": "The bounded local fixture fails at both commits. Low likelihood and low impact; one session can add a bound.", + "severity": "low", + "fix_size": "single" + } + }) +} + +fn review_script(old: &serde_json::Value, introduced: &[&str]) -> String { + format!( + r#" +printf '%s' "$FAKE_CLAUDE_PROMPT" > review-prompt.txt +report=$(sed -n 's/^Private report file: `\(.*\)`\.$/\1/p' review-prompt.txt) +test -n "$report" +printf '%s\n' '{old}' > "$report" +printf '%s\n' 'Security review: {outcome}' > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + outcome = json!({ + "unaddressed_count": introduced.len(), + "findings": introduced, + "pre_existing_count": old.as_array().unwrap().len() + }) + ) +} + +#[test] +fn an_old_finding_is_recorded_privately_without_holding_self_merge() { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, &review_script(&json!([old_finding()]), &[])); + let result = scenario.run(&[ + &scenario.issue_url(7), + "security-review", + "merge", + "security-fix", + ]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let state = scenario.gh_state(); + let records = state["advisories"] + .as_array() + .expect("private draft advisory"); + assert_eq!(records.len(), 1); + assert_eq!(records[0]["state"], "draft"); + assert!(records[0]["severity"].is_null()); + let description = records[0]["description"].as_str().unwrap(); + assert!(description.contains("Fingerprint: `old-input-bound`")); + assert!(description.contains("Private old vulnerability evidence.")); + assert!(description.contains("assert_bounded_input();")); + assert!(description.contains(scenario.launch_git(&["rev-parse", "main"]).trim())); + assert_eq!(state["prs"][0]["state"], "MERGED"); + assert_eq!( + scenario.claude_calls().len(), + 2, + "old findings belong to a Security run" + ); + for private in [ + "Old unchecked bound", + "old-input-bound", + "Private old vulnerability evidence.", + "assert_bounded_input();", + ] { + assert!(!state["prs"][0]["body"].as_str().unwrap().contains(private)); + assert!(!result.stderr.contains(private)); + assert!(!result.stdout.contains(private)); + } + let reports = scenario.entries("home/.thirdshift/logs/acme/widgets/security-reviews"); + assert_eq!(reports.len(), 1); + let kept: serde_json::Value = serde_json::from_slice( + &std::fs::read(scenario.path(&format!( + "home/.thirdshift/logs/acme/widgets/security-reviews/{}/pre-existing.json", + reports[0] + ))) + .unwrap(), + ) + .unwrap(); + assert_eq!(kept, json!([old_finding()])); +} + +#[test] +fn mixed_old_and_introduced_findings_keep_the_introduced_hold_and_old_details_private() { + for harness in ["claude", "codex"] { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session( + 2, + &review_script(&json!([old_finding()]), &["Introduced cross-tenant read"]), + ); + let result = scenario.run(&[ + &scenario.issue_url(7), + "security-review", + "merge", + "harness", + harness, + ]); + assert_eq!(result.code, Some(1), "{harness}: {}", result.stderr); + let state = scenario.gh_state(); + assert_eq!(state["advisories"].as_array().unwrap().len(), 1); + assert_eq!(state["prs"][0]["state"], "OPEN"); + assert_eq!(state["prs"][0]["isDraft"], false); + let body = state["prs"][0]["body"].as_str().unwrap(); + assert!(body.contains("Introduced cross-tenant read")); + assert!(result.stderr.contains("Introduced cross-tenant read")); + for private in [ + "Old unchecked bound", + "old-input-bound", + "Private old vulnerability evidence.", + "assert_bounded_input();", + ] { + assert!(!body.contains(private)); + assert!(!result.stderr.contains(private)); + } + } +} + +#[test] +fn repeated_fingerprints_reuse_private_records_and_preserve_day_shift_grades() { + for (private, record_state) in [ + (false, "draft"), + (false, "published"), + (false, "closed"), + (true, "OPEN"), + (true, "CLOSED"), + ] { + let scenario = Scenario::new(); + let mut state = scenario.gh_state(); + state["private"] = json!(private); + scenario.write_gh_state(&state); + scenario.agent_does_in_session(1, OPENS_PR); + // Duplicates within one report must match just as later reviews do. + scenario.agent_does_in_session( + 2, + &review_script(&json!([old_finding(), old_finding()]), &[]), + ); + let first = scenario.run(&[&scenario.issue_url(7), "security-review", "no-merge"]); + assert_eq!( + first.code, + Some(0), + "{private} {record_state}: {}", + first.stderr + ); + let mut state = scenario.gh_state(); + if private { + assert!(state["advisories"].is_null()); + assert_eq!(state["issues"].as_object().unwrap().len(), 2); + assert_eq!( + state["labels"]["8"], + json!(["security-finding", "needs-triage"]) + ); + assert!( + state["bodies"]["8"] + .as_str() + .unwrap() + .contains("Private old vulnerability evidence.") + ); + state["issues"]["8"] = json!(record_state); + state["labels"]["8"] = json!(["security-finding"]); + } else { + assert_eq!(state["advisories"].as_array().unwrap().len(), 1); + state["advisories"][0]["state"] = json!(record_state); + state["advisories"][0]["severity"] = json!("low"); + } + scenario.write_gh_state(&state); + scenario.agent_does_in_session(3, "true"); + let mut repeated = old_finding(); + repeated["description"] = json!("A later review must not replace the grade or write-up."); + scenario.agent_does_in_session(4, &review_script(&json!([repeated]), &[])); + let second = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!( + second.code, + Some(0), + "{private} {record_state}: {}", + second.stderr + ); + let after = scenario.gh_state(); + assert_eq!(after["prs"][0]["state"], "MERGED"); + if private { + assert_eq!(after["issues"]["8"], state["issues"]["8"]); + assert_eq!(after["labels"]["8"], state["labels"]["8"]); + assert_eq!(after["bodies"]["8"], state["bodies"]["8"]); + assert_eq!(after["issues"].as_object().unwrap().len(), 2); + } else { + assert_eq!(after["advisories"], state["advisories"]); + } + let body = after["prs"][0]["body"].as_str().unwrap(); + assert!(!body.contains("Old unchecked bound")); + assert!(!body.contains("old-input-bound")); + } +} + +#[test] +fn incomplete_private_reports_hold_self_merge_without_publishing_report_diagnostics() { + let valid = review_script(&json!([old_finding()]), &[]); + let mut passes_at_base = old_finding(); + passes_at_base["proof_of_concept"]["merge_base_exit_code"] = json!(0); + let mut no_test = old_finding(); + no_test["proof_of_concept"]["test"] = json!(""); + for script in [ + valid.replace("\"pre_existing_count\":1", "\"pre_existing_count\":0"), + format!("{valid}\nrm \"$report\"\n"), + format!("{valid}\nprintf '%s' 'Private old vulnerability evidence.' > \"$report\"\n"), + review_script(&json!([passes_at_base]), &[]), + review_script(&json!([no_test]), &[]), + ] { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, &script); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + let state = scenario.gh_state(); + assert!(state["advisories"].is_null()); + assert_eq!(state["prs"][0]["state"], "OPEN"); + assert_eq!(state["prs"][0]["isDraft"], false); + let body = state["prs"][0]["body"].as_str().unwrap(); + assert!(body.contains("Security review incomplete")); + assert!(!body.contains("Private old vulnerability evidence.")); + assert!( + !result + .stderr + .contains("Private old vulnerability evidence.") + ); + } +} + +#[test] +fn review_uses_the_published_base_when_the_local_base_is_stale() { + let scenario = Scenario::new(); + scenario.origin_has_commit( + "main", + "base-new.txt", + "base change", + "Advance the Base branch", + ); + let base = scenario.origin_git(&["rev-parse", "main"]); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session( + 2, + &format!( + r#" +printf '%s' "$FAKE_CLAUDE_PROMPT" > review-prompt.txt +merge_base=$(sed -n 's/^Merge base commit: `\(.*\)`\.$/\1/p' review-prompt.txt) +test "$merge_base" = "$(git rev-parse origin/main)" +{} +"#, + review_script(&json!([old_finding()]), &[]) + ), + ); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let state = scenario.gh_state(); + assert!( + state["advisories"][0]["description"] + .as_str() + .unwrap() + .contains(base.trim()) + ); + assert_eq!(state["prs"][0]["state"], "MERGED"); +} #[test] fn command_words_override_the_config_and_review_is_off_by_default() { @@ -30,7 +296,7 @@ fn command_words_override_the_config_and_review_is_off_by_default() { let scenario = Scenario::new(); scenario.user_config_is(config); scenario.agent_does_in_session(1, OPENS_PR); - scenario.agent_does_in_session(2, CLEAN); + scenario.agent_does_in_session(2, &review_script(&json!([]), &[])); let url = scenario.issue_url(7); let mut args = vec![url.as_str(), "merge"]; args.extend(word); @@ -46,9 +312,9 @@ fn invalid_or_missing_final_lines_hold_self_merge_but_report_only_runs_continue( for final_message in [ "Review incomplete", "Security review: {}", - r#"Security review: {"unaddressed_count":0,"findings":["Cross-tenant read"]}"#, - r#"Security review: {"unaddressed_count":1,"findings":[""]}"#, - r#"Security review: {"unaddressed_count":0,"findings":[]}\nMore text"#, + r#"Security review: {"unaddressed_count":0,"findings":["Cross-tenant read"],"pre_existing_count":0}"#, + r#"Security review: {"unaddressed_count":1,"findings":[""],"pre_existing_count":0}"#, + r#"Security review: {"unaddressed_count":0,"findings":[],"pre_existing_count":0}\nMore text"#, ] { for merge in [true, false] { let scenario = Scenario::new(); @@ -134,7 +400,7 @@ gh fake checks "$(git rev-parse origin/main)" '[{{"name":"test","conclusion":"fa "# ), ); - scenario.agent_does_for_in_session(7, 2, CLEAN); + scenario.agent_does_for_in_session(7, 2, &review_script(&json!([]), &[])); scenario.agent_does_for_in_session( 8, 1, @@ -146,7 +412,7 @@ gh pr create --base main --head issue-8 --title 'Fix base CI' --body 'Closes #8' gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}]' "#, ); - scenario.agent_does_for_in_session(8, 2, CLEAN); + scenario.agent_does_for_in_session(8, 2, &review_script(&json!([]), &[])); let result = scenario.run(&[&scenario.issue_url(7), "security-review", "base-fix"]); assert_eq!(result.code, Some(0), "{}", result.stderr); let calls = scenario.claude_calls(); @@ -155,7 +421,7 @@ gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}] calls[3]["prompt"] .as_str() .unwrap() - .contains("git diff main...HEAD") + .contains("against Base branch main") ); assert_eq!(scenario.gh_state()["prs"][1]["state"], "MERGED"); } @@ -164,7 +430,7 @@ gh fake checks "$(git rev-parse HEAD)" '[{"name":"test","conclusion":"success"}] fn an_unaddressed_introduced_finding_holds_self_merge_with_the_pr_ready() { let scenario = Scenario::new(); scenario.agent_does_in_session(1, OPENS_PR); - scenario.agent_does_in_session(2, r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#); + scenario.agent_does_in_session(2, &review_script(&json!([]), &["Cross-tenant read"])); let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); assert_eq!(result.code, Some(1), "{}", result.stderr); assert!( @@ -190,8 +456,9 @@ fn enabled_review_fixes_reach_origin_before_delivery_finishes() { echo checked > security-fix.txt git add security-fix.txt git commit -q -m 'Fix reproduced finding' -{CLEAN} -"# +{clean} +"#, + clean = review_script(&json!([]), &[]) ), ); let result = scenario.run(&[&scenario.issue_url(7), "security-review"]); @@ -207,7 +474,10 @@ git commit -q -m 'Fix reproduced finding' let prompt = calls[1]["prompt"].as_str().unwrap(); assert!(prompt.contains("guidance mode")); assert!(prompt.contains("Do not delegate")); - assert!(prompt.contains("git diff main...HEAD")); + assert!(prompt.contains(&format!( + "git diff {}...HEAD", + scenario.launch_git(&["rev-parse", "main"]).trim() + ))); } // Regression A. @@ -224,10 +494,7 @@ gh fake checks "$(git rev-parse origin/main)" '[{"name":"test","conclusion":"suc "# ), ); - scenario.agent_does_in_session( - 2, - r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#, - ); + scenario.agent_does_in_session(2, &review_script(&json!([]), &["Cross-tenant read"])); scenario.agent_does_in_session( 3, r#" @@ -292,10 +559,7 @@ printf '%s\n' 'API Error: [cyber] Private refusal evidence' > "$FAKE_CLAUDE_FINA fn review_regression_c_continuation_replaces_the_previous_security_outcome() { let scenario = Scenario::new(); scenario.agent_does_in_session(1, OPENS_PR); - scenario.agent_does_in_session( - 2, - r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#, - ); + scenario.agent_does_in_session(2, &review_script(&json!([]), &["Cross-tenant read"])); let first = scenario.run(&[&scenario.issue_url(7), "security-review", "no-merge"]); assert_eq!(first.code, Some(0), "{}", first.stderr); assert!( @@ -312,7 +576,7 @@ git add feature.txt git commit -q -m 'Fix the previously reported finding' "#, ); - scenario.agent_does_in_session(4, CLEAN); + scenario.agent_does_in_session(4, &review_script(&json!([]), &[])); let second = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); assert_eq!(second.code, Some(0), "{}", second.stderr); let state = scenario.gh_state(); @@ -328,3 +592,110 @@ git commit -q -m 'Fix the previously reported finding' "{body}" ); } + +#[test] +fn review_old_finding_can_be_taken_by_next_security_run() { + for private in [false, true] { + let scenario = Scenario::new(); + let mut state = scenario.gh_state(); + state["private"] = json!(private); + scenario.write_gh_state(&state); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, &review_script(&json!([old_finding()]), &[])); + let first = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(first.code, Some(0), "{}", first.stderr); + let before = scenario.claude_calls().len(); + if !private { + let record = scenario.gh_state()["advisories"][0]["html_url"] + .as_str() + .unwrap() + .to_string(); + scenario.agent_does_in_session(3, &format!(r#" +gh label create needs-triage --description 'Needs triage' +issue=$(gh issue create --title 'Bound input' --body 'Add a bound. Private record: {record}' --label needs-triage) +printf '%s\n' "Security fix Ticket: $issue" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#)); + } + scenario.agent_does_for( + 8, + r#" +echo bounded > bounded.txt +git add bounded.txt +git commit -q -m 'Bound input' +gh pr create --base main --head issue-8 --title 'Bound input' --body 'Closes #8' +"#, + ); + let next = scenario.run(&["secure", "base", "main", "security-fix"]); + assert!( + scenario.claude_calls().len() > before, + "Security run must take the reproduced review finding when fixing is allowed; private={private}, stderr={}", + next.stderr + ); + assert_eq!(next.code, Some(0), "{private}: {}", next.stderr); + let state = scenario.gh_state(); + assert_eq!(state["prs"][1]["head"], "issue-8"); + assert_eq!(state["prs"][1]["isDraft"], false); + assert_eq!( + scenario.origin_file("issue-8", "bounded.txt").as_deref(), + Some("bounded\n") + ); + assert!( + !state["prs"][1]["body"] + .as_str() + .unwrap() + .contains("Private old vulnerability evidence.") + ); + } +} + +#[test] +fn switched_checkout_is_rejected_before_private_recording() { + for before_review in [true, false] { + let scenario = Scenario::new(); + let switch = "\ngit switch -c replacement-branch\n"; + scenario.agent_does_in_session( + 1, + &format!("{OPENS_PR}{}", if before_review { switch } else { "" }), + ); + scenario.agent_does_in_session( + 2, + &format!( + "{}{}", + review_script(&json!([old_finding()]), &[]), + if before_review { "" } else { switch } + ), + ); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "merge"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + let records = scenario.gh_state()["advisories"].clone(); + assert!( + records.is_null() || records.as_array().is_some_and(|records| records.is_empty()), + "A changed checkout must be rejected before private recording: {records}; {}", + result.stderr + ); + assert_eq!( + scenario.claude_calls().len(), + if before_review { 1 } else { 2 }, + "A checkout replaced by the opening session must not start a Security review" + ); + } +} + +#[test] +fn review_merge_base_uses_remote_ref_when_local_tag_shadows_it() { + let scenario = Scenario::new(); + scenario.launch_git(&["tag", "origin/main"]); + scenario.origin_has_commit("main", "base-new.txt", "base change", "Advance Base branch"); + let published_base = scenario.origin_git(&["rev-parse", "refs/heads/main"]); + scenario.agent_does_in_session(1, OPENS_PR); + scenario.agent_does_in_session(2, &review_script(&json!([]), &[])); + let result = scenario.run(&[&scenario.issue_url(7), "security-review", "no-merge"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let calls = scenario.claude_calls(); + let prompt = calls[1]["prompt"].as_str().unwrap(); + let expected = format!("Merge base commit: `{}`.", published_base.trim()); + assert!( + prompt.contains(&expected), + "Security review used a local tag instead of published Base branch merge base; expected {expected}, got {prompt}" + ); +} diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 35f88394..13c96552 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -109,6 +109,24 @@ fn assert_contains(text: &str, part: &str) { assert!(text.contains(part), "expected {part:?} in: {text}"); } +/// A complete Security review with no old findings writes its empty private +/// report as well as the public introduced-finding outcome. +fn security_review_script(introduced: &[&str]) -> String { + format!( + r#" +report=$(printf '%s' "$FAKE_CLAUDE_PROMPT" | sed -n 's/^Private report file: `\(.*\)`\.$/\1/p') +test -n "$report" +printf '%s\n' '[]' > "$report" +printf '%s\n' 'Security review: {outcome}' > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + outcome = serde_json::json!({ + "unaddressed_count": introduced.len(), + "findings": introduced, + "pre_existing_count": 0 + }) + ) +} + #[test] fn a_linear_spec_lands_each_ticket_in_order_then_opens_a_ready_spec_pr() { let scenario = linear_spec(); @@ -154,6 +172,7 @@ fn a_linear_spec_lands_each_ticket_in_order_then_opens_a_ready_spec_pr() { #[test] fn security_review_runs_once_after_spec_review_over_the_whole_spec_branch() { let scenario = linear_spec(); + let base_commit = scenario.launch_git(&["rev-parse", "main"]); scenario.agent_does_for_in_session( SPEC, 1, @@ -166,13 +185,16 @@ git commit -q -m 'Complete Spec review' scenario.agent_does_for_in_session( SPEC, 2, - r#" + &format!( + r#" test -f first.txt test -f second.txt test -f spec-reviewed.txt gh pr view issue-20 --json isDraft | grep -q '"isDraft": true' ! git cat-file -e origin/issue-20:spec-reviewed.txt -git diff main...HEAD --name-only > changed.txt +merge_base=$(printf '%s' "$FAKE_CLAUDE_PROMPT" | sed -n 's/^Merge base commit: `\(.*\)`\.$/\1/p') +test "$merge_base" = "$(git rev-parse refs/remotes/origin/main)" +git diff "$merge_base"...HEAD --name-only > changed.txt grep -qx first.txt changed.txt grep -qx second.txt changed.txt grep -qx spec-reviewed.txt changed.txt @@ -180,8 +202,10 @@ rm changed.txt echo fixed > security-fixed.txt git add security-fixed.txt git commit -q -m 'Fix reproduced Security finding' -printf '%s\n' 'Security review: {"unaddressed_count":0,"findings":[]}' > "$FAKE_CLAUDE_FINAL_MESSAGE" +{review} "#, + review = security_review_script(&[]) + ), ); let result = scenario.run(&[&spec_url(&scenario), "security-review", "merge"]); @@ -197,7 +221,13 @@ printf '%s\n' 'Security review: {"unaddressed_count":0,"findings":[]}' > "$FAKE_ assert_contains(prompt, "guidance mode"); assert_contains(prompt, "Do not delegate auditors"); assert_contains(prompt, "https://github.com/acme/widgets/issues/20"); - assert_contains(prompt, "branch issue-20 with `git diff main...HEAD`"); + assert_contains( + prompt, + &format!( + "branch issue-20 against Base branch main with `git diff {}...HEAD`", + base_commit.trim() + ), + ); assert_eq!(calls[3]["branch"], "issue-20"); assert_eq!(spec_pr(&scenario)["state"], "MERGED"); for file in [ @@ -249,7 +279,7 @@ fn security_review_on_a_spec_obeys_config_and_command_overrides() { ] { let scenario = linear_spec(); scenario.user_config_is(config); - scenario.agent_does_for_in_session(SPEC, 2, r#"printf '%s\n' 'Security review: {"unaddressed_count":0,"findings":[]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#); + scenario.agent_does_for_in_session(SPEC, 2, &security_review_script(&[])); let url = spec_url(&scenario); let mut args = vec![url.as_str(), "merge"]; args.extend(word); @@ -271,9 +301,10 @@ fn security_review_on_a_spec_obeys_config_and_command_overrides() { #[test] fn security_findings_refusals_and_incomplete_reviews_hold_only_the_spec_self_merge() { + let introduced = security_review_script(&["Cross-tenant read"]); for (script, cause) in [ ( - r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#, + introduced.as_str(), "Security review left 1 unaddressed introduced finding(s): Cross-tenant read", ), ( @@ -334,7 +365,7 @@ fn security_findings_refusals_and_incomplete_reviews_hold_only_the_spec_self_mer fn a_spec_security_hold_still_repairs_ci_without_repeating_the_review() { let scenario = linear_spec(); scenario.agent_does_for_in_session(SPEC, 1, &checks_on_head(RED)); - scenario.agent_does_for_in_session(SPEC, 2, r#"printf '%s\n' 'Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"]}' > "$FAKE_CLAUDE_FINAL_MESSAGE""#); + scenario.agent_does_for_in_session(SPEC, 2, &security_review_script(&["Cross-tenant read"])); scenario.agent_does_for_in_session( SPEC, 3,