Skip to content

Commit 77989b2

Browse files
committed
fix: verify reverse-rebase result provenance
1 parent db271c4 commit 77989b2

2 files changed

Lines changed: 66 additions & 6 deletions

File tree

‎src/processor/pull.test.ts‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ function fixture(options: {
4040
leaseError?: unknown;
4141
aheadBy?: number;
4242
compareError?: unknown;
43+
rebasedAheadBy?: number;
44+
rebasedBehindBy?: number;
45+
temporaryMergeable?: boolean | null;
4346
} = {}) {
4447
const calls: Call[] = [];
4548
let tempRef = "";
@@ -57,7 +60,15 @@ function fixture(options: {
5760
compareCommits: (args: Record<string, unknown>) => {
5861
record("repos.compareCommits", args);
5962
if (options.compareError) throw options.compareError;
60-
return { data: { ahead_by: options.aheadBy ?? 1 } };
63+
const isRebased = args.head === "rebased";
64+
return {
65+
data: {
66+
ahead_by: isRebased
67+
? options.rebasedAheadBy ?? options.aheadBy ?? 1
68+
: options.aheadBy ?? 1,
69+
behind_by: isRebased ? options.rebasedBehindBy ?? 0 : 0,
70+
},
71+
};
6172
},
6273
},
6374
git: {
@@ -110,6 +121,9 @@ function fixture(options: {
110121
data: {
111122
number: 99,
112123
state: "open",
124+
mergeable: options.temporaryMergeable === undefined
125+
? false
126+
: options.temporaryMergeable,
113127
head: { ref: "fork" },
114128
base: { ref: tempRef },
115129
},
@@ -165,6 +179,7 @@ Deno.test("reverse-rebase verifies SHAs, updates base to merge result, and clean
165179
"repos.compareCommits",
166180
"pulls.create",
167181
"pulls.merge",
182+
"repos.compareCommits",
168183
"git.getRef",
169184
"git.getRef",
170185
"graphql",
@@ -186,9 +201,9 @@ Deno.test("reverse-rebase verifies SHAs, updates base to merge result, and clean
186201
);
187202
assertEquals(calls[3].args.merge_method, "rebase");
188203
assertEquals(calls[3].args.sha, "fork-old");
189-
assertMatch(String(calls[6].args.query), /updateRefs/);
204+
assertMatch(String(calls[7].args.query), /updateRefs/);
190205
assertEquals(
191-
{ ...calls[6].args, query: undefined },
206+
{ ...calls[7].args, query: undefined },
192207
{
193208
query: undefined,
194209
repositoryId: "repo-node",
@@ -198,7 +213,7 @@ Deno.test("reverse-rebase verifies SHAs, updates base to merge result, and clean
198213
force: true,
199214
},
200215
);
201-
assertEquals(calls[10].args.ref, String(create.ref).replace("refs/", ""));
216+
assertEquals(calls[11].args.ref, String(create.ref).replace("refs/", ""));
202217
});
203218

204219
Deno.test("reverse-rebase never cleans up a ref it failed to create", async () => {
@@ -309,3 +324,21 @@ Deno.test("reverse-rebase cleans its temporary ref when comparison fails", async
309324
"git.deleteRef",
310325
]);
311326
});
327+
328+
Deno.test("reverse-rebase rejects a result with injected temporary-base ancestry", async () => {
329+
const { calls, run } = fixture({ rebasedAheadBy: 2 });
330+
assertEquals(await run(), false);
331+
assert(!calls.some((call) => call.name === "graphql"));
332+
assertEquals(calls.at(-1)?.name, "git.deleteRef");
333+
});
334+
335+
Deno.test("reverse-rebase does not label a policy 405 as a content conflict", async () => {
336+
const { calls, run } = fixture({
337+
mergeError: { status: 405 },
338+
temporaryMergeable: null,
339+
});
340+
assertEquals(await run(), false);
341+
assert(!calls.some((call) => call.name === "issues.update"));
342+
assert(calls.some((call) => call.name === "pulls.get"));
343+
assertEquals(calls.at(-1)?.name, "git.deleteRef");
344+
});

‎src/processor/pull.ts‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -565,18 +565,36 @@ export class Pull {
565565
});
566566
} catch (err) {
567567
if (this.isMergeConflictError(err)) {
568-
await this.handleMergeConflict(prNumber, rule);
568+
if (await this.isTemporaryPRConflicted(temporaryPR.number)) {
569+
await this.handleMergeConflict(prNumber, rule);
570+
}
569571
return false;
570572
}
571573
throw err;
572574
}
573575
if (!merged.data.merged || !merged.data.sha) {
574-
await this.handleMergeConflict(prNumber, rule);
576+
if (await this.isTemporaryPRConflicted(temporaryPR.number)) {
577+
await this.handleMergeConflict(prNumber, rule);
578+
}
575579
return false;
576580
}
577581
const rebasedSha = merged.data.sha;
578582
cleanupShas.add(rebasedSha);
579583

584+
const rebasedComparison = await this.github.repos.compareCommits({
585+
owner: this.owner,
586+
repo: this.repo,
587+
base: upstreamSha,
588+
head: rebasedSha,
589+
per_page: 1,
590+
});
591+
if (
592+
rebasedComparison.data.behind_by !== 0 ||
593+
rebasedComparison.data.ahead_by !== forkComparison.data.ahead_by
594+
) {
595+
throw new Error("Rebased commit provenance check failed");
596+
}
597+
580598
const [currentBase, currentTemporary] = await Promise.all([
581599
this.github.git.getRef({
582600
owner: this.owner,
@@ -626,6 +644,15 @@ export class Pull {
626644
}
627645
}
628646

647+
private async isTemporaryPRConflicted(pullNumber: number): Promise<boolean> {
648+
const current = await this.github.pulls.get({
649+
owner: this.owner,
650+
repo: this.repo,
651+
pull_number: pullNumber,
652+
});
653+
return current.data.mergeable === false;
654+
}
655+
629656
private isMergeConflictError(err: unknown): boolean {
630657
return typeof err === "object" && err !== null &&
631658
"status" in err && err.status === 405;

0 commit comments

Comments
 (0)