Fix RedactingFormatter missing secrets nested in array-valued sensitive keys
RedactingFormatter::collect() only added a sensitive key's value to the secret list when it was a string or Stringable. When the value was an array, addSecret() silently dropped it, so none of its leaf strings were collected. PsrLogMessageProcessor json-encodes array context values when interpolating them into the message, so a secret nested that way could already be sitting in the message in plain text by the time this formatter ran, and the output sweep had nothing to remove it with. Add collectSensitiveValue() to walk a sensitive key's value and collect every string leaf, however deeply nested, and use it instead of calling addSecret() directly.
This commit is contained in:
1 parent
fb5cc5f9ee
commit
7f7dc9b07c
3 files changed
+127
-2
No files matched your search
@@ -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.
|
||||
Reference in new issue
Block a user