-
Notifications
You must be signed in to change notification settings - Fork 528
Apply mention limits to output content instead of allowlists #57747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
3d5df12
8fe203e
0046d5a
1a714b0
490c838
92deabd
e09d9df
e67ee15
2f85a7f
a2d2e21
0f1e40e
42cff0c
7ece1b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -191,7 +191,6 @@ async function resolveAllowedMentionsFromPayload(context, github, core, mentions | |
| const allowContext = mentionsConfig?.allowContext !== false; // default: true | ||
| const allowedList = mentionsConfig?.allowed || []; | ||
| const allowedTeams = mentionsConfig?.allowedTeams || []; | ||
| const maxMentions = mentionsConfig?.max || 50; | ||
|
|
||
| try { | ||
| const { owner, repo } = context.repo; | ||
|
|
@@ -234,23 +233,14 @@ async function resolveAllowedMentionsFromPayload(context, github, core, mentions | |
| // If collaborator mentions are disabled, only use known authors (context + allowed list) | ||
| if (!allowCollaboratorMentions) { | ||
| core.info(`[MENTIONS] Collaborator mentions disabled - only allowing context (${deduplicatedKnownAuthors.length} users)`); | ||
| if (deduplicatedKnownAuthors.length > maxMentions) { | ||
| core.warning(`[MENTIONS] Mention limit exceeded: ${deduplicatedKnownAuthors.length} mentions, limiting to ${maxMentions}`); | ||
| } | ||
| return deduplicatedKnownAuthors.slice(0, maxMentions); | ||
| return deduplicatedKnownAuthors; | ||
|
Comment on lines
233
to
+236
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 92deabd: all built-in final-write handlers and synthetic-update helpers parse and pass |
||
| } | ||
|
|
||
| // Build allowed mentions list from known authors and collaborators | ||
| // We pass the known authors as fake mentions in text so they get processed | ||
| const fakeText = deduplicatedKnownAuthors.map(author => `@${author}`).join(" "); | ||
| const mentionResult = await resolveMentionsLazily(fakeText, deduplicatedKnownAuthors, owner, repo, github, core); | ||
| let allowedMentions = mentionResult.allowedMentions; | ||
|
|
||
| // Apply max limit | ||
| if (allowedMentions.length > maxMentions) { | ||
| core.warning(`[MENTIONS] Mention limit exceeded: ${allowedMentions.length} mentions, limiting to ${maxMentions}`); | ||
| allowedMentions = allowedMentions.slice(0, maxMentions); | ||
| } | ||
| const allowedMentions = mentionResult.allowedMentions; | ||
|
|
||
| if (allowedMentions.length > 0) { | ||
| core.info(`[OUTPUT COLLECTOR] Allowed mentions: ${allowedMentions.join(", ")}`); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ const ISSUE_INTENT_RATIONALE_MAX_LENGTH = 280; | |
| /** | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actions/setup/js/safe_output_type_validator.cjs:31-39: yagni: generic ValidateOptions plumbing for a single maxMentions knob. Inline the single sanitizeContent call site instead of threading a new option through the validator layer.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not applying this simplification: validation sanitizes several fields and arrays for each safe-output type. Passing the existing validation options is necessary to share the per-item cap across those fields without changing validation behavior. |
||
| * @typedef {{ | ||
| * allowedAliases?: string[], | ||
| * maxMentions?: number, | ||
| * maxBotMentions?: number, | ||
| * normalizeIssueClosingKeywords?: boolean, | ||
| * dataEnabled?: boolean, | ||
|
|
@@ -90,6 +91,7 @@ function normalizeIssueIntentRationale(rationale, options) { | |
| const sanitizedRationale = sanitizeContent(unfenceMarkdown(rationale), { | ||
| maxLength: ISSUE_INTENT_RATIONALE_MAX_LENGTH, | ||
| allowedAliases: options?.allowedAliases || [], | ||
| maxMentions: options?.maxMentions, | ||
| maxBotMentions: options?.maxBotMentions, | ||
| }).trim(); | ||
| // sanitizeContent appends "\n[Content truncated due to length]" when it truncates, | ||
|
|
@@ -116,6 +118,7 @@ function validateIssueIntentLabels(value, lineNum, itemType, fieldName, options) | |
| const name = sanitizeContent(label, { | ||
| maxLength: 128, | ||
| allowedAliases: options?.allowedAliases || [], | ||
| maxMentions: options?.maxMentions, | ||
| maxBotMentions: options?.maxBotMentions, | ||
| }); | ||
| if (!name) { | ||
|
|
@@ -149,6 +152,7 @@ function validateIssueIntentLabels(value, lineNum, itemType, fieldName, options) | |
| const name = sanitizeContent(label.name, { | ||
| maxLength: 128, | ||
| allowedAliases: options?.allowedAliases || [], | ||
| maxMentions: options?.maxMentions, | ||
| maxBotMentions: options?.maxBotMentions, | ||
| }); | ||
| if (!name) { | ||
|
|
@@ -544,6 +548,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options) | |
| normalizedResult = sanitizeContent(normalizedResult, { | ||
| maxLength: validation.maxLength, | ||
| allowedAliases: options?.allowedAliases || [], | ||
| maxMentions: options?.maxMentions, | ||
| maxBotMentions: options?.maxBotMentions, | ||
| }); | ||
| } | ||
|
|
@@ -557,6 +562,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options) | |
| finalValue = sanitizeContent(unfenceMarkdown(value), { | ||
| maxLength: validation.maxLength || MAX_BODY_LENGTH, | ||
| allowedAliases: options?.allowedAliases || [], | ||
| maxMentions: options?.maxMentions, | ||
| maxBotMentions: options?.maxBotMentions, | ||
| }); | ||
| } | ||
|
|
@@ -635,6 +641,7 @@ function validateField(value, fieldName, validation, itemType, lineNum, options) | |
| ? sanitizeContent(item, { | ||
| maxLength: validation.itemMaxLength || 128, | ||
| allowedAliases: options?.allowedAliases || [], | ||
| maxMentions: options?.maxMentions, | ||
| maxBotMentions: options?.maxBotMentions, | ||
| }) | ||
| : item | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,7 @@ const RUNTIME_TO_MENTION_ALIAS_MAP = { | |
| * @typedef {Object} SanitizeOptions | ||
| * @property {number} [maxLength] - Maximum length of content (default: 524288) | ||
| * @property {string[]} [allowedAliases] - List of aliases (@mentions) that should not be neutralized | ||
| * @property {number} [maxMentions] - Maximum number of unique allowed aliases to preserve | ||
| * @property {number} [maxBotMentions] - Maximum bot trigger references before filtering (default: 10) | ||
|
Comment on lines
41
to
46
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 92deabd: |
||
| */ | ||
|
|
||
|
|
@@ -57,6 +58,8 @@ function sanitizeContent(content, maxLengthOrOptions) { | |
| /** @type {string[]} */ | ||
| let allowedAliasesLowercase = []; | ||
| /** @type {number | undefined} */ | ||
| let maxMentions; | ||
| /** @type {number | undefined} */ | ||
| let maxBotMentions; | ||
|
|
||
| if (typeof maxLengthOrOptions === "number") { | ||
|
|
@@ -66,6 +69,7 @@ function sanitizeContent(content, maxLengthOrOptions) { | |
| // Pre-process allowed aliases to lowercase for efficient comparison | ||
| const normalizedAllowedAliases = normalizeAllowedAliases(maxLengthOrOptions.allowedAliases); | ||
| allowedAliasesLowercase = expandAllowedAliases(normalizedAllowedAliases); | ||
| maxMentions = maxLengthOrOptions.maxMentions; | ||
| maxBotMentions = maxLengthOrOptions.maxBotMentions; | ||
| } | ||
|
|
||
|
|
@@ -124,7 +128,7 @@ function sanitizeContent(content, maxLengthOrOptions) { | |
|
|
||
| // Neutralize mentions after truncation so the length boundary cannot split an | ||
| // inserted code-span delimiter and reactivate a mention. | ||
| sanitized = neutralizeMentions(sanitized, allowedAliasesLowercase); | ||
| sanitized = neutralizeMentions(sanitized, allowedAliasesLowercase, maxMentions); | ||
|
|
||
| // Neutralize GitHub references if restrictions are configured | ||
| sanitized = neutralizeGitHubReferences(sanitized, allowedGitHubRefs); | ||
|
|
@@ -185,18 +189,23 @@ function sanitizeContent(content, maxLengthOrOptions) { | |
| * Neutralize @mentions with selective filtering | ||
| * @param {string} s - The string to process | ||
| * @param {string[]} allowedLowercase - List of allowed aliases (lowercase) | ||
| * @param {number | undefined} maxAllowed - Maximum number of unique allowed aliases to preserve | ||
| * @returns {string} Processed string | ||
| */ | ||
| function neutralizeMentions(s, allowedLowercase) { | ||
| function neutralizeMentions(s, allowedLowercase, maxAllowed) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actions/setup/js/sanitize_content.cjs:195-205: yagni: per-call mention-count state inside sanitizeContent. Drop the extra maxMentions limiter and let the caller pre-filter aliases once.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not applying this suggestion: pre-filtering aliases reintroduces the reported defect by making allowlist order decide authorization. The limiter must evaluate approved identities as they occur in the output; its state is intentionally local to a message/item. |
||
| const wrapInCodeSpan = createRenderSafeCodeSpanWrapper(s); | ||
| const allowedAliasesSeen = new Set(); | ||
| return applyToNonCodeRegions(s, (segment, regionBefore = "", regionAfter = "") => { | ||
| return segment.replace(/(^|[^A-Za-z0-9])@([A-Za-z0-9](?:[A-Za-z0-9_-]{0,37}[A-Za-z0-9])?(?:\/[A-Za-z0-9._-]+)?)/g, (match, prefix, alias, offset) => { | ||
| const isAllowed = allowedLowercase.includes(alias.toLowerCase()); | ||
| if (isAllowed) { | ||
| const normalizedAlias = alias.toLowerCase(); | ||
| const isAllowed = allowedLowercase.includes(normalizedAlias); | ||
| const isWithinLimit = maxAllowed === undefined || allowedAliasesSeen.has(normalizedAlias) || allowedAliasesSeen.size < maxAllowed; | ||
| if (isAllowed && isWithinLimit) { | ||
| allowedAliasesSeen.add(normalizedAlias); | ||
| return `${prefix}@${alias}`; | ||
| } | ||
| if (typeof core !== "undefined" && core.info) { | ||
| core.info(`Escaped mention: @${alias} (not in allowed list)`); | ||
| core.info(isAllowed ? `Escaped mention: @${alias} (mention limit exceeded)` : `Escaped mention: @${alias} (not in allowed list)`); | ||
| } | ||
| const before = prefix || (offset === 0 ? regionBefore : ""); | ||
| const after = segment[offset + match.length] || (offset + match.length === segment.length ? regionAfter : ""); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed: this is already addressed by 92deabd.
validateItemnow creates oneallowedAliasesSeenset for the output item and supplies it to each field sanitizer; the collector’s safe-job path does the same. The regression test verifies the cap spans title and body while allowing a repeated approved identity.