Skip to content

Commit 2968f30

Browse files
committed
fix(node): do not trust commiter date
The date of a commit is provided by the user, and we should not trust it blindly. Instead, we can check when was the last push event to the PR base branch.
1 parent dbd8c65 commit 2968f30

File tree

4 files changed

+40
-0
lines changed

4 files changed

+40
-0
lines changed

Diff for: lib/pr_checker.js

+9
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,15 @@ export default class PRChecker {
548548
});
549549

550550
const totalCommits = afterCommits.length;
551+
if (totalCommits === 0 && this.pr.timelineItems.updatedAt > reviewDate) {
552+
// Some commits were pushed, but all the commits have a commit date prior
553+
// to the last review. It means that either that a long time elapsed
554+
// between the commit and the push, or that the clock on the dev machine
555+
// is wrong, or the commit date was forged.
556+
cli.warn('Something was pushed to the Pull Request branch since the last approving review.');
557+
return false;
558+
}
559+
551560
if (totalCommits > 0) {
552561
cli.warn('Commits were pushed since the last approving review:');
553562
const sliceLength = maxCommits === 0 ? totalCommits : -maxCommits;

Diff for: lib/queries/PR.gql

+3
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ query PR($prid: Int!, $owner: String!, $repo: String!) {
2323
path
2424
}
2525
},
26+
timelineItems(itemTypes: [HEAD_REF_FORCE_PUSHED_EVENT, PULL_REQUEST_COMMIT]) {
27+
updatedAt
28+
},
2629
title,
2730
baseRefName,
2831
headRefName,

Diff for: test/fixtures/semver_major_pr.json

+3
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
}
1717
]
1818
},
19+
"timelineItems": {
20+
"updatedAt": "2017-10-24T11:13:43Z"
21+
},
1922
"title": "lib: awesome changes",
2023
"baseRefName": "main",
2124
"headRefName": "awesome-changes"

Diff for: test/unit/pr_checker.test.js

+25
Original file line numberDiff line numberDiff line change
@@ -2152,6 +2152,31 @@ describe('PRChecker', () => {
21522152
cli.assertCalledWith(expectedLogs);
21532153
});
21542154

2155+
it('should return true if PR can be landed', () => {
2156+
const expectedLogs = {
2157+
warn: [
2158+
['Something was pushed to the Pull Request branch since the last approving review.']
2159+
],
2160+
info: [],
2161+
error: []
2162+
};
2163+
2164+
const checker = new PRChecker(cli, {
2165+
pr: { ...semverMajorPR, timelineItems: { updatedAt: new Date().toISOString() } },
2166+
reviewers: allGreenReviewers,
2167+
comments: commentsWithLGTM,
2168+
reviews: approvingReviews,
2169+
commits: simpleCommits,
2170+
collaborators,
2171+
authorIsNew: () => true,
2172+
getThread: PRData.prototype.getThread
2173+
}, {}, argv);
2174+
2175+
const status = checker.checkCommitsAfterReview();
2176+
assert.deepStrictEqual(status, false);
2177+
cli.assertCalledWith(expectedLogs);
2178+
});
2179+
21552180
it('should skip the check if there are no reviews', () => {
21562181
const { commits } = multipleCommitsAfterReview;
21572182
const expectedLogs = {};

0 commit comments

Comments
 (0)