bcr-pr-reviewer: check review freshness by commit_id, not commit timestamp (#2841)
`getPrApprovers()` decides whether a maintainer's existing approval
still applies to the PR's current content by comparing each review's
`submitted_at` against the head commit's date
(`latestCommit.commit.author.date`). A commit's author date is fully
controlled by whoever creates the commit (`GIT_AUTHOR_DATE` / `git
commit --date`) and GitHub preserves it on push, so the freshness cutoff
is attacker-controlled. By backdating a newly pushed head commit to a
time before an earlier approval, a stale approval of an earlier
innocuous commit keeps counting as valid for the new head the reviewer
never saw. `reviewPR()` feeds this `approvers` set directly into the
auto-approve + squash-merge decision, and since `CODEOWNERS` delegates
the whole `modules/` tree to this bot, the freshness check is the guard
that decides what merges into the registry.
This is the same freshness check hardened in #2764 (which stopped merge
commits from being excluded from the cutoff) and complements #2653
(which pinned the merge to the analyzed head SHA); both kept the
timestamp comparison, which is what this change removes.
The fix compares each review's `commit_id` against the current PR head
SHA instead of comparing timestamps. A review only vouches for the exact
commit it was submitted against, so it is fresh iff its `commit_id`
equals the head. This is timestamp-free and also handles the
merge-commit case without special-casing parent counts.
`index.test.js` is updated to `commit_id` fixtures and adds a regression
test for the backdated-head-commit case; it keeps the checks that a
genuinely head-matching approval still counts and that a later
non-approving review overrides an earlier approval. `node --test`
passes.
diff --git a/actions/bcr-pr-reviewer/index.js b/actions/bcr-pr-reviewer/index.js
index 776b2ee..543a38f 100644
--- a/actions/bcr-pr-reviewer/index.js
+++ b/actions/bcr-pr-reviewer/index.js
@@ -262,17 +262,19 @@
pull_number: prNumber,
});
- // Use the PR's actual current head commit as the review-freshness cutoff. This must
- // NOT exclude merge commits: a merge commit can itself introduce diff content (e.g.
- // conflict resolution, or content brought in from the merged branch), so treating it
- // as if it doesn't exist lets a stale approval of an earlier commit keep counting as
- // approving a HEAD the reviewer never actually saw -- a pusher could get an innocuous
- // early commit approved, then push a merge commit that changes the content, and the
- // stale approval would still be considered valid for the new HEAD.
+ // Determine review freshness by the commit a review was actually submitted
+ // against (its `commit_id`), not by comparing timestamps against the head
+ // commit's date. A commit's author date is fully attacker-controlled
+ // (GIT_AUTHOR_DATE / `git commit --date`) and GitHub preserves it on push,
+ // so a timestamp cutoff can be defeated by backdating a newly pushed head
+ // commit to before an earlier approval: the stale approval then looks "fresh"
+ // for a HEAD the reviewer never saw. A review only vouches for the exact
+ // commit it was submitted against, so it counts iff its `commit_id` equals the
+ // PR's current head SHA. This is timestamp-free and also covers the
+ // merge-commit case (the head is the head regardless of parent count).
const latestCommit = commits[commits.length - 1];
- const latestCommitTime = new Date(latestCommit.commit.author.date);
- console.log(`Latest commit: ${latestCommit.sha}`);
- console.log(`Latest commit time: ${latestCommitTime}`);
+ const headSha = latestCommit.sha;
+ console.log(`Latest commit (PR head): ${headSha}`);
// Get all review events for the PR
const reviewEvents = await octokit.paginate(octokit.rest.pulls.listReviews, {
@@ -281,11 +283,12 @@
pull_number: prNumber,
});
- // For each reviewer, collect their latest review that are newer than the latest commit
+ // For each reviewer, collect their latest review that was submitted against the
+ // current PR head commit.
// Key: reviewer, Value: review
const latestReviews = new Map();
reviewEvents.forEach(review => {
- if (new Date(review.submitted_at) < latestCommitTime) {
+ if (review.commit_id !== headSha) {
return;
}
diff --git a/actions/bcr-pr-reviewer/index.test.js b/actions/bcr-pr-reviewer/index.test.js
index d6e6e47..61870e9 100644
--- a/actions/bcr-pr-reviewer/index.test.js
+++ b/actions/bcr-pr-reviewer/index.test.js
@@ -20,43 +20,77 @@
fakeOctokit.pullsListCommitsMarker = Symbol('listCommits');
fakeOctokit.pullsListReviewsMarker = Symbol('listReviews');
-test('getPrApprovers: a stale approval does not count once a merge commit changes the PR head', async () => {
+test('getPrApprovers: an approval submitted against an earlier commit does not count once a merge commit changes the PR head', async () => {
// C1 gets approved by a maintainer, then C2 (a merge commit -- 2 parents) is
// pushed afterwards and becomes the PR's real HEAD. The maintainer's review
- // predates C2 and never saw its content.
+ // was submitted against C1 and never saw C2's content.
const commits = [
{ sha: 'C1', parents: [{ sha: 'base' }], commit: { author: { date: '2026-08-06T00:00:00Z' } } },
{ sha: 'C2-merge', parents: [{ sha: 'C1' }, { sha: 'other' }], commit: { author: { date: '2026-08-06T00:20:00Z' } } },
];
const reviews = [
- { user: { login: 'maintainer' }, state: 'APPROVED', submitted_at: '2026-08-06T00:10:00Z' },
+ { user: { login: 'maintainer' }, state: 'APPROVED', commit_id: 'C1', submitted_at: '2026-08-06T00:10:00Z' },
];
const approvers = await getPrApprovers(fakeOctokit({ commits, reviews }), 'owner', 'repo', 1);
- assert.equal(approvers.has('maintainer'), false, 'a review submitted before the real HEAD must not count as approving it');
+ assert.equal(approvers.has('maintainer'), false, 'a review submitted against a commit that is no longer the head must not count');
});
-test('getPrApprovers: an approval submitted after a merge commit still counts', async () => {
+test('getPrApprovers: a stale approval stays rejected even when the new head commit is backdated (author.date is attacker-controlled)', async () => {
+ // The attacker gets C1 approved, then pushes C2-evil as the new head but forges
+ // its author.date to BEFORE the approval (GIT_AUTHOR_DATE). A timestamp cutoff
+ // (review.submitted_at >= head author.date) would wrongly treat the stale C1
+ // approval as fresh for C2-evil. Matching on the review's commit_id is immune
+ // to the forged date.
+ const commits = [
+ { sha: 'C1', parents: [{ sha: 'base' }], commit: { author: { date: '2026-08-06T10:00:00Z' } } },
+ // Pushed after the approval, but its author date is forged into the past.
+ { sha: 'C2-evil', parents: [{ sha: 'C1' }], commit: { author: { date: '2020-01-01T00:00:00Z' } } },
+ ];
+ const reviews = [
+ { user: { login: 'maintainer' }, state: 'APPROVED', commit_id: 'C1', submitted_at: '2026-08-06T10:05:00Z' },
+ ];
+
+ const approvers = await getPrApprovers(fakeOctokit({ commits, reviews }), 'owner', 'repo', 1);
+
+ assert.equal(approvers.has('maintainer'), false, 'a backdated head commit must not let a stale approval count as fresh');
+});
+
+test('getPrApprovers: an approval submitted against the current head merge commit still counts', async () => {
const commits = [
{ sha: 'C1', parents: [{ sha: 'base' }], commit: { author: { date: '2026-08-06T00:00:00Z' } } },
{ sha: 'C2-merge', parents: [{ sha: 'C1' }, { sha: 'other' }], commit: { author: { date: '2026-08-06T00:20:00Z' } } },
];
const reviews = [
- { user: { login: 'maintainer' }, state: 'APPROVED', submitted_at: '2026-08-06T00:30:00Z' },
+ { user: { login: 'maintainer' }, state: 'APPROVED', commit_id: 'C2-merge', submitted_at: '2026-08-06T00:30:00Z' },
];
const approvers = await getPrApprovers(fakeOctokit({ commits, reviews }), 'owner', 'repo', 1);
- assert.equal(approvers.has('maintainer'), true, 'a review submitted after the real HEAD should still count');
+ assert.equal(approvers.has('maintainer'), true, 'a review submitted against the current head should count');
});
-test('getPrApprovers: works normally when the PR has no merge commits at all', async () => {
+test('getPrApprovers: the latest review state against the head wins (a later request-changes overrides an earlier approval)', async () => {
const commits = [
{ sha: 'C1', parents: [{ sha: 'base' }], commit: { author: { date: '2026-08-06T00:00:00Z' } } },
];
const reviews = [
- { user: { login: 'maintainer' }, state: 'APPROVED', submitted_at: '2026-08-06T00:05:00Z' },
+ { user: { login: 'maintainer' }, state: 'APPROVED', commit_id: 'C1', submitted_at: '2026-08-06T00:05:00Z' },
+ { user: { login: 'maintainer' }, state: 'CHANGES_REQUESTED', commit_id: 'C1', submitted_at: '2026-08-06T00:09:00Z' },
+ ];
+
+ const approvers = await getPrApprovers(fakeOctokit({ commits, reviews }), 'owner', 'repo', 1);
+
+ assert.equal(approvers.has('maintainer'), false, 'a newer non-approving review against the same head must override the earlier approval');
+});
+
+test('getPrApprovers: works normally when a single-commit PR is approved at head', async () => {
+ const commits = [
+ { sha: 'C1', parents: [{ sha: 'base' }], commit: { author: { date: '2026-08-06T00:00:00Z' } } },
+ ];
+ const reviews = [
+ { user: { login: 'maintainer' }, state: 'APPROVED', commit_id: 'C1', submitted_at: '2026-08-06T00:05:00Z' },
];
const approvers = await getPrApprovers(fakeOctokit({ commits, reviews }), 'owner', 'repo', 1);
