diff --git a/CHANGELOG.md b/CHANGELOG.md index b4209f0e..b2052771 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Changed +* Query strings no longer forced strings to have percent encoding. +### Fixed +* Fixed #463 PII can be leaked if nested query string keys are out of order. ## [4.1.4] - 2025-09-26 ### Fixed diff --git a/src/Scrubber.php b/src/Scrubber.php index 7df84c6d..5d00a817 100644 --- a/src/Scrubber.php +++ b/src/Scrubber.php @@ -124,27 +124,49 @@ public function internalScrub(mixed &$data, array $fields, string $replacement, { if (is_array($data)) { // scrub arrays - $data = $this->scrubArray($data, $fields, $replacement, $path); - } elseif (is_string($data)) { + return $this->scrubArray($data, $fields, $replacement, $path); + } + if (is_string($data)) { // scrub URLs and query strings $query = parse_url($data, PHP_URL_QUERY); if ($query) { - $data = str_replace( + return str_replace( $query, $this->scrubQueryString($query, $fields), $data ); - } else { - // PHP reports warning if parse_str() detects more than max_input_vars items. - @parse_str($data, $parsedData); - if (http_build_query($parsedData) === $data) { - $data = $this->scrubQueryString($data, $fields); - } + } + if ($this->isQueryStringable($data)) { + return $this->scrubQueryString($data, $fields); } } return $data; } + /** + * Checks if a string is a valid query string. + * + * @param string $data The string to check. + * @return bool True if the string is a valid query string, false otherwise. + * + * @since 4.2.0 + */ + public function isQueryStringable(string $data): bool + { + // PHP reports warning if parse_str() detects more than max_input_vars items. + @parse_str($data, $parsedData); + $rebuilt = http_build_query($parsedData); + $parts = explode('&', $data); + $partsRebuilt = array_map(urldecode(...), explode('&', $rebuilt)); + + // Because nested arrays are supported by parse_str(), we need to sort the parts before comparing them. To + // avoid false negatives due to ordering differences. Since the original nested data keys do not need to be in + // order. + sort($parts); + sort($partsRebuilt); + return $parts === $partsRebuilt; + } + /** * Scrubs sensitive data from an array. This will call {@see self::internalScrub()} and can execute recursively. * @@ -211,6 +233,6 @@ protected function scrubQueryString(string $query, array $fields, string $replac // PHP reports warning if parse_str() detects more than max_input_vars items. @parse_str($query, $parsed); $scrubbed = $this->internalScrub($parsed, $fields, $replacement, ''); - return http_build_query($scrubbed); + return str_replace(' ', '+', urldecode(http_build_query($scrubbed))); } } diff --git a/tests/ScrubberTest.php b/tests/ScrubberTest.php index af8b9c4a..22e207d6 100644 --- a/tests/ScrubberTest.php +++ b/tests/ScrubberTest.php @@ -161,7 +161,6 @@ private static function scrubRecursiveDataProvider(): array 'non sensitive data 7' => 'a=stuff&foo=superSecret', 'sensitive data' => '456', array( - 'non sensitive data 3' => '789', 'non sensitive data 5' => '789&5=', 'recursive sensitive data' => 'qwe', 'non sensitive data 3' => 'rty', @@ -184,7 +183,6 @@ private static function scrubRecursiveDataProvider(): array 'non sensitive data 7' => 'a=stuff&foo=xxxxxxxx', 'sensitive data' => '********', array( - 'non sensitive data 3' => '789', 'non sensitive data 5' => '789&5=', 'recursive sensitive data' => '********', 'non sensitive data 3' => 'rty', @@ -246,7 +244,7 @@ private static function scrubRecursiveStringDataProvider(): array ), // $expected array( - '?' . http_build_query( + '?' . str_replace(' ', '+', urldecode(http_build_query( array( 'arg1' => 'val 1', 'sensitive' => 'xxxxxxxx', @@ -255,7 +253,7 @@ private static function scrubRecursiveStringDataProvider(): array 'sensitive' => 'xxxxxxxx', ), ) - ), + ))), ), ); } @@ -268,7 +266,6 @@ private static function scrubRecursiveStringRecursiveDataProvider(): array 'non sensitive data 2' => '456', 'sensitive data' => '456', array( - 'non sensitive data 3' => '789', 'recursive sensitive data' => 'qwe', 'non sensitive data 3' => '?' . http_build_query( array( @@ -298,9 +295,8 @@ private static function scrubRecursiveStringRecursiveDataProvider(): array 'non sensitive data 2' => '456', 'sensitive data' => '********', array( - 'non sensitive data 3' => '789', 'recursive sensitive data' => '********', - 'non sensitive data 3' => '?' . http_build_query( + 'non sensitive data 3' => '?' . str_replace(' ', '+', urldecode(http_build_query( array( 'arg1' => 'val 1', 'sensitive' => 'xxxxxxxx', @@ -311,7 +307,7 @@ private static function scrubRecursiveStringRecursiveDataProvider(): array 'sensitive2' => 'xxxxxxxx' ) ) - ), + ))), array( 'recursive sensitive data' => '********', ) @@ -360,7 +356,6 @@ public static function scrubArrayDataProvider(): array 'non sensitive data 2' => '456', 'sensitive data' => '456', array( - 'non sensitive data 3' => '789', 'recursive sensitive data' => 'qwe', 'non sensitive data 3' => 'rty', array( @@ -377,7 +372,6 @@ public static function scrubArrayDataProvider(): array 'non sensitive data 2' => '456', 'sensitive data' => '********', array( - 'non sensitive data 3' => '789', 'recursive sensitive data' => '********', 'non sensitive data 3' => 'rty', array( @@ -401,4 +395,27 @@ public function testScrubReplacement(): void $this->assertEquals("@@@@@@@@", $result['scrubit']); } + + public function testIssue463(): void + { + $a = ['query' => 'login[username]=test@test.com&login[password]=secret']; + $b = ['query' => 'login[username]=test@test.com&unrelatedField=123&login[password]=secret']; + + $aExpected = ['query' => 'login[username]=test@test.com&login[password]=xxxxxxxx']; + $bExpected = ['query' => 'login[username]=test@test.com&login[password]=xxxxxxxx&unrelatedField=123']; + + $scrubber = new Scrubber([ + 'scrubFields' => [ + 'password', + ], + ]); + + $this->assertEquals(['password'], $scrubber->getScrubFields()); + + $result = $scrubber->scrub($a); + $this->assertEquals($aExpected, $result); + + $result = $scrubber->scrub($b); + $this->assertEquals($bExpected, $result); + } }