Skip to content

Commit 5ab3d52

Browse files
authored
Add allowed-files strict allowlist for protected-file protection on PR safe outputs (#20051)
1 parent 63f1ea4 commit 5ab3d52

16 files changed

Lines changed: 615 additions & 65 deletions

.github/workflows/changeset.lock.yml

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.github/workflows/changeset.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ strict: true
1919
safe-outputs:
2020
push-to-pull-request-branch:
2121
commit-title-suffix: " [skip-ci]"
22+
allowed-files:
23+
- .changeset/**
2224
update-pull-request:
2325
title: false
2426
operation: append

.github/workflows/instructions-janitor.lock.yml

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.github/workflows/instructions-janitor.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ safe-outputs:
2424
title-prefix: "[instructions] "
2525
labels: [documentation, automation, instructions]
2626
draft: false
27+
allowed-files:
28+
- .github/aw/**
29+
protected-files: allowed
2730

2831
tools:
2932
cache-memory: true

actions/setup/js/create_pull_request.cjs

Lines changed: 17 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ const { createCheckoutManager } = require("./dynamic_checkout.cjs");
2323
const { getBaseBranch } = require("./get_base_branch.cjs");
2424
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
2525
const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");
26-
const { checkForManifestFiles, checkForProtectedPaths } = require("./manifest_file_helpers.cjs");
26+
const { checkFileProtection } = require("./manifest_file_helpers.cjs");
2727
const { renderTemplate } = require("./messages_core.cjs");
2828

2929
/**
@@ -420,37 +420,25 @@ async function main(config = {}) {
420420
core.info("Patch size validation passed");
421421
}
422422

423-
// Check for protected file modifications (e.g., package.json, go.mod, .github/ files, AGENTS.md, CLAUDE.md)
424-
// By default, protected file modifications are refused to prevent supply chain attacks.
425-
// Set protected-files: fallback-to-issue to push the branch but create a review issue
426-
// instead of a pull request, so a human can carefully review the changes first.
427-
// Set protected-files: allowed only when the workflow is explicitly designed to manage these files.
428-
/** @type {{ manifestFilesFound: string[], protectedPathsFound: string[] } | null} */
423+
// Check file protection: allowlist (strict) or protected-files policy.
424+
/** @type {string[] | null} Protected files that trigger fallback-to-issue handling */
429425
let manifestProtectionFallback = null;
430426
/** @type {unknown} */
431427
let manifestProtectionPushFailedError = null;
432428
if (!isEmpty) {
433-
const manifestFiles = Array.isArray(config.protected_files) ? config.protected_files : [];
434-
const protectedPathPrefixes = Array.isArray(config.protected_path_prefixes) ? config.protected_path_prefixes : [];
435-
// protected_files_policy is a string enum: "allowed" = allow, "fallback-to-issue" = fallback, "blocked" (default) = deny.
436-
const policy = config.protected_files_policy;
437-
const isAllowed = policy === "allowed";
438-
const isFallback = policy === "fallback-to-issue";
439-
if (!isAllowed) {
440-
const { hasManifestFiles, manifestFilesFound } = checkForManifestFiles(patchContent, manifestFiles);
441-
const { hasProtectedPaths, protectedPathsFound } = checkForProtectedPaths(patchContent, protectedPathPrefixes);
442-
const allFound = [...manifestFilesFound, ...protectedPathsFound];
443-
if (allFound.length > 0) {
444-
if (isFallback) {
445-
// Record for fallback-to-issue handling below; let patch application proceed
446-
manifestProtectionFallback = { manifestFilesFound, protectedPathsFound };
447-
core.warning(`Protected file protection triggered (fallback-to-issue): ${allFound.join(", ")}. Will create review issue instead of pull request.`);
448-
} else {
449-
const message = `Cannot create pull request: patch modifies protected files (${allFound.join(", ")}). Set protected-files: fallback-to-issue to create a review issue instead.`;
450-
core.error(message);
451-
return { success: false, error: message };
452-
}
453-
}
429+
const protection = checkFileProtection(patchContent, config);
430+
if (protection.action === "deny") {
431+
const filesStr = protection.files.join(", ");
432+
const message =
433+
protection.source === "allowlist"
434+
? `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.`
435+
: `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.`;
436+
core.error(message);
437+
return { success: false, error: message };
438+
}
439+
if (protection.action === "fallback") {
440+
manifestProtectionFallback = protection.files;
441+
core.warning(`Protected file protection triggered (fallback-to-issue): ${protection.files.join(", ")}. Will create review issue instead of pull request.`);
454442
}
455443
}
456444

@@ -943,7 +931,7 @@ ${patchPreview}`;
943931
// - Push-failed case: push was rejected (e.g. missing `workflows` permission); provides
944932
// patch artifact download instructions instead of the compare URL.
945933
if (manifestProtectionFallback) {
946-
const allFound = [...manifestProtectionFallback.manifestFilesFound, ...manifestProtectionFallback.protectedPathsFound];
934+
const allFound = manifestProtectionFallback;
947935
const filesFormatted = allFound.map(f => `\`${f}\``).join(", ");
948936

949937
let fallbackBody;

actions/setup/js/create_pull_request.test.cjs

Lines changed: 184 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
// @ts-check
2-
import { describe, it, expect, beforeEach, vi } from "vitest";
2+
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
33
import { createRequire } from "module";
4+
import * as fs from "fs";
5+
import * as path from "path";
6+
import * as os from "os";
47

58
const require = createRequire(import.meta.url);
69

@@ -235,3 +238,183 @@ describe("create_pull_request - security: branch name sanitization", () => {
235238
expect(normalizeBranchName("UPPERCASE")).toBe("uppercase");
236239
});
237240
});
241+
242+
// ──────────────────────────────────────────────────────
243+
// allowed-files strict allowlist
244+
// ──────────────────────────────────────────────────────
245+
246+
describe("create_pull_request - allowed-files strict allowlist", () => {
247+
let tempDir;
248+
let originalEnv;
249+
250+
beforeEach(() => {
251+
originalEnv = { ...process.env };
252+
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
253+
process.env.GITHUB_REPOSITORY = "test-owner/test-repo";
254+
process.env.GITHUB_BASE_REF = "main";
255+
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-allowed-test-"));
256+
257+
global.core = {
258+
info: vi.fn(),
259+
warning: vi.fn(),
260+
error: vi.fn(),
261+
debug: vi.fn(),
262+
setFailed: vi.fn(),
263+
setOutput: vi.fn(),
264+
startGroup: vi.fn(),
265+
endGroup: vi.fn(),
266+
summary: {
267+
addRaw: vi.fn().mockReturnThis(),
268+
write: vi.fn().mockResolvedValue(undefined),
269+
},
270+
};
271+
global.github = {
272+
rest: {
273+
pulls: {
274+
create: vi.fn().mockResolvedValue({ data: { number: 1, html_url: "https://github.com/test" } }),
275+
},
276+
repos: {
277+
get: vi.fn().mockResolvedValue({ data: { default_branch: "main" } }),
278+
},
279+
},
280+
graphql: vi.fn(),
281+
};
282+
global.context = {
283+
eventName: "workflow_dispatch",
284+
repo: { owner: "test-owner", repo: "test-repo" },
285+
payload: {},
286+
};
287+
global.exec = {
288+
exec: vi.fn().mockResolvedValue(0),
289+
getExecOutput: vi.fn().mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }),
290+
};
291+
292+
// Clear module cache so globals are picked up fresh
293+
delete require.cache[require.resolve("./create_pull_request.cjs")];
294+
});
295+
296+
afterEach(() => {
297+
for (const key of Object.keys(process.env)) {
298+
if (!(key in originalEnv)) {
299+
delete process.env[key];
300+
}
301+
}
302+
Object.assign(process.env, originalEnv);
303+
304+
if (tempDir && fs.existsSync(tempDir)) {
305+
fs.rmSync(tempDir, { recursive: true, force: true });
306+
}
307+
308+
delete global.core;
309+
delete global.github;
310+
delete global.context;
311+
delete global.exec;
312+
vi.clearAllMocks();
313+
});
314+
315+
/**
316+
* Creates a minimal git patch touching the given file paths.
317+
*/
318+
function createPatchWithFiles(...filePaths) {
319+
const diffs = filePaths
320+
.map(
321+
p => `diff --git a/${p} b/${p}
322+
new file mode 100644
323+
index 0000000..abc1234
324+
--- /dev/null
325+
+++ b/${p}
326+
@@ -0,0 +1 @@
327+
+content
328+
`
329+
)
330+
.join("\n");
331+
return `From abc123 Mon Sep 17 00:00:00 2001
332+
From: Test Author <test@example.com>
333+
Date: Mon, 1 Jan 2024 00:00:00 +0000
334+
Subject: [PATCH] Test commit
335+
336+
${diffs}
337+
--
338+
2.34.1
339+
`;
340+
}
341+
342+
function writePatch(content) {
343+
const p = path.join(tempDir, "test.patch");
344+
fs.writeFileSync(p, content);
345+
return p;
346+
}
347+
348+
it("should reject files outside the allowed-files allowlist", async () => {
349+
const patchPath = writePatch(createPatchWithFiles("src/index.js"));
350+
351+
const { main } = require("./create_pull_request.cjs");
352+
const handler = await main({ allowed_files: [".github/aw/**"] });
353+
const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
354+
355+
expect(result.success).toBe(false);
356+
expect(result.error).toContain("outside the allowed-files list");
357+
expect(result.error).toContain("src/index.js");
358+
});
359+
360+
it("should reject a mixed patch where some files are outside the allowlist", async () => {
361+
const patchPath = writePatch(createPatchWithFiles(".github/aw/github-agentic-workflows.md", "src/index.js"));
362+
363+
const { main } = require("./create_pull_request.cjs");
364+
const handler = await main({ allowed_files: [".github/aw/**"] });
365+
const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
366+
367+
expect(result.success).toBe(false);
368+
expect(result.error).toContain("outside the allowed-files list");
369+
expect(result.error).toContain("src/index.js");
370+
expect(result.error).not.toContain(".github/aw/github-agentic-workflows.md");
371+
});
372+
373+
it("should still enforce protected-files when allowed-files matches (orthogonal checks)", async () => {
374+
// allowed-files and protected-files are orthogonal: both checks must pass.
375+
// Matching the allowlist does NOT bypass the protected-files policy.
376+
const patchPath = writePatch(createPatchWithFiles(".github/aw/instructions.md"));
377+
378+
const { main } = require("./create_pull_request.cjs");
379+
const handler = await main({
380+
allowed_files: [".github/aw/**"],
381+
protected_path_prefixes: [".github/"],
382+
protected_files_policy: "blocked",
383+
});
384+
const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
385+
386+
expect(result.success).toBe(false);
387+
expect(result.error).toContain("protected files");
388+
});
389+
390+
it("should allow a protected file when both allowed-files matches and protected-files: allowed is set", async () => {
391+
// Both checks are satisfied explicitly: allowlist scope + protected-files permission.
392+
const patchPath = writePatch(createPatchWithFiles(".github/aw/instructions.md"));
393+
394+
const { main } = require("./create_pull_request.cjs");
395+
const handler = await main({
396+
allowed_files: [".github/aw/**"],
397+
protected_path_prefixes: [".github/"],
398+
protected_files_policy: "allowed",
399+
});
400+
const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
401+
402+
// Should not be blocked by either check
403+
expect(result.error || "").not.toContain("protected files");
404+
expect(result.error || "").not.toContain("outside the allowed-files list");
405+
});
406+
407+
it("should still enforce protected-files when allowed-files is not set", async () => {
408+
const patchPath = writePatch(createPatchWithFiles(".github/aw/instructions.md"));
409+
410+
const { main } = require("./create_pull_request.cjs");
411+
const handler = await main({
412+
protected_path_prefixes: [".github/"],
413+
protected_files_policy: "blocked",
414+
});
415+
const result = await handler({ patch_path: patchPath, title: "Test PR", body: "" }, {});
416+
417+
expect(result.success).toBe(false);
418+
expect(result.error).toContain("protected files");
419+
});
420+
});

0 commit comments

Comments
 (0)