fix(setup): warn when the machine user client secret is set outside its environment

.changeset/major-parts-remain.md

@@ -0,0 +1,5 @@
+---
+"@tailor-platform/sdk-plugin-setup": patch
+---
+
+`tailor setup ci env --check` now warns when `TAILOR_PLATFORM_MACHINE_USER_CLIENT_SECRET` is set on the repository or the organization, where any workflow on any branch can read it and the GitHub Environment's protection rules do not guard it, and prints the `gh` commands that move it onto the environment. The check still passes. With `--json`, each entry's `exposedIn` lists where it was found outside its environment.

packages/sdk-plugin-setup/src/commands.ts

@@ -56,7 +56,7 @@ const envCommand = defineAppCommand({
     }),
     check: arg(z.boolean().default(false), {
       description:
-        "Check with gh which of them are registered in GitHub, and print gh commands for the missing ones; fails when a required one is missing",
+        "Check with gh which of them are registered in GitHub, and print gh commands for the missing ones and for a machine user client secret to move onto its environment; fails when a required one is missing",
     }),
   }),
   run: (args) => {

packages/sdk-plugin-setup/src/env-check.test.ts

@@ -11,6 +11,7 @@ import type { GitRunner } from "./git";
 const CLIENT_ID = "TAILOR_PLATFORM_MACHINE_USER_CLIENT_ID";
 const CLIENT_SECRET = "TAILOR_PLATFORM_MACHINE_USER_CLIENT_SECRET";
 const WORKSPACE_ID = "TAILOR_PLATFORM_WORKSPACE_ID";
+const SLACK_BOT_TOKEN = "TAILOR_SLACK_BOT_TOKEN";
 
 type Entries = { secrets?: readonly string[]; variables?: readonly string[] };
 
@@ -23,6 +24,9 @@ type FakeGitHub = Entries & {
   responses?: Record<string, GhOutput | null>;
 };
 
+/** Repository state plus the secrets of the `stg` environment. */
+type ClientSecretPlacement = FakeGitHub & { environmentSecrets?: readonly string[] };
+
 const ok = (stdout: string): GhOutput => ({ status: 0, stdout, stderr: "" });
 const httpError = (code: number, message: string): GhOutput => ({
   status: 1,
@@ -184,28 +188,145 @@ describe("checkEnv", () => {
     checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh });
 
     expect(output.stderr()).toMatch(/TAILOR_SLACK_BOT_TOKEN \(secret\): optional, not set/);
-    expect(output.stderr()).toMatch(/All required secrets and variables are registered/);
+    expect(output.stderr().split("\n")).toContain(`  ✔ ${CLIENT_SECRET} (secret)`);
+    expect(output.stderr()).not.toContain("⚠");
+    expect(output.stdout()).not.toContain("gh secret delete");
+    expect(output.stderr().split("\n").at(-1)).toBe(
+      "All required secrets and variables are registered.",
+    );
   });
 
   test.each([
-    ["repository", { secrets: [CLIENT_SECRET] }],
-    ["organization", { organization: { secrets: [CLIENT_SECRET] } }],
+    ["repository", { secrets: [SLACK_BOT_TOKEN] }],
+    ["organization", { organization: { secrets: [SLACK_BOT_TOKEN] } }],
   ] as const)(
     "counts a secret set on the %s as registered, since the workflow reads it from there",
     (scope, github) => {
       using tmp = setup();
       using output = captureOutput();
       const gh = fakeGh({
         ...github,
-        environments: { stg: { variables: [CLIENT_ID, WORKSPACE_ID] } },
+        environments: { stg: { secrets: [CLIENT_SECRET], variables: [CLIENT_ID, WORKSPACE_ID] } },
+      });
+
+      checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh });
+
+      expect(output.stderr()).toContain(`✔ ${SLACK_BOT_TOKEN} (secret, set on the ${scope})`);
+    },
+  );
+
+  const UNGUARDED = "so the environment's protection rules do not guard it";
+  const STG_SUMMARY =
+    "All required secrets and variables are registered, but any workflow on any branch can read " +
+    `${CLIENT_SECRET} outside the protection rules of environment "stg".`;
+  const SET = `gh secret set ${CLIENT_SECRET} --env=stg --repo=acme/app`;
+  const DELETE = `# gh secret delete ${CLIENT_SECRET} --repo=acme/app`;
+  const HOW_TO_REUSE = "#   How to get: the value set on the";
+  const howToReuse = (scope: string) =>
+    `${HOW_TO_REUSE} ${scope} (GitHub does not show secret values, so use the copy you stored)`;
+
+  test.each([
+    [
+      "only on the repository",
+      { secrets: [CLIENT_SECRET] },
+      `(secret, set on the repository): any workflow on any branch can read it from the repository, ${UNGUARDED}; ` +
+        "set it on the environment and delete it from the repository",
+      [howToReuse("repository"), SET, DELETE],
+    ],
+    [
+      "only on the organization",
+      { organization: { secrets: [CLIENT_SECRET] } },
+      `(secret, set on the organization): any workflow on any branch can read it from the organization, ${UNGUARDED}; ` +
+        "set it on the environment and have an organization owner stop sharing it with this repository",
+      [howToReuse("organization"), SET],
+    ],
+    [
+      "on the repository and the organization",
+      { secrets: [CLIENT_SECRET], organization: { secrets: [CLIENT_SECRET] } },
+      "(secret, set on the repository): any workflow on any branch can read it from the repository and the " +
+        `organization, ${UNGUARDED}; set it on the environment, delete it from the repository, and have an ` +
+        "organization owner stop sharing it with this repository",
+      [howToReuse("repository"), SET, DELETE],
+    ],
+    [
+      "on the environment and also on the repository",
+      { secrets: [CLIENT_SECRET], environmentSecrets: [CLIENT_SECRET] },
+      "(secret): set on the environment and also on the repository, where any workflow on any branch can " +
+        `read it, ${UNGUARDED}; delete it from the repository`,
+      [DELETE],
+    ],
+    [
+      "on the environment and also on the organization",
+      { organization: { secrets: [CLIENT_SECRET] }, environmentSecrets: [CLIENT_SECRET] },
+      "(secret): set on the environment and also on the organization, where any workflow on any branch can " +
+        `read it, ${UNGUARDED}; have an organization owner stop sharing it with this repository`,
+      [],
+    ],
+  ] as const)(
+    "warns, without failing, about the client secret set %s, and prints the gh commands that fix it",
+    (_, { environmentSecrets = [], ...github }: ClientSecretPlacement, line, commands) => {
+      using tmp = setup();
+      using output = captureOutput();
+      const gh = fakeGh({
+        ...github,
+        environments: {
+          stg: { secrets: environmentSecrets, variables: [CLIENT_ID, WORKSPACE_ID] },
+        },
       });
 
       checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh });
 
-      expect(output.stderr()).toContain(`✔ ${CLIENT_SECRET} (secret, set on the ${scope})`);
+      const stderr = output.stderr().split("\n");
+      expect(stderr).toContain(`  ⚠ ${CLIENT_SECRET} ${line}`);
+      expect(output.stderr()).not.toContain(`✔ ${CLIENT_SECRET}`);
+      expect(stderr.at(-1)).toBe(STG_SUMMARY);
+
+      const stdout = output.stdout().split("\n");
+      expect(
+        stdout.filter((text) => text === SET || text === DELETE || text.startsWith(HOW_TO_REUSE)),
+      ).toEqual(commands);
     },
   );
 
+  test("moves a repository client secret to every environment reading it, deleting it once", () => {
+    using tmp = setup(target("branch", "my-app", "stg"), target("tag", "my-app", "production"));
+    using output = captureOutput();
+    const gh = fakeGh({
+      secrets: [CLIENT_SECRET],
+      environments: {
+        stg: { variables: [CLIENT_ID, WORKSPACE_ID] },
+        production: { variables: [CLIENT_ID, WORKSPACE_ID] },
+      },
+    });
+
+    checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh });
+
+    const lines = output.stdout().split("\n");
+    expect(lines).toContain(`gh secret set ${CLIENT_SECRET} --env=stg --repo=acme/app`);
+    expect(lines).toContain(`gh secret set ${CLIENT_SECRET} --env=production --repo=acme/app`);
+    expect(lines.filter((line) => line.includes("gh secret delete"))).toEqual([
+      `# gh secret delete ${CLIENT_SECRET} --repo=acme/app`,
+    ]);
+    expect(output.stderr().split("\n").at(-1)).toBe(
+      "All required secrets and variables are registered, but any workflow on any branch can read " +
+        `${CLIENT_SECRET} outside the protection rules of environments "stg" and "production".`,
+    );
+  });
+
+  test("warns about a repository client secret for an environment that does not exist yet, creating it in the commands", () => {
+    using tmp = setup();
+    using output = captureOutput();
+    const gh = fakeGh({ secrets: [CLIENT_SECRET], variables: [CLIENT_ID, WORKSPACE_ID] });
+
+    checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh });
+
+    expect(output.stderr()).toContain(`⚠ ${CLIENT_SECRET} (secret, set on the repository)`);
+    expect(output.stdout()).toContain('gh api -X PUT "repos/acme/app/environments/stg" --silent');
+    expect(output.stdout().split("\n")).toContain(
+      `gh secret set ${CLIENT_SECRET} --env=stg --repo=acme/app`,
+    );
+  });
+
   test("skips organization-level entries for a repository a user owns", () => {
     using tmp = setup();
     using _output = captureOutput();
@@ -387,6 +508,7 @@ describe("checkEnv", () => {
     using output = captureOutput();
     using _json = vi.spyOn(logger, "jsonMode", "get").mockReturnValue(true);
     const gh = fakeGh({
+      secrets: [CLIENT_SECRET],
       environments: { stg: { secrets: [CLIENT_SECRET], variables: [CLIENT_ID] } },
     });
 
@@ -409,6 +531,12 @@ describe("checkEnv", () => {
               registered: true,
               foundIn: "environment",
               otherTypeIn: null,
+              exposedIn: [],
+            }),
+            expect.objectContaining({
+              name: CLIENT_SECRET,
+              foundIn: "environment",
+              exposedIn: ["repository"],
             }),
             expect.objectContaining({ name: WORKSPACE_ID, registered: false, foundIn: null }),
           ]),

packages/sdk-plugin-setup/src/env-check.ts

@@ -1,6 +1,7 @@
 import { logger } from "@tailor-platform/sdk/cli";
 import {
   describeGroup,
+  ENVIRONMENT_PREFERRED_SECRETS,
   type EnvRequirement,
   type EnvironmentRequirements,
   loadEnvironmentRequirements,
@@ -21,13 +22,17 @@ type Scope = "environment" | "repository" | "organization";
 
 type ScopedNames = { scope: Scope; names: Names };
 
+type SharedScope = Exclude<Scope, "environment">;
+
 type RequirementCheck = EnvRequirement & {
   /** Whether the workflow finds a value, counting the deprecated secret fallback. */
   registered: boolean;
   /** Scope GitHub resolves the entry from; null when no scope has it. */
   foundIn: Scope | null;
   /** Scope holding an entry of the same name but the other type; null when none does. */
   otherTypeIn: Scope | null;
+  /** Scopes outside the environment holding a secret that belongs on it; empty for other entries. */
+  exposedIn: SharedScope[];
 };
 
 type EnvironmentCheck = Omit<EnvironmentRequirements, "requirements"> & {
@@ -44,11 +49,21 @@ function checkRequirement(requirement: EnvRequirement, scopes: ScopedNames[]): R
   const otherTypeIn = foundIn === null ? scopeHolding(OTHER_TYPE[requirement.type]) : null;
   const fallback =
     requirement.type === "variable" && SECRET_FALLBACK_VARIABLES.has(requirement.name);
+  const checksEnvironment = scopes.some(({ scope }) => scope === "environment");
+  const belongsOnEnvironment =
+    checksEnvironment &&
+    requirement.type === "secret" &&
+    ENVIRONMENT_PREFERRED_SECRETS.has(requirement.name);
   return {
     ...requirement,
     registered: foundIn !== null || (fallback && otherTypeIn !== null),
     foundIn,
     otherTypeIn,
+    exposedIn: belongsOnEnvironment
+      ? scopes.flatMap(({ scope, names }) =>
+          scope !== "environment" && names.secret.has(requirement.name) ? [scope] : [],
+        )
+      : [],
   };
 }
 
@@ -244,7 +259,26 @@ function report(checks: EnvironmentCheck[], fullName: string, repository: Reposi
   }
   printReport(checks, fullName, repository);
   if (missing > 0) throw new Error(`${summary} Set them with the gh commands printed above.`);
-  logger.success("All required secrets and variables are registered.");
+  const exposedGroups = checks.filter(({ requirements }) =>
+    requirements.some((check) => check.exposedIn.length > 0),
+  );
+  if (exposedGroups.length === 0) {
+    logger.success("All required secrets and variables are registered.");
+    return;
+  }
+  const secrets = new Set(
+    exposedGroups.flatMap(({ requirements }) =>
+      requirements.filter((check) => check.exposedIn.length > 0).map((check) => check.name),
+    ),
+  );
+  const environments = exposedGroups.flatMap(({ environment }) =>
+    environment === null ? [] : [`"${environment}"`],
+  );
+  logger.warn(
+    "All required secrets and variables are registered, but any workflow on any branch can read " +
+      `${andList.format([...secrets])} outside the protection rules of ` +
+      `${environments.length === 1 ? "environment" : "environments"} ${andList.format(environments)}.`,
+  );
 }
 
 function printReport(checks: EnvironmentCheck[], fullName: string, repository: Repository): void {
@@ -257,13 +291,31 @@ function printReport(checks: EnvironmentCheck[], fullName: string, repository: R
       logger.log(`  ${statusLine(check)}`);
     }
   }
-  const unset = checks
+  const toSet = checks
     .map((check) => ({
       ...check,
-      requirements: check.requirements.filter((r) => r.foundIn === null),
+      requirements: check.requirements.flatMap((r): EnvRequirement[] => {
+        if (r.foundIn === null) return [r];
+        if (r.foundIn === "environment" || r.exposedIn.length === 0) return [];
+        return [
+          {
+            ...r,
+            howTo: `the value set on the ${r.foundIn} (GitHub does not show secret values, so use the copy you stored)`,
+          },
+        ];
+      }),
     }))
     .filter((check) => check.requirements.length > 0);
-  if (unset.length > 0) logger.out(renderGhCommands(unset, repository));
+  const toDelete = [
+    ...new Set(
+      checks.flatMap(({ requirements }) =>
+        requirements.filter((r) => r.exposedIn.includes("repository")).map((r) => r.name),
+      ),
+    ),
+  ];
+  if (toSet.length > 0 || toDelete.length > 0) {
+    logger.out(renderGhCommands(toSet, repository, toDelete));
+  }
 }
 
 function statusLine(check: RequirementCheck): string {
@@ -275,9 +327,30 @@ function statusLine(check: RequirementCheck): string {
   if (check.registered && check.foundIn === null) {
     return `⚠ ${label}: set as a secret, a deprecated fallback; set it as a variable and delete the secret`;
   }
+  if (check.exposedIn.length > 0) return `⚠ ${label}: ${exposureWarning(check)}`;
   if (check.registered) return `✔ ${label}`;
   const status = check.required ? `✖ ${label}: missing` : `- ${label}: optional, not set`;
   if (check.otherTypeIn === null) return status;
   const other = OTHER_TYPE[check.type];
   return `${status}; a ${other} of this name is set on the ${check.otherTypeIn}, but the workflow reads a ${check.type}`;
 }
+
+const andList = new Intl.ListFormat("en", { type: "conjunction" });
+
+const REMOVE_FROM: Record<SharedScope, string> = {
+  repository: "delete it from the repository",
+  organization: "have an organization owner stop sharing it with this repository",
+};
+
+function exposureWarning({ foundIn, exposedIn }: RequirementCheck): string {
+  const places = andList.format(exposedIn.map((scope) => `the ${scope}`));
+  const fixes = [
+    ...(foundIn === "environment" ? [] : ["set it on the environment"]),
+    ...exposedIn.map((scope) => REMOVE_FROM[scope]),
+  ];
+  const exposure =
+    foundIn === "environment"
+      ? `set on the environment and also on ${places}, where any workflow on any branch can read it`
+      : `any workflow on any branch can read it from ${places}`;
+  return `${exposure}, so the environment's protection rules do not guard it; ${andList.format(fixes)}`;
+}

packages/sdk-plugin-setup/src/env.ts

@@ -49,6 +49,9 @@ const CLIENT_SECRET: EnvRequirement = {
   description: "Client secret of the same platform machine user",
   howTo: `${MACHINE_USER_HOW_TO}; use its client secret`,
 };
+/** Secrets that belong on the environment, since its protection rules do not guard them elsewhere. */
+export const ENVIRONMENT_PREFERRED_SECRETS: ReadonlySet<string> = new Set([CLIENT_SECRET.name]);
+
 const WORKSPACE_ID: EnvRequirement = {
   name: "TAILOR_PLATFORM_WORKSPACE_ID",
   type: "variable",
@@ -285,14 +288,17 @@ export function describeGroup(group: EnvironmentRequirements): string {
 }
 
 /**
- * Render `gh` commands that create each missing environment and set its secrets and variables.
+ * Render `gh` commands that create each missing environment and set its secrets and variables,
+ * followed by commented-out commands that delete the given repository secrets.
  * @param environments - Requirements grouped by environment; `exists: true` skips creating it
  * @param repository - Target repository; when null, gh resolves it from the current directory
+ * @param repositorySecretsToDelete - Repository secrets to delete once the environments hold them
  * @returns Shell script text
  */
 export function renderGhCommands(
   environments: (EnvironmentRequirements & { exists?: boolean })[],
   repository: Repository | null = null,
+  repositorySecretsToDelete: string[] = [],
 ): string {
   const repoPath = repository ? `${repository.owner}/${repository.name}` : "{owner}/{repo}";
   const hostPrefix = repository?.host ? `${repository.host}/` : "";
@@ -320,6 +326,15 @@ export function renderGhCommands(
     }
     return lines.join("\n");
   });
+  if (repositorySecretsToDelete.length > 0) {
+    blocks.push(
+      [
+        "# Repository secrets that any workflow on any branch can read. Delete each once every",
+        "# environment and workflow that reads it has its own; GitHub cannot restore a deleted secret.",
+        ...repositorySecretsToDelete.map((name) => `# gh secret delete ${name}${repoFlag}`),
+      ].join("\n"),
+    );
+  }
   const header = [
     "# Run these one at a time: each `gh ... set` prompts for its value.",
     ...(repository