diff --git a/crates/mergify-ci/src/git_refs.rs b/crates/mergify-ci/src/git_refs.rs index 091e178c..82388468 100644 --- a/crates/mergify-ci/src/git_refs.rs +++ b/crates/mergify-ci/src/git_refs.rs @@ -83,8 +83,8 @@ pub struct References { pub source: ReferencesSource, /// The scopes the merge queue decided for this batch, read off /// the engine's git note: see [`batch_scopes`]. `None` everywhere - /// else, the pull request body included, which anyone opening a - /// pull request titled `merge queue: ` writes. + /// else, the pull request body included, which whoever opens the + /// pull request writes. pub batch_scopes: Option, } @@ -240,7 +240,10 @@ fn detect_from_pull_request_event( .and_then(|pr| pr.head.as_ref()) .map_or_else(|| "HEAD".to_string(), |r| r.sha.clone()); + // A fork can serve the note too, when the workflow checks it out + // as `origin`, so the note needs the same admission as the body. if let Some(pr) = &event.pull_request + && pr.is_from_base_repository() && let Some(head_ref) = &pr.head && let Some(branch) = head_ref.r#ref.as_deref() && let Some(note) = notes_reader(branch, &head_ref.sha) @@ -255,8 +258,9 @@ fn detect_from_pull_request_event( } if let Some(meta) = extract_from_event(event, output)? { - // Anyone who can open a pull request writes this body, so the - // value only becomes a base once it looks like one. Either way + // Anyone who can open a pull request from a branch of the + // repository writes this body, so the value only becomes a base + // once it looks like one. Either way // a payload we can't use is not metadata: fall through to the // PR base below, which is the *wrong* base for a genuine batch // build, so warn rather than let it be diagnosed silently. @@ -361,8 +365,9 @@ pub fn real_notes_reader(branch: &str, head_sha: &str) -> Option` in the body and git-refs // emitted it as `base`, where the workflow's next git call // read it as an option. A value that is not an object name is - // not metadata: fall through to the PR base and say so. + // not metadata: fall through to the PR base and say so. Forks + // are refused since MRGFY-8854, but a branch of the repository + // still writes the body. let dir = tempfile::tempdir().unwrap(); let path = write_event( &dir, @@ -806,8 +851,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": "```yaml\nchecking_base_sha: --output=/home/runner/.gitconfig\n```", - "head": {"sha": "mq-head", "ref": "mq/main/0"}, - "base": {"sha": "pr-base"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); @@ -842,8 +887,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": "```yaml\nchecking_base_sha: \"cafef00d\\n::error::pwned\"\n```", - "head": {"sha": "mq-head", "ref": "mq/main/0"}, - "base": {"sha": "pr-base"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); @@ -877,7 +922,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": format!("```yaml\nchecking_base_sha: {MQ_BASE}\n```"), - "head": {"sha": "mq-head", "ref": "mq/main/0"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); @@ -943,8 +989,8 @@ mod tests { #[test] fn mq_note_carries_batch_scopes_but_body_does_not() { - // The body is written by whoever opens a pull request titled - // `merge queue: `, so only the engine's note decides scopes. + // The body is written by whoever opens the pull request, so + // only the engine's note decides scopes. let dir = tempfile::tempdir().unwrap(); let path = write_event( &dir, @@ -952,7 +998,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": format!("```yaml\nchecking_base_sha: {MQ_BASE}\nscopes: [body]\n```"), - "head": {"sha": "mq-head", "ref": "mq/main/0"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); diff --git a/crates/mergify-ci/src/github_event.rs b/crates/mergify-ci/src/github_event.rs index 4495e9fc..bca24fb7 100644 --- a/crates/mergify-ci/src/github_event.rs +++ b/crates/mergify-ci/src/github_event.rs @@ -15,6 +15,17 @@ pub struct GitRef { pub sha: String, #[serde(default)] pub r#ref: Option, + /// `null` on a pull request's `head` once its fork is deleted. + #[serde(default)] + pub repo: Option, +} + +#[derive(Debug, Clone, Default, Deserialize)] +pub struct RefRepository { + /// Optional so that a `repo` without it costs only the trust in + /// [`PullRequest::is_from_base_repository`], not the whole event. + #[serde(default)] + pub id: Option, } #[derive(Debug, Clone, Default, Deserialize)] @@ -31,6 +42,31 @@ pub struct PullRequest { pub head: Option, } +impl PullRequest { + /// Whether the head branch lives in the base repository, which is + /// where the engine always opens its merge queue drafts. + /// + /// This is what lets the CLI trust merge queue metadata, never the + /// `merge queue: ` title, which the author picks (MRGFY-8854). It + /// rules out a fork, whose author writes the code, the body, and, + /// when the workflow checks the fork out as `origin`, the git note + /// too. It does not rule out a reader opening a pull request from a + /// branch someone else pushed: they write its title and body, but + /// not its code. The branch prefix and the author are no better: + /// `queue_branch_prefix` and `draft_bot_account` are configurable. + /// + /// `false` when either side has no repository id, including the + /// `null` head of a deleted fork. + #[must_use] + pub fn is_from_base_repository(&self) -> bool { + let repo_id = |r: &Option| r.as_ref()?.repo.as_ref()?.id; + matches!( + (repo_id(&self.head), repo_id(&self.base)), + (Some(head), Some(base)) if head == base + ) + } +} + #[derive(Debug, Clone, Default, Deserialize)] pub struct Repository { #[serde(default)] @@ -94,6 +130,21 @@ mod tests { assert_eq!(ev.pull_request.unwrap().number, Some(7)); } + #[test] + fn is_from_base_repository_needs_both_ids_and_equal() { + let pr = |raw: &str| serde_json::from_str::(raw).unwrap(); + assert!(pr(r#"{"head": {"sha": "h", "repo": {"id": 1}}, "base": {"sha": "b", "repo": {"id": 1}}}"#).is_from_base_repository()); + for raw in [ + r#"{"head": {"sha": "h", "repo": {"id": 2}}, "base": {"sha": "b", "repo": {"id": 1}}}"#, + r#"{"head": {"sha": "h", "repo": null}, "base": {"sha": "b", "repo": {"id": 1}}}"#, + r#"{"head": {"sha": "h", "repo": {"id": 1}}, "base": {"sha": "b"}}"#, + r#"{"head": {"sha": "h"}, "base": {"sha": "b"}}"#, + r#"{"head": {"sha": "h", "repo": {}}, "base": {"sha": "b", "repo": {}}}"#, + ] { + assert!(!pr(raw).is_from_base_repository(), "admitted {raw}"); + } + } + #[test] fn deserialize_push_event_shape() { let raw = r#"{"before": "a", "after": "b", "repository": {"default_branch": "main"}}"#; diff --git a/crates/mergify-ci/src/queue_metadata.rs b/crates/mergify-ci/src/queue_metadata.rs index dfa9c0b4..1816bc7d 100644 --- a/crates/mergify-ci/src/queue_metadata.rs +++ b/crates/mergify-ci/src/queue_metadata.rs @@ -21,6 +21,8 @@ use mergify_core::Output; use crate::github_event::GitHubEvent; +#[cfg(doc)] +use crate::github_event::PullRequest; /// Parse the first ```yaml``` fenced block out of `body` into a /// generic value, keeping every field. Returns `None` when the body @@ -51,9 +53,10 @@ pub fn parse_yaml_block(body: &str) -> Option { /// Extract MQ metadata from an event payload's pull-request body. /// -/// Emits a warning on `output` (stderr for human mode) when the PR is -/// an MQ draft but the body is missing or lacks the fenced block — -/// matches Python's stderr warnings. +/// The title only nominates the pull request: the body is read when +/// [`PullRequest::is_from_base_repository`] also holds. Emits a warning +/// on `output` (stderr for human mode) when it does not, or when the +/// body is missing or lacks the fenced block. pub fn extract_from_event( ev: &GitHubEvent, output: &mut dyn Output, @@ -67,6 +70,12 @@ pub fn extract_from_event( if !title.starts_with("merge queue: ") { return Ok(None); } + if !pr.is_from_base_repository() { + output.status( + "WARNING: pull request titled like a merge queue draft does not come from the base repository, skipping metadata extraction", + )?; + return Ok(None); + } let Some(body) = pr.body.as_deref() else { output.status("WARNING: MQ pull request without body, skipping metadata extraction")?; return Ok(None); @@ -154,16 +163,31 @@ mod tests { assert!(cap.stderr().is_empty()); } - #[test] - fn extract_warns_on_mq_pr_without_body() { - let ev = GitHubEvent { + /// A `merge queue: ` pull request whose head and base live in the + /// repositories with these ids (`None`: the payload's `repo` is + /// `null`, as on the head of a deleted fork). + fn mq_event(head_repo: Option, base_repo: Option, body: Option<&str>) -> GitHubEvent { + let git_ref = |id: Option| crate::github_event::GitRef { + repo: id.map(|id| crate::github_event::RefRepository { id: Some(id) }), + ..Default::default() + }; + GitHubEvent { pull_request: Some(crate::github_event::PullRequest { - title: Some("merge queue: deploy".into()), - body: None, + title: Some("merge queue: batch".into()), + body: body.map(Into::into), + head: Some(git_ref(head_repo)), + base: Some(git_ref(base_repo)), ..Default::default() }), ..Default::default() - }; + } + } + + const BODY: &str = "blah\n```yaml\nchecking_base_sha: deadbeef\n```"; + + #[test] + fn extract_warns_on_mq_pr_without_body() { + let ev = mq_event(Some(1), Some(1), None); let mut cap = Captured::human(); let result = extract_from_event(&ev, &mut cap.output).unwrap(); assert!(result.is_none()); @@ -173,17 +197,32 @@ mod tests { #[test] fn extract_returns_metadata_for_mq_pr() { - let body = "blah\n```yaml\nchecking_base_sha: deadbeef\n```"; - let ev = GitHubEvent { - pull_request: Some(crate::github_event::PullRequest { - title: Some("merge queue: batch".into()), - body: Some(body.into()), - ..Default::default() - }), - ..Default::default() - }; + let ev = mq_event(Some(1), Some(1), Some(BODY)); let mut cap = Captured::human(); let meta = extract_from_event(&ev, &mut cap.output).unwrap().unwrap(); assert_eq!(meta["checking_base_sha"], "deadbeef"); } + + #[test] + fn extract_ignores_mq_title_outside_the_base_repository() { + // MRGFY-8854: anyone can title a pull request `merge queue: `. + // From a fork, from a deleted fork, or with no repository to + // compare, the body is not the engine's. + for (head, base) in [ + (Some(2), Some(1)), + (None, Some(1)), + (Some(1), None), + (None, None), + ] { + let ev = mq_event(head, base, Some(BODY)); + let mut cap = Captured::human(); + let result = extract_from_event(&ev, &mut cap.output).unwrap(); + assert!(result.is_none(), "{head:?} -> {base:?} admitted"); + let stderr = cap.stderr(); + assert!( + stderr.contains("does not come from the base repository"), + "got: {stderr:?}" + ); + } + } }