Add allowed-files strict allowlist for protected-file protection on PR safe outputs

.github/workflows/changeset.md

@@ -19,6 +19,8 @@ strict: true
 safe-outputs:
   push-to-pull-request-branch:
     commit-title-suffix: " [skip-ci]"
+    allowed-files:
+      - .changeset/**
   update-pull-request:
     title: false
     operation: append

.github/workflows/instructions-janitor.md

@@ -24,6 +24,9 @@ safe-outputs:
     title-prefix: "[instructions] "
     labels: [documentation, automation, instructions]
     draft: false
+    allowed-files:
+      - .github/aw/**
+    protected-files: allowed
 
 tools:
   cache-memory: true

actions/setup/js/create_pull_request.cjs

@@ -23,7 +23,7 @@ const { createCheckoutManager } = require("./dynamic_checkout.cjs");
 const { getBaseBranch } = require("./get_base_branch.cjs");
 const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
 const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");
-const { checkForManifestFiles, checkForProtectedPaths } = require("./manifest_file_helpers.cjs");
+const { checkFileProtection } = require("./manifest_file_helpers.cjs");
 const { renderTemplate } = require("./messages_core.cjs");
 
 /**
@@ -420,37 +420,25 @@ async function main(config = {}) {
       core.info("Patch size validation passed");
     }
 
-    // Check for protected file modifications (e.g., package.json, go.mod, .github/ files, AGENTS.md, CLAUDE.md)
-    // By default, protected file modifications are refused to prevent supply chain attacks.
-    // Set protected-files: fallback-to-issue to push the branch but create a review issue
-    // instead of a pull request, so a human can carefully review the changes first.
-    // Set protected-files: allowed only when the workflow is explicitly designed to manage these files.
-    /** @type {{ manifestFilesFound: string[], protectedPathsFound: string[] } | null} */
+    // Check file protection: allowlist (strict) or protected-files policy.
+    /** @type {string[] | null} Protected files that trigger fallback-to-issue handling */
     let manifestProtectionFallback = null;
     /** @type {unknown} */
     let manifestProtectionPushFailedError = null;
     if (!isEmpty) {
-      const manifestFiles = Array.isArray(config.protected_files) ? config.protected_files : [];
-      const protectedPathPrefixes = Array.isArray(config.protected_path_prefixes) ? config.protected_path_prefixes : [];
-      // protected_files_policy is a string enum: "allowed" = allow, "fallback-to-issue" = fallback, "blocked" (default) = deny.
-      const policy = config.protected_files_policy;
-      const isAllowed = policy === "allowed";
-      const isFallback = policy === "fallback-to-issue";
-      if (!isAllowed) {
-        const { hasManifestFiles, manifestFilesFound } = checkForManifestFiles(patchContent, manifestFiles);
-        const { hasProtectedPaths, protectedPathsFound } = checkForProtectedPaths(patchContent, protectedPathPrefixes);
-        const allFound = [...manifestFilesFound, ...protectedPathsFound];
-        if (allFound.length > 0) {
-          if (isFallback) {
-            // Record for fallback-to-issue handling below; let patch application proceed
-            manifestProtectionFallback = { manifestFilesFound, protectedPathsFound };
-            core.warning(`Protected file protection triggered (fallback-to-issue): ${allFound.join(", ")}. Will create review issue instead of pull request.`);
-          } else {
-            const message = `Cannot create pull request: patch modifies protected files (${allFound.join(", ")}). Set protected-files: fallback-to-issue to create a review issue instead.`;
-            core.error(message);
-            return { success: false, error: message };
-          }
-        }
+      const protection = checkFileProtection(patchContent, config);
+      if (protection.action === "deny") {
+        const filesStr = protection.files.join(", ");
+        const message =
+          protection.source === "allowlist"
+            ? `Cannot create pull request: patch modifies files outside the allowed-files list (${filesStr}). Add the files to the allowed-files configuration field or remove them from the patch.`
+            : `Cannot create pull request: patch modifies protected files (${filesStr}). Add them to the allowed-files configuration field or set protected-files: fallback-to-issue to create a review issue instead.`;
+        core.error(message);
+        return { success: false, error: message };
+      }
+      if (protection.action === "fallback") {
+        manifestProtectionFallback = protection.files;
+        core.warning(`Protected file protection triggered (fallback-to-issue): ${protection.files.join(", ")}. Will create review issue instead of pull request.`);
       }
     }
 
@@ -943,7 +931,7 @@ ${patchPreview}`;
     // - Push-failed case: push was rejected (e.g. missing `workflows` permission); provides
     //   patch artifact download instructions instead of the compare URL.
     if (manifestProtectionFallback) {
-      const allFound = [...manifestProtectionFallback.manifestFilesFound, ...manifestProtectionFallback.protectedPathsFound];
+      const allFound = manifestProtectionFallback;
       const filesFormatted = allFound.map(f => `\`${f}\``).join(", ");
 
       let fallbackBody;

actions/setup/js/create_pull_request.test.cjs

@@ -1,6 +1,9 @@
 // @ts-check
-import { describe, it, expect, beforeEach, vi } from "vitest";
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
 import { createRequire } from "module";
+import * as fs from "fs";
+import * as path from "path";
+import * as os from "os";
 
 const require = createRequire(import.meta.url);
 
@@ -235,3 +238,183 @@ describe("create_pull_request - security: branch name sanitization", () => {
     expect(normalizeBranchName("UPPERCASE")).toBe("uppercase");
   });
 });
+
+// ──────────────────────────────────────────────────────
+// allowed-files strict allowlist
+// ──────────────────────────────────────────────────────
+
+describe("create_pull_request - allowed-files strict allowlist", () => {
+  let tempDir;
+  let originalEnv;
+
+  beforeEach(() => {
+    originalEnv = { ...process.env };
+    process.env.GH_AW_WORKFLOW_ID = "test-workflow";
+    process.env.GITHUB_REPOSITORY = "test-owner/test-repo";
+    process.env.GITHUB_BASE_REF = "main";
+    tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-allowed-test-"));
+
+    global.core = {
+      info: vi.fn(),
+      warning: vi.fn(),
+      error: vi.fn(),
+      debug: vi.fn(),
+      setFailed: vi.fn(),
+      setOutput: vi.fn(),
+      startGroup: vi.fn(),
+      endGroup: vi.fn(),
+      summary: {
+        addRaw: vi.fn().mockReturnThis(),
+        write: vi.fn().mockResolvedValue(undefined),
+      },
+    };
+    global.github = {
+      rest: {
+        pulls: {
+          create: vi.fn().mockResolvedValue({ data: { number: 1, html_url: "https://github.com/test" } }),
+        },
+        repos: {
+          get: vi.fn().mockResolvedValue({ data: { default_branch: "main" } }),
+        },
+      },
+      graphql: vi.fn(),
+    };
+    global.context = {
+      eventName: "workflow_dispatch",
+      repo: { owner: "test-owner", repo: "test-repo" },
+      payload: {},
+    };
+    global.exec = {
+      exec: vi.fn().mockResolvedValue(0),
+      getExecOutput: vi.fn().mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }),
+    };
+
+    // Clear module cache so globals are picked up fresh
+    delete require.cache[require.resolve("./create_pull_request.cjs")];
+  });
+
+  afterEach(() => {
+    for (const key of Object.keys(process.env)) {
+      if (!(key in originalEnv)) {
+        delete process.env[key];
+      }
+    }
+    Object.assign(process.env, originalEnv);
+
+    if (tempDir && fs.existsSync(tempDir)) {
+      fs.rmSync(tempDir, { recursive: true, force: true });
+    }
+
+    delete global.core;
+    delete global.github;
+    delete global.context;
+    delete global.exec;
+    vi.clearAllMocks();
+  });
+
+  /**
+   * Creates a minimal git patch touching the given file paths.
+   */
+  function createPatchWithFiles(...filePaths) {
+    const diffs = filePaths
+      .map(
+        p => `diff --git a/${p} b/${p}
+new file mode 100644
+index 0000000..abc1234
+--- /dev/null
++++ b/${p}
+@@ -0,0 +1 @@
++content
+`
+      )
+      .join("\n");
+    return `From abc123 Mon Sep 17 00:00:00 2001
+From: Test Author <test@example.com>
+Date: Mon, 1 Jan 2024 00:00:00 +0000
+Subject: [PATCH] Test commit
+
+${diffs}
+--
+2.34.1
+`;
+  }
+
+  function writePatch(content) {
+    const p = path.join(tempDir, "test.patch");
+    fs.writeFileSync(p, content);
+    return p;
+  }
+
+  it("should reject files outside the allowed-files allowlist", async () => {
+    const patchPath = writePatch(createPatchWithFiles("src/index.js"));
+
+    const { main } = require("./create_pull_request.cjs");
+    const handler = await main({ allowed_files: [".github/aw/**"] });
+    const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
+
+    expect(result.success).toBe(false);
+    expect(result.error).toContain("outside the allowed-files list");
+    expect(result.error).toContain("src/index.js");
+  });
+
+  it("should reject a mixed patch where some files are outside the allowlist", async () => {
+    const patchPath = writePatch(createPatchWithFiles(".github/aw/github-agentic-workflows.md", "src/index.js"));
+
+    const { main } = require("./create_pull_request.cjs");
+    const handler = await main({ allowed_files: [".github/aw/**"] });
+    const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
+
+    expect(result.success).toBe(false);
+    expect(result.error).toContain("outside the allowed-files list");
+    expect(result.error).toContain("src/index.js");
+    expect(result.error).not.toContain(".github/aw/github-agentic-workflows.md");
+  });
+
+  it("should still enforce protected-files when allowed-files matches (orthogonal checks)", async () => {
+    // allowed-files and protected-files are orthogonal: both checks must pass.
+    // Matching the allowlist does NOT bypass the protected-files policy.
+    const patchPath = writePatch(createPatchWithFiles(".github/aw/instructions.md"));
+
+    const { main } = require("./create_pull_request.cjs");
+    const handler = await main({
+      allowed_files: [".github/aw/**"],
+      protected_path_prefixes: [".github/"],
+      protected_files_policy: "blocked",
+    });
+    const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
+
+    expect(result.success).toBe(false);
+    expect(result.error).toContain("protected files");
+  });
+
+  it("should allow a protected file when both allowed-files matches and protected-files: allowed is set", async () => {
+    // Both checks are satisfied explicitly: allowlist scope + protected-files permission.
+    const patchPath = writePatch(createPatchWithFiles(".github/aw/instructions.md"));
+
+    const { main } = require("./create_pull_request.cjs");
+    const handler = await main({
+      allowed_files: [".github/aw/**"],
+      protected_path_prefixes: [".github/"],
+      protected_files_policy: "allowed",
+    });
+    const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
+
+    // Should not be blocked by either check
+    expect(result.error || "").not.toContain("protected files");
+    expect(result.error || "").not.toContain("outside the allowed-files list");
+  });
+
+  it("should still enforce protected-files when allowed-files is not set", async () => {
+    const patchPath = writePatch(createPatchWithFiles(".github/aw/instructions.md"));
+
+    const { main } = require("./create_pull_request.cjs");
+    const handler = await main({
+      protected_path_prefixes: [".github/"],
+      protected_files_policy: "blocked",
+    });
+    const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
+
+    expect(result.success).toBe(false);
+    expect(result.error).toContain("protected files");
+  });
+});