Repository navigation
Normalize labels before security filtering - #74752
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The validation-order change introduces a high-severity bypass for blocked @mention labels: sanitization rewrites the value before matching, so existing blocked patterns can stop matching. Please preserve raw-form security filtering while returning sanitized labels.
Highlights
- Blocked-pattern matching must remain effective for labels transformed by mention neutralization.
- Add regression coverage for blocked
@user-stylepatterns.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| // Sanitize labels before applying security-sensitive filters | ||
| let validLabels = labels | ||
| .filter(label => label != null && label !== false && label !== 0) | ||
| .map(label => sanitizeLabelContent(String(label).trim())) |
There was a problem hiding this comment.
This reordering lets blocked @mention labels bypass the blocked-pattern check because sanitizeLabelContent wraps @name as @name, so a configured blocked pattern like @name no longer matches and the label is emitted.
💡 Preserve the raw value for security filtering
Run blocked-pattern checks against the trimmed raw label as well as the sanitized label (or otherwise reject when either form matches), while still returning the sanitized value. Add a regression test for a blocked @user pattern. This change is intended to harden the security boundary, but the sanitizer's mention-neutralization creates a new mismatch with existing blocked-pattern configurations.
There was a problem hiding this comment.
Updated in d4918e8: blocked-pattern checks now match both the trimmed raw label and the sanitized, truncated value, while returning only the sanitized value.
There was a problem hiding this comment.
What this change does: It normalizes each label before checking removal, blocked, and allowed patterns. It also adds regression coverage for zero-width and fullwidth spellings of a blocked label.
Nice to have:
-
The two regression cases duplicate one test (
actions/setup/js/safe_output_validator.test.cjs:L293-L298)- What this is: Both tests call
validateLabelswith the same options and assert the same["bug"]result; only the Unicode input changes. - Problem: The setup and assertions are repeated, leaving two copies to maintain for one behavior.
- Fix: Use one parameterized
it.eachcase with the two inputs and the shared call and assertions. - If we skip it: The tests work, but each additional normalization variant needs another copy of the same seven lines.
- What this is: Both tests call
Verdict: Ship.
Lean: -7 lines possible.
Generated by ✂️ Ponytail Reviewer for #74752 · codex · routed: gpt56 max · ~4 AIC · ⌖ ~1 AIC · ⊞ 118
Comment /ponytail to run again
| expect(result.value).toEqual(["bug"]); | ||
| }); | ||
|
|
||
| it("should block fullwidth labels that sanitize to a blocked label", () => { |
There was a problem hiding this comment.
Nice to have: these two cases repeat the same setup and assertions; parameterize one test over the zero-width and fullwidth inputs to remove 7 lines while keeping both regression cases and one shared expected result.
There was a problem hiding this comment.
Updated in d4918e8: the zero-width and fullwidth regression cases are now parameterized with shared setup and assertions.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. Requesting changes: the fix closes the zero-width and fullwidth bypasses, but it changes how blocked and allowed patterns match.
📋 Key Themes & Highlights
Key Themes
- Regression in pattern matching: Patterns are compared only against the sanitized label. Literal entries with
@,<,>,&,'or"stop matching, on both the blocked and allowed lists. Confirmed by running the base and head validators side by side. - Test gap: The two new tests use plain-word patterns, so they cannot catch this.
- Scope: The full vitest suite could not run in this environment (vitest is not installed and npm fails on TLS here). The new assertions pass in a standalone check, but the existing
safe_output_validator.test.cjssuite still needs a run.
Positive Highlights
- ✅ Validation order is now correct: sanitize, then removal, blocklist and allowlist checks.
- ✅ The new zero-width and fullwidth tests describe the bypass clearly.
- ✅ Deduplication and truncation are unchanged and still applied after filtering.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~4 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
| const labelStr = String(label).trim(); | ||
| const isBlocked = blockedPatterns.some(pattern => matchesSimpleGlob(labelStr, pattern)); | ||
| validLabels = validLabels.filter(label => { | ||
| const isBlocked = blockedPatterns.some(pattern => matchesSimpleGlob(label, pattern)); |
There was a problem hiding this comment.
[/diagnosing-bugs] Blocked-pattern matching now runs only on the sanitized label, but the configured patterns are not sanitized. Literal entries containing @, ', <, >, & or " no longer match. Example: validateLabels(["bug", "@copilot"], undefined, 10, ["@copilot"]) returns ["bug", "@copilot"] on this branch but ["bug"] before the change. The blocklist is weaker for any operator who configures such entries.
💡 Suggested fix
Match each pattern against both the trimmed raw label and the sanitized label, and keep the sanitized value as the output. Matching either form only tightens the blocklist. The zero-width and fullwidth cases still block because the sanitized form matches.
const matchesAny = (raw, value, patterns) => patterns.some(p => matchesSimpleGlob(raw, p) || matchesSimpleGlob(value, p));I checked this in a scratch copy. It keeps the new zero-width and fullwidth blocks and restores the pre-change result for @copilot, <tag> and it's.
@copilot please address this.
| const labelStr = String(label).trim(); | ||
| return allowedLabels.some(pattern => matchesSimpleGlob(labelStr, pattern)); | ||
| }); | ||
| validLabels = validLabels.filter(label => allowedLabels.some(pattern => matchesSimpleGlob(label, pattern))); |
There was a problem hiding this comment.
[/diagnosing-bugs] The same regression reaches the allowed list. validateLabels(["@bot"], ["@bot"], 10) returned [" @bot "] before this change. It now fails with "No valid labels found after sanitization", because the sanitized `@bot` never matches the raw pattern. Apply the raw-or-sanitized match here too, and cover it with a test.
@copilot please address this.
| }); | ||
|
|
||
| it("should block labels containing zero-width characters that sanitize to a blocked label", () => { | ||
| const result = validator.validateLabels(["bug", "block\u200Bed"], undefined, 10, ["blocked"]); |
There was a problem hiding this comment.
[/tdd] These tests use plain-word patterns, so they cannot catch the regressions above. Add cases where the pattern itself contains a character the sanitizer changes, and assert the expected result for both blocked and allowed lists.
💡 Suggested test
it("should still block literal blocked patterns containing sanitized characters", () => {
const result = validator.validateLabels(["bug", "@copilot"], undefined, 10, ["@copilot"]);
expect(result.value).toEqual(["bug"]);
});@copilot please address this.
There was a problem hiding this comment.
Added in d4918e8: regression tests cover transformed @mention patterns in blocked and allowed lists, alongside Unicode normalization and truncation.
There was a problem hiding this comment.
🟡 Changes recommended
Label truncation occurs after policy checks, allowing a validated value to become a blocked label that still reaches the label handlers.
1 open issue
What changed in this PR
Updates gh-aw’s shared safe-output label validator to address the Unicode-based blocked-label bypass reported in #74548.
Changes:
- Sanitizes labels before removal detection and blocked/allowed pattern checks.
- Adds regression tests for zero-width and fullwidth variants of blocked labels.
| File | Description |
|---|---|
actions/setup/js/safe_output_validator.test.cjs |
Adds Unicode blocked-label regression cases. |
actions/setup/js/safe_output_validator.cjs |
Moves label sanitization ahead of security-sensitive filters. |
Review effort: Max (auto)
| // Sanitize labels before applying security-sensitive filters | ||
| let validLabels = labels | ||
| .filter(label => label != null && label !== false && label !== 0) | ||
| .map(label => sanitizeLabelContent(String(label).trim())) |
There was a problem hiding this comment.
Fixed in d4918e8: labels are sanitized and truncated to 64 code units before blocked/allowed checks. Added an expansion-and-truncation regression where raw input does not match but the resulting value does.
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74772 PR FinisherSubmitted for publication (not yet confirmed pushed): merge of Validation: ran Review replies/resolutions deferred until the push is published. CI needs a maintainer re-trigger. PR finisher dispatch requested for the following outstanding work: (see Sous-chef lines below) Sous-chef head: ab75cfb
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Blocked-label patterns were checked before Unicode sanitization, allowing labels with zero-width or fullwidth characters to normalize into a blocked label after the check.