diff --git a/cloudflare-worker/src/github.js b/cloudflare-worker/src/github.js index 023cebc..95b8455 100644 --- a/cloudflare-worker/src/github.js +++ b/cloudflare-worker/src/github.js @@ -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)}`); } } @@ -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); +} diff --git a/cloudflare-worker/src/index.js b/cloudflare-worker/src/index.js index 9244a57..41b5b03 100644 --- a/cloudflare-worker/src/index.js +++ b/cloudflare-worker/src/index.js @@ -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'; @@ -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 @@ -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; @@ -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; } } @@ -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 }); } @@ -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 }); @@ -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})`); @@ -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; @@ -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; }; @@ -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)); @@ -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)); diff --git a/cloudflare-worker/test/index.test.js b/cloudflare-worker/test/index.test.js index 8ce9622..33b7211 100644 --- a/cloudflare-worker/test/index.test.js +++ b/cloudflare-worker/test/index.test.js @@ -1038,6 +1038,11 @@ describe('Backport Cherry-pick and Bypass Validation', () => { } return new Response(JSON.stringify([]), { status: 200 }); } + if (url.includes('/collaborators/')) { + const username = decodeURIComponent(url.split('/collaborators/')[1].split('/')[0]); + const perm = prOptions.collaboratorPermissions?.[username] || 'read'; + return new Response(JSON.stringify({ permission: perm }), { status: 200 }); + } if (url.includes('/check-runs')) { if (options && options.method === 'POST') { const body = JSON.parse(options.body); @@ -1272,6 +1277,40 @@ describe('Backport Cherry-pick and Bypass Validation', () => { assert.strictEqual(commitCheck.conclusion, 'failure'); assert.match(commitCheck.output.text, /Pull request must originate from a feature branch/); }); + + test('passes on protected head branch if maintainer with CONTRIBUTOR author_association posted an [allow branch] comment', async () => { + const comments = [ + { body: 'Looks intentional, [allow branch]', author_association: 'CONTRIBUTOR', user: { login: 'nmeyerhans' } } + ]; + const response = await sendWebhookPR('Contribute feature', 'main', 'NONE', validCommit, comments, { + headBranch: 'stable', + checkBranch: true, + collaboratorPermissions: { nmeyerhans: 'admin' } + }); + assert.strictEqual(response.status, 200); + + const commitCheck = findCommitCheck(); + assert.ok(commitCheck); + assert.strictEqual(commitCheck.conclusion, 'success'); + assert.match(commitCheck.output.text, /allowed via override command/); + }); + + test('fails on protected head branch if contributor with CONTRIBUTOR author_association posted [allow branch] comment but lacks write permission', async () => { + const comments = [ + { body: 'Can someone [allow branch] please?', author_association: 'CONTRIBUTOR', user: { login: 'someuser' } } + ]; + const response = await sendWebhookPR('Contribute feature', 'main', 'NONE', validCommit, comments, { + headBranch: 'stable', + checkBranch: true, + collaboratorPermissions: { someuser: 'read' } + }); + assert.strictEqual(response.status, 200); + + const commitCheck = findCommitCheck(); + assert.ok(commitCheck); + assert.strictEqual(commitCheck.conclusion, 'failure'); + assert.match(commitCheck.output.text, /Pull request must originate from a feature branch/); + }); }); test('successfully paginates to find bypass comment on page 2 when page 1 is full of other comments', async () => { @@ -1581,6 +1620,139 @@ describe('Backport Cherry-pick and Bypass Validation', () => { assert.strictEqual(commitCheck.conclusion, 'success'); }); + // Maintainers with private organization membership are reported as + // CONTRIBUTOR/NONE, so the handler falls back to their repository permission. + async function sendIssueComment(comment, collaboratorPermissions = {}) { + postedCheckRuns = []; + let prFetched = false; + const permissionLookups = []; + + fetchMock = async (url, options) => { + if (url.includes('/access_tokens')) { + return new Response(JSON.stringify({ token: 'mocktoken' }), { status: 200 }); + } + if (url.includes('/formalities.json')) { + return new Response(JSON.stringify({ check_branch: false, require_linked_github_account: false, require_body: false }), { status: 200 }); + } + { const lr = graphqlLabelsHandler(url, options, []); if (lr) return lr; } + if (url.includes('/collaborators/')) { + const username = decodeURIComponent(url.split('/collaborators/')[1].split('/')[0]); + permissionLookups.push(username); + const permission = collaboratorPermissions[username]; + if (!permission) { + return new Response(JSON.stringify({ message: 'Not Found' }), { status: 404 }); + } + return new Response(JSON.stringify({ permission }), { status: 200 }); + } + if (url.endsWith('/pulls/123')) { + prFetched = true; + return new Response(JSON.stringify({ + number: 123, + title: 'test pr', + body: '', + base: { ref: 'main', sha: 'base-sha' }, + head: { ref: 'feature-branch', sha: 'abcdef1234567890' }, + author_association: 'NONE', + commits_url: 'https://api.github.com/repos/test/repo/pulls/123/commits', + user: { login: 'somecontributor', type: 'User' } + }), { status: 200 }); + } + if (url.includes('/commits/abcdef1234567890')) { + if (options?.headers?.Accept === 'application/vnd.github.patch') { + return new Response('Mock patch content', { status: 200 }); + } + return new Response(JSON.stringify({ + parents: [{ sha: 'parent-sha' }], + commit: { + message: 'mypkg: update\n\nSigned-off-by: John Doe ', + author: { name: 'John Doe', email: 'john@doe.com' }, + committer: { name: 'John Doe', email: 'john@doe.com' }, + verification: { verified: true, key_id: 'GPGKEYID' } + } + }), { status: 200 }); + } + if (url.includes('/commits')) { + return new Response(JSON.stringify([{ + sha: 'abcdef1234567890', + html_url: 'https://github.com/test/repo/commit/abcdef1234567890', + commit: { + message: 'mypkg: update\n\nSigned-off-by: John Doe ', + author: { name: 'John Doe', email: 'john@doe.com' }, + committer: { name: 'John Doe', email: 'john@doe.com' }, + verification: { verified: true, key_id: 'GPGKEYID' } + } + }]), { status: 200 }); + } + if (url.includes('/issues/123/comments')) { + return new Response(JSON.stringify([]), { status: 200 }); + } + if (url.includes('/check-runs')) { + if (options && options.method === 'POST') { + postedCheckRuns.push(JSON.parse(options.body)); + } + return new Response(JSON.stringify({}), { status: 201 }); + } + return new Response(JSON.stringify({}), { status: 200 }); + }; + + const payload = JSON.stringify({ + action: 'created', + issue: { number: 123, pull_request: { url: 'https://api.github.com/repos/test/repo/pulls/123' } }, + comment, + repository: { full_name: 'test/repo' }, + installation: { id: 456 } + }); + + const secret = 'mysecret'; + const signature = await calculateHmac(secret, payload); + const response = await worker.fetch(new Request('http://localhost/webhook', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'x-github-event': 'issue_comment', + 'x-hub-signature-256': signature + }, + body: payload + }), { WEBHOOK_SECRET: secret, APP_ID: '123', PRIVATE_KEY: 'YW55Y29udGVudA==' }); + + return { response, prFetched, permissionLookups }; + } + + test('processes a comment from a maintainer whose organization membership is private', async () => { + const { response, prFetched, permissionLookups } = await sendIssueComment( + { body: 'recheck please', author_association: 'CONTRIBUTOR', user: { login: 'privatemaintainer', type: 'User' } }, + { privatemaintainer: 'admin' } + ); + + assert.strictEqual(response.status, 200); + assert.deepStrictEqual(permissionLookups, ['privatemaintainer']); + assert.ok(prFetched); + assert.ok(postedCheckRuns.some(cr => cr.name === 'FormalityCheck / Git & Commits')); + }); + + test('ignores a comment from a user without write access without fetching the pull request', async () => { + const { response, prFetched, permissionLookups } = await sendIssueComment( + { body: 'recheck please', author_association: 'CONTRIBUTOR', user: { login: 'randomuser', type: 'User' } }, + { randomuser: 'read' } + ); + + assert.strictEqual(response.status, 200); + assert.strictEqual(await response.text(), 'Ignored non-maintainer issue comment'); + assert.deepStrictEqual(permissionLookups, ['randomuser']); + assert.strictEqual(prFetched, false, 'Non-maintainer comments must not pay for the pull request lookup'); + }); + + test('ignores bot comments without spending a permission lookup', async () => { + const { response, prFetched, permissionLookups } = await sendIssueComment( + { body: '[allow branch]', author_association: 'NONE', user: { login: 'github-actions[bot]', type: 'Bot' } } + ); + + assert.strictEqual(response.status, 200); + assert.strictEqual(await response.text(), 'Ignored non-maintainer issue comment'); + assert.deepStrictEqual(permissionLookups, []); + assert.strictEqual(prFetched, false); + }); + async function sendBackportWebhookPR(prTitle, baseBranch, commitMessage, prPatchContent, upstreamPatchContent = null) { postedCheckRuns = []; fetchMock = async (url, options) => {