Explorar el Código

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) <noreply@anthropic.com>
Bryan Thompson hace 3 meses
padre
commit
2534b1b6ab

+ 0 - 28
.github/external-contributors.json

@@ -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."
-    }
-  ]
-}

+ 20 - 0
.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/"
+  ]
+}

+ 109 - 0
.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 };

+ 18 - 20
.github/workflows/close-external-prs.yml

@@ -7,20 +7,20 @@ on:
 permissions:
 permissions:
   pull-requests: write
   pull-requests: write
   issues: write
   issues: write
+  contents: read
 
 
 jobs:
 jobs:
   check-membership:
   check-membership:
     if: vars.DISABLE_EXTERNAL_PR_CHECK != 'true'
     if: vars.DISABLE_EXTERNAL_PR_CHECK != 'true'
     runs-on: ubuntu-latest
     runs-on: ubuntu-latest
     steps:
     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
       - 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
         uses: actions/github-script@v7
         with:
         with:
           script: |
           script: |
-            const fs = require('fs');
             const author = context.payload.pull_request.user.login;
             const author = context.payload.pull_request.user.login;
 
 
             const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
             const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
@@ -34,24 +34,22 @@ jobs:
               return;
               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({
             await github.rest.issues.createComment({
               owner: context.repo.owner,
               owner: context.repo.owner,

+ 21 - 114
.github/workflows/external-pr-scope-guard.yml

@@ -1,16 +1,17 @@
 name: External PR Scope Guard
 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
 # 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:
 on:
   pull_request_target:
   pull_request_target:
@@ -24,125 +25,31 @@ jobs:
   scope-guard:
   scope-guard:
     runs-on: ubuntu-latest
     runs-on: ubuntu-latest
     steps:
     steps:
-      - uses: actions/checkout@v4   # base repo (trusted) — for the allowlist
+      - uses: actions/checkout@v4   # base repo (trusted)
       - uses: actions/github-script@v7
       - uses: actions/github-script@v7
         with:
         with:
           script: |
           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({
             const { data: perm } = await github.rest.repos.getCollaboratorPermissionLevel({
               owner: context.repo.owner, repo: context.repo.repo, username: author,
               owner: context.repo.owner, repo: context.repo.repo, username: author,
             });
             });
             if (['admin', 'write'].includes(perm.permission)) {
             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;
               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) {
-              core.setFailed(
-                `As an allowlisted external contributor, ${author} may only modify ${MARKETPLACE}. ` +
-                `This PR also changes: ${offlimits.join(', ')}.`
-              );
-              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) {
+            if (!result.ok) {
               core.setFailed(
               core.setFailed(
-                `Scope guard: an allowlisted external contributor may only ADD entries for their own repos.\n - ` +
-                problems.join('\n - ')
+                `Scope guard: a non-member PR may only ADD marketplace.json entries for an allowlisted source org.\n - ` +
+                result.problems.join('\n - ')
               );
               );
               return;
               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.`);