Add max limit enforcement to add_comment handler (SEC-003)#15745
Add max limit enforcement to add_comment handler (SEC-003)#15745
Conversation
- Add MAX_COMMENT_LENGTH (65536), MAX_MENTIONS (10), MAX_LINKS (50) constants - Implement enforceCommentLimits() function with E006/E007/E008 error codes - Validate comment body before and after adding footer/metadata - Add comprehensive tests for limit enforcement (24 new test cases) - Export constants and function for testing - All tests pass (42 tests total) - SEC-003 conformance check now passes for add_comment.cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| } | ||
|
|
||
| // Count mentions (@username pattern) - max limit exceeded check | ||
| const mentions = (body.match(/@\w+/g) || []).length; |
There was a problem hiding this comment.
@copilot use mentions parser from the mentions sanitizer
There was a problem hiding this comment.
Pull request overview
This PR adds resource limit enforcement to the add_comment.cjs handler to satisfy Safe Outputs specification requirement SEC-003. The implementation validates comment body length, mentions, and links before GitHub API calls to prevent resource exhaustion attacks.
Changes:
- Implements
enforceCommentLimits()function with validation for comment length (65,536 chars), mentions (10), and links (50) - Adds double validation: once after temp ID replacement (user content) and once after adding footer/metadata (final output)
- Exports constants and validation function for testing
- Adds comprehensive test suite with 24 test cases covering boundary conditions, error messages, and edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| actions/setup/js/add_comment.cjs | Adds MAX_* constants, enforceCommentLimits() function with "max limit exceeded" comments, and dual validation checkpoints before API calls |
| actions/setup/js/add_comment.test.cjs | Adds 24 test cases covering limit enforcement, boundary values, error messages, and edge cases using eval() pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new Error(`E006: Comment body exceeds maximum length of ${MAX_COMMENT_LENGTH} characters (got ${body.length})`); | ||
| } | ||
|
|
||
| // Count mentions (@username pattern) - max limit exceeded check | ||
| const mentions = (body.match(/@\w+/g) || []).length; | ||
| if (mentions > MAX_MENTIONS) { | ||
| throw new Error(`E007: Comment contains ${mentions} mentions, maximum is ${MAX_MENTIONS}`); | ||
| } | ||
|
|
||
| // Count links (http:// and https:// URLs) - max limit exceeded check | ||
| const links = (body.match(/https?:\/\/[^\s]+/g) || []).length; | ||
| if (links > MAX_LINKS) { | ||
| throw new Error(`E008: Comment contains ${links} links, maximum is ${MAX_LINKS}`); |
There was a problem hiding this comment.
The error codes used (E006, E007, E008) don't match their definitions in the Safe Outputs specification (docs/src/content/docs/reference/safe-outputs-specification.md:2885-2894). According to the spec:
- E006 is defined as INVALID_LABEL
- E007 is defined as API_ERROR
- E008 is defined as SANITIZATION_FAILED
However, they're being used here for resource limit violations (length exceeded, mentions exceeded, links exceeded). This creates confusion and could cause issues if these error codes are used for their documented purposes elsewhere. Consider either updating the specification to document these as resource limit error codes, or using different error codes (E011-E013) that don't conflict with existing definitions.
| throw new Error(`E006: Comment body exceeds maximum length of ${MAX_COMMENT_LENGTH} characters (got ${body.length})`); | |
| } | |
| // Count mentions (@username pattern) - max limit exceeded check | |
| const mentions = (body.match(/@\w+/g) || []).length; | |
| if (mentions > MAX_MENTIONS) { | |
| throw new Error(`E007: Comment contains ${mentions} mentions, maximum is ${MAX_MENTIONS}`); | |
| } | |
| // Count links (http:// and https:// URLs) - max limit exceeded check | |
| const links = (body.match(/https?:\/\/[^\s]+/g) || []).length; | |
| if (links > MAX_LINKS) { | |
| throw new Error(`E008: Comment contains ${links} links, maximum is ${MAX_LINKS}`); | |
| throw new Error(`E011: Comment body exceeds maximum length of ${MAX_COMMENT_LENGTH} characters (got ${body.length})`); | |
| } | |
| // Count mentions (@username pattern) - max limit exceeded check | |
| const mentions = (body.match(/@\w+/g) || []).length; | |
| if (mentions > MAX_MENTIONS) { | |
| throw new Error(`E012: Comment contains ${mentions} mentions, maximum is ${MAX_MENTIONS}`); | |
| } | |
| // Count links (http:// and https:// URLs) - max limit exceeded check | |
| const links = (body.match(/https?:\/\/[^\s]+/g) || []).length; | |
| if (links > MAX_LINKS) { | |
| throw new Error(`E013: Comment contains ${links} links, maximum is ${MAX_LINKS}`); |
| } | ||
|
|
||
| // Count mentions (@username pattern) - max limit exceeded check | ||
| const mentions = (body.match(/@\w+/g) || []).length; |
There was a problem hiding this comment.
The mention regex /@\w+/g doesn't match GitHub's actual username requirements. It will:
- Match all-numeric patterns like
@123(which aren't valid GitHub usernames and conflict with issue numbers) - Miss valid usernames containing hyphens like
@octo-cat
GitHub usernames must start with a letter and can contain letters, numbers, and hyphens. Consider using a more accurate pattern like /@[a-zA-Z][a-zA-Z0-9-]*/g to match actual GitHub mention syntax.
| const mentions = (body.match(/@\w+/g) || []).length; | |
| const mentions = (body.match(/@[a-zA-Z][a-zA-Z0-9-]*/g) || []).length; |
The
add_comment.cjshandler was missing resource limit validation, allowing unbounded comment bodies, mentions, and links. This violates Safe Outputs specification requirement MR3 (limit enforcement before API calls).Changes
Limit constants:
MAX_COMMENT_LENGTH = 65536(GitHub API limit)MAX_MENTIONS = 10MAX_LINKS = 50Validation function:
Validation points:
Tests:
Result:
add_comment.cjsWarning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
https://api.github.com/repos/github/gh-aw/contents/.github%2Fworkflows%2Faudit-workflows.md/opt/hostedtoolcache/node/24.13.0/x64/bin/node /opt/hostedtoolcache/node/24.13.0/x64/bin/node --conditions node --conditions development --experimental-import-meta-resolve --require /home/REDACTED/work/gh-aw/gh-aw/actions/setup/js/node_modules/vitest/suppress-warnings.cjs /home/REDACTED/work/gh-aw/gh-aw/actions/setup/js/node_modules/vitest/dist/workers/forks.js /mark_pull_requecommit bin/bash grep -A elism /safe_inputs_config_loader.cjs /usr/bin/grep 20 ^\s*agent: /usr/bin/grep grep(http block)If you need me to access, download, or install something from one of these locations, you can either:
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.