Skip to content

Commit dfb0685

Browse files
committed
fix: escapeUrl("0") should return "0", not ""
previously, we assumed that when removing XSS-like script URLs, the only non-truthy result would be a failure in preg_replace, or an empty string. A value of "0" would therefor pass through the script-tag check, but then be converted into the empty string because it is non-truthy. instead, we now specifically test for those values which we want to convert into an empty string, and do so immediately. This allows the value of "0" to remain intact when passed through a escape methods such as encodeUrlParam(...) or escapeUrl(...) Arguably, this is not the "real" solution. Theoretically, this happens because of the expectation that escapeXssInUrl should only be called on "full" URLs, and is intended to remove URLs such as "javascript:alert('I am an XSS attack')". However, escapeXssInUrl is used by escapeUrl, which is in-turn used by encodeUrlParam. I believe this to potentially be a bug in encodeUrlParam, which (by its name) seems to be intended to actually do something similar to PHP's builtin urlencode(...) function. I consider this to be a separate issue, however, and this change would both fix the encodeUrlParam case for this specific issue, while not negatively-impacting the pathological case of the string "0" being passed to escapeUrl directly.
1 parent cac46e0 commit dfb0685

File tree

2 files changed

+14
-2
lines changed

2 files changed

+14
-2
lines changed

lib/internal/Magento/Framework/Escaper.php

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -335,8 +335,16 @@ public function escapeXssInUrl($data)
335335
*/
336336
private function escapeScriptIdentifiers(string $data): string
337337
{
338-
$filteredData = preg_replace('/[\x00-\x1F\x7F\xA0]/u', '', $data) ?: '';
339-
$filteredData = preg_replace(self::$xssFiltrationPattern, ':', $filteredData) ?: '';
338+
$filteredData = preg_replace('/[\x00-\x1F\x7F\xA0]/u', '', $data);
339+
if ($filteredData === false || $filteredData === '') {
340+
return '';
341+
}
342+
343+
$filteredData = preg_replace(self::$xssFiltrationPattern, ':', $filteredData);
344+
if ($filteredData === false) {
345+
return '';
346+
}
347+
340348
if (preg_match(self::$xssFiltrationPattern, $filteredData)) {
341349
$filteredData = $this->escapeScriptIdentifiers($filteredData);
342350
}

lib/internal/Magento/Framework/Test/Unit/EscaperTest.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,10 @@ public function testEscapeXssInUrl($input, $expected)
343343
public function escapeDataProvider()
344344
{
345345
return [
346+
[
347+
'0',
348+
'0',
349+
],
346350
[
347351
'javascript%3Aalert%28String.fromCharCode%280x78%29%2BString.'
348352
. 'fromCharCode%280x73%29%2BString.fromCharCode%280x73%29%29',

0 commit comments

Comments
 (0)