Skip to content

Security | Also mask lowercased variant of filter params #41

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 3 commits into
base: main
Choose a base branch
from
Open

Security | Also mask lowercased variant of filter params #41

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
552db1b
Also mask lowercased variant of filter params
onlime Mar 25, 2025
79e31d1
Revert 552db1b replaceParameters() argument renaming to avoid BC break
onlime Mar 30, 2025
01e5ef0
Add Arr::maskCaseInsensitive() macro to mask request params in a case…
onlime Apr 15, 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
10 changes: 6 additions & 4 deletions src/Models/RequestLog.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,13 @@ protected function getFiltered(array $data)
return $this->replaceParameters($data, RequestLoggerFacade::getFilters());
}

protected function replaceParameters(array $array, array $hidden, string $value = '********'): array
protected function replaceParameters(array $array, array $filters, string $maskChar = '*'): array
{
foreach ($hidden as $parameter) {
if (Arr::get($array, $parameter)) {
Arr::set($array, $parameter, '********');
foreach ($filters as $param) {
foreach ([$param, mb_strtolower($param)] as $key) {
if ($value = Arr::get($array, $key)) {
Arr::set($array, $key, str_repeat($maskChar, mb_strlen($value)));
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doe this work when Arr::get() and Arr::get() are case sensitive? 🤔 What if for example the field is fooBar but the filter applied is for foobar?

It is important that if the field was 'fooBar' => 'secret' in the data then it should be 'fooBar' => '******' in the filtered data, not be converted to 'foobar' => '******'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep the luxury of Arr::get() and Arr::set(), but they are case-sensitive, as they just use regular PHP array access by key, which is always case sensitive. In my proposed solution, the original key would never be changed. But 'fooBar' => 'secret' would not get masked at all, if config('request-logger.filters') contain foobar. So it's just a workaround for most cases, but not a complete case-insensitive solution.

We would either need to write an Arr macro of a case-insensitive Arr::get() variant (so we could keep the luxury of dotted notation key access of nested items), or give up on Arr::get() and implement a simpler solution that does it fully case-insensitive but without dotted notation support. I am not 100% sure if dotted notation is needed, but in RequestLog::getLoggableResponseContent() it looks like it is, as you're masking the whole json decoded response content. With dotted notation support, it might get quite tricky and would make more sense to PR this to the framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bilfeldt it took me quite a bit longer, as there is no easy solution for this. I have committed a Arr::maskCaseInsensitive() macro in 01e5ef0, which should handle all use cases. Personally, I have tested this in my project, so if you're happy with this solution, please go ahead and port my (PEST) tests:

it('masks parts of an array by case-insensitive key matching', function (
    array $array,
    array $keys,
    array $expected,
    string $mask = '*'
) {
    expect(Arr::maskCaseInsensitive($array, $keys, $mask))->toBe($expected);
})->with([
    'mask-some' => [
        ['foo' => 123, 'bar' => 'some@thing', 'baz' => ['aaa', 'bbb', 456]],
        ['bar', 'baz.0', 'baz.2'],
        ['foo' => 123, 'bar' => '**********', 'baz' => ['***', 'bbb', '***']],
    ],
    'mask-sub-array' => [
        ['foo' => 123, 'bar' => ['aaa', 'bbb', 456]],
        ['bar'],
        ['foo' => 123, 'bar' => '********'],
    ],
    'mask-empty-values' => [
        ['foo' => [], 'bar' => null, 'baz' => ['key1' => 0]],
        ['foo', 'bar', 'baz.key1'],
        ['foo' => [], 'bar' => null, 'baz' => ['key1' => '*']],
    ],
    'mask-case-insensitive' => [
        ['Authorization' => 'Bearer 1234', 'fooBar' => ['Baz' => 123], 'filter' => ['Search' => 'abc']],
        ['authorization', 'Foobar.baz', 'filter.search'],
        ['Authorization' => '***********', 'fooBar' => ['Baz' => '***'], 'filter' => ['Search' => '***']],
    ],
    'mask-fixed-length' => [
        ['foo' => 123, 'bar' => 'some@thing', 'baz' => ['aaa', 'bbb', 456]],
        ['bar', 'baz.0', 'baz.2'],
        ['foo' => 123, 'bar' => 'xxxxxxxx', 'baz' => ['xxxxxxxx', 'bbb', 'xxxxxxxx']],
        'xxxxxxxx',
    ],
]);

}
}

Expand Down