From 83b45b829c65f99ae17ec2becb0cc6aff803eabc Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 05:08:59 -0400 Subject: [PATCH] Record private repository Security findings as labelled issues (#540) --- src/github.rs | 2 +- src/github/advisories.rs | 110 +++++++++++++++++++++++++++--- src/pass.rs | 38 +++++++---- src/security.rs | 7 +- tests/fakes/gh.rs | 57 +++++++++++++++- tests/security_run.rs | 140 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 327 insertions(+), 27 deletions(-) diff --git a/src/github.rs b/src/github.rs index cafd268ac..7b17c8125 100644 --- a/src/github.rs +++ b/src/github.rs @@ -13,7 +13,7 @@ use crate::labels::{Label, Labels}; use crate::process::{self, Control, Interruption}; mod advisories; -pub use advisories::{DraftAdvisory, Package}; +pub use advisories::{DraftAdvisory, Package, SecurityRecords}; #[cfg(test)] mod execution_tests; diff --git a/src/github/advisories.rs b/src/github/advisories.rs index 8c5938a80..009bb2312 100644 --- a/src/github/advisories.rs +++ b/src/github/advisories.rs @@ -1,10 +1,31 @@ -//! Private advisory operations. Publishing and closing belong to the Day shift. +//! Private Security finding records. Public repositories use draft advisories; +//! private repositories whose advisory endpoint is unavailable use issues. use anyhow::{Context, Result, bail}; use serde::Serialize; use serde_json::{Value, json}; use super::GitHub; +use crate::issue::IssueUrl; +use crate::labels::{Label, NEEDS_TRIAGE}; + +const SECURITY_FINDING: Label = Label::new( + "security-finding", + "A Security finding recorded privately for the Day shift", +); + +pub enum SecurityRecords { + Advisories(Vec), + Issues(Vec), +} + +impl SecurityRecords { + pub fn remember(&mut self, record: Value) { + match self { + Self::Advisories(records) | Self::Issues(records) => records.push(record), + } + } +} #[derive(Clone, Debug, Serialize)] pub struct Package { @@ -21,10 +42,14 @@ pub struct DraftAdvisory { } impl DraftAdvisory { - pub fn already_recorded(&self, advisories: &[Value]) -> bool { + pub fn already_recorded(&self, records: &SecurityRecords) -> bool { let marker = format!("Fingerprint: `{}`", self.fingerprint); - advisories.iter().any(|advisory| { - advisory["description"] + let (records, field) = match records { + SecurityRecords::Advisories(records) => (records, "description"), + SecurityRecords::Issues(records) => (records, "body"), + }; + records.iter().any(|record| { + record[field] .as_str() .is_some_and(|description| description.lines().any(|line| line == marker)) }) @@ -32,16 +57,83 @@ impl DraftAdvisory { } impl GitHub { - /// Every state and every page, including closed and published records. - pub fn security_advisories(&self, repo: &str) -> Result> { - self.gh_api_items( + /// Every state and every page. A 404 selects issues only when repository + /// metadata confirms they will be private. + pub fn security_records(&self, repo: &str) -> Result { + match self.gh_api_items( &format!("repos/{repo}/security-advisories?per_page=100"), "", - ) + ) { + Ok(records) => Ok(SecurityRecords::Advisories(records)), + Err(error) if format!("{error:#}").contains("(HTTP 404)") => { + let repository = self.gh_json(&["api", &format!("repos/{repo}")])?; + if repository["private"].as_bool() != Some(true) { + bail!( + "security advisories unavailable; refusing to record findings in public issues" + ); + } + let records = self.gh_api_items( + &format!("repos/{repo}/issues?state=all&labels=security-finding&per_page=100"), + "", + )?; + Ok(SecurityRecords::Issues( + records + .into_iter() + .filter(|record| record.get("pull_request").is_none()) + .collect(), + )) + } + Err(error) => Err(error), + } + } + + /// Use the storage selected while reading the repository's records. + pub fn create_security_record( + &self, + repo: &str, + records: &SecurityRecords, + draft: &DraftAdvisory, + ) -> Result { + match records { + SecurityRecords::Advisories(_) => self.create_security_advisory(repo, draft), + SecurityRecords::Issues(_) => { + self.ensure_labels(repo, &[SECURITY_FINDING, NEEDS_TRIAGE])?; + let output = self.output_with_input( + &[ + "issue", + "create", + "--repo", + repo, + "--title", + &draft.summary, + "--body-file", + "-", + "--label", + "security-finding,needs-triage", + ], + Some(draft.description.as_bytes()), + )?; + if !output.status.success() { + // API errors can echo private evidence, so do not relay them. + bail!( + "creating a private Security finding issue failed ({})", + output.status + ); + } + let url = std::str::from_utf8(&output.stdout) + .context("creating a private Security finding issue returned invalid UTF-8")?; + IssueUrl::parse(url.trim()).map_err(|_| { + anyhow::anyhow!( + "creating a private Security finding issue returned no issue URL" + ) + })?; + Ok(json!({"body": draft.description})) + } + } } /// The create endpoint creates a draft. No severity or version claim. - pub fn create_security_advisory(&self, repo: &str, draft: &DraftAdvisory) -> Result { + fn create_security_advisory(&self, repo: &str, draft: &DraftAdvisory) -> Result { let body = serde_json::to_vec(&json!({ "summary": draft.summary, "description": draft.description, diff --git a/src/pass.rs b/src/pass.rs index d3495355f..bb49eba45 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -14,7 +14,7 @@ use std::fmt::Display; use std::path::PathBuf; -use crate::github::DraftAdvisory; +use crate::github::{DraftAdvisory, SecurityRecords}; use anyhow::Result; use serde_json::Value; @@ -100,10 +100,14 @@ pub trait Outside { fn check_node(&mut self) -> Result<()>; /// Audit the Base branch in a disposable checkout and validate its artifacts. fn audit(&mut self, base: &str) -> (Result, Option); - /// Private records in every advisory state. - fn security_advisories(&mut self) -> Result>; - /// Record one finding privately as a draft. - fn create_security_advisory(&mut self, finding: &DraftAdvisory) -> Result; + /// Private advisory or issue records in every state. + fn security_records(&mut self) -> Result; + /// Record one finding in the repository's private storage. + fn create_security_record( + &mut self, + records: &SecurityRecords, + finding: &DraftAdvisory, + ) -> Result; /// Run `dispatch` to its end, on the Harness the pass checked. fn dispatch(&mut self, dispatch: Dispatch) -> Ended; } @@ -201,12 +205,16 @@ impl Outside for LaunchAndGitHub<'_> { crate::security::audit::run(self.launch.git(), self.repo, base, self.harness) } - fn security_advisories(&mut self) -> Result> { - GitHub::new().security_advisories(&self.repo.slug()) + fn security_records(&mut self) -> Result { + GitHub::new().security_records(&self.repo.slug()) } - fn create_security_advisory(&mut self, finding: &DraftAdvisory) -> Result { - GitHub::new().create_security_advisory(&self.repo.slug(), finding) + fn create_security_record( + &mut self, + records: &SecurityRecords, + finding: &DraftAdvisory, + ) -> Result { + GitHub::new().create_security_record(&self.repo.slug(), records, finding) } /// The run's asks are the Architect plan's or the Ready issue's, from @@ -247,7 +255,7 @@ mod in_memory { use anyhow::{Result, anyhow, bail}; use super::{Dispatch, Outside}; - use crate::github::DraftAdvisory; + use crate::github::{DraftAdvisory, SecurityRecords}; use crate::github::{Issue, ListedIssue}; use crate::issue::{IssueUrl, Repo}; use crate::labels::{Edit, Label, Labels}; @@ -616,12 +624,16 @@ mod in_memory { (audited, self.session_log.clone()) } - fn security_advisories(&mut self) -> Result> { + fn security_records(&mut self) -> Result { self.calls.push(Call::AdvisoryList); - Ok(self.advisories.clone()) + Ok(SecurityRecords::Advisories(self.advisories.clone())) } - fn create_security_advisory(&mut self, finding: &DraftAdvisory) -> Result { + fn create_security_record( + &mut self, + _records: &SecurityRecords, + finding: &DraftAdvisory, + ) -> Result { self.calls .push(Call::CreateAdvisory(finding.fingerprint.clone())); let advisory = json!({"description": finding.description, "state": "draft"}); diff --git a/src/security.rs b/src/security.rs index baf06c8f0..7555223e2 100644 --- a/src/security.rs +++ b/src/security.rs @@ -118,14 +118,15 @@ fn audit_and_record( if audited.findings.is_empty() { return Ok(recorded); } - let mut known = outside.security_advisories()?; + let mut known = outside.security_records()?; for finding in audited.findings { if finding.already_recorded(&known) { recorded.existing += 1; } else { - known.push(outside.create_security_advisory(&finding)?); + let record = outside.create_security_record(&known, &finding)?; + known.remember(record); recorded.created += 1; - outside.step("recorded a Security finding as a private draft advisory".to_string()); + outside.step("recorded a Security finding privately".to_string()); } } Ok(recorded) diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 1b3f89a57..05acf8146 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -4,6 +4,8 @@ //! //! ```text //! {"repo": "owner/repo", +//! "private"?: true, +//! "advisory_error"?: "", //! "issues": {"": "OPEN" | "CLOSED"}, //! "titles"?: {"": ""}, //! "created"?: {"<number>": "<ISO 8601 time>"}, @@ -651,6 +653,14 @@ fn issue_fields(state: &Json, n: &str, issue_state: &Json) -> Json { ("url", string(url)), ("labels", Array(labels)), ("createdAt", created), + ( + "body", + state + .get("bodies") + .and_then(|bodies| bodies.get(n)) + .cloned() + .unwrap_or_else(|| string("")), + ), ]) } @@ -808,6 +818,7 @@ fn issue_close(state: &mut Json, positional: &[String], flags: &Flags) { /// Open an issue with `--title`, `--body` and the comma-separated `--label`s, /// numbered one past the highest issue, and print its URL. fn issue_create(state: &mut Json, flags: &Flags) { + use std::io::Read; let labels: Vec<&str> = flag(flags, "label") .map(|labels| labels.split(',').collect()) .unwrap_or_default(); @@ -827,7 +838,13 @@ fn issue_create(state: &mut Json, flags: &Flags) { state.at_mut("issues").set(&n, string("OPEN")); let title = flag(flags, "title").expect("no --title"); state.entry("titles", object([])).set(&n, string(title)); - let body = flag(flags, "body").expect("no --body"); + let mut input = String::new(); + let body = if flag(flags, "body-file") == Some("-") { + std::io::stdin().read_to_string(&mut input).unwrap(); + &input + } else { + flag(flags, "body").expect("no --body") + }; state.entry("bodies", object([])).set(&n, string(body)); let labels = labels.into_iter().map(string).collect(); state.entry("labels", object([])).set(&n, Array(labels)); @@ -951,6 +968,12 @@ fn advisory_api(state: &mut Json, positional: &[String], flags: &Flags) { .unwrap_or((&positional[0], "")); let (repo, rest) = repo_prefix(path).unwrap(); check_repo_is(state, Some(repo)); + if let Some(error) = state.get("advisory_error") { + die(error.str(), 1); + } + if state.get("private").is_some_and(Json::truthy) { + die("gh: Not Found (HTTP 404)", 1); + } let method = flag(flags, "method").unwrap_or("GET"); if method == "GET" { let wanted = query @@ -1017,6 +1040,38 @@ fn advisory_api(state: &mut Json, positional: &[String], flags: &Flags) { fn api(state: &mut Json, positional: &[String], flags: &Flags) { let path = &positional[0]; + if path == &format!("repos/{}", state.at("repo").str()) { + println!( + "{}", + object([( + "private", + Bool(state.get("private").is_some_and(Json::truthy)) + )]) + ); + return; + } + if let Some((repo, rest)) = repo_prefix(path) + && rest.starts_with("issues?") + { + check_repo_is(state, Some(repo)); + assert!(rest.contains("state=all")); + assert!(rest.contains("labels=security-finding")); + assert!(rest.contains("per_page=100")); + assert!(flags.contains_key("paginate")); + assert_eq!(flag(flags, "jq"), Some(".[]")); + let Json::Object(issues) = state.at("issues") else { + panic!("issues is an object") + }; + for (n, issue_state) in issues { + if issue_labels(state, n) + .iter() + .any(|label| label.str().eq_ignore_ascii_case("security-finding")) + { + println!("{}", issue_fields(state, n, issue_state)); + } + } + return; + } if path.contains("/security-advisories") { advisory_api(state, positional, flags); return; diff --git a/tests/security_run.rs b/tests/security_run.rs index 42fa9e784..7a107640d 100644 --- a/tests/security_run.rs +++ b/tests/security_run.rs @@ -69,6 +69,146 @@ fn records_a_finding_privately_as_a_draft_without_severity_or_versions() { ); } +#[test] +fn a_private_repository_records_each_finding_as_a_labelled_issue_with_the_advisory_body() { + let scenario = Scenario::new(); + let mut github = scenario.gh_state(); + github["private"] = json!(true); + scenario.write_gh_state(&github); + scenario.agent_does(&audit_script( + &json!([finding("input-size"), finding("storage-bound")]).to_string(), + )); + let result = scenario.run(&["secure"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let state = scenario.gh_state(); + assert!(state["advisories"].is_null()); + assert_eq!(state["issues"].as_object().unwrap().len(), 3); + let commit = scenario.origin_git(&["rev-parse", "main"]); + for (number, fingerprint) in [("8", "input-size"), ("9", "storage-bound")] { + assert_eq!(state["titles"][number], "Unchecked input size"); + assert_eq!( + state["labels"][number], + json!(["security-finding", "needs-triage"]) + ); + let expected = format!( + "Found by thirdshift's Security run.\n\nFingerprint: `{fingerprint}`\nAudited commit: `{}`\n\nPrivate candidate write-up.\n\n```json\n{}\n```\n", + commit.trim(), + serde_json::to_string_pretty(&finding(fingerprint)).unwrap() + ); + assert_eq!(state["bodies"][number], expected); + } + assert!(!result.stderr.contains("Private candidate write-up.")); +} + +#[test] +fn repeat_private_audits_match_open_and_closed_findings_after_triage_without_duplicates() { + for state in ["OPEN", "CLOSED"] { + let scenario = Scenario::new(); + let mut github = scenario.gh_state(); + github["private"] = json!(true); + scenario.write_gh_state(&github); + scenario.agent_does(&audit_script( + &json!([finding("input-size"), finding("input-size-extra")]).to_string(), + )); + let first = scenario.run(&["secure"]); + assert_eq!(first.code, Some(0), "{}", first.stderr); + assert!( + first + .stderr + .contains("2 new finding(s), 0 already recorded"), + "{}", + first.stderr + ); + let mut github = scenario.gh_state(); + for number in ["8", "9"] { + github["issues"][number] = json!(state); + github["labels"][number] = json!(["Security-Finding"]); + } + scenario.write_gh_state(&github); + let second = scenario.run(&["secure"]); + assert_eq!(second.code, Some(0), "{}", second.stderr); + assert!( + second + .stderr + .contains("0 new finding(s), 2 already recorded"), + "{}", + second.stderr + ); + assert_eq!(scenario.gh_state(), github); + assert_eq!(scenario.gh_calls_of("issue", "create").len(), 2); + } +} + +#[test] +fn private_finding_labels_are_created_with_descriptions_and_existing_labels_are_kept() { + for existing in [ + vec!["bug"], + vec!["bug", "Security-Finding"], + vec!["bug", "NEEDS-TRIAGE"], + vec!["bug", "Security-Finding", "NEEDS-TRIAGE"], + ] { + let scenario = Scenario::new(); + scenario.repo_has_labels(&existing); + let mut github = scenario.gh_state(); + github["private"] = json!(true); + scenario.write_gh_state(&github); + scenario.agent_does(&audit_script(&json!([finding("input-size")]).to_string())); + let result = scenario.run(&["secure"]); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let created = scenario.gh_calls_of("label", "create"); + let mut expected_labels: Vec<String> = + existing.iter().map(|name| name.to_string()).collect(); + let mut missing = 0; + for (label, description) in [ + ( + "security-finding", + "A Security finding recorded privately for the Day shift", + ), + ("needs-triage", "Maintainer needs to evaluate this issue"), + ] { + if !existing.iter().any(|name| name.eq_ignore_ascii_case(label)) { + missing += 1; + assert!( + created.iter().any(|call| call[2] == label + && call + .windows(2) + .any(|args| args == ["--description", description])), + "{created:?}" + ); + expected_labels.push(label.to_string()); + } + } + assert_eq!(created.len(), missing); + assert_eq!(scenario.repo_labels(), expected_labels); + assert_eq!( + scenario.issue_labels(8), + ["security-finding", "needs-triage"] + ); + } +} + +#[test] +fn advisory_errors_never_record_findings_in_a_public_repository_or_on_non_404_failures() { + for (private, error) in [ + (false, "gh: Not Found (HTTP 404)"), + (true, "gh: Forbidden (HTTP 403)"), + (true, "gh: Bad Gateway (HTTP 502)"), + ] { + let scenario = Scenario::new(); + let mut github = scenario.gh_state(); + github["private"] = json!(private); + github["advisory_error"] = json!(error); + scenario.write_gh_state(&github); + scenario.agent_does(&audit_script(&json!([finding("input-size")]).to_string())); + let result = scenario.run(&["secure"]); + assert_ne!(result.code, Some(0), "{}", result.stderr); + assert!(scenario.gh_calls_of("issue", "create").is_empty()); + assert!(scenario.gh_calls_of("label", "create").is_empty()); + assert_eq!(scenario.gh_state()["issues"].as_object().unwrap().len(), 1); + assert!(!result.stderr.contains("Private candidate write-up.")); + } +} + #[test] fn reads_the_package_from_an_npm_manifest() { let scenario = Scenario::new();