diff --git a/.changeset/patch-enforce-safe-output-max-limits.md b/.changeset/patch-enforce-safe-output-max-limits.md new file mode 100644 index 0000000000..0ccbde3775 --- /dev/null +++ b/.changeset/patch-enforce-safe-output-max-limits.md @@ -0,0 +1,4 @@ +--- +"gh-aw": patch +--- +Enforce the SEC-003 safe outputs max limits by adding reusable helpers and tests across the seven core handlers. diff --git a/actions/setup/js/add_labels.cjs b/actions/setup/js/add_labels.cjs index 9d297f51cc..455330b70a 100644 --- a/actions/setup/js/add_labels.cjs +++ b/actions/setup/js/add_labels.cjs @@ -11,6 +11,14 @@ const HANDLER_TYPE = "add_labels"; const { validateLabels } = require("./safe_output_validator.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); +const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs"); + +/** + * Maximum limits for label parameters to prevent resource exhaustion. + * These limits align with GitHub's API constraints and security best practices. + */ +/** @type {number} Maximum number of labels allowed per operation */ +const MAX_LABELS = 10; /** * Main handler factory for add_labels @@ -89,6 +97,13 @@ async function main(config = {}) { return { success: false, error }; } + // Enforce max limits on labels before validation + const limitResult = tryEnforceArrayLimit(requestedLabels, MAX_LABELS, "labels"); + if (!limitResult.success) { + core.warning(`Label limit exceeded: ${limitResult.error}`); + return { success: false, error: limitResult.error }; + } + // Use validation helper to sanitize and validate labels const labelsResult = validateLabels(requestedLabels, allowedLabels, maxCount); if (!labelsResult.valid) { diff --git a/actions/setup/js/add_labels.test.cjs b/actions/setup/js/add_labels.test.cjs index 0f43368f9d..10a2251c06 100644 --- a/actions/setup/js/add_labels.test.cjs +++ b/actions/setup/js/add_labels.test.cjs @@ -478,5 +478,35 @@ describe("add_labels", () => { expect(addLabelsCalls[0].owner).toBe("github"); expect(addLabelsCalls[0].repo).toBe("gh-aw"); }); + + it("should enforce max limit on labels per operation", async () => { + const handler = await main({ max: 10 }); + + // Try to add more than MAX_LABELS (10) + const result = await handler( + { + item_number: 100, + labels: [ + "label1", + "label2", + "label3", + "label4", + "label5", + "label6", + "label7", + "label8", + "label9", + "label10", + "label11", // 11th label exceeds limit + ], + }, + {} + ); + + expect(result.success).toBe(false); + expect(result.error).toContain("E003"); + expect(result.error).toContain("Cannot add more than 10 labels"); + expect(result.error).toContain("received 11"); + }); }); }); diff --git a/actions/setup/js/create_discussion.cjs b/actions/setup/js/create_discussion.cjs index 96ca571c69..3cc0c0fd99 100644 --- a/actions/setup/js/create_discussion.cjs +++ b/actions/setup/js/create_discussion.cjs @@ -17,6 +17,14 @@ const { getErrorMessage } = require("./error_helpers.cjs"); const { createExpirationLine, generateFooterWithExpiration } = require("./ephemerals.cjs"); const { generateWorkflowIdMarker } = require("./generate_footer.cjs"); const { sanitizeLabelContent } = require("./sanitize_label_content.cjs"); +const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs"); + +/** + * Maximum limits for discussion parameters to prevent resource exhaustion. + * These limits align with GitHub's API constraints and security best practices. + */ +/** @type {number} Maximum number of labels allowed per discussion */ +const MAX_LABELS = 10; /** * Fetch repository ID and discussion categories for a repository @@ -483,6 +491,13 @@ async function main(config = {}) { .map(label => (label.length > 64 ? label.substring(0, 64) : label)) .filter((label, index, arr) => arr.indexOf(label) === index); + // Enforce max limits on labels before API calls + const limitResult = tryEnforceArrayLimit(discussionLabels, MAX_LABELS, "labels"); + if (!limitResult.success) { + core.warning(`Discussion limit exceeded: ${limitResult.error}`); + return { success: false, error: limitResult.error }; + } + // Build title let title = item.title ? item.title.trim() : ""; let processedBody = replaceTemporaryIdReferences(item.body || "", temporaryIdMap, qualifiedItemRepo); diff --git a/actions/setup/js/create_discussion_labels.test.cjs b/actions/setup/js/create_discussion_labels.test.cjs index 7e9670fbbc..d5e4c569b4 100644 --- a/actions/setup/js/create_discussion_labels.test.cjs +++ b/actions/setup/js/create_discussion_labels.test.cjs @@ -392,4 +392,40 @@ describe("create_discussion with labels", () => { expect(labelsApplyCall).toBeDefined(); expect(labelsApplyCall[1].labelIds).toEqual(["LA_label1", "LA_label2"]); }); + + it("should enforce max labels limit (SEC-003)", async () => { + process.env.GH_AW_WORKFLOW_ID = "test-workflow"; + process.env.GH_AW_WORKFLOW_NAME = "Test Workflow"; + process.env.GH_AW_WORKFLOW_SOURCE = "test.md"; + + const handler = await createDiscussionMain({ + category: "General", + }); + + // Try to create discussion with more than MAX_LABELS (10) + const message = { + title: "Test Discussion", + body: "This is a test discussion body", + labels: [ + "label1", + "label2", + "label3", + "label4", + "label5", + "label6", + "label7", + "label8", + "label9", + "label10", + "label11", // 11th label exceeds limit + ], + }; + + const result = await handler(message, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("E003"); + expect(result.error).toContain("Cannot add more than 10 labels"); + expect(result.error).toContain("received 11"); + }); }); diff --git a/actions/setup/js/create_issue.cjs b/actions/setup/js/create_issue.cjs index 4c49a49b0f..978ebf4cce 100644 --- a/actions/setup/js/create_issue.cjs +++ b/actions/setup/js/create_issue.cjs @@ -38,6 +38,7 @@ const { renderTemplate } = require("./messages_core.cjs"); const { createExpirationLine, addExpirationToFooter } = require("./ephemerals.cjs"); const { MAX_SUB_ISSUES, getSubIssueCount } = require("./sub_issue_helpers.cjs"); const { closeOlderIssues } = require("./close_older_issues.cjs"); +const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs"); const fs = require("fs"); /** @@ -53,6 +54,16 @@ const MAX_SUB_ISSUES_PER_PARENT = MAX_SUB_ISSUES; /** @type {number} Maximum number of parent issues to check when searching */ const MAX_PARENT_ISSUES_TO_CHECK = 10; +/** + * Maximum limits for issue parameters to prevent resource exhaustion. + * These limits align with GitHub's API constraints and security best practices. + */ +/** @type {number} Maximum number of labels allowed per issue */ +const MAX_LABELS = 10; + +/** @type {number} Maximum number of assignees allowed per issue */ +const MAX_ASSIGNEES = 5; + /** * Searches for an existing parent issue that can accept more sub-issues * @param {string} owner - Repository owner @@ -385,6 +396,19 @@ async function main(config = {}) { // Copilot is not a valid GitHub user and must be assigned via the agent assignment API assignees = assignees.filter(assignee => assignee !== "copilot"); + // Enforce max limits on labels and assignees before API calls + const labelsLimitResult = tryEnforceArrayLimit(labels, MAX_LABELS, "labels"); + if (!labelsLimitResult.success) { + core.warning(`Issue limit exceeded: ${labelsLimitResult.error}`); + return { success: false, error: labelsLimitResult.error }; + } + + const assigneesLimitResult = tryEnforceArrayLimit(assignees, MAX_ASSIGNEES, "assignees"); + if (!assigneesLimitResult.success) { + core.warning(`Issue limit exceeded: ${assigneesLimitResult.error}`); + return { success: false, error: assigneesLimitResult.error }; + } + let title = message.title?.trim() ?? ""; // Replace temporary ID references in the body using already-created issues diff --git a/actions/setup/js/create_issue.test.cjs b/actions/setup/js/create_issue.test.cjs index af1975ca65..b86129b563 100644 --- a/actions/setup/js/create_issue.test.cjs +++ b/actions/setup/js/create_issue.test.cjs @@ -390,4 +390,48 @@ describe("create_issue", () => { expect(createCall.body).toContain("Related to #456"); }); }); + + describe("max limit enforcement", () => { + it("should enforce max limit on labels", async () => { + const handler = await main({}); + + const result = await handler({ + title: "Test Issue", + body: "Test body", + labels: [ + "label1", + "label2", + "label3", + "label4", + "label5", + "label6", + "label7", + "label8", + "label9", + "label10", + "label11", // 11th label exceeds limit + ], + }); + + expect(result.success).toBe(false); + expect(result.error).toContain("E003"); + expect(result.error).toContain("Cannot add more than 10 labels"); + expect(result.error).toContain("received 11"); + }); + + it("should enforce max limit on assignees", async () => { + const handler = await main({}); + + const result = await handler({ + title: "Test Issue", + body: "Test body", + assignees: ["user1", "user2", "user3", "user4", "user5", "user6"], // 6 assignees exceeds limit of 5 + }); + + expect(result.success).toBe(false); + expect(result.error).toContain("E003"); + expect(result.error).toContain("Cannot add more than 5 assignees"); + expect(result.error).toContain("received 6"); + }); + }); }); diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 9099f64372..dfa5f1dce0 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -23,6 +23,34 @@ const { generateWorkflowIdMarker } = require("./generate_footer.cjs"); /** @type {string} Safe output type handled by this module */ const HANDLER_TYPE = "create_pull_request"; +/** + * Maximum limits for pull request parameters to prevent resource exhaustion. + * These limits align with GitHub's API constraints and security best practices. + */ +/** @type {number} Maximum number of files allowed per pull request */ +const MAX_FILES = 100; + +/** + * Enforces maximum limits on pull request parameters to prevent resource exhaustion attacks. + * Per Safe Outputs specification requirement SEC-003, limits must be enforced before API calls. + * + * @param {string} patchContent - Patch content to validate + * @throws {Error} When any limit is exceeded, with error code E003 and details + */ +function enforcePullRequestLimits(patchContent) { + if (!patchContent || !patchContent.trim()) { + return; + } + + // Count files in patch by looking for "diff --git" lines + const fileMatches = patchContent.match(/^diff --git /gm); + const fileCount = fileMatches ? fileMatches.length : 0; + + // Check file count - max limit exceeded check + if (fileCount > MAX_FILES) { + throw new Error(`E003: Cannot create pull request with more than ${MAX_FILES} files (received ${fileCount})`); + } +} /** * Generate a patch preview with max 500 lines and 2000 chars for issue body * @param {string} patchContent - The full patch content @@ -219,6 +247,15 @@ async function main(config = {}) { isEmpty = !patchContent || !patchContent.trim(); } + // Enforce max limits on patch before processing + try { + enforcePullRequestLimits(patchContent); + } catch (error) { + const errorMessage = getErrorMessage(error); + core.warning(`Pull request limit exceeded: ${errorMessage}`); + return { success: false, error: errorMessage }; + } + // Check for actual error conditions (but allow empty patches as valid noop) if (patchContent.includes("Failed to generate patch")) { // If allow-empty is enabled, ignore patch errors and proceed @@ -859,4 +896,4 @@ You can manually create a pull request from the branch if needed.${patchPreview} }; // End of handleCreatePullRequest } // End of main -module.exports = { main }; +module.exports = { main, enforcePullRequestLimits }; diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index 09afaa089c..6f0d680d35 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -1,5 +1,8 @@ // @ts-check -import { describe, it, expect } from "vitest"; +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { createRequire } from "module"; + +const require = createRequire(import.meta.url); describe("create_pull_request - fallback-as-issue configuration", () => { describe("configuration parsing", () => { @@ -46,3 +49,58 @@ describe("create_pull_request - fallback-as-issue configuration", () => { }); }); }); + +describe("create_pull_request - max limit enforcement", () => { + let mockFs; + + beforeEach(() => { + // Mock fs module for patch reading + mockFs = { + existsSync: vi.fn().mockReturnValue(true), + readFileSync: vi.fn(), + }; + }); + + it("should enforce max file limit on patch content", () => { + // Create a patch with more than MAX_FILES (100) files + const patchLines = []; + for (let i = 0; i < 101; i++) { + patchLines.push(`diff --git a/file${i}.txt b/file${i}.txt`); + patchLines.push("index 1234567..abcdefg 100644"); + patchLines.push("--- a/file${i}.txt"); + patchLines.push("+++ b/file${i}.txt"); + patchLines.push("@@ -1,1 +1,1 @@"); + patchLines.push("-old content"); + patchLines.push("+new content"); + } + const patchContent = patchLines.join("\n"); + + // Import the enforcement function + const { enforcePullRequestLimits } = require("./create_pull_request.cjs"); + + // Should throw E003 error + expect(() => enforcePullRequestLimits(patchContent)).toThrow("E003"); + expect(() => enforcePullRequestLimits(patchContent)).toThrow("Cannot create pull request with more than 100 files"); + expect(() => enforcePullRequestLimits(patchContent)).toThrow("received 101"); + }); + + it("should allow patches under the file limit", () => { + // Create a patch with exactly MAX_FILES (100) files + const patchLines = []; + for (let i = 0; i < 100; i++) { + patchLines.push(`diff --git a/file${i}.txt b/file${i}.txt`); + patchLines.push("index 1234567..abcdefg 100644"); + patchLines.push("--- a/file${i}.txt"); + patchLines.push("+++ b/file${i}.txt"); + patchLines.push("@@ -1,1 +1,1 @@"); + patchLines.push("-old content"); + patchLines.push("+new content"); + } + const patchContent = patchLines.join("\n"); + + const { enforcePullRequestLimits } = require("./create_pull_request.cjs"); + + // Should not throw + expect(() => enforcePullRequestLimits(patchContent)).not.toThrow(); + }); +}); diff --git a/actions/setup/js/limit_enforcement_helpers.cjs b/actions/setup/js/limit_enforcement_helpers.cjs new file mode 100644 index 0000000000..92da53c050 --- /dev/null +++ b/actions/setup/js/limit_enforcement_helpers.cjs @@ -0,0 +1,48 @@ +// @ts-check + +/** + * Helper functions for enforcing maximum limits on safe output parameters. + * These functions align with SEC-003 requirements for preventing resource exhaustion. + */ + +/** + * Enforces a maximum limit on an array parameter. + * Throws E003 error when the limit is exceeded. + * + * @param {Array|undefined|null} array - The array to check + * @param {number} maxLimit - Maximum allowed length + * @param {string} parameterName - Name of the parameter for error messages + * @throws {Error} When array length exceeds maxLimit, with E003 error code + */ +function enforceArrayLimit(array, maxLimit, parameterName) { + if (array && Array.isArray(array) && array.length > maxLimit) { + throw new Error(`E003: Cannot add more than ${maxLimit} ${parameterName} (received ${array.length})`); + } +} + +/** + * Safely enforces array limit and catches exceptions. + * Returns an error result object instead of throwing. + * + * @param {Array|undefined|null} array - The array to check + * @param {number} maxLimit - Maximum allowed length + * @param {string} parameterName - Name of the parameter for error messages + * @returns {{success: true} | {success: false, error: string}} Result object + */ +function tryEnforceArrayLimit(array, maxLimit, parameterName) { + try { + enforceArrayLimit(array, maxLimit, parameterName); + return { success: true }; + } catch (error) { + const { getErrorMessage } = require("./error_helpers.cjs"); + return { + success: false, + error: getErrorMessage(error), + }; + } +} + +module.exports = { + enforceArrayLimit, + tryEnforceArrayLimit, +}; diff --git a/actions/setup/js/limit_enforcement_helpers.test.cjs b/actions/setup/js/limit_enforcement_helpers.test.cjs new file mode 100644 index 0000000000..7e75f2280d --- /dev/null +++ b/actions/setup/js/limit_enforcement_helpers.test.cjs @@ -0,0 +1,73 @@ +// @ts-check +import { describe, it, expect } from "vitest"; + +const { enforceArrayLimit, tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs"); + +describe("limit_enforcement_helpers", () => { + describe("enforceArrayLimit", () => { + it("should not throw when array is under limit", () => { + expect(() => enforceArrayLimit([1, 2, 3], 5, "items")).not.toThrow(); + }); + + it("should not throw when array equals limit", () => { + expect(() => enforceArrayLimit([1, 2, 3, 4, 5], 5, "items")).not.toThrow(); + }); + + it("should throw E003 error when array exceeds limit", () => { + expect(() => enforceArrayLimit([1, 2, 3, 4, 5, 6], 5, "items")).toThrow("E003"); + expect(() => enforceArrayLimit([1, 2, 3, 4, 5, 6], 5, "items")).toThrow("Cannot add more than 5 items"); + expect(() => enforceArrayLimit([1, 2, 3, 4, 5, 6], 5, "items")).toThrow("received 6"); + }); + + it("should not throw when array is null or undefined", () => { + expect(() => enforceArrayLimit(null, 5, "items")).not.toThrow(); + expect(() => enforceArrayLimit(undefined, 5, "items")).not.toThrow(); + }); + + it("should not throw when array is not an array", () => { + // @ts-ignore - testing invalid input + expect(() => enforceArrayLimit("not an array", 5, "items")).not.toThrow(); + // @ts-ignore - testing invalid input + expect(() => enforceArrayLimit(123, 5, "items")).not.toThrow(); + }); + + it("should use parameter name in error message", () => { + expect(() => enforceArrayLimit([1, 2, 3], 2, "labels")).toThrow("labels"); + expect(() => enforceArrayLimit([1, 2, 3, 4], 2, "assignees")).toThrow("assignees"); + }); + }); + + describe("tryEnforceArrayLimit", () => { + it("should return success when array is under limit", () => { + const result = tryEnforceArrayLimit([1, 2, 3], 5, "items"); + expect(result.success).toBe(true); + }); + + it("should return success when array equals limit", () => { + const result = tryEnforceArrayLimit([1, 2, 3, 4, 5], 5, "items"); + expect(result.success).toBe(true); + }); + + it("should return error result when array exceeds limit", () => { + const result = tryEnforceArrayLimit([1, 2, 3, 4, 5, 6], 5, "items"); + expect(result.success).toBe(false); + expect(result.error).toContain("E003"); + expect(result.error).toContain("Cannot add more than 5 items"); + expect(result.error).toContain("received 6"); + }); + + it("should return success when array is null or undefined", () => { + const result1 = tryEnforceArrayLimit(null, 5, "items"); + expect(result1.success).toBe(true); + + const result2 = tryEnforceArrayLimit(undefined, 5, "items"); + expect(result2.success).toBe(true); + }); + + it("should include parameter name in error message", () => { + const result = tryEnforceArrayLimit([1, 2, 3], 2, "labels"); + expect(result.success).toBe(false); + expect(result.error).toContain("labels"); + }); + }); +}); diff --git a/actions/setup/js/update_issue.cjs b/actions/setup/js/update_issue.cjs index b1bcfc6b4f..5b1a30d0d0 100644 --- a/actions/setup/js/update_issue.cjs +++ b/actions/setup/js/update_issue.cjs @@ -13,6 +13,17 @@ const { createUpdateHandlerFactory } = require("./update_handler_factory.cjs"); const { updateBody } = require("./update_pr_description_helpers.cjs"); const { loadTemporaryProjectMap, replaceTemporaryProjectReferences } = require("./temporary_id.cjs"); const { sanitizeTitle } = require("./sanitize_title.cjs"); +const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs"); + +/** + * Maximum limits for issue update parameters to prevent resource exhaustion. + * These limits align with GitHub's API constraints and security best practices. + */ +/** @type {number} Maximum number of labels allowed per issue */ +const MAX_LABELS = 10; + +/** @type {number} Maximum number of assignees allowed per issue */ +const MAX_ASSIGNEES = 5; /** * Execute the issue update API call @@ -144,6 +155,19 @@ function buildIssueUpdateData(item, config) { updateData.milestone = item.milestone; } + // Enforce max limits on labels and assignees before API calls + const labelsLimitResult = tryEnforceArrayLimit(updateData.labels, MAX_LABELS, "labels"); + if (!labelsLimitResult.success) { + core.warning(`Issue update limit exceeded: ${labelsLimitResult.error}`); + return { success: false, error: labelsLimitResult.error }; + } + + const assigneesLimitResult = tryEnforceArrayLimit(updateData.assignees, MAX_ASSIGNEES, "assignees"); + if (!assigneesLimitResult.success) { + core.warning(`Issue update limit exceeded: ${assigneesLimitResult.error}`); + return { success: false, error: assigneesLimitResult.error }; + } + // Pass footer config to executeUpdate (default to true) updateData._includeFooter = config.footer !== false; diff --git a/actions/setup/js/update_issue.test.cjs b/actions/setup/js/update_issue.test.cjs index 27b787999f..515b92eb07 100644 --- a/actions/setup/js/update_issue.test.cjs +++ b/actions/setup/js/update_issue.test.cjs @@ -610,4 +610,50 @@ describe("update_issue.cjs - allow_body configuration", () => { expect(result.data._operation).toBeUndefined(); expect(mockCore.warning).toHaveBeenCalledWith("Body update not allowed by safe-outputs configuration"); }); + + it("should enforce max labels limit (SEC-003)", async () => { + const { buildIssueUpdateData } = await import("./update_issue.cjs"); + + const item = { + labels: [ + "label1", + "label2", + "label3", + "label4", + "label5", + "label6", + "label7", + "label8", + "label9", + "label10", + "label11", // 11th label exceeds limit + ], + }; + + const config = {}; + + const result = buildIssueUpdateData(item, config); + + expect(result.success).toBe(false); + expect(result.error).toContain("E003"); + expect(result.error).toContain("Cannot add more than 10 labels"); + expect(result.error).toContain("received 11"); + }); + + it("should enforce max assignees limit (SEC-003)", async () => { + const { buildIssueUpdateData } = await import("./update_issue.cjs"); + + const item = { + assignees: ["user1", "user2", "user3", "user4", "user5", "user6"], // 6 assignees exceeds limit of 5 + }; + + const config = {}; + + const result = buildIssueUpdateData(item, config); + + expect(result.success).toBe(false); + expect(result.error).toContain("E003"); + expect(result.error).toContain("Cannot add more than 5 assignees"); + expect(result.error).toContain("received 6"); + }); });