From 2534b1b6ab883b0395149cb2fc3f22ad2036bcd7 Mon Sep 17 00:00:00 2001 From: Bryan Thompson Date: Thu, 25 Jun 2026 11:35:27 -0500 Subject: [PATCH] refactor(ci): key external-PR allowlist on source org, not individuals Replace the per-username allowlist with a source-org allowlist so no individual is named in the repo. A non-member PR stays open only if it adds marketplace.json entries whose source.url is under an allowlisted prefix and changes nothing else; merge still requires CI + maintainer approval. - external-pr-allowed-sources.json: flat allowed_sources prefixes (no usernames) - scripts/external-pr-scope.js: shared additions-only / allowed-source logic - close-external-prs.yml + external-pr-scope-guard.yml: both use the shared module Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/external-contributors.json | 28 ---- .github/external-pr-allowed-sources.json | 20 +++ .github/scripts/external-pr-scope.js | 109 ++++++++++++++ .github/workflows/close-external-prs.yml | 38 +++-- .github/workflows/external-pr-scope-guard.yml | 137 +++--------------- 5 files changed, 169 insertions(+), 163 deletions(-) delete mode 100644 .github/external-contributors.json create mode 100644 .github/external-pr-allowed-sources.json create mode 100644 .github/scripts/external-pr-scope.js diff --git a/.github/external-contributors.json b/.github/external-contributors.json deleted file mode 100644 index 68d41298..00000000 --- a/.github/external-contributors.json +++ /dev/null @@ -1,28 +0,0 @@ -{ - "$comment": [ - "Allowlist of external (non-member) GitHub users explicitly permitted to open pull", - "requests against this repository. Used by .github/workflows/close-external-prs.yml", - "(to skip the auto-close) and .github/workflows/external-pr-scope-guard.yml (to", - "constrain what an allowlisted author may change).", - "", - "Presence here grants ONLY the right to OPEN a reviewable PR. It does not grant merge", - "rights: all CI still runs and a maintainer approval is still required before merge.", - "", - "allowed_sources: the scope guard requires every plugin entry an allowlisted author ADDS", - "to point at a source.url under one of these prefixes (case-insensitive, boundary-safe", - "prefix match on the normalized https URL). Keep it as tight as the author's actual repos." - ], - "contributors": [ - { - "github_username": "flovogt", - "name": "Florian Vogt", - "affiliation": "SAP SE", - "allowed_sources": [ - "github.com/UI5/", - "github.com/SAP/", - "github.com/SAP-samples/" - ], - "note": "SAP UI5 org; maintains the ui5, ui5-typescript-conversion, and ui5-modernization plugins." - } - ] -} diff --git a/.github/external-pr-allowed-sources.json b/.github/external-pr-allowed-sources.json new file mode 100644 index 00000000..6f4a1c10 --- /dev/null +++ b/.github/external-pr-allowed-sources.json @@ -0,0 +1,20 @@ +{ + "$comment": [ + "Source orgs/repos from which a pull request opened by a non-member is allowed to stay", + "open (instead of being auto-closed). Used by .github/workflows/close-external-prs.yml", + "and .github/workflows/external-pr-scope-guard.yml via .github/scripts/external-pr-scope.js.", + "", + "This keys on the SOURCE repo, not on individuals — no usernames are listed. A non-member", + "PR is in scope only if it ADDS marketplace.json entries whose source.url is under one of", + "these prefixes and changes nothing else. Being in scope grants ONLY the right to open a", + "reviewable PR: all CI still runs and a maintainer approval is still required before merge.", + "", + "Matching is case-insensitive and boundary-safe on the normalized https URL (leading", + "https:// and trailing .git removed): github.com/ui5 does NOT match github.com/ui5-evil." + ], + "allowed_sources": [ + "github.com/UI5/", + "github.com/SAP/", + "github.com/SAP-samples/" + ] +} diff --git a/.github/scripts/external-pr-scope.js b/.github/scripts/external-pr-scope.js new file mode 100644 index 00000000..62a51323 --- /dev/null +++ b/.github/scripts/external-pr-scope.js @@ -0,0 +1,109 @@ +'use strict'; +// Shared logic for the external-PR allowlist (keyed on source org, not on individuals). +// +// A pull request opened by a non-member is "in scope" only if it ADDS plugin entries to +// .claude-plugin/marketplace.json whose source.url is under an allowlisted prefix +// (.github/external-pr-allowed-sources.json) and changes nothing else — no other files, +// no removals, no edits to existing entries. +// +// Used by: +// - close-external-prs.yml (skip the auto-close when in scope) +// - external-pr-scope-guard.yml (required status check: fail a non-member PR that is out of scope) +// +// Security: evaluate() reads the head marketplace.json as DATA via the API and parses it; +// it never checks out or executes head code. The allowlist + this script are read from the +// trusted base checkout. + +const fs = require('fs'); +const MARKETPLACE = '.claude-plugin/marketplace.json'; + +function normalizeUrl(u) { + return String(u).trim().toLowerCase() + .replace(/^git\+/, '') + .replace(/^https?:\/\//, '') + .replace(/\.git$/, '') + .replace(/\/+$/, ''); // no trailing slash; matching adds the boundary +} + +function loadAllowed(allowlistPath) { + const j = JSON.parse(fs.readFileSync(allowlistPath, 'utf8')); + return (j.allowed_sources || []).map(normalizeUrl); +} + +function pluginsByName(json) { + const map = {}; + for (const p of (json && json.plugins) || []) { if (p && p.name) map[p.name] = p; } + return map; +} + +function sourceAllowed(url, allowed) { + const n = normalizeUrl(url); + if (n.split('/').length < 3) return false; // require a real host/org/repo path + // Boundary-safe: exact repo, or strictly under the allowed prefix. + return allowed.some(a => n === a || n.startsWith(a + '/')); +} + +// Pure decision over an already-computed diff. Returns { ok, problems, added, removed, modified }. +function analyze({ changedFiles, base, head, allowed }) { + const problems = []; + + const off = changedFiles.filter(n => n !== MARKETPLACE); + if (off.length) problems.push(`changes files other than ${MARKETPLACE}: ${off.join(', ')}`); + + const baseNames = new Set(Object.keys(base)); + const headNames = new Set(Object.keys(head)); + const removed = [...baseNames].filter(n => !headNames.has(n)); + const added = [...headNames].filter(n => !baseNames.has(n)); + const modified = [...headNames].filter( + n => baseNames.has(n) && JSON.stringify(base[n]) !== JSON.stringify(head[n]) + ); + + if (removed.length) problems.push(`removes existing entr${removed.length > 1 ? 'ies' : 'y'}: ${removed.join(', ')}`); + if (modified.length) problems.push(`modifies existing entr${modified.length > 1 ? 'ies' : 'y'}: ${modified.join(', ')}`); + if (!off.length && !added.length && !removed.length && !modified.length) { + problems.push('makes no in-scope change (expected additions to marketplace.json)'); + } + + for (const name of added) { + const url = head[name] && head[name].source && head[name].source.url; + if (!url) { problems.push(`added "${name}" has no source.url to validate`); continue; } + if (!sourceAllowed(url, allowed)) { + problems.push(`added "${name}" points at ${url}, outside the allowed sources`); + } + } + + return { ok: problems.length === 0, problems, added, removed, modified }; +} + +async function readPlugins(github, owner, repo, ref) { + try { + const { data } = await github.rest.repos.getContent({ owner, repo, ref, path: MARKETPLACE }); + return pluginsByName(JSON.parse(Buffer.from(data.content, 'base64').toString('utf8'))); + } catch (e) { + return null; + } +} + +// API wrapper used by both workflows. Fetches the diff and delegates to analyze(). +async function evaluate({ github, context, allowlistPath }) { + const pr = context.payload.pull_request; + const owner = context.repo.owner, repo = context.repo.repo; + + const allowed = loadAllowed(allowlistPath); + if (!allowed.length) return { ok: false, problems: ['allowed_sources is empty'], added: [], removed: [], modified: [] }; + + const files = await github.paginate(github.rest.pulls.listFiles, { + owner, repo, pull_number: pr.number, per_page: 100, + }); + const changedFiles = files.map(f => f.filename); + + const base = await readPlugins(github, owner, repo, pr.base.sha); + const head = await readPlugins(github, pr.head.repo.owner.login, pr.head.repo.name, pr.head.sha); + if (base === null || head === null) { + return { ok: false, problems: ['could not read marketplace.json at base and/or head'], added: [], removed: [], modified: [] }; + } + + return analyze({ changedFiles, base, head, allowed }); +} + +module.exports = { normalizeUrl, sourceAllowed, analyze, readPlugins, evaluate, MARKETPLACE }; diff --git a/.github/workflows/close-external-prs.yml b/.github/workflows/close-external-prs.yml index 83b568ea..46537bc9 100644 --- a/.github/workflows/close-external-prs.yml +++ b/.github/workflows/close-external-prs.yml @@ -7,20 +7,20 @@ on: permissions: pull-requests: write issues: write + contents: read jobs: check-membership: if: vars.DISABLE_EXTERNAL_PR_CHECK != 'true' runs-on: ubuntu-latest steps: - # pull_request_target: this checks out the BASE repo (trusted), so the allowlist - # we read below is this repo's version, never the fork's. + # pull_request_target: checks out the BASE repo (trusted), so the allowlist + shared + # script below are this repo's versions, never the fork's. - uses: actions/checkout@v4 - - name: Check if author has write access + - name: Close PR unless author is a member or the PR is an in-scope external contribution uses: actions/github-script@v7 with: script: | - const fs = require('fs'); const author = context.payload.pull_request.user.login; const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ @@ -34,24 +34,22 @@ jobs: return; } - // Allowlisted external contributors (e.g. enterprise partners who cannot use the - // submission form) may OPEN a PR. This does NOT grant merge: all CI still runs, - // the external-pr-scope-guard check constrains what they can change, and an - // Anthropic maintainer approval is still required by branch protection. - try { - const list = JSON.parse(fs.readFileSync('.github/external-contributors.json', 'utf8')); - const allowed = (list.contributors || []).some( - c => (c.github_username || '').toLowerCase() === author.toLowerCase() - ); - if (allowed) { - console.log(`${author} is an allowlisted external contributor, allowing PR (scope guard + maintainer review still apply)`); - return; - } - } catch (e) { - console.log(`Could not read external-contributors allowlist: ${e.message}`); + // Non-member: allow the PR to stay open ONLY if it is an in-scope external + // contribution — it adds marketplace.json entries pointing at an allowlisted + // source org and changes nothing else (see .github/external-pr-allowed-sources.json). + // This grants only the right to open a reviewable PR; the External PR Scope Guard + // required check and a maintainer approval still gate the merge. + const { evaluate } = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/external-pr-scope.js`); + const result = await evaluate({ + github, context, + allowlistPath: `${process.env.GITHUB_WORKSPACE}/.github/external-pr-allowed-sources.json`, + }); + if (result.ok && result.added.length > 0) { + console.log(`In-scope external contribution (adds: ${result.added.join(', ')}) — allowing PR.`); + return; } - console.log(`${author} has ${data.permission} access, closing PR`); + console.log(`Closing PR from ${author}: ${result.problems.join('; ') || 'out of scope'}`); await github.rest.issues.createComment({ owner: context.repo.owner, diff --git a/.github/workflows/external-pr-scope-guard.yml b/.github/workflows/external-pr-scope-guard.yml index 3bcd4a92..a586f3ea 100644 --- a/.github/workflows/external-pr-scope-guard.yml +++ b/.github/workflows/external-pr-scope-guard.yml @@ -1,16 +1,17 @@ name: External PR Scope Guard -# Constrains what an allowlisted EXTERNAL contributor (see .github/external-contributors.json) -# may change in a pull request. Anthropic members (write/admin) are unrestricted and skip this -# check. For an allowlisted external author this is a REQUIRED status check (configure in branch -# protection) that fails unless: -# 1. the PR changes ONLY .claude-plugin/marketplace.json (no workflow edits, no other plugins' -# files, and crucially not the allowlist itself), and -# 2. the marketplace.json delta is additions-only — no existing entry is modified or removed, and -# every ADDED entry's source.url points at a repo under that author's allowed_sources. +# Required status check that constrains what a NON-MEMBER pull request may change. +# Members (write/admin) are unrestricted and skip this check. For a non-member PR this +# fails unless the PR is an in-scope external contribution per .github/scripts/external-pr-scope.js: +# it changes ONLY .claude-plugin/marketplace.json, the delta is additions-only (no existing +# entry modified or removed), and every ADDED entry's source.url is under an allowlisted prefix +# in .github/external-pr-allowed-sources.json. +# +# Add the scope-guard job as a REQUIRED status check in branch protection for it to block merge. # # Security: runs on pull_request_target but checks out only the BASE repo (trusted) for the -# allowlist; the head's marketplace.json is fetched as DATA via the API and parsed, never executed. +# allowlist + script; the head marketplace.json is fetched as DATA via the API and parsed, +# never executed. on: pull_request_target: @@ -24,125 +25,31 @@ jobs: scope-guard: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 # base repo (trusted) — for the allowlist + - uses: actions/checkout@v4 # base repo (trusted) - uses: actions/github-script@v7 with: script: | - const fs = require('fs'); - const pr = context.payload.pull_request; - const author = pr.user.login; - const MARKETPLACE = '.claude-plugin/marketplace.json'; + const author = context.payload.pull_request.user.login; - // Anthropic members are unrestricted. const { data: perm } = await github.rest.repos.getCollaboratorPermissionLevel({ owner: context.repo.owner, repo: context.repo.repo, username: author, }); if (['admin', 'write'].includes(perm.permission)) { - console.log(`${author} is ${perm.permission} (Anthropic member) — scope guard not applicable.`); + console.log(`${author} is ${perm.permission} (member) — scope guard not applicable.`); return; } - // Look up the author in the allowlist (read from the trusted base checkout). - let entry; - try { - const list = JSON.parse(fs.readFileSync('.github/external-contributors.json', 'utf8')); - entry = (list.contributors || []).find( - c => (c.github_username || '').toLowerCase() === author.toLowerCase() - ); - } catch (e) { - core.setFailed(`Could not read .github/external-contributors.json: ${e.message}`); - return; - } - if (!entry) { - // Not allowlisted: the Close External PRs workflow governs this author. Nothing to guard. - console.log(`${author} is not on the external-contributor allowlist — scope guard is a no-op (the close workflow handles this PR).`); - return; - } - const allowed = (entry.allowed_sources || []).map(normalizeUrl); - if (allowed.length === 0) { - core.setFailed(`Allowlist entry for ${author} has no allowed_sources — cannot validate scope.`); - return; - } - - // (1) Changed-files gate: marketplace.json only. - const files = await github.paginate(github.rest.pulls.listFiles, { - owner: context.repo.owner, repo: context.repo.repo, pull_number: pr.number, per_page: 100, + const { evaluate } = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/external-pr-scope.js`); + const result = await evaluate({ + github, context, + allowlistPath: `${process.env.GITHUB_WORKSPACE}/.github/external-pr-allowed-sources.json`, }); - const offlimits = files.map(f => f.filename).filter(n => n !== MARKETPLACE); - if (offlimits.length > 0) { + + if (!result.ok) { core.setFailed( - `As an allowlisted external contributor, ${author} may only modify ${MARKETPLACE}. ` + - `This PR also changes: ${offlimits.join(', ')}.` + `Scope guard: a non-member PR may only ADD marketplace.json entries for an allowlisted source org.\n - ` + + result.problems.join('\n - ') ); return; } - if (!files.some(f => f.filename === MARKETPLACE)) { - console.log('No change to marketplace.json — nothing to validate.'); - return; - } - - // (2) Semantic base-vs-head diff of marketplace.json (head fetched as data, not executed). - const base = await readPlugins(context.repo.owner, context.repo.repo, pr.base.sha); - const head = await readPlugins(pr.head.repo.owner.login, pr.head.repo.name, pr.head.sha); - if (base === null || head === null) { - core.setFailed('Could not read marketplace.json at base and/or head.'); - return; - } - - const baseNames = new Set(Object.keys(base)); - const headNames = new Set(Object.keys(head)); - const removed = [...baseNames].filter(n => !headNames.has(n)); - const added = [...headNames].filter(n => !baseNames.has(n)); - const modified = [...headNames].filter( - n => baseNames.has(n) && JSON.stringify(base[n]) !== JSON.stringify(head[n]) - ); - - const problems = []; - if (removed.length) problems.push(`removes existing entr${removed.length > 1 ? 'ies' : 'y'}: ${removed.join(', ')}`); - if (modified.length) problems.push(`modifies existing entr${modified.length > 1 ? 'ies' : 'y'}: ${modified.join(', ')}`); - - for (const name of added) { - const url = head[name] && head[name].source && head[name].source.url; - if (!url) { problems.push(`added "${name}" has no source.url to validate`); continue; } - const n = normalizeUrl(url); - // Require a real host/org/repo path (rejects a bare org URL). - if (n.split('/').length < 3) { - problems.push(`added "${name}" source.url ${url} is not a valid repo URL`); - continue; - } - // Boundary-safe: an exact repo match, or a path strictly under the allowed prefix. - // (Prevents "github.com/ui5" from matching "github.com/ui5-evil/x".) - if (!allowed.some(a => n === a || n.startsWith(a + '/'))) { - problems.push(`added "${name}" points at ${url}, outside ${author}'s allowed_sources (${entry.allowed_sources.join(', ')})`); - } - } - - if (problems.length) { - core.setFailed( - `Scope guard: an allowlisted external contributor may only ADD entries for their own repos.\n - ` + - problems.join('\n - ') - ); - return; - } - console.log(`Scope guard passed: ${author} adds ${added.length} entr${added.length === 1 ? 'y' : 'ies'} (${added.join(', ') || 'none'}), all within allowed_sources.`); - - // --- helpers --- - function normalizeUrl(u) { - return String(u).trim().toLowerCase() - .replace(/^git\+/, '') - .replace(/^https?:\/\//, '') - .replace(/\.git$/, '') - .replace(/\/+$/, ''); // no trailing slash; matching adds the boundary - } - async function readPlugins(owner, repo, ref) { - try { - const { data } = await github.rest.repos.getContent({ owner, repo, ref, path: MARKETPLACE }); - const json = JSON.parse(Buffer.from(data.content, 'base64').toString('utf8')); - const map = {}; - for (const p of (json.plugins || [])) { if (p && p.name) map[p.name] = p; } - return map; - } catch (e) { - console.log(`readPlugins(${owner}/${repo}@${ref}): ${e.message}`); - return null; - } - } + console.log(`Scope guard passed: adds ${result.added.join(', ') || 'none'}, all within allowed sources.`);