Skip to content

cloudflare-worker: fallback to collaborator permissions check #70

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
BKPepe wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cloudflare-worker: fallback to collaborator permissions check #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 28 additions & 6 deletions cloudflare-worker/src/github.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,13 @@ export async function githubApiCall(url, token, method = 'GET', payload = null,
// probe paths that may not exist (e.g. Makefile discovery in findPkgRoot).
const isExpected404 = response.status === 404 &&
method === 'GET' && url.includes('/contents/');
// 404/422 on GET /commits/ is expected only for opt-in callers
// (options.silent), e.g. the fork fallback when a commit SHA vanished
// after a force-push / rebase.
const isSilencedCommitMiss = options.silent === true &&
// 404/422 on a GET is expected only for opt-in callers (options.silent):
// the fork fallback when a commit SHA vanished after a force-push /
// rebase, and permission probes for users who are not collaborators.
const isSilencedMiss = options.silent === true &&
(response.status === 404 || response.status === 422) &&
method === 'GET' && url.includes('/commits/');
if (!isExpected404 && !isSilencedCommitMiss) {
method === 'GET';
if (!isExpected404 && !isSilencedMiss) {
console.error(`GitHub API call failed: ${method} ${url} -> HTTP ${response.status}: ${text.trim().slice(0, 500)}`);
}
}
Expand Down Expand Up @@ -318,3 +318,25 @@ export async function graphqlCheckExistence(token, repoFullname, ref, probes) {

return results;
}

// `permission` reports the base access level (admin/write/read/none), while
// `role_name` also names the finer-grained or custom role behind it.
const WRITE_ACCESS_LEVELS = ['admin', 'write', 'maintain'];

// Checks whether a user holds write access to a repository. Used as a fallback
// when author_association is CONTRIBUTOR or NONE, which is what GitHub reports
// for maintainers whose organization membership is private.
// A 404 here is the normal answer for "not a collaborator", so it is silenced
// rather than logged as an API failure.
export async function fetchUserRepoPermission(repoFullname, username, token, onCall) {
if (!repoFullname || !username) return false;
onCall?.();
const url = `https://api.github.com/repos/${repoFullname}/collaborators/${encodeURIComponent(username)}/permission`;
const res = await githubApiCall(url, token, 'GET', null, 'application/vnd.github+json', { silent: true });
if (res.code !== 200 || !res.data) {
return false;
}
const permission = (res.data.permission || '').toLowerCase();
const roleName = (res.data.role_name || '').toLowerCase();
return WRITE_ACCESS_LEVELS.includes(permission) || WRITE_ACCESS_LEVELS.includes(roleName);
}
101 changes: 74 additions & 27 deletions cloudflare-worker/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { DEFAULT_CONFIG, LABEL_GUIDELINES, LABEL_ADD_PACKAGE, LABEL_DROP_PACKAGE } from './config.js';
import { parseYaml, getLabelsForChangedFiles, getAllChangedFiles } from './labeler.js';
import { verifySignature, getInstallationToken } from './crypto.js';
import { githubApiCall, fetchRepositoryConfig, graphqlBatchFetchFiles, graphqlFetchRepoLabels, ensureLabelExists } from './github.js';
import { githubApiCall, fetchRepositoryConfig, graphqlBatchFetchFiles, graphqlFetchRepoLabels, ensureLabelExists, fetchUserRepoPermission } from './github.js';
import { validateFormalities, validateMakefileContext, validateEmbeddedPatches, validatePkgReleaseBumps, validateUciConfigs } from './validators.js';
import { handleScheduled } from './stale.js';
import { handleIssueLabeller, applyIssueLabelling, parseIssueLabellerYaml, DEFAULT_ISSUE_LABELLER_CONFIG } from './issue-labeller.js';
Expand All @@ -10,6 +10,44 @@ import { handleIssueLabeller, applyIssueLabelling, parseIssueLabellerYaml, DEFAU
// Head branches that must not be used as the origin of a pull request.
const PROTECTED_HEAD_BRANCHES = ['master', 'main', 'stable', 'openwrt-25.12', 'openwrt-24.10'];

// author_association values that identify a maintainer on their own. Anything
// else needs the repository permission lookup (see createMaintainerResolver).
const MAINTAINER_ASSOCIATIONS = ['OWNER', 'MEMBER', 'COLLABORATOR'];

// GitHub reports author_association CONTRIBUTOR (or NONE) for maintainers who
// keep their organization membership private, which would lock them out of the
// override commands. Fall back to the repository permission they actually hold.
// Answers are memoized per request: the same login shows up as PR author and as
// comment author, and every lookup costs a subrequest.
function createMaintainerResolver(repoFullname, token, subrequestBudget) {
const cache = new Map();

return async function isMaintainerUser(user, association) {
if (MAINTAINER_ASSOCIATIONS.includes((association || '').toUpperCase())) {
return true;
}
const login = user?.login;
// Bots hold write access on some repositories but never act as maintainers
// here, and asking about them would just burn a subrequest per comment.
if (!login || user?.type === 'Bot' || login.endsWith('[bot]')) {
return false;
}
if (cache.has(login)) {
return cache.get(login);
}
// Leave the reserve untouched so the terminal writes (labels, PR comment,
// check-runs) at the end of the handler always have headroom left.
if (subrequestBudget.limit - subrequestBudget.reserve - subrequestBudget.used <= 0) {
return false;
}
// Cache the pending promise, not the result, so concurrent questions about
// the same login share a single lookup.
const pending = fetchUserRepoPermission(repoFullname, login, token, () => { subrequestBudget.used++; });
cache.set(login, pending);
return pending;
};
}

// Parses an env var as an integer, falling back to `fallback` only when the
// value is unset/unparseable — unlike `parseInt(x, 10) || fallback`, this
// correctly honors an explicitly configured 0 instead of treating it the
Expand All @@ -20,7 +58,7 @@ function parseEnvInt(value, fallback) {
return Number.isNaN(parsed) ? fallback : parsed;
}

async function scanPrComments(repoFullname, prNumber, token, onCall) {
async function scanPrComments(repoFullname, prNumber, token, onCall, isMaintainerUser) {
let page = 1;
let hasCherryPickBypassComment = false;
let hasBranchBypassComment = false;
Expand All @@ -38,16 +76,15 @@ async function scanPrComments(repoFullname, prNumber, token, onCall) {
if (c.body?.startsWith('## Formality Check:')) {
existingCommentId = c.id;
}
const assoc = (c.author_association || '').toUpperCase();
const isCommentMaintainer = ['OWNER', 'MEMBER', 'COLLABORATOR'].includes(assoc);
if (isCommentMaintainer) {
const body = c.body || '';
if (/\[allow[ -]cherry[ -]pick\]/i.test(body)) {
hasCherryPickBypassComment = true;
}
if (/\[allow[ -]branch\]/i.test(body)) {
hasBranchBypassComment = true;
}
const body = c.body || '';
const hasCherryPickPattern = /\[allow[ -]cherry[ -]pick\]/i.test(body);
const hasBranchPattern = /\[allow[ -]branch\]/i.test(body);

// Only comments that actually carry an override command are worth
// resolving: for anyone outside the org that costs an extra subrequest.
if ((hasCherryPickPattern || hasBranchPattern) && await isMaintainerUser(c.user, c.author_association)) {
if (hasCherryPickPattern) hasCherryPickBypassComment = true;
if (hasBranchPattern) hasBranchBypassComment = true;
}
}

Expand Down Expand Up @@ -159,12 +196,6 @@ async function handleWebhook(request, env) {
return new Response("Ignored issue comment action", { status: 200 });
}

const commentAssoc = (data.comment?.author_association || '').toUpperCase();
const isCommentMaintainer = ['OWNER', 'MEMBER', 'COLLABORATOR'].includes(commentAssoc);
if (!isCommentMaintainer) {
return new Response("Ignored non-maintainer issue comment", { status: 200 });
}

if (!data.issue?.pull_request) {
return new Response("Comment is not on a pull request", { status: 200 });
}
Expand All @@ -187,12 +218,17 @@ async function handleWebhook(request, env) {
return new Response("Could not generate installation access token", { status: 500 });
}

const repoFullname = data.repository?.full_name;
if (!repoFullname) {
console.error("Webhook processing failed: Missing repository full name in payload.");
return new Response("Missing repository", { status: 400 });
}

// --- ISSUES EVENT: Issue Labeller ---
if (event === "issues") {
if (data.action !== "opened") {
return new Response("Ignored issues action", { status: 200 });
}
const repoFullname = data.repository.full_name;
const issueConfig = await fetchRepositoryConfig(data, token, DEFAULT_CONFIG, null);
if (!issueConfig.enable_issue_labeller) {
return new Response("Issue labeller disabled for this repository", { status: 200 });
Expand Down Expand Up @@ -231,15 +267,22 @@ async function handleWebhook(request, env) {
return githubApiCall(...args);
};

const isMaintainerUser = createMaintainerResolver(repoFullname, token, subrequestBudget);

if (event === "issue_comment") {
const repoFullnameFromPayload = data.repository?.full_name;
const prNumberFromIssue = data.issue?.number;
if (!repoFullnameFromPayload || !prNumberFromIssue) {
console.error(`Webhook processing failed: Missing repository (${repoFullnameFromPayload}) or PR number (${prNumberFromIssue}) in issue_comment payload.`);
return new Response("Missing repository or pull request number", { status: 400 });
if (!prNumberFromIssue) {
console.error("Webhook processing failed: Missing pull request number in issue_comment payload.");
return new Response("Missing pull request number", { status: 400 });
}

// Settle the commenter before anything else: comments from everyone else
// are dropped here, so they never pay for the pull request lookup below.
if (!await isMaintainerUser(data.comment?.user, data.comment?.author_association)) {
return new Response("Ignored non-maintainer issue comment", { status: 200 });
}

const prUrl = `https://api.github.com/repos/${repoFullnameFromPayload}/pulls/${prNumberFromIssue}`;
const prUrl = `https://api.github.com/repos/${repoFullname}/pulls/${prNumberFromIssue}`;
const prRes = await trackedApiCall(prUrl, token);
if (prRes.code !== 200) {
throw new Error(`Failed to fetch PR details from ${prUrl} (HTTP ${prRes.code})`);
Expand All @@ -261,7 +304,6 @@ async function handleWebhook(request, env) {



const repoFullname = data.repository.full_name;
const baseBranch = data.pull_request.base.ref;
const headBranch = data.pull_request.head.ref;
const prNumber = data.pull_request.number;
Expand Down Expand Up @@ -484,7 +526,7 @@ async function handleWebhook(request, env) {
let fetchCommentsRetried = false;
const getCommentsScan = () => {
if (fetchCommentsPromise === null) {
fetchCommentsPromise = scanPrComments(repoFullname, prNumber, token, () => { subrequestBudget.used++; }).catch(() => null);
fetchCommentsPromise = scanPrComments(repoFullname, prNumber, token, () => { subrequestBudget.used++; }, isMaintainerUser).catch(() => null);
}
return fetchCommentsPromise;
};
Expand Down Expand Up @@ -530,10 +572,14 @@ async function handleWebhook(request, env) {

const prBody = data.pull_request.body || '';
const association = (data.pull_request.author_association || 'NONE').toUpperCase();
const isMaintainer = ['OWNER', 'MEMBER', 'COLLABORATOR'].includes(association);
// Resolved on demand, like the comment scan above: only the branch and
// cherry-pick bypasses below care, and for an outside contributor the answer
// costs a subrequest (see createMaintainerResolver).
const isPrAuthorMaintainer = () => isMaintainerUser(data.pull_request.user, association);

if (CONFIG.check_branch) {
if (PROTECTED_HEAD_BRANCHES.includes(headBranch)) {
const isMaintainer = await isPrAuthorMaintainer();
const scanResult = await getCommentsScanWithRetry() || { hasBranchBypassComment: false, existingCommentId: null };
const bypassBranchCheck = scanResult.hasBranchBypassComment || (isMaintainer && /\[allow[ -]branch\]/i.test(prBody));

Expand Down Expand Up @@ -730,6 +776,7 @@ async function handleWebhook(request, env) {

if (isBackportPr) {
if (!(fullCommit.commit.message || '').toLowerCase().includes('cherry picked from')) {
const isMaintainer = await isPrAuthorMaintainer();
const scanResult = await getCommentsScanWithRetry() || { hasCherryPickBypassComment: false, existingCommentId: null };
const bypassCherryPickCheck = scanResult.hasCherryPickBypassComment || (isMaintainer && /\[allow[ -]cherry[ -]pick\]/i.test(prBody));

Expand Down
Loading