Provision GitHub gatekeeper OAuth credentials in previews

.github/workflows/preview.yml

@@ -129,6 +129,17 @@ jobs:
           # a gateway in a *different* account (which sets this secret to `false`), and the google
           # provider, whose SDK cannot take the binding's fetch. Both fail the deploy.
           CF_AI_GATEWAY_USE_BINDING: ${{ secrets.CF_AI_GATEWAY_USE_BINDING || 'true' }}
+          # The GitHub gatekeeper's OAuth app, uploaded to *that worker's* Previews settings the
+          # same way, and for the same reason, as the backend's three above. Optional: with both
+          # unset the preview still deploys and only the GitHub connector is dead in it.
+          #
+          # `PREVIEW_`-prefixed because GitHub refuses to store a repository secret whose name
+          # begins with `GITHUB_`. The app behind these must be one registered for previews alone,
+          # with the callback URL `https://<PREVIEW_WORKERS_DEV_HOST>/gatekeeper/github/oauth` and
+          # wildcard matching left on — every preview is a different subdomain of that host, so
+          # nothing narrower validates, and anything on that subdomain can receive its codes.
+          PREVIEW_GITHUB_CLIENT_ID: ${{ secrets.PREVIEW_GITHUB_CLIENT_ID }}
+          PREVIEW_GITHUB_CLIENT_SECRET: ${{ secrets.PREVIEW_GITHUB_CLIENT_SECRET }}
           # Together these are the preview name, which is the first label of its hostname: a preview
           # reads as `pr123-my-branch-router.<subdomain>.workers.dev`. The number is what keeps two
           # branches that slugify alike from sharing one instance, and is how the nightly sweep

scripts/env-passthrough.test.ts

@@ -78,8 +78,9 @@ const EXPECTED: Record<string, ExpectedArea> = {
       "CF_ACCESS_AUD", "CF_ACCESS_ISS", "CF_AI_GATEWAY", "CF_AI_GATEWAY_ACCOUNT_ID",
       "CF_AI_GATEWAY_API_TOKEN", "CF_AI_GATEWAY_PROVIDERS", "CF_AI_GATEWAY_USE_BINDING",
       "CI_COMMIT_SHA", "CI_PIPELINE_IID", "CLOUDFLARE_ACCOUNT_ID", "CLOUDFLARE_API_TOKEN",
-      "GITHUB_REPOSITORY", "GITHUB_TOKEN", "PREVIEW_ADMINS", "PREVIEW_NAME",
-      "PREVIEW_PR_NUMBER", "PREVIEW_WORKERS_DEV_HOST", "PREVIEW_WRANGLER", "VITE_BACKEND_HOST",
+      "GITHUB_REPOSITORY", "GITHUB_TOKEN", "PREVIEW_ADMINS", "PREVIEW_GITHUB_CLIENT_ID",
+      "PREVIEW_GITHUB_CLIENT_SECRET", "PREVIEW_NAME", "PREVIEW_PR_NUMBER",
+      "PREVIEW_WORKERS_DEV_HOST", "PREVIEW_WRANGLER", "VITE_BACKEND_HOST",
     ],
   },
 };

scripts/preview/preview.ts

@@ -26,10 +26,11 @@
 // other seventeen set `preview_urls: false` and are reached over service bindings alone; the
 // deploy asserts that, since a URL appearing on one of them is a way around the router.
 //
-// The backend's secrets — its admins and the Cloudflare Access application that authenticates the
-// instance — are uploaded to the worker's Previews settings between tiers 1 and 2 and are never
-// written into a config, because Wrangler prints config values and this workflow's logs are public.
-// See uploadSecrets, and backendSecrets in staging-config.ts.
+// Secrets — the backend's admins and the Cloudflare Access application that authenticates the
+// instance, and each gatekeeper's OAuth app credentials where one is configured for previews — are
+// uploaded to the owning worker's Previews settings just before that worker's own tier, and are
+// never written into a config, because Wrangler prints config values and this workflow's logs are
+// public. See uploadSecrets, and backendSecrets / resolveGatekeeperSecrets in staging-config.ts.
 
 import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
@@ -45,6 +46,7 @@ import {
   generatePreviewConfigs,
   previewPullRequestNumber,
   previewUrlFor,
+  resolveGatekeeperSecrets,
   resolvePreviewName,
   writePreviewConfig,
   type StagingConfig,
@@ -322,9 +324,10 @@ async function deployBaselineWorker(
 
 /**
  * Give a worker its secrets — the backend's admins and Cloudflare Access pair (see
- * backendSecrets). None of them is in the generated config, because Wrangler prints the values it
- * finds there and this workflow's logs are public; `secret bulk` prints only names and `********`,
- * and the values arrive on stdin rather than in argv.
+ * backendSecrets), or a gatekeeper's OAuth app credentials (see resolveGatekeeperSecrets). None of
+ * them is in the generated config, because Wrangler prints the values it finds there and this
+ * workflow's logs are public; `secret bulk` prints only names and `********`, and the values arrive
+ * on stdin rather than in argv.
  *
  * `preview secret bulk` writes the *Worker's Previews settings*, which every preview of that worker
  * inherits, so one upload covers every preview and each run refreshes them. The plain `secret bulk`
@@ -346,32 +349,38 @@ async function uploadSecrets(
 }
 
 /**
- * Upload the backend's secrets, creating its baseline worker first if it does not exist yet.
+ * Upload one worker's secrets, creating its baseline worker first if it does not exist yet.
  *
- * This runs before the backend's own preview rather than relying on deployPreview's self-heal,
+ * This runs before that worker's own preview rather than relying on deployPreview's self-heal,
  * because a worker has no Previews settings to write to until it exists — and a preview created
- * before the settings existed would come up with no admins and, worse, no Access application, so it
- * would fall back to password signup on a public URL.
+ * before the settings existed would inherit none of them. For the backend that means no admins and,
+ * worse, no Access application, so it would fall back to password signup on a public URL; for a
+ * gatekeeper it means a connector that is live but throws on the first click.
+ *
+ * The settings belong to the *worker*, so every preview of it shares them and each run overwrites
+ * what the last one wrote. That is only sound because these values are the same for every preview —
+ * one set of deployment admins, one Access application, one OAuth app per gatekeeper — and a
+ * concurrent deploy of another pull request writes the identical bytes.
  */
-async function uploadBackendSecrets(
-  backend: DeployablePackage,
+async function uploadPreviewSecrets(
+  pkg: DeployablePackage,
   wranglerCommand: string,
   secrets: Record<string, string>,
 ): Promise<void> {
-  let result = await uploadSecrets(backend, wranglerCommand, secrets, { previews: true });
+  let result = await uploadSecrets(pkg, wranglerCommand, secrets, { previews: true });
   if (result.status !== 0 && isMissingWorkerError(`${result.stdout}\n${result.stderr}`)) {
-    await deployBaselineWorker(backend, wranglerCommand);
+    await deployBaselineWorker(pkg, wranglerCommand);
     // The baseline is briefly live without these, but it is only reachable through the *baseline*
     // router — which is deployed after it, in tier 3, on the same first run.
-    const baseline = await uploadSecrets(backend, wranglerCommand, secrets, { previews: false });
+    const baseline = await uploadSecrets(pkg, wranglerCommand, secrets, { previews: false });
     if (baseline.status !== 0) {
-      throw new Error(`wrangler secret bulk failed for baseline worker ${backend.name} with exit ` +
+      throw new Error(`wrangler secret bulk failed for baseline worker ${pkg.name} with exit ` +
           `code ${baseline.status}`);
     }
-    result = await uploadSecrets(backend, wranglerCommand, secrets, { previews: true });
+    result = await uploadSecrets(pkg, wranglerCommand, secrets, { previews: true });
   }
   if (result.status !== 0) {
-    throw new Error(`wrangler preview secret bulk failed for ${backend.name} with exit code ` +
+    throw new Error(`wrangler preview secret bulk failed for ${pkg.name} with exit code ` +
         `${result.status}`);
   }
 }
@@ -570,17 +579,22 @@ function tiers(packages: readonly DeployablePackage[]): {
 
 async function deploy({ dryRun }: { dryRun: boolean }): Promise<void> {
   // First, before a single config is written: a missing CF_ACCESS_AUD/CF_ACCESS_ISS has to fail
-  // here rather than after eighteen previews are live with whatever auth they defaulted to.
+  // here rather than after eighteen previews are live with whatever auth they defaulted to, and a
+  // gatekeeper's OAuth app split across a renamed secret rather than after that gatekeeper is live
+  // holding half of one.
   const secrets = backendSecrets();
+  const oauthApps = resolveGatekeeperSecrets();
   const { previewName, workersDevHost, baseUrl, packages } = generatePreviewConfigs();
   const { gatekeepers, backend, router } = tiers(packages);
 
   if (dryRun) {
     console.log(`\ndry-run plan for preview "${previewName}" at ${baseUrl}:`);
     console.log(`  tier 1 (${gatekeepers.length} gatekeepers, concurrently):`);
     for (const pkg of gatekeepers) {
+      const oauth = oauthApps.get(pkg.name);
       console.log(`    ${pkg.name} ` +
-          `(no hostname; served at ${baseUrl}/gatekeeper/${gatekeeperShortName(pkg.name)})`);
+          `(no hostname; served at ${baseUrl}/gatekeeper/${gatekeeperShortName(pkg.name)})` +
+          (oauth ? `, holding the ${Object.keys(oauth).join(", ")} secrets` : ""));
     }
     console.log(`  tier 2: ${backend.name} (no hostname; served at ` +
         `${baseUrl}/api), bound to the tier 1 previews, holding the ` +
@@ -597,6 +611,10 @@ async function deploy({ dryRun }: { dryRun: boolean }): Promise<void> {
     // Keyed by worker name, because that is what a service binding names.
     const gatekeeperPreviews = await mapWithConcurrency(gatekeepers, GATEKEEPER_CONCURRENCY,
         async (pkg) => {
+          // Before this gatekeeper's preview, not after, and for the same reason the backend's go
+          // before its own: a preview inherits the Previews settings that exist when it is created.
+          const oauth = oauthApps.get(pkg.name);
+          if (oauth) await uploadPreviewSecrets(pkg, wrangler.command, oauth);
           const preview = await deployPreview(pkg, previewName, wrangler.command);
           assertNoPreviewUrl(pkg, preview.url);
           return [pkg.name, preview.id];
@@ -607,7 +625,7 @@ async function deploy({ dryRun }: { dryRun: boolean }): Promise<void> {
     patchPreviewServiceBindings(router, gatekeeperIds);
     // Before the backend's preview, not after: a preview inherits the Previews settings that exist
     // when it is created.
-    await uploadBackendSecrets(backend, wrangler.command, secrets);
+    await uploadPreviewSecrets(backend, wrangler.command, secrets);
     const backendPreview = await deployPreview(backend, previewName, wrangler.command);
     assertNoPreviewUrl(backend, backendPreview.url);
 

scripts/preview/staging-config.test.ts

@@ -20,6 +20,7 @@ import {
   previewPullRequestNumber,
   resolveAccess,
   resolveAiGateway,
+  resolveGatekeeperSecrets,
   resolvePreviewName,
   resolveTarget,
   routerPreviewUrl,
@@ -50,6 +51,15 @@ const SECRETS = backendSecrets({
   access: ACCESS,
   aiGateway: resolveAiGateway(AI_GATEWAY),
 });
+const GITHUB_OAUTH = { githubClientId: "Iv1.preview0000000f", githubClientSecret: "9".repeat(40) };
+const GATEKEEPER_SECRETS = resolveGatekeeperSecrets(GITHUB_OAUTH);
+
+/** One gatekeeper's OAuth secrets, so a gatekeeper losing its entry fails here rather than silently. */
+function oauthFor(pkgName: string): Record<string, string> {
+  const secrets = GATEKEEPER_SECRETS.get(pkgName);
+  assert.ok(secrets, `resolveGatekeeperSecrets returned nothing for ${pkgName}`);
+  return secrets;
+}
 
 /**
  * Every resource binding declared anywhere in the generated configs, as `[where, resource]`.
@@ -334,8 +344,11 @@ test("no generated config declares a secret's variable", () => {
       ["baseline", config.vars],
       ["preview", config.previews?.vars],
     ];
+    // Every gatekeeper's pair is CLIENT_ID/CLIENT_SECRET, so one gatekeeper's names cover all of
+    // them — and no worker in the instance has any business declaring those as plain-text vars.
+    const secretNames = [...Object.keys(SECRETS), ...Object.keys(oauthFor("gatekeeper-github"))];
     for (const [half, vars] of halves) {
-      for (const key of Object.keys(SECRETS)) {
+      for (const key of secretNames) {
         assert.ok(!Object.hasOwn(vars ?? {}, key),
             `${name} ${half} vars declare ${key}; upload it as a secret from preview.ts instead`);
       }
@@ -357,6 +370,10 @@ test("no generated config carries a secret's value anywhere", () => {
   const sensitive = [
     ...Object.entries(SECRETS).filter(([key]) => !generic.has(key)),
     ["an admin's email", ADMIN],
+    // A gatekeeper's OAuth app travels the same way, and a client secret in a public log is an app
+    // anyone can impersonate until it is rotated.
+    ...Object.entries(oauthFor("gatekeeper-github"))
+        .map(([key, value]) => [`gatekeeper-github's ${key}`, value]),
   ];
   for (const [name, config] of buildAll().configs) {
     const serialized = JSON.stringify(config);
@@ -472,3 +489,36 @@ test("the Access application has no default, in either direction", () => {
   assert.throws(() => resolveAccess({ aud: ACCESS.aud, iss: "" }), /CF_ACCESS_ISS must be set/);
   assert.throws(() => resolveAccess({ aud: "", iss: ACCESS.iss }), /CF_ACCESS_AUD must be set/);
 });
+
+test("a gatekeeper's OAuth app is optional, but never half of one", () => {
+  // Unlike the Access pair, an unconfigured gatekeeper is a working preview with one dead
+  // connector, so the empty case is a warning rather than a throw. Half of one is neither: the
+  // gatekeeper deploys, and every attempt to connect it throws "not configured" instead.
+  assert.deepEqual(oauthFor("gatekeeper-github"), {
+    CLIENT_ID: GITHUB_OAUTH.githubClientId,
+    CLIENT_SECRET: GITHUB_OAUTH.githubClientSecret,
+  });
+  assert.equal(resolveGatekeeperSecrets({ githubClientId: "", githubClientSecret: "" }).size, 0);
+  for (const half of [
+    { githubClientId: GITHUB_OAUTH.githubClientId, githubClientSecret: "" },
+    { githubClientId: "", githubClientSecret: GITHUB_OAUTH.githubClientSecret },
+  ]) {
+    assert.throws(() => resolveGatekeeperSecrets(half),
+        /PREVIEW_GITHUB_CLIENT_ID and PREVIEW_GITHUB_CLIENT_SECRET must be set together/);
+  }
+});
+
+test("every gatekeeper handed an OAuth app is a package that exists, and reads that pair", () => {
+  // The map is keyed by package name, and preview.ts looks each gatekeeper up in it by that name —
+  // so a renamed or deleted package makes the upload silently stop happening rather than fail.
+  const names = new Set(readDeployablePackages(PACKAGES_DIR).map((pkg) => pkg.name));
+  for (const [pkgName, secrets] of GATEKEEPER_SECRETS) {
+    assert.ok(names.has(pkgName),
+        `resolveGatekeeperSecrets names ${pkgName}, which is not a deployable package`);
+    assert.ok(isGatekeeperPackage(pkgName),
+        `resolveGatekeeperSecrets names ${pkgName}, which is not a gatekeeper`);
+    // The variable names the worker reads (`env.CLIENT_ID` / `env.CLIENT_SECRET`), which is also
+    // what the deploy wizard's DEFAULT_CRED_INPUTS asks a real instance for.
+    assert.deepEqual(Object.keys(secrets).toSorted(), ["CLIENT_ID", "CLIENT_SECRET"]);
+  }
+});

scripts/preview/staging-config.ts

@@ -19,8 +19,10 @@
 //     namespace / R2 bucket per preview, and where service bindings get patched to point at
 //     sibling previews rather than the baselines.
 //
-// Gatekeeper OAuth app credentials (CLIENT_ID/CLIENT_SECRET) are deliberately absent: previews
-// exercise routing, auth and the agent, not third-party connector flows.
+// Gatekeeper OAuth app credentials (CLIENT_ID/CLIENT_SECRET) are absent from the generated configs
+// for the same reason the backend's secrets are — Wrangler prints what it finds in one, and this
+// workflow's logs are public. Where an OAuth app is configured for previews, preview.ts uploads the
+// pair to that gatekeeper's Previews settings instead; see resolveGatekeeperSecrets.
 
 import { writeFileSync } from "node:fs";
 import { execFileSync } from "node:child_process";
@@ -610,6 +612,71 @@ export function resolveAiGateway({
   };
 }
 
+/**
+ * Record one gatekeeper's OAuth app credentials under the variable names the worker reads them as.
+ *
+ * The pair moves together: a gatekeeper holding one half is not half-connectable, it throws "The
+ * GitHub gatekeeper is not configured." on the first click, so a typo in one secret's name fails
+ * the deploy rather than surfacing in a preview nobody is reading the logs of.
+ */
+function addOAuthApp(
+  into: Map<string, Record<string, string>>,
+  pkgName: string,
+  envPrefix: string,
+  clientId: string | undefined,
+  clientSecret: string | undefined,
+): void {
+  if (clientId && clientSecret) {
+    into.set(pkgName, { CLIENT_ID: clientId, CLIENT_SECRET: clientSecret });
+    return;
+  }
+  if (clientId || clientSecret) {
+    throw new Error(`${envPrefix}_CLIENT_ID and ${envPrefix}_CLIENT_SECRET must be set together: ` +
+        `they are one OAuth app, and ${pkgName} refuses to start a flow with half of it`);
+  }
+  console.warn(`${envPrefix}_CLIENT_ID is unset: ${pkgName} is deployed unconfigured, and ` +
+      "connecting it in this preview will fail.");
+}
+
+/**
+ * The OAuth app credentials a gatekeeper's previews are given, as `package name -> secrets`.
+ *
+ * Optional, per gatekeeper: an unconfigured one still deploys, and only that connector is dead in
+ * the preview — unlike {@link resolveAccess}, whose absence changes how the whole instance
+ * authenticates. Most gatekeepers have no preview OAuth app at all, which is why previews are for
+ * routing, auth and the agent first and third-party flows only where someone registered one.
+ *
+ * Registering one is not just a pair of secrets. A preview's redirect URI is
+ * `https://<preview>-router.<workers.dev subdomain>/gatekeeper/<short>/oauth`, and the host changes
+ * with every pull request — so the app's callback URL has to be
+ * `https://<workers.dev subdomain>/gatekeeper/<short>/oauth` with GitHub's wildcard matching left
+ * on, which is what lets each preview's subdomain validate against it. That also means any worker
+ * on that subdomain can receive an authorization code for this app, so the app it belongs to should
+ * be a throwaway registered for previews and never the one a real deployment uses.
+ *
+ * `PREVIEW_`-prefixed rather than the `GITHUB_CLIENT_ID` run-dev-server.ts reads from a developer's
+ * shell: GitHub refuses to store a repository secret whose name begins with `GITHUB_`, and the
+ * distinct name keeps the preview app and a maintainer's local app from being confused for one
+ * another.
+ *
+ * Adding a second gatekeeper is a parameter pair here, one `addOAuthApp` line, the matching pair in
+ * .github/workflows/preview.yml, and an entry in env-passthrough.test.ts.
+ *
+ * The environment is read in the parameter defaults rather than the body so that
+ * env-passthrough.test.ts, whose discovery is textual, can see every name.
+ */
+export function resolveGatekeeperSecrets({
+  githubClientId = process.env.PREVIEW_GITHUB_CLIENT_ID,
+  githubClientSecret = process.env.PREVIEW_GITHUB_CLIENT_SECRET,
+}: {
+  githubClientId?: string;
+  githubClientSecret?: string;
+} = {}): Map<string, Record<string, string>> {
+  const configured = new Map<string, Record<string, string>>();
+  addOAuthApp(configured, "gatekeeper-github", "PREVIEW_GITHUB", githubClientId, githubClientSecret);
+  return configured;
+}
+
 /**
  * The backend's secrets, keyed by the variable name it reads them as.
  *