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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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
1 commit
Select commit
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
42 changes: 32 additions & 10 deletions src/Scrubber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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)));
}
}
37 changes: 27 additions & 10 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 @@ -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',
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,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',
Expand All @@ -311,7 +307,7 @@ private static function scrubRecursiveStringRecursiveDataProvider(): array
'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,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);
}
}