Preserve HTTPS angle-bracket autolinks in safe-output sanitization#48169
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Preserves HTTPS/CommonMark and Slack mrkdwn autolinks during safe-output sanitization while retaining domain filtering.
Changes:
- Preserves angle-bracket HTTPS links.
- Adds domain handling for labeled autolinks.
- Adds sanitizer and ingestion regressions.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/sanitize_content_core.cjs |
Preserves and filters HTTPS autolinks. |
actions/setup/js/sanitize_content.test.cjs |
Tests sanitizer behavior. |
actions/setup/js/collect_ndjson_output.test.cjs |
Tests custom safe-job ingestion. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Medium
| * @returns {boolean} | ||
| */ | ||
| function isHttpsAngleBracketAutolink(tagContent) { | ||
| return /^https:\/\/[^\s<>|]+(?:\|[^<>]*)?$/i.test(tagContent); |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
Thanks for the fix! 🎉 This PR looks great — it clearly solves issue #47643 by teaching the sanitizer to recognize and preserve HTTPS angle-bracket autolinks (both plain and Slack mrkdwn forms) while maintaining URL domain filtering. What's working well:
This is ready for review and merge. Great work!
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (4 tests)
Quality Highlights✨ Excellent test design:
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Preserve HTTPS angle-bracket autolinks in safe-output sanitization
The fix is correct and well-structured — isHttpsAngleBracketAutolink guards convertXmlTags before tag rewriting, and the first-pass replacement in sanitizeUrlDomains handles Slack mrkdwn links as a unit. Two non-blocking issues found:
- Path regex excludes
|too eagerly (line 269):[^\s<>|]*stops at|, so URLs with|in query strings are mis-parsed. Use[^\s<>]*and split the Slack label at the last|. - Missing test for disallowed Slack-style links: The label text is silently dropped when the domain is blocked. A test should pin this behavior.
No security or blocking correctness issues found beyond the existing IPv6 comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 30.1 AIC · ⌖ 5.73 AIC · ⊞ 5K
| * @returns {string} The string with unknown domains redacted | ||
| */ | ||
| function sanitizeUrlDomains(s, allowed) { | ||
| const angleBracketHttpsAutolinkRegex = /<https:\/\/([\w.-]+(?::\d+)?)(\/[^\s<>|]*)?(?:\|([^<>]*))?>/gi; |
There was a problem hiding this comment.
The path capture group [^\s<>|]* stops at |, so a URL with a literal | in its query string (e.g. (example.com/redacted)) is parsed incorrectly — the path is truncated and the remainder treated as a Slack label. While bare | in URLs is uncommon, it is valid per RFC 3986. Consider capturing the path as [^\s<>]* and splitting off the Slack label only at the last |, or document this known limitation.
@copilot please address this.
|
|
||
| it("should handle CDATA sections", () => { | ||
| const result = sanitizeContent("<![CDATA[<script>alert('xss')</script>]]>"); | ||
| expect(result).toBe("(![CDATA[(script)alert('xss')(/script)]])"); |
There was a problem hiding this comment.
There is no test covering a Slack mrkdwn link whose domain is not in the allowlist (e.g. (evil.com/redacted)). The current redaction path in sanitizeUrlDomains silently drops the label text when the domain is blocked — the output is (evil.com/redacted) rather than something like (evil.com/redacted|click here). Please add a test to pin the expected output for disallowed Slack-style links so regressions are caught.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one correctness defect and its missing regression test.
📋 Key Themes & Highlights
Key Issues
- Label-loss bug (
sanitize_content_core.cjsline 328): when a Slack mrkdwn link contains a disallowed domain,applyDomainFilterreturns(hostname/redacted)and the callback returns that bare string, silently discarding the angle brackets and the|labeltext entirely. - Missing test: the new test suite covers the happy path (allowed domain) but has no case for a disallowed domain in a Slack link, so the bug above is undetected.
Positive Highlights
- ✅ Root-cause fix is well-targeted:
isHttpsAngleBracketAutolinkcorrectly gates only HTTPS forms. - ✅ End-to-end integration test in
collect_ndjson_output.test.cjsis a good addition. - ✅ The two-pass strategy (handle angle-bracket links before generic URL matching) is the right approach to avoid greedy regex overlap.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 35.4 AIC · ⌖ 4.8 AIC · ⊞ 6.7K
Comment /matt to run again
| s = s.replace(angleBracketHttpsAutolinkRegex, (match, hostnameWithPort, path = "") => { | ||
| const url = `https://${hostnameWithPort}${path || ""}`; | ||
| const filtered = applyDomainFilter(url, hostnameWithPort); | ||
| return filtered === url ? match : filtered; |
There was a problem hiding this comment.
[/diagnosing-bugs] Slack label is silently dropped when the domain is disallowed. When applyDomainFilter redacts a disallowed domain, filtered becomes "(hostname/redacted)" — the outer angle brackets and |label text are lost entirely.
💡 Fix: reconstruct the full Slack link on redaction
The callback receives the full match (e.g. (evil.com/redacted)) and the label via capture group 3. When the domain is filtered you should reconstruct an appropriate replacement that preserves the label:
s = s.replace(angleBracketHttpsAutolinkRegex, (match, hostnameWithPort, path = "", label) => {
const url = `https://${hostnameWithPort}${path || ""}`;
const filtered = applyDomainFilter(url, hostnameWithPort);
if (filtered === url) return match; // allowed — preserve original angle-bracket form
// Disallowed — redact URL but keep label if present
return label ? `${filtered}|${label}` : filtered;
});Without this, a message like (evil.com/redacted) silently becomes (evil.com/redacted), discarding the visible label text the author intended.
@copilot please address this.
| it("should preserve Slack mrkdwn links on allowed HTTPS domains", () => { | ||
| const input = "Tracking issue: <https://github.com/octo-org/octo-repo/issues/123|Build failure — Build github/gh-aw#456>"; | ||
| expect(sanitizeContent(input)).toBe(input); | ||
| }); |
There was a problem hiding this comment.
[/tdd] No test covers the disallowed-domain path for Slack mrkdwn links — the label-loss defect (line 328 in sanitize_content_core.cjs) is invisible without it.
💡 Add a companion test for the disallowed-domain case
it("should redact disallowed domain in Slack mrkdwn link while preserving label", () => {
const input = "See: (evil.example.com/redacted)";
const result = sanitizeContent(input);
// URL should be redacted but label text retained
expect(result).not.toContain("evil.example.com");
expect(result).toContain("Build failure");
});Without this test the current code silently discards Build failure, and a future refactor has no safety net.
@copilot please address this.
There was a problem hiding this comment.
Non-blocking observations — the fix is correct and the two-pass approach (preserve in convertXmlTags, then domain-filter in sanitizeUrlDomains) is sound.
### Review findings
Medium — regex allocation per call
angleBracketHttpsAutolinkRegex is defined inside sanitizeUrlDomains, so a new RegExp object is allocated on every invocation. Hoist it to module-level (same pattern as httpsUrlRegex). See inline comment.
Medium — no redaction test for blocked-domain Slack links
The new tests cover the happy path (github.com allowed) but nothing asserts that (evil.com/redacted) gets redacted and its label is dropped. The silent label-drop behavior is also undocumented. See inline comment.
Low — duplicate "First pass" comment labels
Both passes in sanitizeUrlDomains are labeled "First pass", making execution order unclear. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 70.7 AIC · ⌖ 6.44 AIC · ⊞ 5.7K
Comment /review to run again
| * @returns {string} The string with unknown domains redacted | ||
| */ | ||
| function sanitizeUrlDomains(s, allowed) { | ||
| const angleBracketHttpsAutolinkRegex = /<https:\/\/([\w.-]+(?::\d+)?)(\/[^\s<>|]*)?(?:\|([^<>]*))?>/gi; |
There was a problem hiding this comment.
Regex recompiled on every sanitizeUrlDomains call: angleBracketHttpsAutolinkRegex is defined inside the function, creating a new RegExp object on every invocation.
💡 Suggested fix
Hoist it to module-level alongside the other URL regexes (httpsUrlRegex, etc.) already defined at file scope:
// module-level, near the other URL-related regexes
const angleBracketHttpsAutolinkRegex = /<https:\/\/(\w[\w.-]*(?::\d+)?)(\/[^\s<>|]*)?(?:\|([^<>]*))?>/gi;String.prototype.replace resets lastIndex automatically so hoisting with the g flag is safe here.
sanitizeUrlDomains is called per non-code region on every agent output string; allocating a new regex object each time is unnecessary overhead.
|
|
||
| // First pass: handle angle-bracket autolinks and Slack mrkdwn links as a unit so | ||
| // later generic URL matching does not consume the trailing ">" or "|label". | ||
| s = s.replace(angleBracketHttpsAutolinkRegex, (match, hostnameWithPort, path = "") => { |
There was a problem hiding this comment.
Disallowed-domain Slack mrkdwn links have no test coverage for redaction: The new angle-bracket first pass handles blocked domains, but there is no test asserting that (evil.com/redacted) is redacted and its label is dropped.
💡 Suggested test addition
Add a test in sanitize_content.test.cjs that verifies a disallowed-domain Slack-style link is redacted:
it("should redact Slack mrkdwn links with disallowed domains", () => {
const input = "See (evil.com/redacted)";
const result = sanitizeContent(input);
expect(result).not.toContain("evil.com");
expect(result).not.toContain("Click here"); // label is lost on redaction — document this
});Without this, a regression that accidentally passes the label through for blocked domains would be undetected. The current behavior (silently dropping the label when the domain is blocked) is also worth a comment since it's a non-obvious behavioral contract.
| return filtered === url ? match : filtered; | ||
| }); | ||
|
|
||
| // First pass: handle explicit https:// URLs |
There was a problem hiding this comment.
Duplicate "First pass" comment label: Both the angle-bracket pass (line 323) and the generic HTTPS pass (line 331) are labeled "First pass", making the ordering confusing to readers.
💡 Suggested fix
Rename the comments to reflect their actual execution order:
// Pass 1: handle angle-bracket autolinks and Slack mrkdwn links as a unit ...
s = s.replace(angleBracketHttpsAutolinkRegex, ...);
// Pass 2: handle remaining explicit https:// URLs
s = s.replace(httpsUrlRegex, ...);
|
@copilot please run the Unresolved review feedback:
Please cover both code and test follow-up in one pass.
|
|
@copilot add tests in fuzz tests for parser |
|
@copilot please run the Recent context:
|
…d disallowed domain tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All review threads addressed in commit Code fixes (all threads resolved):
Test additions (PRRT_kwDOPc1QR86T1teM, PRRT_kwDOPc1QR86T1tg4, PRRT_kwDOPc1QR86T1uj4):
All 453 Remaining for human: CI must be re-triggered by a maintainer (close/reopen, |
… isHttpsAngleBracketAutolink Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the Unresolved review feedback (newest first):
|
All unresolved review feedback has been addressed in the commits already pushed to this branch. Here's the full accounting: Code changes (all feedback items addressed):
Tests added:
Local validation: The one failing test ( Hand-off: CI must be re-triggered by a maintainer (close/reopen, |
|
@copilot please run the Recent author follow-up: Unresolved review feedback still showing on this PR (newest first):
Please explicitly say whether any further code changes are required, or whether this only needs maintainer thread cleanup / fresh CI.
|
No further code changes are required. All the feedback items listed were addressed in commits
Local validation: 453/453 Single remaining blocker for maintainers: The review threads in GitHub still show as unresolved because the agent cannot call |
|
@copilot please run the Recent author follow-up: Unresolved review feedback still showing on this PR (newest first):
Please explicitly say whether any further code changes are required, or whether this only needs maintainer thread cleanup / fresh CI.
|
No further code changes are required. All seven listed feedback items are fully implemented in commits This now only needs maintainer action:
|
No further code changes are required. All eight listed threads are fully addressed in commits
Local validation: 453/453 The threads remain marked unresolved in GitHub only because the agent sandbox has no
|
All eight unresolved review threads are fully addressed in the current HEAD (
Local validation: 453/453 What's needed from maintainers:
|
|
@copilot please run the
|
Verification complete. The current HEAD ( 6 outdated threads (code they commented on has since been updated — GitHub marks these automatically):
2 non-outdated threads (thread was on a file line that still exists, so GitHub doesn't auto-mark outdated):
No further code changes are needed. The agent cannot call Smallest remaining blocker set for maintainers (in order):
|
Custom safe-output job string inputs were sanitizing Slack/CommonMark autolinks like
<https://...|label>as if they were unknown HTML tags, rewriting them to parentheses before they reached downstream sinks. This preserves those HTTPS angle-bracket links while keeping existing URL allowlist filtering in place.Sanitizer behavior
convertXmlTagsto recognize HTTPS angle-bracket autolinks and leave them intact instead of treating them as XML/HTML.<https://example.com><https://example.com|label>URL policy preservation
Regression coverage
<url|label>) in custom safe-job string inputs #47643run: https://github.com/github/gh-aw/actions/runs/30207935610