Skip to content

Fixed #463 PII can be leaked if nested query string keys are out of order #663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danielmorell merged 3 commits into from
Nov 3, 2025
Merged

Fixed #463 PII can be leaked if nested query string keys are out of order #663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e59140
Fixed #463 PII can be leaked if nested query string keys are out of o…
danielmorell Oct 4, 2025
8932e30
Resolved percent encoding differences with original strings.
danielmorell Oct 31, 2025
e5d2392
Fixed a comment said the opposite of what it should.
danielmorell Nov 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
93 changes: 80 additions & 13 deletions src/Scrubber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,27 +124,87 @@ 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
$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 (self::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 static function isQueryStringable(string $data): bool
{
// Because we only scrub values based on keys, we can safely assume that the string is not a query string if
// it does not contain an '=' character.
if (!str_contains($data, '=')) {
return false;
}
// 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);
$partsEncoded = array_map(urldecode(...), $parts);
$rebuiltParts = explode('&', $rebuilt);
$partsRebuiltEncoded = array_map(urldecode(...), $rebuiltParts);

// 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($partsEncoded);
sort($rebuiltParts);
sort($partsRebuiltEncoded);
return $parts === $rebuiltParts || $partsEncoded === $partsRebuiltEncoded;
}

/**
* Checks if the given string contains percent encoded data.
*
* @param string $data The string to check.
* @return bool
*
* @since 4.2.0
*/
public static function hasPercentEncodedData(string $data): bool
{
$parts = explode('%', $data);
if (count($parts) === 1) {
return false;
}

$percentEncoded = false;
foreach ($parts as $part) {
if (strlen($part) < 2) {
continue;
}
// Check if the first two characters are hexadecimal digits.
if (ctype_xdigit(substr($part, 0, 2))) {
$percentEncoded = true;
break;
}
}
return $percentEncoded;
}

/**
* Scrubs sensitive data from an array. This will call {@see self::internalScrub()} and can execute recursively.
*
Expand All @@ -160,7 +220,7 @@ protected function scrubArray(
array &$arr,
array $fields,
string $replacement = '********',
string $path = ''
string $path = '',
): array {
if (!$fields || !$arr) {
return $arr;
Expand All @@ -169,12 +229,12 @@ protected function scrubArray(
$scrubber = $this;
$scrubberFn = function (
&$val,
$key
$key,
) use (
$fields,
$replacement,
$scrubber,
&$path
&$path,
) {
$current = !$path ? (string)$key : $path . '.' . $key;

Expand Down Expand Up @@ -208,9 +268,16 @@ protected function scrubArray(
*/
protected function scrubQueryString(string $query, array $fields, string $replacement = 'xxxxxxxx'): string
{
// Check if the query string contains percent encoded characters.
$percentEncoded = self::hasPercentEncodedData($query);
// PHP reports warning if parse_str() detects more than max_input_vars items.
@parse_str($query, $parsed);
$scrubbed = $this->internalScrub($parsed, $fields, $replacement, '');

// If the original query string was not percent encoded, we need to decode the scrubbed data.
if (!$percentEncoded) {
return str_replace(' ', '+', urldecode(http_build_query($scrubbed)));
}
return http_build_query($scrubbed);
}
}
108 changes: 94 additions & 14 deletions tests/ScrubberTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand Down Expand Up @@ -230,23 +228,23 @@ private static function scrubRecursiveStringDataProvider(): array
return array(
// $testData
array(
'?' . http_build_query(
'?' . str_replace(' ', '+', urldecode(http_build_query(
array(
'arg1' => 'val 1',
'sensitive' => 'scrubit',
'arg2' => array(
'arg3' => 'val 3',
'sensitive' => 'scrubit',
),
)
),
),
))),
),
array( // $scrubFields
'sensitive',
),
// $expected
array(
'?' . http_build_query(
'?' . str_replace(' ', '+', urldecode(http_build_query(
array(
'arg1' => 'val 1',
'sensitive' => 'xxxxxxxx',
Expand All @@ -255,7 +253,7 @@ private static function scrubRecursiveStringDataProvider(): array
'sensitive' => 'xxxxxxxx',
),
)
),
))),
),
);
}
Expand All @@ -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(
Expand Down Expand Up @@ -298,7 +295,6 @@ 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(
array(
Expand All @@ -308,9 +304,9 @@ private static function scrubRecursiveStringRecursiveDataProvider(): array
'arg3' => 'val 3',
'sensitive' => 'xxxxxxxx',
'SENSITIVE' => 'xxxxxxxx',
'sensitive2' => 'xxxxxxxx'
)
)
'sensitive2' => 'xxxxxxxx',
),
),
),
array(
'recursive sensitive data' => '********',
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -401,4 +395,90 @@ 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);
}

/**
* @dataProvider isQueryStringableDataProvider
*/
public function testIsQueryStringable(string $string, bool $expected): void
{
$this->assertSame($expected, Scrubber::isQueryStringable($string));
}

public static function isQueryStringableDataProvider(): array
{
return array(
array('foo=bar', true),
array('foo =bar', false),
array('foo=bar&baz=qux', true),
array('foo[one]=bar&foo[two]=qux', true),
array('foo[one]=bar&bar=42&foo[two]=qux', true),
array('foo[one]=bar&bar=42&foo[two]=qux%3F', true),
);
}

public function testHasPercentEncodedData(): void
{
$this->assertTrue(
Scrubber::hasPercentEncodedData('login[username]=test@test.com&login[password]=secret&foo=bar%3F'),
);
// Invalid percent encoding
$this->assertFalse(
Scrubber::hasPercentEncodedData('login[username]=test@test.com&login[password]=secret&foo=bar%Ye'),
);
// No percent encoded data
$this->assertFalse(
Scrubber::hasPercentEncodedData('login[username]=test@test.com&login[password]=secret&foo=bar'),
);
}

public function testNoPercentEncoding(): void
{
$input = ['query' => 'login[username]=test@test.com&login[password]=secret'];
$expected = ['query' => 'login[username]=test@test.com&login[password]=xxxxxxxx'];
$scrubber = new Scrubber([
'scrubFields' => [
'password',
],
]);

$result = $scrubber->scrub($input);
$this->assertEquals($expected, $result);
}

public function testPercentEncoding(): void
{
$input = ['query' => 'login[username]=test@test.com&login[password]=secret&foo=bar%3F'];
$expected = ['query' => 'login%5Busername%5D=test%40test.com&login%5Bpassword%5D=xxxxxxxx&foo=bar%3F'];
$scrubber = new Scrubber([
'scrubFields' => [
'password',
],
]);

$result = $scrubber->scrub($input);
$this->assertEquals($expected, $result);
}
}