diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..177a260 --- /dev/null +++ b/PR.md @@ -0,0 +1,82 @@ +# Fix RedactingFormatter missing secrets nested inside array-valued sensitive keys + +## Problem + +`RedactingFormatter` masks a sensitive context/extra key by replacing its whole value +with the mask in the record, and separately sweeps the wrapped formatter's rendered +output for the original secret values, specifically to catch copies that leak through +other channels such as `PsrLogMessageProcessor` interpolating a value into the message +before the formatter ever runs (see the class docblock and the existing +`testRedactsSecretAlreadyInterpolatedIntoTheMessage` test). + +That sweep only works if the secret value was collected first. `RedactingFormatter::collect()` +only calls `addSecret()` on the value of a sensitive key directly: + +```php +if (\is_string($key) && \in_array(strtolower($key), $this->sensitiveKeys, true)) { + // no need to recurse, the whole value is masked in the record anyway + $this->addSecret($value, $secrets); + continue; +} +``` + +`addSecret()` only accepts strings and `Stringable` objects. When the value of a +sensitive key is an array, `addSecret()` silently does nothing with it, so none of its +leaf strings are ever added to the `$secrets` list used later during the sweep. + +`PsrLogMessageProcessor::__invoke()` JSON-encodes array context values when +interpolating them into `{placeholder}` tokens in the message: + +```php +} elseif (\is_array($val)) { + $replacements[$placeholder] = 'array'.Utils::jsonEncode($val, null, true); +} +``` + +So a call such as: + +```php +$log->info('password reset payload: {password}', ['password' => ['new' => 'CorrectHorseBatteryStaple42']]); +``` + +ends up with the message `password reset payload: array{"new":"CorrectHorseBatteryStaple42"}` +before `RedactingFormatter` ever sees the record. The context key gets masked correctly +(`"password":"[REDACTED]"`), but the secret embedded in the message is never swept, +because it was never collected: the final formatted output still contains +`CorrectHorseBatteryStaple42` in plain text. + +Root cause: `src/Monolog/Formatter/RedactingFormatter.php`, `collect()` at line 205-210 +(pre-fix), which calls `addSecret()` directly on a sensitive key's value instead of +recursing into it when it is an array. + +## Fix + +Added a small helper, `collectSensitiveValue()`, that walks a value already known to be +sensitive (because it sits under a matched key) and collects every string leaf it +contains, however deeply nested, respecting the same `MAX_DEPTH` guard used elsewhere in +the class for self-referencing structures. `collect()` now calls this helper instead of +`addSecret()` directly for a sensitive key's value. + +Files changed: +- `src/Monolog/Formatter/RedactingFormatter.php` +- `tests/Monolog/Formatter/RedactingFormatterTest.php` (new regression test) + +## Verification + +- `vendor/bin/phpunit tests/Monolog/Formatter/RedactingFormatterTest.php`: 38/38 tests, + 74 assertions, exit 0 (was 37/37 before this change; the new test is + `testRedactsSecretNestedInAnArrayValueAlreadyInterpolatedIntoTheMessage`). +- `vendor/bin/phpunit` (full suite): 1277 tests, 2368 assertions, exit 0. Same 37 skipped + tests as an unmodified checkout (missing ext-mongodb/ext-redis, no local Elasticsearch, + no ZendServer, no `hg` binary in this environment) and the same pre-existing single + PHPUnit deprecation, confirmed identical via `git stash` against a clean HEAD run. +- `vendor/bin/phpstan analyse`: no errors. +- Counterfactual: reverted only the fix in `RedactingFormatter.php` (kept the new test) + via `git stash` on that one file. The new test then fails with: + `Failed asserting that 'password reset payload: array{"new":"tok-supersecret-value"} {"password":"[REDACTED]"}' does not contain "tok-supersecret-value"`. + Restoring the fix turns the suite green again (38/38). + +## Flags for account owner + +None. No CONTRIBUTING.md, AGENTS.md/CLAUDE.md, or AI-authorship policy was found in this +repository, and no CLA/DCO check runs in its CI workflows. diff --git a/src/Monolog/Formatter/RedactingFormatter.php b/src/Monolog/Formatter/RedactingFormatter.php index ed766d2..7bc5a9e 100644 --- a/src/Monolog/Formatter/RedactingFormatter.php +++ b/src/Monolog/Formatter/RedactingFormatter.php @@ -203,8 +203,10 @@ final class RedactingFormatter implements WrappingFormatterInterface if (\is_array($data)) { foreach ($data as $key => $value) { if (\is_string($key) && \in_array(strtolower($key), $this->sensitiveKeys, true)) { - // no need to recurse, the whole value is masked in the record anyway - $this->addSecret($value, $secrets); + // the value itself is masked in the record as a whole, but a nested + // array's leaves can still leak elsewhere (e.g. json-encoded into the + // message by PsrLogMessageProcessor), so collect every leaf too + $this->collectSensitiveValue($value, $secrets, $depth + 1); continue; } @@ -239,6 +241,30 @@ final class RedactingFormatter implements WrappingFormatterInterface } } + /** + * Collects every string leaf found inside a value already known to be sensitive + * (the value of a key matching one of the configured sensitive keys), regardless + * of how deeply it is nested in arrays. + * + * @param list $secrets + */ + private function collectSensitiveValue(mixed $value, array &$secrets, int $depth): void + { + if ($depth > self::MAX_DEPTH) { + return; + } + + if (\is_array($value)) { + foreach ($value as $item) { + $this->collectSensitiveValue($item, $secrets, $depth + 1); + } + + return; + } + + $this->addSecret($value, $secrets); + } + /** * Reflects the properties of an object matching a #[SensitiveParameter] constructor parameter * diff --git a/tests/Monolog/Formatter/RedactingFormatterTest.php b/tests/Monolog/Formatter/RedactingFormatterTest.php index ef4349c..f97ecdf 100644 --- a/tests/Monolog/Formatter/RedactingFormatterTest.php +++ b/tests/Monolog/Formatter/RedactingFormatterTest.php @@ -243,6 +243,23 @@ class RedactingFormatterTest extends MonologTestCase $this->assertStringContainsString('calling api with [REDACTED]', $output); } + public function testRedactsSecretNestedInAnArrayValueAlreadyInterpolatedIntoTheMessage() + { + // PsrLogMessageProcessor json-encodes array context values into the message, so a + // secret nested inside an array under a sensitive key leaks there before this + // formatter ever sees the record; sweeping has to catch it too, not just the key. + $formatter = new RedactingFormatter(new LineFormatter('%message% %context%', 'Y-m-d')); + $processor = new PsrLogMessageProcessor(); + + $output = $formatter->format($processor($this->getRecord( + message: 'password reset payload: {password}', + context: ['password' => ['new' => 'tok-supersecret-value']], + ))); + + $this->assertStringNotContainsString('tok-supersecret-value', $output); + $this->assertStringContainsString('[REDACTED]', $output); + } + public function testRedactsSecretEmbeddedInAnExceptionMessage() { $formatter = new RedactingFormatter(new JsonFormatter());