Skip to content

fix(ci): stop trusting a merge queue title from a fork #1881

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mergify merged 1 commit into from
Oct 5, 2026
Merged

fix(ci): stop trusting a merge queue title from a fork #1881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 70 additions & 23 deletions crates/mergify-ci/src/git_refs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<BatchScopes>,
}

Expand Down Expand Up @@ -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)
Expand All @@ -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.
Expand Down Expand Up @@ -361,8 +365,9 @@ pub fn real_notes_reader(branch: &str, head_sha: &str) -> Option<serde_json::Val
///
/// The gate this backs is on the *pull request body* payload alone,
/// and that asymmetry is the point. `queue_metadata::extract_from_event`
/// admits a body on the title prefix `merge queue: ` and nothing
/// else, so whoever opens a pull request writes the value; the git
/// admits a body on the title prefix `merge queue: ` from a branch of
/// the base repository, so anyone who can open a pull request from one
/// writes the value; the git
/// note comes from the engine, over a push to `origin`. A value
/// admitted here leaves the CLI as `base`: into `$GITHUB_OUTPUT` and
/// the `--format=shell` eval for the caller's workflow, into
Expand Down Expand Up @@ -589,8 +594,8 @@ mod tests {

#[test]
fn is_object_name_admits_only_full_hex_object_names() {
// Anyone who can open a pull request writes the body this
// gates, and the value ends up as `base` in the caller's
// Anyone who can open a pull request from a branch of the
// repository writes the body this gates, and the value ends up as `base` in the caller's
// workflow. A leading `-` above all: every consumer of `base`
// reads it as an option (MRGFY-8845).
for value in [
Expand Down Expand Up @@ -702,7 +707,8 @@ mod tests {
"pull_request": {
"title": "merge queue: batch",
"body": format!("prelude\n```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}},
},
}),
);
Expand All @@ -720,6 +726,43 @@ mod tests {
assert_eq!(refs.source, ReferencesSource::MergeQueue);
}

#[test]
fn mq_title_from_a_fork_is_not_a_merge_queue_draft() {
// MRGFY-8854, the ticket's reproduction: a fork's pull request
// titled `merge queue: ` whose body names its own head as
// `checking_base_sha`. Admitted, the diff is empty, every scope
// reads false and `source` turns on the merge-queue scope. A
// workflow that checks the fork out as `origin` fetches the
// fork's note, which says the same.
let dir = tempfile::tempdir().unwrap();
let path = write_event(
&dir,
&serde_json::json!({
"pull_request": {
"title": "merge queue: attacker batch",
"body": format!("```yaml\nchecking_base_sha: {MQ_BASE}\n```"),
"head": {"sha": MQ_BASE, "ref": "attacker-fork-branch", "repo": {"id": 2}},
"base": {"sha": "pr-base", "repo": {"id": 1}},
},
}),
);
let forged_note = |_: &str, _: &str| {
Some(serde_json::json!({"checking_base_sha": MQ_BASE, "scopes": []}))
};
let mut cap = Captured::human();
let refs = env::testing::with_vars(
[
("GITHUB_EVENT_NAME", Some("pull_request_target")),
("GITHUB_EVENT_PATH", Some(path.to_str().unwrap())),
("BUILDKITE", None),
],
|| detect(&mut cap.output, &forged_note).unwrap(),
);
assert_eq!(refs.base.as_deref(), Some("pr-base"));
assert_eq!(refs.source, ReferencesSource::GithubEventPullRequest);
assert!(refs.batch_scopes.is_none());
}

#[test]
fn mq_body_with_post_contract_batch_still_yields_base() {
// The ticket's exact failure mode (MRGFY-8104): a batch body
Expand All @@ -737,8 +780,8 @@ mod tests {
"pull_request": {
"title": "merge queue: batch",
"body": format!("```yaml\nchecking_base_sha: {MQ_BASE}\nprevious_failed_batches:\n - batch_pr_number: 42\n checked_pull_requests:\n - 7\n```"),
"head": {"sha": "mq-head", "ref": "mq/main/0"},
"base": {"sha": "wrong-base"},
"head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}},
"base": {"sha": "wrong-base", "repo": {"id": 1}},
},
}),
);
Expand Down Expand Up @@ -768,8 +811,8 @@ mod tests {
"pull_request": {
"title": "merge queue: batch",
"body": "```yaml\npull_requests:\n - number: 7\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}},
},
}),
);
Expand All @@ -793,21 +836,23 @@ mod tests {

#[test]
fn mq_body_with_option_like_base_warns_and_falls_through() {
// MRGFY-8845: `extract_from_event` admits any pull request
// MRGFY-8845: `extract_from_event` admitted any pull request
// whose title starts with "merge queue: ", so the reporter's
// fork PR put `--output=<path>` 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,
&serde_json::json!({
"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}},
},
}),
);
Expand Down Expand Up @@ -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}},
},
}),
);
Expand Down Expand Up @@ -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}},
},
}),
);
Expand Down Expand Up @@ -943,16 +989,17 @@ 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,
&serde_json::json!({
"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}},
},
}),
);
Expand Down
51 changes: 51 additions & 0 deletions crates/mergify-ci/src/github_event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ pub struct GitRef {
pub sha: String,
#[serde(default)]
pub r#ref: Option<String>,
/// `null` on a pull request's `head` once its fork is deleted.
#[serde(default)]
pub repo: Option<RefRepository>,
}

#[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<u64>,
}

#[derive(Debug, Clone, Default, Deserialize)]
Expand All @@ -31,6 +42,31 @@ pub struct PullRequest {
pub head: Option<GitRef>,
}

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<GitRef>| 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)]
Expand Down Expand Up @@ -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::<PullRequest>(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"}}"#;
Expand Down
75 changes: 57 additions & 18 deletions crates/mergify-ci/src/queue_metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -51,9 +53,10 @@ pub fn parse_yaml_block(body: &str) -> Option<serde_json::Value> {

/// 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,
Expand All @@ -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);
Expand Down Expand Up @@ -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<u64>, base_repo: Option<u64>, body: Option<&str>) -> GitHubEvent {
let git_ref = |id: Option<u64>| 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());
Expand All @@ -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:?}"
);
}
}
}
Loading