Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/engine/src/chat-job.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ function setup(models: MockLanguageModelV4[] = []) {
body: '',
author: 'octocat',
authorAssociation: 'MEMBER',
botOnOwnBranch: false,
draft: false,
state: 'open',
merged: false,
Expand Down
9 changes: 8 additions & 1 deletion packages/engine/src/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ export interface PullRequest {
body: string;
author: string;
authorAssociation: string;
/** A bot's pull request from a branch in this repository, not a fork. */
botOnOwnBranch: boolean;
draft: boolean;
state: string;
/** True once the pull request has been merged. Closed-without-merge stays false. */
Expand Down Expand Up @@ -78,7 +80,12 @@ export interface GitHubClient {
* Whether the PR author can write to the repository: the author association first, then the
* actual repository permission. Errors count as untrusted.
*/
isTrustedAuthor(pr: { author: string; authorAssociation: string }): Promise<boolean>;
isTrustedAuthor(pr: {
author: string;
authorAssociation: string;
botOnOwnBranch: boolean;
headSha: string;
}): Promise<boolean>;
/** Whether an account can write to the repository. Errors count as no. */
hasWriteAccess(username: string): Promise<boolean>;
/** Reads a file at a ref, or `null` when it does not exist. */
Expand Down
1 change: 1 addition & 0 deletions packages/engine/src/review-job.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ function setup(models: MockLanguageModelV4[] = []) {
body: '',
author: 'octocat',
authorAssociation: 'MEMBER',
botOnOwnBranch: false,
draft: false,
state: 'open',
merged: false,
Expand Down
2 changes: 1 addition & 1 deletion packages/engine/src/review-job.ts
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ function reviewSandbox(
*/
async function approvalRestriction(
{ github }: GitHubConnection,
pr: { author: string; authorAssociation: string },
pr: { author: string; authorAssociation: string; botOnOwnBranch: boolean; headSha: string },
config: RepoConfig
): Promise<string | undefined> {
if (config.reviews.approveOutsideContributors) return undefined;
Expand Down
1 change: 1 addition & 0 deletions packages/engine/src/scope.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ function setup() {
body: '',
author: 'octocat',
authorAssociation: 'MEMBER',
botOnOwnBranch: false,
draft: false,
state: 'open',
merged: false,
Expand Down
7 changes: 6 additions & 1 deletion packages/github/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ export class OctokitGitHubClient implements GitHubClient {
return pulls.getPullRequest(this.octokit, this.ref, pullNumber);
}

isTrustedAuthor(pr: { author: string; authorAssociation: string }) {
isTrustedAuthor(pr: {
author: string;
authorAssociation: string;
botOnOwnBranch: boolean;
headSha: string;
}) {
return pulls.isTrustedAuthor(this.octokit, this.ref, pr);
}

Expand Down
78 changes: 78 additions & 0 deletions packages/github/src/pulls.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import {
dismissChangeRequests,
getFailedChecks,
getLinkedIssues,
getPullRequest,
isTrustedAuthor,
resolveReviewThreads,
startCheckRun
} from './pulls';
Expand Down Expand Up @@ -269,3 +271,79 @@ test('dismissChangeRequests with `only` leaves the reviews it rejects', async ()
expect(dismissed).toEqual([1]);
expect(count).toBe(1);
});

const pullRequestWith = (type: string, headRepository: number | null) =>
({
rest: {
pulls: {
get: async () => ({
data: {
user: { login: 'renovate[bot]', type },
author_association: 'NONE',
base: { ref: 'main', sha: 'base', repo: { id: 1, clone_url: 'url' } },
head: { sha: 'head', repo: headRepository ? { id: headRepository } : null }
}
})
}
}
}) as unknown as Octokit;

test('getPullRequest marks only a bot on a branch of this repository', async () => {
const ref = { owner: 'o', repo: 'r' };
const own = await getPullRequest(pullRequestWith('Bot', 1), ref, 1);
const fork = await getPullRequest(pullRequestWith('Bot', 2), ref, 1);
const deleted = await getPullRequest(pullRequestWith('Bot', null), ref, 1);
const person = await getPullRequest(pullRequestWith('User', 1), ref, 1);
expect(own.botOnOwnBranch).toBe(true);
expect(fork.botOnOwnBranch).toBe(false);
expect(deleted.botOnOwnBranch).toBe(false);
expect(person.botOnOwnBranch).toBe(false);
});

const trustOctokit = (commit: () => Promise<{ data: { author: { login: string } | null } }>) =>
({
rest: {
repos: {
getCommit: commit,
getCollaboratorPermissionLevel: async () => ({ data: { permission: 'none' } })
}
}
}) as unknown as Octokit;

const botPullRequest = {
author: 'renovate[bot]',
authorAssociation: 'NONE',
botOnOwnBranch: true,
headSha: 'head'
};

test('isTrustedAuthor trusts a bot whose own commit heads its branch', async () => {
const octokit = trustOctokit(async () => ({ data: { author: { login: 'renovate[bot]' } } }));
expect(await isTrustedAuthor(octokit, { owner: 'o', repo: 'r' }, botPullRequest)).toBe(true);
});

test("isTrustedAuthor does not trust a bot on someone else's or an unattributed commit", async () => {
const ref = { owner: 'o', repo: 'r' };
const other = trustOctokit(async () => ({ data: { author: { login: 'someone' } } }));
const unattributed = trustOctokit(async () => ({ data: { author: null } }));
expect(await isTrustedAuthor(other, ref, botPullRequest)).toBe(false);
expect(await isTrustedAuthor(unattributed, ref, botPullRequest)).toBe(false);
});

test('isTrustedAuthor does not trust a bot when its commit cannot be read', async () => {
const octokit = trustOctokit(async () => {
throw new Error('not found');
});
expect(await isTrustedAuthor(octokit, { owner: 'o', repo: 'r' }, botPullRequest)).toBe(false);
});

test('isTrustedAuthor does not read the commit of a bot from a fork', async () => {
let read = false;
const octokit = trustOctokit(async () => {
read = true;
return { data: { author: { login: 'renovate[bot]' } } };
});
const fork = { ...botPullRequest, botOnOwnBranch: false };
expect(await isTrustedAuthor(octokit, { owner: 'o', repo: 'r' }, fork)).toBe(false);
expect(read).toBe(false);
});
24 changes: 22 additions & 2 deletions packages/github/src/pulls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ export async function getPullRequest(octokit: Octokit, ref: RepoRef, pullNumber:
body: data.body ?? '',
author: data.user?.login ?? 'unknown',
authorAssociation: data.author_association,
/** A bot's pull request from a branch in this repository, not a fork. */
botOnOwnBranch: data.user?.type === 'Bot' && data.head.repo?.id === data.base.repo.id,
draft: data.draft ?? false,
state: data.state,
/** True once the pull request has been merged. Closed-without-merge stays false. */
Expand All @@ -39,17 +41,35 @@ const TRUSTED_ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']);
/**
* Whether the PR author can write to the repository. The author association is checked first;
* members with private org membership can show up as CONTRIBUTOR there, so anyone else is
* checked against their actual repository permission. Errors count as untrusted.
* checked against their actual repository permission, which GitHub reports as none for a GitHub
* App's bot however much access the App has. A bot is trusted instead when the head commit on
* its own branch in this repository is its own, since only write access can put it there.
* Errors count as untrusted.
*/
export async function isTrustedAuthor(
octokit: Octokit,
ref: RepoRef,
pr: { author: string; authorAssociation: string }
pr: { author: string; authorAssociation: string; botOnOwnBranch: boolean; headSha: string }
): Promise<boolean> {
if (TRUSTED_ASSOCIATIONS.has(pr.authorAssociation)) return true;
if (pr.botOnOwnBranch) return isCommitBy(octokit, ref, pr.headSha, pr.author);
Comment thread
hansi-codes[bot] marked this conversation as resolved.
Resolved
return hasWriteAccess(octokit, ref, pr.author);
}

async function isCommitBy(
octokit: Octokit,
ref: RepoRef,
sha: string,
login: string
): Promise<boolean> {
try {
const { data } = await octokit.rest.repos.getCommit({ ...ref, ref: sha });
return data.author?.login === login;
} catch {
return false;
}
}

/** Reads a file at a ref, or `null` when it does not exist. */
export async function getFileContent(
octokit: Octokit,
Expand Down