setRedactedKeys cannot support non-regex and regex filters at the same time

[nickdnk]

Note: I edited this issue heavily after creating it, as I realised I'd made some silly suggestions.

Describe the bug

After changing our system to use the new setRedactedKeys() method, error reporting seems to have broken.

My debugging points to line line 799 of Report.php, where the raw redacted filter values are injected directly into preg_match(), which requires a pre- and postfix delimiter, however this makes no sense if we're not using regex in our filters. We'd have to pass values like /string-to-filter/ in order to avoid errors.

foreach ($this->config->getRedactedKeys() as $redactedKey) {
    if (@preg_match($redactedKey, $key) === 1) {
        return true;
    } elseif (Utils::stringCaseEquals($redactedKey, $key)) {
        return true;
    }
}

It's not really possible to do what you want here: https://docs.bugsnag.com/platforms/php/other/configuration-options/#redacted-keys as that would require you to escape non-regex strings and not escape actual regex strings. It would have to be two different arrays and you'd merge them, somehow, which would likely be a breaking change. Passing a regular "access_token" string as the example on your docs shows will not work as it will error with: preg_match(): Delimiter must not be alphanumeric or backslash.

Until this is resolved I'll have to go back to the deprecated setFilters() method.

[imjoehaines]

After changing our system to use the new setRedactedKeys() method, error reporting seems to have broken.

Can you describe how it has broken? Are reports not being sent or is redaction not applying?

It's not really possible to do what you want here: https://docs.bugsnag.com/platforms/php/other/configuration-options/#redacted-keys as that would require you to escape non-regex strings and not escape actual regex strings

We use the error suppression operator when calling preg_match which suppresses the delimiter warning. It returns false when passed an invalid regex and so will fall into the Utils::stringCaseEquals case — see https://3v4l.org/1SZcm

Using the example from the docs:

$bugsnag = Bugsnag\Client::make();

$bugsnag->setRedactedKeys([
    'access_token',
    '/^cc_/',
]);

$bugsnag->notifyException(new Exception('oh no'), function (Bugsnag\Report $report) {
    $report->setMetaData([
        'example' => [
            'access_token' => 'abc123',
            'cc_number' => '12394847563',
            'cc_ccv' => '192',
            'cc_expiry' => '2021-12-21',
            'not_access_token' => 'xyz987',
            'not_cc_details' => '12345',
        ]
    ]);
});

Results in redaction working as expected in the dashboard:

[nickdnk]

Hello @imjoehaines

Errors don't get reported and our error handler triggers twice, breaking the entire error-handling part of our application.

I think our problems are related to the use of the @-suppressor. We don't register Bugsnag (as a shutdown handler), but have our own and only use Bugsnag to report errors. I think the suppressor triggers our search for "last error" (using error_get_last()) in our shutdown handler, because Bugsnag is the last code to produce an error via preg_match(), as it runs after the actual error that caused Bugsnag to get involved in the first place.

I would highly recommend that you avoid using error suppression. I realize this can quickly become a religious discussion, but I consider it a very poor practice, and as evident from my problems with the library it has actually caused us grief. We can't really work around this without redesigning our entire error-handling. With that said I also understand that changing this would probably require a breaking change and a new major release. But given how the library works now there is really no way for us to use setRedactedKeys.

Ideally you would have had a wrapper for each redacted element (pseudo-code incoming):

$stringElement = new FilteredString("access_token");
$regexElement = new FilteredRegex("/something_regex/");

$bugsnag->addRedactedString($stringElement);
$bugsnag->addRedactedRegexString($regexElement);

Or, if the addRedactedProperty method accepted a common interface instead:

$stringElement = new FilteredString("access_token");
$regexElement = new FilteredRegex("/something_regex/");

$bugsnag->addRedactedProperty($stringElement);
$bugsnag->addRedactedProperty($regexElement);

And then:

foreach ($this->config->getRedactedKeys() as $redactedKey) {
    if ($redactedKey instanceOf FilteredRegex)) {
        if (preg_match($redactedKey->getValue(), $key) === 1) {
            return true;
        }
    } else {
        // Not sure what this method does, but obviously you'd want to search for the string, maybe with strpos().
        if (Utils::stringCaseEquals($redactedKey->getValue(), $key)) {
            return true;
        }
    }
}

• this would also force users to pass valid regex to their redaction keys and not just have them silently not work at all, which is what happens when you use @.

There are many ways to achieve this, of course, so this is just an example of how I would have avoided the @-suppressor. I think this answer explains quite well why it's a bad idea: https://stackoverflow.com/a/960288/1650180

[imjoehaines]

Hey @nickdnk, thanks for the detailed response

I’m aware of issues with the error suppression operator, but there are cases where it is necessary — passing unknown strings to preg_match being one case, file system interactions being another. PHP doesn’t offer another way to check if a string is a regex and we want to keep the current setRedactedKeys design as it matches our other notifiers

It sounds like the issue is your error handler reacting to suppressed errors when it should be ignoring them. To fix this, you should check if the error code is active e.g.:

https://github.com/bugsnag/bugsnag-php/blob/36d7a22d60e72b6c750ad1600dc5ae5d4073598d/src/Configuration.php#L674-L678

This is explained in the PHP manual for set_error_handler, but also applies to shutdown functions using error_get_last

Without this check your error handler will be incompatible with many libraries that are forced to use error suppression. For example the popular Flysystem package has to use @ when dealing with local files

[nickdnk]

@imjoehaines Thanks - I'll look into this and see if I can make it ignore suppressed errors. I understand that you don't want to change it, I was just offering an example of how it could have been avoided.

[xljones]

Hey @nickdnk, closing out this issue as changes to your error handler to ignore suppressed errors should fix this :)

[trevorgehman]

Not sure why, but since updating to PHP 8 we started seeing these warnings as well when running code inside Laravel Tinker. As a temporary workaround, we're changing our redacted keys strings to regular expressions.

I know this would be a breaking change, but following Laravel's pattern for defining regex for validation rules might be an option:

[
    'regex:/^.+@.+$/i',
];

https://laravel.com/docs/8.x/validation#rule-regex

[mattdyoung]

@trevorgehman - it's worth noting that Bugsnag exception handling is not expected to work correctly when running in Laravel Tinker. This is mentioned in our documentation: https://docs.bugsnag.com/platforms/php/laravel/#reporting-unhandled-exceptions