Fix SEC-003 false positives in safe-outputs conformance check#17790
Fix SEC-003 false positives in safe-outputs conformance check#17790
Conversation
…t patterns Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix false positives in the SEC-003 conformance check by scoping it to only check files that perform GitHub API operations and updating the grep pattern to recognize the project's actual limit enforcement functions.
Changes:
- Added a filter to only check files containing
octokit.calls (lines 114-117) - Updated the grep pattern to include
enforceArrayLimit,tryEnforceArrayLimit, andlimit_enforcement_helpers(line 120) - Moved test/utility file exclusions before the octokit scope check (line 112)
Comments suppressed due to low confidence (1)
scripts/check-safe-outputs-conformance.sh:120
- The grep pattern updates on line 120 correctly add the enforcement function names (enforceArrayLimit, tryEnforceArrayLimit, limit_enforcement_helpers), but they will never be evaluated because line 115 filters out all handler files. Since no files match the pattern
octokit.(handlers usegithub.rest.andgithub.graphqlinstead), the check will skip all files and incorrectly report a pass. Additionally, the existing SEC-002 check has the same issue on line 90.
if ! grep -qE "\.length.*>.*\.max|enforceMaxLimit|checkLimit|max.*exceeded|enforceArrayLimit|tryEnforceArrayLimit|limit_enforcement_helpers" "$handler"; then
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Check if handler enforces max limits | ||
| if ! grep -q "\.length.*>.*\.max\|enforceMaxLimit\|checkLimit\|max.*exceeded" "$handler"; then | ||
| # Only check files that perform GitHub API operations | ||
| if ! grep -q "octokit\." "$handler"; then |
There was a problem hiding this comment.
The pattern octokit. does not match the actual GitHub API usage pattern in this codebase. Handler files use github.rest. and github.graphql (where github is a global variable from @actions/github-script), not octokit.. This means the scope filter will match zero files, causing SEC-003 to report a false pass even though it's not checking any handlers. The same issue exists in SEC-002 (line 90). The pattern should be changed to match actual API calls, such as github\.rest\.|github\.graphql or simply \.rest\.|\.graphql.
This issue also appears on line 120 of the same file.
octokit.calls (like SEC-002)enforceArrayLimit,tryEnforceArrayLimit, andlimit_enforcement_helpersbash scripts/check-safe-outputs-conformance.shreports[PASS] SEC-003: All handlers enforce max limitsOriginal prompt
This section details on the original issue you should resolve
<issue_title>[Safe Outputs Conformance] SEC-003: Max limit enforcement check produces 198 false positives due to overly broad scope and unrecognized enforcement pattern</issue_title>
<issue_description>### Conformance Check Failure
Check ID: SEC-003
Severity: MEDIUM
Category: Security / Check Design
Problem Description
The SEC-003 check in
scripts/check-safe-outputs-conformance.shcurrently flags 198 files as potentially not enforcing max limits. This high count is caused by two compounding issues:Overly broad file scope: The check iterates over all
*.cjsfiles inactions/setup/js/, which includes fuzz test harnesses, utility modules, sanitizers, MCP transport files, constants, and other non-handler files that have no responsibility for enforcing max limits.Unrecognized enforcement pattern: The project's actual max limit enforcement is implemented via
enforceArrayLimit()andtryEnforceArrayLimit()inactions/setup/js/limit_enforcement_helpers.cjs. The SEC-003 check pattern (\.length.*>.*\.max\|enforceMaxLimit\|checkLimit\|max.*exceeded) does not match these function names, so any handler that correctly delegates to the sharedlimit_enforcement_helpers.cjsmodule is still flagged.The result is that the check cannot distinguish files that correctly use the shared enforcement helper from files that genuinely lack any limit enforcement. This makes the check ineffective for its intended security purpose.
Affected Components
scripts/check-safe-outputs-conformance.sh(lines 106–123,check_max_limitsfunction)actions/setup/js/limit_enforcement_helpers.cjs(exportsenforceArrayLimit,tryEnforceArrayLimit)actions/setup/js/excluding those matchingtest|parse|buffer|factory— including utility, sanitizer, MCP transport, fuzz harness, and message filesView representative false-positive categories
fuzz_markdown_code_region_balancer_harness.cjs,fuzz_mentions_harness.cjs,fuzz_sanitize_incoming_text_harness.cjssanitize_content.cjs,sanitize_output.cjs,sanitize_title.cjsmcp_http_transport.cjs,mcp_server_core.cjs,mcp_logger.cjsmessages.cjs,messages_footer.cjs,messages_staged.cjsconstants.cjs,error_codes.cjs,is_truthy.cjs,git_helpers.cjslimit_enforcement_helpers.cjsCurrent Behavior
Files that correctly
require('./limit_enforcement_helpers')and callenforceArrayLimit()are still flagged because the grep pattern does not recognize those function names.Expected Behavior
The SEC-003 check should:
octokit).enforceArrayLimit,tryEnforceArrayLimit, as well as anyrequire.*limit_enforcement_helpersimport.Remediation Steps
This task can be assigned to a Copilot coding agent with the following steps:
octokit.calls (consistent with how SEC-002 scopes its check).Verification
After remediation, verify the fix by running:
SEC-003 should report significantly fewer failures, limited to files with genuine missing enforcement.
References
docs/src/content/docs/reference/safe-outputs-specification.mdscripts/check-safe-outputs-conformance.sh(lines 106–123)actions/setup/js/limit_enforcement_helpers.cjs✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.