fix(sanitizer): align URL authority parsing with browser URL parsing - #50929
Conversation
The protocol-relative host pattern in sanitizeUrlDomains stops at '@', so '//github.com@evil.com/x' was matched as the allowlisted host github.com and passed through unredacted. Browsers on an HTTPS page resolve it to https://github.com@evil.com/x and connect to evil.com, making it an exfiltration channel (zero-click via camo when rendered in a markdown image). stripUrlUserinfo only covers explicit scheme:// URLs. Add stripProtocolRelativeUserinfo, anchored the same way as the protocol-relative pass, and run it before the allowlist check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a90e42f6-2a27-400b-9dad-12475e14a1e5
Add a dedicated describe block covering the userinfo-authority bypass across both URL forms: https:// and protocol-relative, bare/markdown-image/HTML-src, with port, chained and user:password userinfo, mixed case, and multiple occurrences. Includes negative cases so redaction does not fire on an '@' in a path or query string, on '//' path segments inside an allowed absolute URL, or when the real host is allowlisted (userinfo stripped, URL preserved). 10 of the 15 fail without the stripProtocolRelativeUserinfo fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a90e42f6-2a27-400b-9dad-12475e14a1e5
There was a problem hiding this comment.
Pull request overview
Fixes protocol-relative URL userinfo spoofing in the content sanitizer.
Changes:
- Strips userinfo before protocol-relative domain filtering.
- Adds spoofing and regression tests.
- Adds a patch changeset.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/sanitize_content_core.cjs |
Adds protocol-relative userinfo stripping. |
actions/setup/js/sanitize_content.test.cjs |
Tests URL spoofing scenarios. |
.changeset/fix-protocol-relative-url-userinfo-bypass.md |
Documents the security fix. |
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: Balanced
| // in the output for an allowed domain. Protocol-relative URLs (//host/path) | ||
| // are stripped too, since browsers resolve them to https:// and they are | ||
| // subject to the same allowlist check below. | ||
| s = stripUrlUserinfo(s); | ||
| s = stripProtocolRelativeUserinfo(s); |
There was a problem hiding this comment.
This is obsolete as of a1091df: stripUrlIgnorableWhitespace removes browser-ignored tab/CR/LF inside captured authorities before userinfo and host comparison, and the authority regexes now include those characters so they are normalized before filtering. Regression tests cover raw and entity-encoded tab/newline variants for both protocol-relative and https:// forms.
|
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
|
|
No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Review: fix(sanitizer): redact protocol-relative URLs with userinfo authority
The fix is correct and well-structured:
stripProtocolRelativeUserinfouseslastIndexOf("@")— correctly handles chaineda@b@hostforms.- Anchoring (
^\|[\s([{"']) consistently matches the existing protocol-relative regex insanitizeUrlDomains, so//inside an absolute URL path is not double-processed. - Both
stripProtocolRelativeUserinfoandprotoRelativeUrlRegexuse identical delimiter sets, ensuring the two passes stay in sync. - Test suite is thorough: covers no-
@, chained userinfo, port-in-userinfo, case normalization, HTML src, and negative cases (path/query@not treated as userinfo).
The one open concern — ASCII tab/newline bypass in authority — is already flagged in an existing review comment and is a pre-existing gap not introduced by this PR.
Approving.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 26.4 AIC · ⊞ 5.3K
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — this is a clean, well-reasoned security fix with comprehensive regression coverage. Approving.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause correctly identified:
stripUrlUserinforequires a(redacted) and so never covered//host/path` forms - ✅ Fix matches the existing design exactly — same anchoring, same last-
@semantics, same call site insanitizeUrlDomains - ✅ 15 focused regression tests, 10 of which fail on the pre-fix code — strong TDD evidence that the test suite truly exercises the fixed path
- ✅ Negative cases guard against false positives (
@in path/query,//segments inside absolute URLs) - ✅ Security impact clearly articulated in PR body and changeset
One Pre-Existing Gap (noted by prior review)
An existing comment already flags that ASCII control characters (\t, \r, \n) in the authority can bypass both userinfo-stripping passes. That issue is orthogonal to this PR; no action needed here.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.3 AIC · ⊞ 7.1K
Comment /matt to run again
Rubber-duck review found the previous commit was bypassable, and that the https:// half of the advisory was NOT in fact already fixed. Four parser differentials, each verified with a reproducing input: 1. Greedy authority swallowed the next URL's anchor. [^\s/?#]* consumed ",https:" in "https://x.com,https://github.com@evil.com/", so the global scan resumed past the second URL and never stripped its userinfo. This re-opened the original advisory. Restrict the authority to stop at the delimiters that terminate a URL in prose and markup. 2. The URL-start delimiter set omitted "<" and "=", so "<img src=//host@evil>" and "[a](<//host@evil>)" were never examined. Widen it, and share one constant between the strip and filter passes so they cannot disagree. 3. Backslashes bypassed filtering but are slashes to a URL parser, so "\\host/x" and "/\host/x" reached an unfiltered host. Accept them as separators and normalize to "//". This also closes the no-userinfo case. 4. Tab/CR/LF terminated the authority for us but are discarded by URL parsers, so "//github.com<TAB>A@evil.com/" presented the allowlisted host. Tolerate them inside the authority and strip them before the host comparison; a rewrite still only happens when an "@" is present, so ordinary hosts followed by prose are untouched. 14 more regression tests (29 total); 10 fail against the previous commit. 731 sanitizer tests pass. Perf on par with baseline on 500KB adversarial inputs (no ReDoS). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a90e42f6-2a27-400b-9dad-12475e14a1e5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a90e42f6-2a27-400b-9dad-12475e14a1e5
|
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
|
|
No ADR enforcement needed: PR #50929 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 98/100 — Excellent
📊 Metrics (37 tests)
Per-test classification (37 tests)
(8 additional tests omitted from table for brevity; all classified Verdict
|
There was a problem hiding this comment.
The security fix is well-implemented. All identified bypass vectors are addressed:
- Userinfo bypass (
user@host) — correctly stripped for both(redacted) and//` protocol-relative URLs before domain filtering. - Tab/CR/LF injection (WHATWG URL parsing) —
stripUrlIgnorableWhitespace()removes these beforelastIndexOf("@"), and authority regexes allow[\t\r\n]so they are captured and then cleaned. - Backslash separator —
[/\\]{2}matches//,/\,\/,\\variants; separator is normalized to//in output. - Adjacent-URL boundary —
URL_AUTHORITY_CHARexcludes URL-start delimiters, preventing authority from consuming neighboring URLs. - Shared delimiter constant —
URL_START_DELIMITERSis reused across strip and filter passes so they cannot disagree on URL boundaries.
Note: the existing Copilot inline comment at line 435 claiming tab/newline bypass is stale — this PR explicitly handles that exact vector via stripUrlIgnorableWhitespace and the (?:URL_AUTHORITY_CHAR|[\t\r\n])* pattern. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 34 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — this is a thorough, well-documented security fix. Approving with one observation.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause analysis is excellent: four distinct parser/regex differentials are each identified, explained, and fixed
- ✅ Test suite is exemplary — 29 tests covering every attack vector including edge cases (chained userinfo, mixed case, entity-encoded whitespace, Windows paths), with 10 confirmed failing against the prior commit
- ✅ Shared
URL_START_DELIMITERSconstant between the strip and filter passes closes a class of bypass where the two passes could disagree — this is the right abstraction - ✅
stripUrlIgnorableWhitespacecorrectly models the specific WHATWG preprocessing rules and is safe (only rewrites when@is present after cleaning) - ✅ Backslash normalization in
stripProtocolRelativeUserinfoensures that(host/redacted)forms cannot survive as an allowed URL shape - ✅ PR description is exemplary: root cause, scope, rejected alternatives, and validation evidence are all present
One observation (non-blocking)
The entity-encoded-tab test ( ) passes, implying HTML entity decoding runs before the URL passes in sanitizeContent. That ordering is an implicit correctness dependency — if the pipeline order ever changes, the entity bypass re-opens without a failing test to catch it. A comment near stripUrlIgnorableWhitespace noting "callers must ensure HTML entities are decoded before this pass" would make it explicit.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.4 AIC · ⊞ 7.1K
Comment /matt to run again
|
@copilot run pr-finisher skill |
Ran the |
|
🎉 This pull request is included in a new release. Release: |
Overview
Fixes a family of URL-allowlist bypasses in the content sanitizer (
actions/setup/js/sanitize_content_core.cjs) caused by disagreement between the sanitizer's regex-based URL parsing and how browsers/URL parsers actually resolve authority (userinfo/host). An attacker could craft a URL that displays an allowlisted host to the sanitizer while a browser resolves a different, attacker-controlled host — enabling a zero-click exfiltration channel via GitHub's camo image proxy.Vulnerability class
Everything before the last
@in a URL authority is userinfo (credentials), not the host.(evil.com/redacted) and(evil.com/redacted) both connect toevil.com, even thoughgithub.laiyagushi.comappears first and could pass a naive allowlist check. Embedded in a markdown image (`, this becomes a zero-click exfiltration vector since GitHub's camo proxy fetches image URLs server-side when a comment is rendered.Key changes
actions/setup/js/sanitize_content_core.cjsURL_START_DELIMITERSandURL_AUTHORITY_CHARshared regex components so the userinfo-stripping pass and the domain-filtering pass use identical boundary rules and cannot disagree.stripUrlIgnorableWhitespace()to discard tab/CR/LF inside an authority before host comparison, matching browser URL-parser preprocessing (previously these were treated as terminators, allowing bypass).stripProtocolRelativeUserinfo()to handle userinfo spoofing in protocol-relative URLs ((evil.com/redacted)), whichstripUrlUserinfo()alone could not cover since it requires a scheme.<and=so HTML attributes and CommonMark angle-bracket destinations are examined.\host,(host/redacted)), since URL parsers treat\as/.actions/setup/js/sanitize_content.test.cjshttps:///protocol-relative URLs, markdown images, HTML attributes, backslash separators, and whitespace-based bypass attempts..changeset/fix-protocol-relative-url-userinfo-bypass.mdSecurity impact
Testing
sanitize_content.test.cjsdirectly exercise each bypass form (userinfo spoofing, protocol-relative, backslash separators, embedded whitespace) and assert the spoofed host is redacted rather than the allowlisted one.> Generated by PR Description Updater for fix(sanitizer): align URL authority parsing with browser URL parsing #50929 · auto · 44 AIC · ⊞ 6.8K · ◷