From 392824043aad1743189c788df6649249b99a49f8 Mon Sep 17 00:00:00 2001 From: Josef Schlehofer Date: Thu, 23 Jul 2026 21:47:11 +0200 Subject: [PATCH] cloudflare-worker: fallback to collaborator permissions check GitHub reports author_association CONTRIBUTOR (or NONE) for users whose organization membership is private, even when they are maintainers with write access. Those maintainers cannot use the [allow branch] and [allow cherry-pick] overrides, and their comments do not trigger a recheck. Where the association is not enough, ask the repository permission API (/collaborators/{username}/permission) and accept admin, write or maintain. Every answer costs a subrequest, so one resolver memoizes them for the whole request. Bots and comments without an override command are never resolved, comments from everyone else are dropped before the pull request is fetched, and the PR author is resolved only where a bypass can apply. Lookups stop once the reserved subrequest headroom is reached. A 404 means the user is not a collaborator, so it is no longer logged as an API failure. Signed-off-by: Josef Schlehofer --- cloudflare-worker/src/github.js | 34 +++++- cloudflare-worker/src/index.js | 101 +++++++++++----- cloudflare-worker/test/index.test.js | 172 +++++++++++++++++++++++++++ 3 files changed, 274 insertions(+), 33 deletions(-) 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) => {