Improve Str::sanitizeUrl to reject malformed javascript URLs (#19892)

* Improve Str::sanitizeUrl to reject malformed javascript URLs

* cleanup

* Update SupportServiceProvider.php

* Update UrlSanitizerTest.php

* Update 06-security.md

---------

Co-authored-by: Dan Harrin <git@danharrin.com>

Author: ukeloop

docs/09-advanced/06-security.md

@@ -67,7 +67,7 @@ Many Filament configuration methods accept closures that can return dynamic valu
 
 For example, the `url()` method on columns, entries, and actions renders an `<a href="...">` tag with whatever value you provide. If you pass a URL sourced from user input without validation, a malicious value like `javascript:alert(document.cookie)` could be rendered as a clickable link, leading to XSS. Always validate that URLs use a safe scheme such as `http` or `https` before passing them to Filament.
 
-Filament ships a `Str::sanitizeUrl()` helper that returns the URL when it is schemeless (relative) or uses the `http`/`https` scheme, and returns `null` for anything else. It also normalizes obfuscation tricks such as leading whitespace, embedded control characters (`\t`, `\n`, `\r`, NUL bytes), and mixed-case schemes before checking — so values like `"\tJaVa\nScRiPt:alert(1)"` are rejected, and even on a safe URL the return value has those bytes stripped so they cannot reach the rendered HTML:
+Filament ships a `Str::sanitizeUrl()` helper that returns the URL when it is schemeless (relative) or uses the `http`/`https` scheme, and returns `null` for anything else. Before checking the scheme, it accounts for the obfuscation tricks that browsers silently undo when parsing an `href` value — HTML entity references (numeric like `&#9;`/`&#x09;` and named like `&Tab;`/`&NewLine;`/`&colon;`), percent-encoded control characters (`%09`, `%0A`), embedded raw control characters and whitespace (`\t`, `\n`, `\r`, NUL bytes), and mixed-case schemes — so values like `"\tJaVa\nScRiPt:alert(1)"` or `"java&#x09;script:alert(1)"` are rejected. The return value is the original input unchanged when it passes the check; the helper never rewrites a URL.
 
 ```php
 use Filament\Tables\Columns\TextColumn;
@@ -93,7 +93,7 @@ TextColumn::make('contact')
 
 - check that the host belongs to a domain you control (open-redirect protection),
 - check that the URL is safe for the server to fetch (SSRF protection),
-- decode percent-encoded or HTML-entity-encoded payloads (the browser's URL parser doesn't either, so this is intentional, but it means callers that decode the value before rendering need their own check on the decoded form),
+- guarantee safety for non-standard rendering contexts — the safety analysis assumes the URL will be placed in an HTML attribute like `href`, where the browser performs a single HTML-entity decode and strips whitespace/control characters before parsing the scheme. If your code applies additional transformations to the return value before rendering (for example, calling `urldecode()` and then setting `location.href`), apply your own scheme check to the transformed value,
 - validate that an `http(s)` URL is reachable or trusted in any other way.
 
 If you need any of those guarantees, layer your own check on top of the helper's return value.

packages/support/src/SupportServiceProvider.php

@@ -327,20 +327,44 @@ public function packageBooted(): void
                 return null;
             }
 
-            // Reject whitespace and control characters instead of stripping
-            // them: browsers ignore those bytes when parsing URLs, so an
-            // input like "\tjavascript:..." would resolve to a scheme the
-            // visible string does not suggest.
-            if (preg_match('/[\s\x00-\x1F\x7F]/', $url)) {
+            // Reject URLs containing raw whitespace or control characters. Legitimate URLs
+            // percent-encode these; a raw byte here is either a typo or an obfuscation attempt.
+            if (preg_match('/[\x00-\x20\x7F]/', $url)) {
                 return null;
             }
 
-            if (preg_match('#^([a-z][a-z0-9+\-.]*):#i', $url, $matches)) {
-                $allowedSchemes = array_map(strtolower(...), $allowedSchemes);
+            // Predict the scheme the browser will see after HTML-decoding the attribute value.
+            // We pre-decode ASCII numeric entities ourselves because `html_entity_decode(ENT_HTML5)`
+            // replaces "forbidden" C0 control character entities (e.g. `�`) with U+FFFD, which
+            // would mask them from the control-character strip below.
+            $decoded = preg_replace_callback(
+                '/&#(?:x([0-9a-f]+)|([0-9]+));?/i',
+                function (array $match): string {
+                    $code = ($match[1] !== '') ? hexdec($match[1]) : (int) $match[2];
 
-                if (! in_array(strtolower($matches[1]), $allowedSchemes, strict: true)) {
-                    return null;
-                }
+                    return ($code <= 127) ? chr((int) $code) : $match[0];
+                },
+                $url,
+            );
+            $decoded = html_entity_decode($decoded, ENT_QUOTES | ENT_HTML5, 'UTF-8');
+
+            // Reject if either the HTML-decoded form or its percent-decoded form contains
+            // control characters — catches `%09`-style obfuscation that would be dangerous
+            // if the URL is later decoded by other code.
+            if (
+                preg_match('/[\x00-\x1F\x7F]/', $decoded) ||
+                preg_match('/[\x00-\x1F\x7F]/', rawurldecode($decoded))
+            ) {
+                return null;
+            }
+
+            // Scheme check uses the HTML-decoded form only — the browser does NOT percent-decode
+            // the scheme, so a `%6A` literally in the scheme position is not dangerous.
+            if (
+                preg_match('/^([a-z][a-z0-9+\-.]*):/i', $decoded, $matches) &&
+                (! in_array(strtolower($matches[1]), array_map(strtolower(...), $allowedSchemes), strict: true))
+            ) {
+                return null;
             }
 
             return $url;

tests/src/Support/UrlSanitizerTest.php

@@ -163,6 +163,124 @@
     expect(Str::sanitizeUrl("javascript\x7F:alert(1)"))->toBeNull();
 });
 
+it('rejects URLs containing whitespace after HTML entity decoding', function (): void {
+    expect(Str::sanitizeUrl('java	script:alert(1)'))->toBeNull();
+    expect(Str::sanitizeUrl('java
script:alert(1)'))->toBeNull();
+    expect(Str::sanitizeUrl('java
script:alert(1)'))->toBeNull();
+});
+
+it('rejects URLs containing encoded control characters', function (): void {
+    expect(Str::sanitizeUrl('java%09script:alert(1)'))->toBeNull();
+    expect(Str::sanitizeUrl('java%0Ascript:alert(1)'))->toBeNull();
+});
+
+it('rejects URLs containing raw control characters', function (): void {
+    expect(Str::sanitizeUrl("javascript\x7F:alert(1)"))->toBeNull();
+});
+
+it('rejects URLs containing HTML entity encoded separators', function (): void {
+    expect(Str::sanitizeUrl('javascript:alert(1)'))->toBeNull();
+    expect(Str::sanitizeUrl('javascript:alert(1)'))->toBeNull();
+});
+
+it('rejects URLs containing control characters after HTML entity decoding', function (): void {
+    expect(Str::sanitizeUrl('javascript:alert(1)'))->toBeNull();
+});
+
+it('rejects URLs containing HTML5 named character entities for control characters', function (): void {
+    expect(Str::sanitizeUrl('java	script:alert(1)'))->toBeNull();
+    expect(Str::sanitizeUrl('java
script:alert(1)'))->toBeNull();
+});
+
+it('passes legitimate URLs with multiple query parameters through unchanged', function (): void {
+    expect(Str::sanitizeUrl('https://example.com/?a=1&b=2'))->toBe('https://example.com/?a=1&b=2');
+    expect(Str::sanitizeUrl('https://example.com/search?q=hello&page=2&sort=desc'))
+        ->toBe('https://example.com/search?q=hello&page=2&sort=desc');
+});
+
+it('passes legitimate URLs containing escaped ampersand entities through unchanged', function (): void {
+    expect(Str::sanitizeUrl('https://example.com/?a=1&b=2'))->toBe('https://example.com/?a=1&b=2');
+});
+
+it('passes URLs whose query string literally contains the text `javascript:` through unchanged', function (): void {
+    expect(Str::sanitizeUrl('https://example.com/?q=javascript%3Aalert(1)'))
+        ->toBe('https://example.com/?q=javascript%3Aalert(1)');
+});
+
+it('does not recursively decode double-encoded entities — single decode matches browser behaviour', function (): void {
+    expect(Str::sanitizeUrl('https://example.com/?q=java	script:1'))
+        ->toBe('https://example.com/?q=java	script:1');
+});
+
+it('rejects schemes assembled entirely from numeric HTML entities', function (): void {
+    // j is `j`, a is `a`, etc. Defends against an attacker
+    // disguising the whole scheme name in entities so it doesn't read as
+    // "javascript" in the raw source.
+    expect(Str::sanitizeUrl('javascript:alert(1)'))->toBeNull();
+});
+
+it('rejects schemes assembled from mixed entity and percent encoding', function (): void {
+    // `java	script%3Aalert(1)` — entity decodes to TAB (control-char
+    // rejection); percent-encoded colon is irrelevant because the TAB
+    // rejection fires first.
+    expect(Str::sanitizeUrl('java	script%3Aalert(1)'))->toBeNull();
+});
+
+it('rejects NULL byte hidden in a numeric entity', function (): void {
+    // � decodes to NUL via the manual pre-decode (html_entity_decode
+    // would otherwise replace it with U+FFFD and hide the attack).
+    expect(Str::sanitizeUrl('java�script:alert(1)'))->toBeNull();
+    expect(Str::sanitizeUrl('java�script:alert(1)'))->toBeNull();
+});
+
+it('does not decode named HTML entities that require a trailing semicolon when the semicolon is missing', function (): void {
+    // `&Tab` without trailing `;` is not a valid HTML5 entity (only legacy
+    // entities like `&amp` decode without the semicolon). Both the browser
+    // and `html_entity_decode(ENT_HTML5)` leave it as literal text, so the
+    // URL passes through. We document the behaviour either way.
+    $result = Str::sanitizeUrl('https://example.com/?q=&Tab');
+    expect($result)->toBe('https://example.com/?q=&Tab');
+});
+
+it('rejects schemes containing `+` that are not on the allowlist', function (): void {
+    // RFC 3986 allows `+` in schemes (e.g. `coap+tcp`, `git+ssh`). Make sure
+    // unusual but valid-looking schemes are still gated by the allowlist.
+    expect(Str::sanitizeUrl('git+ssh://example.com/repo.git'))->toBeNull();
+    expect(Str::sanitizeUrl('coap+tcp://example.com/'))->toBeNull();
+});
+
+it('passes through URLs whose path or fragment legitimately contains a literal `:`', function (): void {
+    // A `:` inside a path segment isn't a scheme delimiter — only the
+    // first `:` matters, and our regex correctly anchors with `^`.
+    expect(Str::sanitizeUrl('https://example.com/path:with:colons'))
+        ->toBe('https://example.com/path:with:colons');
+    expect(Str::sanitizeUrl('https://example.com/#section:1'))
+        ->toBe('https://example.com/#section:1');
+});
+
+it('handles URLs that consist of only a scheme and colon, with no body', function (): void {
+    expect(Str::sanitizeUrl('https:'))->toBe('https:');
+    expect(Str::sanitizeUrl('javascript:'))->toBeNull();
+});
+
+it('returns `null` when the allowlist is empty and the URL has an absolute scheme', function (): void {
+    expect(Str::sanitizeUrl('https://example.com', []))->toBeNull();
+    expect(Str::sanitizeUrl('http://example.com', []))->toBeNull();
+});
+
+it('does not catastrophically backtrack on long pathological inputs', function (): void {
+    // Sanity check against accidental ReDoS: a long URL of just allowed
+    // scheme characters should complete in well under a millisecond.
+    $long = 'https://example.com/' . str_repeat('a', 10000);
+
+    $start = hrtime(true);
+    $result = Str::sanitizeUrl($long);
+    $elapsedMs = (hrtime(true) - $start) / 1_000_000;
+
+    expect($result)->toBe($long);
+    expect($elapsedMs)->toBeLessThan(50.0);
+});
+
 it('rejects `javascript:` with whitespace before the colon', function (): void {
     expect(Str::sanitizeUrl('javascript :alert(1)'))->toBeNull()
         ->and(Str::sanitizeUrl("javascript\t:alert(1)"))->toBeNull();