Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
11 changes: 9 additions & 2 deletions packages/engine/src/testing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,15 @@ export class MemoryGitHub implements GitHubClient, GitHubClientFactory {
return { ...this.pullRequest };
}

async isTrustedAuthor(pr: { author: string; authorAssociation: string }): Promise<boolean> {
if (['OWNER', 'MEMBER', 'COLLABORATOR'].includes(pr.authorAssociation)) return true;
async isTrustedAuthor(pr: {
author: string;
authorAssociation: string;
botOnOwnBranch: boolean;
headSha: string;
}): Promise<boolean> {
if (['OWNER', 'MEMBER', 'COLLABORATOR'].includes(pr.authorAssociation) || pr.botOnOwnBranch) {
return true;
}
return this.hasWriteAccess(pr.author);
}

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
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