fix: make vouched_ci approvals work from the web dyno and verify ids match logins

README.md

@@ -283,7 +283,9 @@ To be vouched, you must sign every commit you push with a key on your GitHub acc
 through the GitHub web UI are signed by GitHub, not by you, and are not accepted), push your own
 commits yourself (nobody else's, including "apply suggestion" or cherry-picked patches from other
 people), and rebase rather than merge branches other than the pull request's base into your branch.
-Your numeric user id is available at `https://api.github.com/users/<login>`.
+Your numeric user id is available at `https://api.github.com/users/<login>`. The permissions run
+(including the dry run) checks that each `vouched_ci` entry's `id` still resolves to its `login` on
+GitHub and fails otherwise, so a mistyped id or a renamed account is caught before it is merged.
 
 Vouched users can run arbitrary code in your fork pull request CI, treat the list like write access
 to the repository's Actions runners. Using this feature requires the GitHub App to have the

src/index.ts

@@ -10,7 +10,7 @@ import {
   createNodeMiddleware,
   EmitterWebhookEvent,
 } from '@octokit/webhooks';
-import { isMainRepo, isSecurityAdvisoryRepo, hook, IS_DRY_RUN } from './helpers.js';
+import { isMainRepo, isSecurityAdvisoryRepo, hook } from './helpers.js';
 import {
   MessageBuilder,
   createMessageBlock,
@@ -599,10 +599,6 @@ webhooks.on(
       return;
     }
 
-    if (IS_DRY_RUN) {
-      ctx.log(tag, 'would approve run', run.id, `(${run.name})`);
-      return;
-    }
     try {
       await octokit.actions.approveWorkflowRun({ ...coords, run_id: run.id });
     } catch (err) {

src/octokit.ts

@@ -61,7 +61,11 @@ export async function getVouchedCIOctokit(org: string): Promise<Octokit> {
     const creds = appCredentialsFromString(SHERIFF_GITHUB_APP_CREDS!);
     const authOpts = await getAuthOptionsForOrg(org, creds, {
       permissions: {
-        actions: IS_DRY_RUN ? 'read' : 'write',
+        // The webhook server is started without `--do-it-for-real-this-time`
+        // (that flag only exists for the permissions cron), so `IS_DRY_RUN` is
+        // always true here and must not narrow this token: approving runs is
+        // the whole point of the vouched_ci handler.
+        actions: 'write',
         contents: 'read',
         metadata: 'read',
         pull_requests: 'read',

src/permissions/run.ts

@@ -41,6 +41,7 @@ import {
 } from './pr-creation-cap.js';
 import { components } from '@octokit/openapi-types';
 import { isDeepStrictEqual } from 'util';
+import { verifyVouchedUsers } from '../vouched-ci.js';
 
 const queue = _queue as unknown as typeof _queue.default;
 
@@ -645,6 +646,35 @@ async function main() {
 
     const octokit = await getOctokit(config.organization);
 
+    // Every vouched_ci id must still resolve to its configured login. The
+    // webhook handler only vouches for a user whose id and login both match, so
+    // a stale or mistyped entry would silently never approve anything (or, if
+    // the id was meant for someone else, point at the wrong account). This
+    // needs GitHub, so it runs here rather than in validateConfigFast, and it
+    // fails the dry run too so the .permissions CI check goes red.
+    if (config.vouched_ci?.length) {
+      const problems = await verifyVouchedUsers(config.vouched_ci, async (id) => {
+        try {
+          return (await octokit.request('GET /user/{account_id}', { account_id: id })).data;
+        } catch (err) {
+          if ((err as { status?: unknown })?.status === 404) return null;
+          throw err;
+        }
+      });
+      if (problems.length) {
+        throw new Error(
+          `Invalid vouched_ci entries for "${config.organization}":\n${problems
+            .map((p) => `  - ${p}`)
+            .join('\n')}`,
+        );
+      }
+      console.info(
+        chalk.green('Verified'),
+        chalk.cyan(`${config.vouched_ci.length}`),
+        'vouched_ci user(s) resolve to their configured logins',
+      );
+    }
+
     // Check if the webhook is configured
     const HOOK_URL = SHERIFF_HOST_URL;
     if (HOOK_URL) {

src/vouched-ci.test.ts

@@ -6,6 +6,7 @@ import {
   findVouchedUser,
   isApprovableRun,
   matchPullRequestForRun,
+  verifyVouchedUsers,
   MAX_PULL_REQUEST_COMMITS,
   VouchedCICommit,
   VouchedCIInput,
@@ -346,3 +347,49 @@ describe('matchPullRequestForRun', () => {
     assert.equal(matchPullRequestForRun([pr(1)], run({ head_repository: null })).pullRequest, null);
   });
 });
+
+describe('verifyVouchedUsers', () => {
+  const lookup =
+    (users: Record<number, string>) =>
+    async (id: number): Promise<{ login: string } | null> =>
+      id in users ? { login: users[id] } : null;
+
+  it('accepts entries whose id resolves to the configured login', async () => {
+    const problems = await verifyVouchedUsers([alice, bob], lookup({ 1001: 'alice', 2002: 'bob' }));
+    assert.deepEqual(problems, []);
+  });
+
+  it('compares logins case-insensitively, like GitHub', async () => {
+    const problems = await verifyVouchedUsers([alice], lookup({ 1001: 'Alice' }));
+    assert.deepEqual(problems, []);
+  });
+
+  it('reports an entry whose id belongs to a different login', async () => {
+    const problems = await verifyVouchedUsers(
+      [alice, bob],
+      lookup({ 1001: 'mallory', 2002: 'bob' }),
+    );
+    assert.equal(problems.length, 1);
+    assert.match(problems[0], /"alice" \(1001\): id 1001 belongs to "mallory", not "alice"/);
+  });
+
+  it('reports an entry whose id does not exist', async () => {
+    const problems = await verifyVouchedUsers([alice], lookup({}));
+    assert.equal(problems.length, 1);
+    assert.match(problems[0], /"alice" \(1001\): no GitHub user has id 1001/);
+  });
+
+  it('reports every bad entry, not just the first', async () => {
+    const problems = await verifyVouchedUsers([alice, bob], lookup({ 2002: 'eve' }));
+    assert.equal(problems.length, 2);
+  });
+
+  it('propagates lookup failures other than a missing user', async () => {
+    await assert.rejects(
+      verifyVouchedUsers([alice], async () => {
+        throw new Error('boom');
+      }),
+      /boom/,
+    );
+  });
+});

src/vouched-ci.ts

@@ -213,3 +213,37 @@ export function matchPullRequestForRun<T extends VouchedCIPullRequest>(
       .join(', ')}) share head ${run.head_sha}`,
   };
 }
+
+/**
+ * Resolves a numeric GitHub user id to the account's current login, or `null`
+ * when no account has that id (`GET /user/{account_id}` answers 404).
+ */
+export type VouchedUserLookup = (id: number) => Promise<{ login: string } | null>;
+
+/**
+ * Confirms that every `vouched_ci` entry's id really belongs to its login by
+ * resolving each id through GitHub (`GET /user/{account_id}`). The handler only
+ * ever vouches for a user whose id *and* login match the entry, so an entry
+ * that fails this check is dead config at best and a typo pointing at an
+ * unrelated account at worst; either way the permissions run should refuse it.
+ * Returns one message per bad entry, empty when every entry checks out.
+ */
+export async function verifyVouchedUsers(
+  vouched: VouchedUser[],
+  fetchUserById: VouchedUserLookup,
+): Promise<string[]> {
+  const problems: string[] = [];
+  for (const user of vouched) {
+    const resolved = await fetchUserById(user.id);
+    if (!resolved) {
+      problems.push(
+        `vouched_ci user "${user.login}" (${user.id}): no GitHub user has id ${user.id}`,
+      );
+    } else if (resolved.login.toLowerCase() !== user.login.toLowerCase()) {
+      problems.push(
+        `vouched_ci user "${user.login}" (${user.id}): id ${user.id} belongs to "${resolved.login}", not "${user.login}"`,
+      );
+    }
+  }
+  return problems;
+}