-
-
Notifications
You must be signed in to change notification settings - Fork 11
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.
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
base: main
Are you sure you want to change the base?
Conversation
and for the ones that suffer from the same flaw, without destroying all UPDATE `request_logs` SET `headers` = REGEXP_REPLACE(`headers`, 'Bearer:?\s*[^"]+', '********') WHERE `headers` LIKE '%authorization%'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @onlime 👍
The masking "pattern/lenght" is a nice addition. Regarding the case-sensitivity then I think it makes sense making the filtering case-insensitive, but we need to make sure that fields names are not converted in the process.
src/Models/RequestLog.php
Outdated
foreach ([$param, mb_strtolower($param)] as $key) { | ||
if ($value = Arr::get($array, $key)) { | ||
Arr::set($array, $key, str_repeat($maskChar, mb_strlen($value))); | ||
} |
There was a problem hiding this comment.
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' => '******'
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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',
],
]);
Only use value to mask by string length if single char is provided, otherwise use it as replacement
…-insensitive manner
The current set of default
request-logger.filters
inconfig/request-logger.php
is quite misleading:You could assume that
Request
header keys are Pascal-Cased, as the sample filters containAuthorization
. This is widely used, even though the standards say they are case insensitive and should be lowercased.In my case, this led to a security issue, as none of my webhook's
authorization
headers were masked inRequestLog
model'sheaders
attribute. The current implementation ofRequestLog::replaceParameters()
is case-sensitive, but theRequest
headers were actually lowercased.Arr::get()
andArr::set()
are also case-sensitive.As a workaround, I am now search-replacing both variants, the original filter key plus its lowercased version. So we can safely leave
Authorization
in the default config.In addition, this PR now masks the filtered values by the same length of asterisks (BEFORE: fixed-length
********
), for improved debugging.