diff --git a/prompts/security-fix.md b/prompts/security-fix.md index 82d9c42..1fcc6cf 100644 --- a/prompts/security-fix.md +++ b/prompts/security-fix.md @@ -7,17 +7,30 @@ With fixing allowed, publishes a terse Ticket or Spec with Tickets for the most ``` Publish the fix for this reproduced Security finding on Base branch ``. Read the private record at and the record below. -Follow the completed reproduction's fix size: single stages one Ticket; spec publishes a Spec with Tickets using `thirdshift-to-spec` and `thirdshift-to-tickets`. -On a public repository with a single fix, stage the proposed public issue text instead of creating the public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue from it. On a private repository, reuse the finding's issue as the top issue and preserve its body and evidence; add the bigger fix's Tickets as its native sub-issues. A spec still publishes its issues itself, as before. +Follow the completed reproduction's fix size: single stages one Ticket; spec stages a Spec with Tickets, drafted with `thirdshift-to-spec` and `thirdshift-to-tickets`. +On a public repository, stage the proposed public issue text instead of creating any public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue and its Tickets from it. On a private repository, reuse the finding's issue as the top issue and preserve its body and evidence; add the bigger fix's Tickets as its native sub-issues. Only a private bigger fix publishes its issues itself. Every new issue is terse: it says only what the fix changes and links the private record. It carries none of the write-up, trace, evidence, reproduction notes, proof-of-concept test or exploit details, even paraphrased. Keep those in the private record. This overrides the skills' templates. -A Spec's Tickets must be new, open, labelled `ready-for-agent`, linked as native sub-issues with their native blocking links, and have no sub-issues of their own. Read the links back before finishing. +A Spec's Tickets must be new, open, labelled `ready-for-agent`, linked as native sub-issues with their native blocking links, and have no sub-issues of their own. On a public repository thirdshift makes those links from the staged text; read the staged blocking edges back before finishing. This session changes no repository source, commits and pushes nothing, opens no pull request, and does not implement the fix. thirdshift checks every issue, marks the top issue ready, labels all fix issues security-fix and dispatches its Run or Spec run. End your final message for a single public fix with exactly: Proposed public issue: Title: Body: <body> -For a spec, end with exactly `Security fix Spec: <Issue URL>`, naming the top issue. +For a public spec, end with exactly: +Proposed public spec: +Title: <spec title> +Body: +<spec body> +Ticket: <ticket 1 title> +Body: +<ticket 1 body> +Blocked by: none +Ticket: <ticket 2 title> +Body: +<ticket 2 body> +Blocked by: 1 +with each later Ticket naming the 1-based positions of the Tickets that block it, or none. For a private bigger fix, end with exactly `Security fix Spec: <Issue URL>`, naming the top issue. Private record: <recorded Security finding> diff --git a/site/prompts/index.html b/site/prompts/index.html index d9b84fb..c317b8d 100644 --- a/site/prompts/index.html +++ b/site/prompts/index.html @@ -240,17 +240,30 @@ <h3 id="prompt-security-fix-title">Security fix publishing</h3> <p class="sent-by">Sent by a Security run, before any unit</p> <pre class="job-text"><code>Publish the fix for this reproduced Security finding on Base branch `<mark class="ph"><base></mark>`. Read the private record at <private record URL> and the record below. -Follow the completed reproduction's fix size: single stages one Ticket; spec publishes a Spec with Tickets using `thirdshift-to-spec` and `thirdshift-to-tickets`. -On a public repository with a single fix, stage the proposed public issue text instead of creating the public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue from it. On a private repository, reuse the finding's issue as the top issue and preserve its body and evidence; add the bigger fix's Tickets as its native sub-issues. A spec still publishes its issues itself, as before. +Follow the completed reproduction's fix size: single stages one Ticket; spec stages a Spec with Tickets, drafted with `thirdshift-to-spec` and `thirdshift-to-tickets`. +On a public repository, stage the proposed public issue text instead of creating any public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue and its Tickets from it. On a private repository, reuse the finding's issue as the top issue and preserve its body and evidence; add the bigger fix's Tickets as its native sub-issues. Only a private bigger fix publishes its issues itself. Every new issue is terse: it says only what the fix changes and links the private record. It carries none of the write-up, trace, evidence, reproduction notes, proof-of-concept test or exploit details, even paraphrased. Keep those in the private record. This overrides the skills' templates. -A Spec's Tickets must be new, open, labelled `ready-for-agent`, linked as native sub-issues with their native blocking links, and have no sub-issues of their own. Read the links back before finishing. +A Spec's Tickets must be new, open, labelled `ready-for-agent`, linked as native sub-issues with their native blocking links, and have no sub-issues of their own. On a public repository thirdshift makes those links from the staged text; read the staged blocking edges back before finishing. This session changes no repository source, commits and pushes nothing, opens no pull request, and does not implement the fix. thirdshift checks every issue, marks the top issue ready, labels all fix issues security-fix and dispatches its Run or Spec run. End your final message for a single public fix with exactly: Proposed public issue: Title: <title> Body: <body> -For a spec, end with exactly `Security fix Spec: <mark class="ph"><Issue URL></mark>`, naming the top issue. +For a public spec, end with exactly: +Proposed public spec: +Title: <spec title> +Body: +<spec body> +Ticket: <ticket 1 title> +Body: +<ticket 1 body> +Blocked by: none +Ticket: <ticket 2 title> +Body: +<ticket 2 body> +Blocked by: 1 +with each later Ticket naming the 1-based positions of the Tickets that block it, or none. For a private bigger fix, end with exactly `Security fix Spec: <mark class="ph"><Issue URL></mark>`, naming the top issue. Private record: <recorded Security finding> diff --git a/src/github.rs b/src/github.rs index d7a918b..4a01614 100644 --- a/src/github.rs +++ b/src/github.rs @@ -177,6 +177,55 @@ impl GitHub { ]) } + /// The numeric database id of `issue`, for API calls that take one + /// instead of the issue number (sub-issues, dependencies). + fn issue_db_id(&self, issue: &IssueUrl) -> Result<u64> { + let json = self.gh_json(&[ + "api", + &format!("repos/{}/issues/{}", issue.repo_slug(), issue.number), + "--jq", + ".id", + ])?; + json.as_u64() + .context("gh api did not return the issue's database id") + } + + /// Link `child` as a native sub-issue of `parent`. + pub fn add_sub_issue(&self, parent: &IssueUrl, child: &IssueUrl) -> Result<()> { + let child_id = self.issue_db_id(child)?; + self.post_by_id( + parent, + &format!("issues/{}/sub_issues", parent.number), + "sub_issue_id", + child_id, + ) + } + + /// Add a native "blocked by" edge: `ticket` is blocked by `blocker`. + pub fn add_blocked_by(&self, ticket: &IssueUrl, blocker: &IssueUrl) -> Result<()> { + let blocker_id = self.issue_db_id(blocker)?; + self.post_by_id( + ticket, + &format!("issues/{}/dependencies/blocked_by", ticket.number), + "issue_id", + blocker_id, + ) + } + + /// `POST` one database-id field to the `endpoint` below + /// `repos/<owner>/<repo>`: the shared shape behind issue links that take + /// an id instead of an issue number. + fn post_by_id(&self, issue: &IssueUrl, endpoint: &str, field: &str, id: u64) -> Result<()> { + self.gh(&[ + "api", + "--method", + "POST", + &format!("repos/{}/{}", issue.repo_slug(), endpoint), + "-F", + &format!("{field}={id}"), + ]) + } + /// Every open issue labelled `label` in the repository `repo`, an /// `owner/repo`, newest first. They come from GitHub's issue list, not its /// search, whose index can be a while behind an issue just opened. diff --git a/src/prompt.rs b/src/prompt.rs index b0d8001..a76397e 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -360,19 +360,22 @@ pub fn security_reproduction(commit: &str, finding: &str, test: &std::path::Path pub const SECURITY_FIX_LINE: &str = "Security fix Ticket: "; pub const SECURITY_FIX_SPEC_LINE: &str = "Security fix Spec: "; pub const STAGED_PROPOSAL_MARKER: &str = "Proposed public issue:"; +pub const STAGED_SPEC_MARKER: &str = "Proposed public spec:"; pub const STAGED_TITLE_MARKER: &str = "Title:"; pub const STAGED_BODY_MARKER: &str = "Body:"; +pub const STAGED_TICKET_MARKER: &str = "Ticket:"; +pub const STAGED_BLOCKED_BY_MARKER: &str = "Blocked by:"; pub fn security_fix(base: &str, url: &str, finding: &str) -> String { format!( "Publish the fix for this reproduced Security finding on Base branch `{base}`.\n\ Read the private record at {url} and the record below.\n\ - Follow the completed reproduction's fix size: single stages one Ticket; spec publishes a Spec with Tickets using `thirdshift-to-spec` and `thirdshift-to-tickets`.\n\ - On a public repository with a single fix, stage the proposed public issue text instead of creating the public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue from it. On a private repository, reuse the finding's issue as the top issue and preserve its body and evidence; add the bigger fix's Tickets as its native sub-issues. A spec still publishes its issues itself, as before.\n\ + Follow the completed reproduction's fix size: single stages one Ticket; spec stages a Spec with Tickets, drafted with `thirdshift-to-spec` and `thirdshift-to-tickets`.\n\ + On a public repository, stage the proposed public issue text instead of creating any public issue directly: do not run `gh issue create` yourself. thirdshift validates the staged text and creates the `needs-triage` top issue and its Tickets from it. On a private repository, reuse the finding's issue as the top issue and preserve its body and evidence; add the bigger fix's Tickets as its native sub-issues. Only a private bigger fix publishes its issues itself.\n\ Every new issue is terse: it says only what the fix changes and links the private record. It carries none of the write-up, trace, evidence, reproduction notes, proof-of-concept test or exploit details, even paraphrased. Keep those in the private record. This overrides the skills' templates.\n\ - A Spec's Tickets must be new, open, labelled `ready-for-agent`, linked as native sub-issues with their native blocking links, and have no sub-issues of their own. Read the links back before finishing.\n\ + A Spec's Tickets must be new, open, labelled `ready-for-agent`, linked as native sub-issues with their native blocking links, and have no sub-issues of their own. On a public repository thirdshift makes those links from the staged text; read the staged blocking edges back before finishing.\n\ This session changes no repository source, commits and pushes nothing, opens no pull request, and does not implement the fix. thirdshift checks every issue, marks the top issue ready, labels all fix issues security-fix and dispatches its Run or Spec run.\n\ - End your final message for a single public fix with exactly:\n{STAGED_PROPOSAL_MARKER}\n{STAGED_TITLE_MARKER} <title>\n{STAGED_BODY_MARKER}\n<body>\nFor a spec, end with exactly `{SECURITY_FIX_SPEC_LINE}<Issue URL>`, naming the top issue.\n\n\ + End your final message for a single public fix with exactly:\n{STAGED_PROPOSAL_MARKER}\n{STAGED_TITLE_MARKER} <title>\n{STAGED_BODY_MARKER}\n<body>\nFor a public spec, end with exactly:\n{STAGED_SPEC_MARKER}\n{STAGED_TITLE_MARKER} <spec title>\n{STAGED_BODY_MARKER}\n<spec body>\n{STAGED_TICKET_MARKER} <ticket 1 title>\n{STAGED_BODY_MARKER}\n<ticket 1 body>\n{STAGED_BLOCKED_BY_MARKER} none\n{STAGED_TICKET_MARKER} <ticket 2 title>\n{STAGED_BODY_MARKER}\n<ticket 2 body>\n{STAGED_BLOCKED_BY_MARKER} 1\nwith each later Ticket naming the 1-based positions of the Tickets that block it, or none. For a private bigger fix, end with exactly `{SECURITY_FIX_SPEC_LINE}<Issue URL>`, naming the top issue.\n\n\ Private record:\n{finding}\n\n{HEADLESS}" ) } diff --git a/src/security/fixing.rs b/src/security/fixing.rs index f8e98fe..5853194 100644 --- a/src/security/fixing.rs +++ b/src/security/fixing.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use anyhow::{Context, Result, bail}; -use chrono::Utc; +use chrono::{DateTime, Utc}; use crate::git::Git; use crate::github::{GitHub, SecurityRecord}; @@ -75,39 +75,34 @@ pub fn publish( let message = sessions.run_to_final_message(Purpose::Security, "security-fix-publishing", &prompt)?; let github = GitHub::new(); - // A single public fix stages its proposed text for validation; - // thirdshift creates the issue from it, and the session has no - // independent public-issue path. Anything else keeps the - // session-created issue path until staged specs land. - let issue = if size == FixSize::Single && record.private_issue_number().is_none() { + // A public fix stages its proposed text for validation; + // thirdshift creates the issues from it, and the session has no + // independent public-issue path. A private bigger fix keeps the + // session-created issue path: its issues stay private with the + // repository. + let issue = if record.private_issue_number().is_none() { // The session has no independent public-issue path: anything // created during its window is a side-effect leak outside the // validated staged path. Close it to stop further interaction, // then fail loudly so the Day shift triages the exposure. let leaked = github.issues_created_since(&repo.slug(), started)?; if !leaked.is_empty() { - for side in &leaked { - github.close_issue(side, "Closed by thirdshift: the Security fix publishing session created this public issue outside the validated staged path.")?; - } - let numbers = leaked - .iter() - .map(|side| format!("#{}", side.number)) - .collect::<Vec<_>>() - .join(", "); - bail!( - "the Security fix publishing session created public issue(s) outside the staged path: {numbers}" - ); + return fail_on_side_effects(&github, &leaked); + } + if size == FixSize::Single { + let Some((title, body)) = message.as_deref().and_then(parse_staged_proposal) else { + bail!( + "the Security fix publishing session did not stage its proposed public issue text" + ); + }; + // Validate the staged text before any public issue exists; + // a rejection blocks creation, so no public issue leaks + // private write-up text. + record.check_public_fix_text(&format!("{title}\n\n{body}"), url)?; + github.create_issue_in(repo, &title, &body, &[NEEDS_TRIAGE])? + } else { + publish_staged_spec(&github, repo, record, url, message.as_deref(), started)? } - let Some((title, body)) = message.as_deref().and_then(parse_staged_proposal) else { - bail!( - "the Security fix publishing session did not stage its proposed public issue text" - ); - }; - // Validate the staged text before any public issue exists; - // a rejection blocks creation, so no public issue leaks - // private write-up text. - record.check_public_fix_text(&format!("{title}\n\n{body}"), url)?; - github.create_issue_in(repo, &title, &body, &[NEEDS_TRIAGE])? } else { let line = message .as_deref() @@ -189,6 +184,198 @@ pub fn publish( }) } +/// One staged Spec Ticket the publishing session proposed: its title, its +/// body, and the 0-based indices of the Tickets it is blocked by. +struct StagedTicket { + title: String, + body: String, + blocked_by: Vec<usize>, +} + +/// The public Spec the publishing session staged for validation: its +/// proposed top-issue `(title, body)` and its Tickets, instead of +/// session-created public issues. +struct StagedSpec { + title: String, + body: String, + tickets: Vec<StagedTicket>, +} + +/// The staged Spec the publishing session proposed, with every Ticket's +/// blocking edges as 0-based indices. `None` when the final message holds +/// no well-formed staged spec: no marker, no top title or body, no Ticket, +/// an empty title or body, or a blocking edge that is not `none` or a list +/// of 1-based Ticket positions naming another Ticket. +fn parse_staged_spec_proposal(message: &str) -> Option<StagedSpec> { + let (_, staged) = message.rsplit_once(prompt::STAGED_SPEC_MARKER)?; + let staged = staged.trim_start_matches(['\n', '\r']); + let (title_line, rest) = staged.split_once('\n')?; + let title = title_line.strip_prefix(prompt::STAGED_TITLE_MARKER)?.trim(); + let rest = rest.strip_prefix(prompt::STAGED_BODY_MARKER)?; + let (spec_body, tickets_text) = match rest.split_once(prompt::STAGED_TICKET_MARKER) { + Some((body, tickets)) => (body, Some(tickets)), + None => (rest, None), + }; + let tickets_text = tickets_text?; + if title.is_empty() || spec_body.trim().is_empty() { + return None; + } + let mut tickets = Vec::new(); + for section in tickets_text.split(prompt::STAGED_TICKET_MARKER) { + let (title_line, rest) = section.split_once('\n')?; + let title = title_line.trim(); + if title.is_empty() { + return None; + } + let rest = rest.strip_prefix(prompt::STAGED_BODY_MARKER)?; + let (body, blocked) = rest.split_once(prompt::STAGED_BLOCKED_BY_MARKER)?; + if body.trim().is_empty() { + return None; + } + tickets.push((title.to_string(), body.trim().to_string(), blocked)); + } + if tickets.is_empty() { + return None; + } + let count = tickets.len(); + let mut parsed = Vec::with_capacity(count); + for (index, (title, body, blocked)) in tickets.into_iter().enumerate() { + parsed.push(StagedTicket { + title, + body, + blocked_by: parse_blocked_by(blocked, index, count)?, + }); + } + Some(StagedSpec { + title: title.to_string(), + body: spec_body.trim().to_string(), + tickets: parsed, + }) +} + +/// The `Blocked by:` trailer of one staged Ticket as 0-based indices into +/// its Spec's Tickets: `none` is empty, else comma-separated 1-based +/// positions of earlier Tickets on a single line, since each later Ticket +/// names the Tickets that block it. Anything but blank lines after that +/// first line is malformed staged text: ignoring it would silently drop a +/// stated ordering edge, so validation fails closed instead. `None` unless +/// every position names an earlier Ticket of the `count` staged: a forward +/// or cyclic edge would dispatch Tickets that can never unblock. +fn parse_blocked_by(blocked: &str, index: usize, count: usize) -> Option<Vec<usize>> { + let mut lines = blocked.lines(); + let line = lines.next().unwrap_or_default().trim(); + if lines.any(|line| !line.trim().is_empty()) { + return None; + } + let line = line.trim_end_matches(['.', ';']); + if line.eq_ignore_ascii_case("none") { + return Some(Vec::new()); + } + let mut edges = Vec::new(); + for position in line + .split(',') + .map(str::trim) + .filter(|edge| !edge.is_empty()) + { + let position: usize = position.parse().ok()?; + if position == 0 || position > count || position > index { + return None; + } + let edge = position - 1; + if !edges.contains(&edge) { + edges.push(edge); + } + } + if edges.is_empty() { + return None; + } + Some(edges) +} + +/// Validate the staged Spec proposal and create its public issues: the +/// `needs-triage` top issue and one `ready-for-agent` Ticket per staged +/// Ticket, linked as native sub-issues with their staged blocking edges. +/// Every staged text is validated before any public issue exists, so a +/// rejection creates nothing and leaks no private write-up text. +fn publish_staged_spec( + github: &GitHub, + repo: &Repo, + record: &SecurityRecord, + url: &str, + message: Option<&str>, + started: DateTime<Utc>, +) -> Result<IssueUrl> { + let Some(staged) = message.and_then(parse_staged_spec_proposal) else { + bail!("the Security fix publishing session did not stage its proposed public spec text"); + }; + record.check_public_fix_text(&format!("{}\n\n{}", staged.title, staged.body), url)?; + for ticket in &staged.tickets { + record.check_public_fix_text(&format!("{}\n\n{}", ticket.title, ticket.body), url)?; + } + let spec = github.create_issue_in(repo, &staged.title, &staged.body, &[NEEDS_TRIAGE])?; + let mut tickets = Vec::with_capacity(staged.tickets.len()); + for ticket in &staged.tickets { + tickets.push(github.create_issue_in( + repo, + &ticket.title, + &ticket.body, + &[READY_FOR_AGENT], + )?); + } + for ticket in &tickets { + github.add_sub_issue(&spec, ticket)?; + } + for (ticket, staged) in tickets.iter().zip(&staged.tickets) { + for edge in &staged.blocked_by { + github.add_blocked_by(ticket, &tickets[*edge])?; + } + } + // The session has no independent public-issue path, so anything in its + // window that is neither the staged spec nor one of its Tickets is a + // side-effect leak outside the validated staged path. The pre-creation + // sweep runs before any staged issue exists; this one runs after + // linking, so the staged issues themselves must not count. + let unlinked = unlinked_issues( + github.issues_created_since(&repo.slug(), started)?, + &spec, + &tickets, + ); + if !unlinked.is_empty() { + return fail_on_side_effects(github, &unlinked); + } + Ok(spec) +} + +/// The issues in the publishing session's `window` that are neither the +/// staged `spec` nor one of its `tickets`: side-effect leaks outside the +/// validated staged path. +fn unlinked_issues(window: Vec<IssueUrl>, spec: &IssueUrl, tickets: &[IssueUrl]) -> Vec<IssueUrl> { + window + .into_iter() + .filter(|issue| { + issue.number != spec.number + && !tickets.iter().any(|ticket| ticket.number == issue.number) + }) + .collect() +} + +/// Close `leaks`, public issues a Security fix publishing session created +/// outside the validated staged path, then fail loudly so the Day shift +/// triages the exposure. +fn fail_on_side_effects(github: &GitHub, leaks: &[IssueUrl]) -> Result<IssueUrl> { + for leak in leaks { + github.close_issue(leak, "Closed by thirdshift: the Security fix publishing session created this public issue outside the validated staged path.")?; + } + let numbers = leaks + .iter() + .map(|leak| format!("#{}", leak.number)) + .collect::<Vec<_>>() + .join(", "); + bail!( + "the Security fix publishing session created public issue(s) outside the staged path: {numbers}" + ) +} + /// The single public fix the publishing session staged for validation: /// its proposed `(title, body)`, instead of a session-created public issue. fn parse_staged_proposal(message: &str) -> Option<(String, String)> { @@ -232,4 +419,60 @@ mod tests { let message = "Security fix Ticket: https://github.com/acme/widgets/issues/8\n"; assert!(parse_staged_proposal(message).is_none()); } + + #[test] + fn staged_spec_parses_top_issue_and_tickets_with_blocking() { + let message = "Reasoning\nProposed public spec:\nTitle: Bound accepted input\nBody:\nBound input across storage and transport. Private record: https://example.invalid/advisory\nTicket: Bound storage input\nBody:\nBound storage input. Private record: https://example.invalid/advisory\nBlocked by: none\nTicket: Bound transport input\nBody:\nBound transport input. Private record: https://example.invalid/advisory\nBlocked by: 1\n"; + let spec = parse_staged_spec_proposal(message).expect("staged spec parses"); + assert_eq!(spec.title, "Bound accepted input"); + assert!(spec.body.contains("storage and transport")); + assert_eq!(spec.tickets.len(), 2); + assert_eq!(spec.tickets[0].title, "Bound storage input"); + assert!(spec.tickets[0].blocked_by.is_empty()); + assert_eq!(spec.tickets[1].title, "Bound transport input"); + assert_eq!(spec.tickets[1].blocked_by, vec![0]); + } + + #[test] + fn staged_spec_rejects_a_bare_url_final_line() { + let message = "Security fix Spec: https://github.com/acme/widgets/issues/8\n"; + assert!(parse_staged_spec_proposal(message).is_none()); + } + + #[test] + fn unlinked_issues_keeps_only_issues_outside_the_staged_spec() { + let issue = |number: u64| { + IssueUrl::parse(&format!("https://github.com/acme/widgets/issues/{number}")).unwrap() + }; + let spec = issue(8); + let tickets = vec![issue(9), issue(10)]; + let window = vec![issue(8), issue(9), issue(10), issue(11)]; + assert_eq!(unlinked_issues(window, &spec, &tickets), vec![issue(11)]); + let window = vec![issue(8), issue(9), issue(10)]; + assert!(unlinked_issues(window, &spec, &tickets).is_empty()); + } + + #[test] + fn staged_spec_rejects_a_trailing_blocked_by_edge_on_its_own_line() { + let message = "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: First ticket\nBody:\nFirst ticket. Private record: https://example.invalid/advisory\nBlocked by: none\nTicket: Second ticket\nBody:\nSecond ticket. Private record: https://example.invalid/advisory\nBlocked by: none\nTicket: Third ticket\nBody:\nThird ticket. Private record: https://example.invalid/advisory\nBlocked by: 1\n2\n"; + assert!( + parse_staged_spec_proposal(message).is_none(), + "a second Blocked by edge on its own line was silently dropped instead of rejected" + ); + } + + #[test] + fn staged_spec_rejects_bad_blocking_edges() { + for message in [ + "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: Only ticket\nBody:\nOnly ticket. Private record: https://example.invalid/advisory\nBlocked by: 1\n", + "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: Only ticket\nBody:\nOnly ticket. Private record: https://example.invalid/advisory\nBlocked by: 2\n", + "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\nTicket: First ticket\nBody:\nFirst ticket. Private record: https://example.invalid/advisory\nBlocked by: 2\nTicket: Second ticket\nBody:\nSecond ticket. Private record: https://example.invalid/advisory\nBlocked by: none\n", + "Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input. Private record: https://example.invalid/advisory\n", + ] { + assert!( + parse_staged_spec_proposal(message).is_none(), + "bad staged spec parses: {message}" + ); + } + } } diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 21bd3dd..1b83ed9 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -53,6 +53,12 @@ //! `gh api --method DELETE repos/<repo>/issues/<number>/labels/<label>` takes //! that label, whatever its case, off the issue, failing if the issue does //! not have it, as GitHub does. +//! `gh api --method POST repos/<repo>/issues/<parent>/sub_issues -F +//! sub_issue_id=<id>` links the issue as a native sub-issue of the parent. +//! `gh api --method POST repos/<repo>/issues/<ticket>/dependencies/blocked_by +//! -F issue_id=<id>` links the ticket as blocked by the issue. Database ids +//! are the issue numbers, and `gh api repos/<repo>/issues/<number> --jq .id` +//! answers with one. //! //! `gh api user` answers with the signed-in user's profile, whose public //! `email` is `user_email`, else null. `gh fake fails 'api user'` makes it @@ -649,6 +655,8 @@ fn issue_fields(state: &Json, n: &str, issue_state: &Json) -> Json { }; object([ ("number", number(n.parse::<i64>().unwrap())), + // The fake's database ids are the issue numbers. + ("id", number(n.parse::<i64>().unwrap())), ("state", issue_state.clone()), ("title", title), ("url", string(url)), @@ -710,6 +718,74 @@ fn issue_labels_put(state: &mut Json, args: &[&str]) { save(state); } +/// `gh api --method POST repos/<repo>/issues/<parent>/sub_issues -F +/// sub_issue_id=<id>`: link the issue as a native sub-issue of the parent. +/// The fake's database ids are the issue numbers. +fn api_add_sub_issue(state: &mut Json, args: &[&str]) { + let (positional, _) = parse(args); + let parent = positional.first().and_then(|path| { + let (repo, rest) = repo_prefix(path)?; + check_repo_is(state, Some(repo)); + rest.strip_prefix("issues/")? + .strip_suffix("/sub_issues") + .filter(|parent| state.at("issues").has(parent)) + }); + let child = args + .windows(2) + .filter(|pair| pair[0] == "-F") + .filter_map(|pair| pair[1].strip_prefix("sub_issue_id=")) + .next() + .filter(|child| state.at("issues").has(child)); + let (Some(parent), Some(child)) = (parent, child) else { + die( + &format!("fake gh: unsupported api POST {}", python_list(args)), + 2, + ) + }; + let mut sub = listed(state, "sub_issues", parent); + if !sub.iter().any(|ticket| ticket.python().as_str() == child) { + sub.push(number(child.parse::<i64>().unwrap())); + } + state + .entry("sub_issues", object([])) + .set(parent, Array(sub)); + save(state); +} + +/// `gh api --method POST repos/<repo>/issues/<ticket>/dependencies/blocked_by +/// -F issue_id=<id>`: link the ticket as blocked by the issue. The fake's +/// database ids are the issue numbers. +fn api_add_blocked_by(state: &mut Json, args: &[&str]) { + let (positional, _) = parse(args); + let ticket = positional.first().and_then(|path| { + let (repo, rest) = repo_prefix(path)?; + check_repo_is(state, Some(repo)); + rest.strip_prefix("issues/")? + .strip_suffix("/dependencies/blocked_by") + .filter(|ticket| state.at("issues").has(ticket)) + }); + let blocker = args + .windows(2) + .filter(|pair| pair[0] == "-F") + .filter_map(|pair| pair[1].strip_prefix("issue_id=")) + .next() + .filter(|blocker| state.at("issues").has(blocker)); + let (Some(ticket), Some(blocker)) = (ticket, blocker) else { + die( + &format!("fake gh: unsupported api POST {}", python_list(args)), + 2, + ) + }; + let mut blocked = listed(state, "blocked_by", ticket); + if !blocked.iter().any(|edge| edge.python().as_str() == blocker) { + blocked.push(number(blocker.parse::<i64>().unwrap())); + } + state + .entry("blocked_by", object([])) + .set(ticket, Array(blocked)); + save(state); +} + /// `gh api --method DELETE repos/<repo>/issues/<number>/labels/<label>`: take /// the label, whatever its case, off the issue. fn issue_label_delete(state: &mut Json, args: &[&str]) { @@ -1089,6 +1165,10 @@ fn api(state: &mut Json, positional: &[String], flags: &Flags) { && n.parse::<u64>().is_ok() { check_repo_is(state, Some(repo)); + if flag(flags, "jq") == Some(".id") { + println!("{n}"); + return; + } println!("{}", issue_fields(state, n, state.at("issues").at(n))); return; } @@ -1974,6 +2054,20 @@ pub fn main(args: Vec<String>) { } ["api", "--method", "PUT", rest @ ..] => issue_labels_put(&mut state, rest), ["api", "--method", "DELETE", rest @ ..] => issue_label_delete(&mut state, rest), + ["api", "--method", "POST", rest @ ..] + if rest + .first() + .is_some_and(|path| path.ends_with("/sub_issues")) => + { + api_add_sub_issue(&mut state, rest) + } + ["api", "--method", "POST", rest @ ..] + if rest + .first() + .is_some_and(|path| path.contains("/dependencies/blocked_by")) => + { + api_add_blocked_by(&mut state, rest) + } ["api", path, ..] if generate_notes_repo(path).is_some() => { generate_notes(&state, &args[1..]); } diff --git a/tests/security_run.rs b/tests/security_run.rs index ea23b6a..328b703 100644 --- a/tests/security_run.rs +++ b/tests/security_run.rs @@ -414,14 +414,13 @@ printf 'Proposed public issue:\nTitle: Bound accepted input\nBody:\nReject overs ) } -fn publish_spec(record: &str) -> String { +/// The staged spec path: the session returns its proposed public spec text +/// without running `gh issue create` itself. thirdshift validates every +/// staged text before creating any public issue. +fn stage_spec(record: &str) -> String { format!( r#" -url=$(gh issue create --title "Bound accepted input" --body "Bound input across storage and transport. Private record: {record}" --label needs-triage) -gh issue create --title "Bound storage input" --body "Bound storage input. Private record: {record}" --label ready-for-agent -gh issue create --title "Bound transport input" --body "Bound transport input. Private record: {record}" --label ready-for-agent -gh fake sub-issues 8 '[9,10]' -printf 'Security fix Spec: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +printf 'Proposed public spec:\nTitle: Bound accepted input\nBody:\nBound input across storage and transport. Private record: {record}\nTicket: Bound storage input\nBody:\nBound storage input. Private record: {record}\nBlocked by: none\nTicket: Bound transport input\nBody:\nBound transport input. Private record: {record}\nBlocked by: 1\n' > "$FAKE_CLAUDE_FINAL_MESSAGE" "# ) } @@ -448,11 +447,10 @@ fn a_bigger_public_fix_publishes_tickets_and_ends_as_the_spec_run() { .replace("high single", "high spec") .replace("Fix size: single", "Fix size: spec") ); - state["blocked_by"]["10"] = json!([9]); scenario.write_gh_state(&state); scenario.agent_does_in_session( 1, - &publish_spec("https://github.com/acme/widgets/security/advisories/GHSA-finding-0"), + &stage_spec("https://github.com/acme/widgets/security/advisories/GHSA-finding-0"), ); scenario.agent_does_for(9, &implement_spec_ticket(9, 8)); scenario.agent_does_for( @@ -483,6 +481,23 @@ fn a_bigger_public_fix_publishes_tickets_and_ends_as_the_spec_run() { ); } assert_eq!(state["labels"]["8"], json!(["security-fix", "in-progress"])); + // thirdshift created the Spec's native links from the staged text. + assert_eq!(state["sub_issues"]["8"], json!([9, 10])); + assert_eq!(state["blocked_by"]["10"], json!([9])); + // The post-publish window sweep must leave the staged issues alone. + assert_eq!( + state["issues"]["8"], "OPEN", + "the staged spec must survive the window sweep: {}", + state["issues"] + ); + assert!( + !result.stderr.contains("outside the staged path"), + "{}", + result.stderr + ); + let spec_body = state["bodies"]["8"].as_str().unwrap(); + assert!(spec_body.contains("storage and transport"), "{spec_body}"); + assert!(!spec_body.contains("Private candidate write-up")); let calls = scenario.claude_calls(); assert!( calls[0]["prompt"] @@ -496,21 +511,53 @@ fn a_bigger_public_fix_publishes_tickets_and_ends_as_the_spec_run() { .unwrap() .contains("thirdshift-to-tickets") ); + assert!( + calls[0]["prompt"] + .as_str() + .unwrap() + .contains("instead of creating any public issue directly"), + "{}", + calls[0]["prompt"].as_str().unwrap() + ); assert_eq!(calls.len(), 4); } #[test] fn invalid_security_fix_specs_are_not_marked_ready_or_dispatched() { + // A public spec has no session-created path: every entry here is either + // a missing staged spec or staged text that validation rejects before + // thirdshift creates anything. let record = "https://github.com/acme/widgets/security/advisories/GHSA-finding-0"; + let staged = stage_spec(record); for script in [ - format!("{}gh fake sub-issues 8 '[]'\n", publish_spec(record)), - publish_spec(record).replace("Security fix Spec:", "Security fix Ticket:"), - publish_spec(record).replace("Bound storage input. Private record:", "Private candidate write-up. Private record:"), - publish_spec(record).replace("Bound storage input. Private record: https://github.com/acme/widgets/security/advisories/GHSA-finding-0", "Bound storage input."), - format!("{}gh fake created 9 '2020-01-01T00:00:00Z'\n", publish_spec(record)), - format!("{}gh issue close 9\n", publish_spec(record)), - format!("{}gh fake labels 9 '[\"ready-for-human\"]'\n", publish_spec(record)), - format!("{}gh fake sub-issues 9 '[7]'\n", publish_spec(record)), + // No staged spec at all. + "printf '%s\\n' 'No protocol block' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n".to_string(), + // A session-created URL final line stages nothing. + "printf 'Security fix Spec: https://github.com/acme/widgets/issues/8\\n' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n".to_string(), + // The top issue copies private write-up text. + staged.replace( + "Bound input across storage and transport. Private record:", + "Private candidate write-up. Private record:", + ), + // A Ticket copies private write-up text. + staged.replace( + "Bound storage input. Private record:", + "Private candidate write-up. Private record:", + ), + // The top issue omits the private record link. + staged.replace( + "Bound input across storage and transport. Private record: https://github.com/acme/widgets/security/advisories/GHSA-finding-0", + "Bound input across storage and transport.", + ), + // A Ticket omits the private record link. + staged.replace( + "Bound transport input. Private record: https://github.com/acme/widgets/security/advisories/GHSA-finding-0", + "Bound transport input.", + ), + // A Ticket blocked by itself. + staged.replace("Blocked by: 1", "Blocked by: 2"), + // A Ticket blocked by no such Ticket. + staged.replace("Blocked by: 1", "Blocked by: 3"), ] { let scenario = with_reproduced_findings(&["high"]); let mut state = scenario.gh_state(); @@ -519,12 +566,97 @@ fn invalid_security_fix_specs_are_not_marked_ready_or_dispatched() { scenario.agent_does_in_session(1, &script); let result = scenario.run(&["secure", "security-fix"]); assert_eq!(result.code, Some(1), "{script}: {}", result.stderr); - assert_eq!(scenario.gh_state()["labels"]["8"], json!(["needs-triage"])); + assert!( + scenario.gh_state()["bodies"].get("8").is_none(), + "{script}: a rejected staged spec must not create a public issue: {}", + scenario.gh_state()["bodies"] + ); assert!(scenario.gh_state()["prs"].as_array().unwrap().is_empty()); assert_eq!(scenario.claude_calls().len(), 1); } } +#[test] +fn a_spec_session_side_effect_public_issue_fails_the_fix_and_is_closed() { + // The staged spec path validates before creating: a session that stages + // valid spec text but also creates a public issue on the side must fail + // loudly, and thirdshift closes the leak instead of reporting success. + let record = "https://github.com/acme/widgets/security/advisories/GHSA-finding-0"; + let scenario = with_reproduced_findings(&["high"]); + let mut state = scenario.gh_state(); + state["advisories"][0]["description"] = json!( + state["advisories"][0]["description"] + .as_str() + .unwrap() + .replace("high single", "high spec") + ); + scenario.write_gh_state(&state); + scenario.agent_does_in_session( + 1, + &format!( + "{}\ngh issue create --title 'Leaked write-up' --body 'Private candidate write-up. Private record: {record}' --label needs-triage > /dev/null\n", + stage_spec(record), + ), + ); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!( + result + .stderr + .contains("created public issue(s) outside the staged path: #8"), + "{}", + result.stderr + ); + let state = scenario.gh_state(); + assert_eq!( + state["issues"]["8"], "CLOSED", + "the session-created side-effect issue must be closed: {}", + state["issues"] + ); + assert!( + state["bodies"].get("9").is_none(), + "no validated issue may follow a side-effect leak: {}", + state["bodies"] + ); + assert!(state["prs"].as_array().unwrap().is_empty()); + assert_eq!(scenario.claude_calls().len(), 1); +} + +#[test] +fn a_staged_spec_with_cyclic_blocking_edges_is_rejected_before_creating_anything() { + // Ticket 1 names a later Ticket as its blocker and vice versa: no + // execution order satisfies both edges, so validation must reject the + // staged text before thirdshift creates any public issue. + let record = "https://github.com/acme/widgets/security/advisories/GHSA-finding-0"; + let scenario = with_reproduced_findings(&["high"]); + let mut state = scenario.gh_state(); + state["advisories"][0]["description"] = json!( + state["advisories"][0]["description"] + .as_str() + .unwrap() + .replace("high single", "high spec") + ); + scenario.write_gh_state(&state); + scenario.agent_does_in_session( + 1, + &format!( + "printf 'Proposed public spec:\\nTitle: Bound accepted input\\nBody:\\nBound input across storage and transport. Private record: {record}\\nTicket: Bound storage input\\nBody:\\nBound storage input. Private record: {record}\\nBlocked by: 2\\nTicket: Bound transport input\\nBody:\\nBound transport input. Private record: {record}\\nBlocked by: 1\\n' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n", + ), + ); + let result = scenario.run(&["secure", "security-fix"]); + assert_eq!( + result.code, + Some(1), + "cyclic staged spec must be rejected: {}", + result.stderr + ); + assert!( + scenario.gh_state()["bodies"].get("8").is_none(), + "a rejected staged spec must not create a public issue: {}", + scenario.gh_state()["bodies"] + ); +} + #[test] fn fixing_selects_the_most_severe_record_then_dispatches_one_ticket_before_auditing() { let scenario = with_reproduced_findings(&["low", "critical", "critical", "high"]); @@ -670,7 +802,7 @@ fn a_staged_proposal_is_created_by_thirdshift_not_the_session() { let calls = scenario.claude_calls(); let prompt = calls[0]["prompt"].as_str().unwrap(); assert!( - prompt.contains("instead of creating the public issue directly"), + prompt.contains("instead of creating any public issue directly"), "{prompt}" ); } @@ -907,7 +1039,7 @@ fn an_audit_goes_on_to_one_reproduced_fix_when_fixing_is_allowed() { scenario.agent_does_in_session(3, &reproduction_script("reproduced critical spec")); scenario.agent_does_in_session( 4, - &publish_spec("https://github.com/acme/widgets/security/advisories/GHSA-test-test-0002"), + &stage_spec("https://github.com/acme/widgets/security/advisories/GHSA-test-test-0002"), ); scenario.agent_does_for(9, &implement_spec_ticket(9, 8)); scenario.agent_does_for(10, &implement_spec_ticket(10, 8)); @@ -1087,10 +1219,14 @@ printf '%s\n' 'Security fix Spec: https://github.com/acme/widgets/issues/7' > "$ state["advisories"][0]["description"] = json!(evidence); scenario.agent_does_in_session( 1, - &publish_spec("https://github.com/acme/widgets/security/advisories/GHSA-finding-0"), + &stage_spec("https://github.com/acme/widgets/security/advisories/GHSA-finding-0"), ); } - state["blocked_by"][(ticket + 1).to_string()] = json!([ticket]); + if private { + // The private session links its own Tickets; the public staged + // path has thirdshift create the blocking edge itself. + state["blocked_by"][(ticket + 1).to_string()] = json!([ticket]); + } scenario.write_gh_state(&state); scenario.agent_does_for(ticket, "exit 1\n"); let failed = scenario.run(&["secure", "security-fix", "parallel", "1"]);