[Safe Outputs] Add max limit enforcement to 7 core handlers (SEC-003)#15806
[Safe Outputs] Add max limit enforcement to 7 core handlers (SEC-003)#15806
Conversation
- create_issue.cjs: Add MAX_LABELS=10, MAX_ASSIGNEES=5 limits - create_discussion.cjs: Add MAX_LABELS=10 limit - create_pull_request.cjs: Add MAX_FILES=100 limit - update_issue.cjs: Add MAX_LABELS=10, MAX_ASSIGNEES=5 limits - add_labels.cjs: Add MAX_LABELS=10 limit with E003 error - update_discussion.cjs: No array params, no limits needed - assign_issue.cjs: Single assignee, no limits needed All handlers now throw E003 errors when limits exceeded per SEC-003 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Wrap all enforcement calls in try-catch to return error results - Add test for add_labels max limit enforcement - Add tests for create_issue max labels and assignees - Add tests for create_pull_request max file count - Export enforcePullRequestLimits for testing - All handlers now properly return E003 errors instead of throwing Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
actions/setup/js/add_labels.cjs
Outdated
| try { | ||
| enforceLabelLimits(requestedLabels); | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); |
There was a problem hiding this comment.
Replaced manual error message extraction (error instanceof Error ? error.message : String(error)) with getErrorMessage() helper across all handlers.
Commit: 6bacee9
- Create limit_enforcement_helpers.cjs with enforceArrayLimit and tryEnforceArrayLimit - Replace manual error handling with getErrorMessage helper - Update all 5 handlers to use new helpers (add_labels, create_issue, create_discussion, update_issue, create_pull_request) - Add comprehensive tests for limit enforcement helpers - Simplifies code and improves consistency across handlers Addresses @pelikhan feedback on PR review Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Created reusable helper in
Commit: 6bacee9 |
|
@copilot add tests |
|
@copilot continue work |
…ate_issue - Add test for create_discussion max labels limit (11 labels -> E003 error) - Add tests for update_issue max labels and assignees limits - Ensures comprehensive test coverage for all handlers with array limits - All tests pass (39 tests total including 2 new limit enforcement tests) Addresses @pelikhan feedback on test coverage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added comprehensive test coverage for SEC-003 max limit enforcement:
All tests pass. Total test coverage now includes:
Commit: 14bc93d |
|
🧪 Smoke Temporary ID is now testing temporary ID functionality... |
|
🧪 Smoke Project is now testing project operations... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
✅ Changeset Generator completed successfully! |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
❌ Smoke Copilot SDK failed. Please review the logs for details. |
|
✅ Smoke Temporary ID completed successfully. Temporary ID validation passed. |
Agent Container Tool Check ✅
Result: 12/12 tools available ✅ All required development tools are accessible in the agent container environment.
|
|
✅ Smoke Project completed successfully. All project operations validated. |
|
Smoke test results:
|
Smoke Test: 22028525097PRs Tested:
Results: ✅ GitHub MCP | ✅ GH CLI | ✅ Playwright | ✅ File I/O | ✅ Bash | ✅ Discussion | ✅ Build | ✅ Review | ✅ Dispatch Status: PASS cc
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Smoke test reviewed the SEC-003 max limit enforcement changes. Implementation looks solid with proper limit_enforcement_helpers usage across handlers.
📰 BREAKING: Report filed by Smoke Copilot for issue #15806
| 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"); |
There was a problem hiding this comment.
Good practice: importing repo helpers for cross-repository validation.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude for issue #15806
There was a problem hiding this comment.
Pull request overview
This pull request implements SEC-003 compliance by adding maximum limit enforcement to 7 core safe output handlers to prevent resource exhaustion attacks. The implementation introduces a reusable helper module (limit_enforcement_helpers.cjs) and applies limit checks to handlers that accept array parameters (labels, assignees, files), rejecting operations that exceed configured thresholds.
Changes:
- Created reusable limit enforcement helper functions (
enforceArrayLimitandtryEnforceArrayLimit) with comprehensive test coverage - Added max limit enforcement to 5 handlers with array parameters:
add_labels.cjs,create_issue.cjs,create_discussion.cjs,create_pull_request.cjs, andupdate_issue.cjs - Verified that
assign_issue.cjsandupdate_discussion.cjscorrectly handle single values or no arrays, requiring no changes - Added comprehensive tests for limit enforcement across all applicable handlers
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/limit_enforcement_helpers.cjs |
New helper module providing reusable enforceArrayLimit and tryEnforceArrayLimit functions for SEC-003 compliance |
actions/setup/js/limit_enforcement_helpers.test.cjs |
Comprehensive test coverage for the limit enforcement helper functions |
actions/setup/js/add_labels.cjs |
Added MAX_LABELS=10 enforcement before label validation |
actions/setup/js/add_labels.test.cjs |
Added test verifying 11 labels triggers limit error |
actions/setup/js/create_issue.cjs |
Added MAX_LABELS=10 and MAX_ASSIGNEES=5 enforcement before API calls |
actions/setup/js/create_issue.test.cjs |
Added tests for both labels and assignees limit enforcement |
actions/setup/js/create_discussion.cjs |
Added MAX_LABELS=10 enforcement for discussion labels |
actions/setup/js/create_discussion_labels.test.cjs |
Added test verifying label limit enforcement for discussions |
actions/setup/js/create_pull_request.cjs |
Added MAX_FILES=100 enforcement with dedicated function to count files in patch |
actions/setup/js/create_pull_request.test.cjs |
Added tests for file count limit enforcement with 100 and 101 file scenarios |
actions/setup/js/update_issue.cjs |
Added MAX_LABELS=10 and MAX_ASSIGNEES=5 enforcement for issue updates |
actions/setup/js/update_issue.test.cjs |
Added tests for both labels and assignees limit enforcement on updates |
.changeset/patch-enforce-safe-output-max-limits.md |
Changeset documenting the patch-level change for SEC-003 compliance |
Comments suppressed due to low confidence (6)
actions/setup/js/limit_enforcement_helpers.test.cjs:54
- The test expects error code E003, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. Please update the test expectations to check for E002 instead of E003.
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");
actions/setup/js/create_issue.test.cjs:434
- The test expects error code E003, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. Please update the test expectations to check for E002 instead of E003.
expect(result.error).toContain("E003");
expect(result.error).toContain("Cannot add more than 5 assignees");
expect(result.error).toContain("received 6");
actions/setup/js/create_pull_request.cjs:51
- The error code E003 is used for limit enforcement, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. E003 is defined as UNAUTHORIZED_DOMAIN for URL domain filtering. Please update to use E002 instead.
throw new Error(`E003: Cannot create pull request with more than ${MAX_FILES} files (received ${fileCount})`);
actions/setup/js/update_issue.test.cjs:657
- The test expects error code E003, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. Please update the test expectations to check for E002 instead of E003.
expect(result.error).toContain("E003");
expect(result.error).toContain("Cannot add more than 5 assignees");
expect(result.error).toContain("received 6");
actions/setup/js/limit_enforcement_helpers.cjs:15
- The JSDoc comment states "Throws E003 error" but should say "Throws E002 error" to match the correct error code for LIMIT_EXCEEDED according to the Safe Outputs Specification.
* Throws E003 error when the limit is exceeded.
*
* @param {Array<any>|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
actions/setup/js/create_pull_request.test.cjs:94
- The strings use double quotes instead of backticks, which means the variable i will not be interpolated. The lines should use template literals (backticks) instead of regular strings to properly interpolate the file index. This will cause the test to generate 100 identical patch entries instead of 100 different files, which may not properly test the file counting logic.
patchLines.push("--- a/file${i}.txt");
patchLines.push("+++ b/file${i}.txt");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("should throw E003 error when array exceeds limit", () => { | ||
| expect(() => enforceArrayLimit([1, 2, 3, 4, 5, 6], 5, "items")).toThrow("E003"); |
There was a problem hiding this comment.
The test expects error code E003, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. Please update the test expectation to check for E002 instead of E003.
This issue also appears on line 51 of the same file.
| expect(result.error).toContain("E003"); | ||
| expect(result.error).toContain("Cannot add more than 10 labels"); | ||
| expect(result.error).toContain("received 11"); |
There was a problem hiding this comment.
The test expects error code E003, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. Please update the test expectations to check for E002 instead of E003.
This issue also appears on line 432 of the same file.
| 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"); |
There was a problem hiding this comment.
The strings in lines 68-71 use double quotes instead of backticks, which prevents variable interpolation. All instances of "...\${i}..." should be changed to template literals using backticks: `...${i}...`. Without this, the test generates 101 identical "diff --git a/file${i}.txt b/file${i}.txt" lines instead of 101 unique files, which means the file counting logic may not be properly tested. The regex pattern ^diff --git /gm would still match 101 times, so the test might pass, but it's testing the wrong thing.
This issue also appears on line 93 of the same file.
| * 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 |
There was a problem hiding this comment.
The JSDoc comment references "error code E003" but should reference "error code E002" to match the correct error code for LIMIT_EXCEEDED according to the Safe Outputs Specification.
This issue also appears on line 51 of the same file.
| * Throws E003 error when the limit is exceeded. | ||
| * | ||
| * @param {Array<any>|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})`); |
There was a problem hiding this comment.
The error code E003 is used for limit enforcement, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. E003 is defined as UNAUTHORIZED_DOMAIN for URL domain filtering.
The specification states:
- E002: LIMIT_EXCEEDED - "Operation count exceeds configured max"
- E003: UNAUTHORIZED_DOMAIN - "URL contains non-allowlisted domain"
This inconsistency should be corrected to use E002 instead of E003 for array limit enforcement across all handlers and helper functions.
This issue also appears on line 10 of the same file.
| * Throws E003 error when the limit is exceeded. | |
| * | |
| * @param {Array<any>|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})`); | |
| * Throws E002 error when the limit is exceeded. | |
| * | |
| * @param {Array<any>|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 E002 error code | |
| */ | |
| function enforceArrayLimit(array, maxLimit, parameterName) { | |
| if (array && Array.isArray(array) && array.length > maxLimit) { | |
| throw new Error(`E002: Cannot add more than ${maxLimit} ${parameterName} (received ${array.length})`); |
| const result = await handler(message, {}); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.error).toContain("E003"); |
There was a problem hiding this comment.
The test expects error code E003, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. Please update the test expectations to check for E002 instead of E003.
| expect(result.error).toContain("E003"); | |
| expect(result.error).toContain("E002"); |
| expect(result.error).toContain("E003"); | ||
| expect(result.error).toContain("Cannot add more than 10 labels"); | ||
| expect(result.error).toContain("received 11"); |
There was a problem hiding this comment.
The test expects error code E003, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. Please update the test expectations to check for E002 instead of E003.
This issue also appears on line 655 of the same file.
| // Should throw E003 error | ||
| expect(() => enforcePullRequestLimits(patchContent)).toThrow("E003"); |
There was a problem hiding this comment.
The test expects error code E003, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. Please update the test expectations to check for E002 instead of E003.
| // Should throw E003 error | |
| expect(() => enforcePullRequestLimits(patchContent)).toThrow("E003"); | |
| // Should throw E002 error | |
| expect(() => enforcePullRequestLimits(patchContent)).toThrow("E002"); |
| ); | ||
|
|
||
| expect(result.success).toBe(false); | ||
| expect(result.error).toContain("E003"); |
There was a problem hiding this comment.
The test expects error code E003, but according to the Safe Outputs Specification (section 9.5 Error Code Catalog), E002 should be used for LIMIT_EXCEEDED errors. Please update the test expectations to check for E002 instead of E003.
| expect(result.error).toContain("E003"); | |
| expect(result.error).toContain("E002"); |
create_issue.cjs(labels, assignees)create_discussion.cjs(labels)create_pull_request.cjs(files)update_issue.cjs(labels, assignees)update_discussion.cjs(no arrays)add_labels.cjs(explicit E003 enforcement)assign_issue.cjs(single assignee per call)make agent-finishbefore completionSummary: All 7 core handlers now enforce max limits with E003 errors per SEC-003 specification using reusable helpers. Comprehensive test coverage includes limit enforcement tests for all handlers with array parameters. Conformance check passes for all target handlers.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
[Safe Outputs] Add max limit enforcement to 7 core handlers (SEC-003)
create_issue.cjs(labels, assignees)create_discussion.cjs(labels)create_pull_request.cjs(files)update_issue.cjs(labels, assignees)update_discussion.cjs(no arrays)add_labels.cjs(explicit E003 enforcement)assign_issue.cjs(single assignee per call)make agent-finishbefore completionSummary: All 7 core handlers now enforce max limits with E003 errors per SEC-003 specification using reusable helpers. Comprehensive test coverage includes limit enforcement tests for all handlers with array parameters. Conformance check passes for all target handlers.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips]((gh.io/redacted) in the docs.
Changeset