Filter PII rather than removing headers entirely

[soundsgoodsofar]

It's often very useful to know what headers were passed, even without the value. Currently the PII filter is removing the key and value entirely. There shouldn't really be any security concern with passing the redacted header keys. I'm currently hardcoding '[Filtered]' so that the display matches in the Sentry UI. Ideally I would pass a null value, but that currently shows up as "" in Sentry.

An even better solution would be to provide a mechanism to inject a closure for applying PII redaction, so that people could do what they want if they don't like the default. There are some problems in how the sentry-laravel package does its Service Provider configuration that make swapping those out much more difficult than need be (I'll open an issue on that project as well).

I want to also add that the Integration classes have some pretty bad design patterns. Combined with the extensive use of final and private keywords, it makes fixes like this way more difficult than they need to be.

Example:
$keysToRemove = ['authorization', 'cookie', 'set-cookie', 'remote_addr'];

Things like that should be determined by config, not hard-coded into functions. Combined with private instead of protected, it makes changes far more complicated than they should be.

[ste93cry]

There shouldn't really be any security concern with passing the redacted header keys

I agree 👍

An even better solution would be to provide a mechanism to inject a closure for applying PII redaction, so that people could do what they want if they don't like the default.

You can use the before_send option or a custom event processor to make changes to the event while enabling the send_default_pii option: in this way, you have full control over what to scrub and what not

Things like that should be determined by config, not hard-coded into functions.

In the SDK 1.x the list of keys to redact was configurable, however since then it has been decided that SDKs should not be responsible for data scrubbing besides a really minimal config and the work has instead been relegated to Relay. Since this is a Unified API-wide decision, there are no plans to go another way for this specific SDK

[soundsgoodsofar]

You can use the before_send option or a custom event processor to make changes to the event while enabling the send_default_pii option: in this way, you have full control over what to scrub and what not

Things like that should be determined by config, not hard-coded into functions.

In the SDK 1.x the list of keys to redact was configurable, however since then it has been decided that SDKs should not be responsible for data scrubbing besides a really minimal config and the work has instead been relegated to Relay. Since this is a Unified API-wide decision, there are no plans to go another way for this specific SDK

Makes sense, but I would still argue that this line in RequestIntegration should pull from config. If you don't think redacting PII is your library's job, that's understandable. But if you are going to offer a solution, I don't personally see any reason it shouldn't be adjustable in config.

$keysToRemove = ['authorization', 'cookie', 'set-cookie', 'remote_addr'];

Anyway, for the scope of this PR, is the change to redact the headers acceptable? Should it be null vs '[Filtered]'? If there's appetite for this I'll fix the unit tests (they're just failing because they expect the old values).

[ste93cry]

I don't personally see any reason it shouldn't be adjustable in config

The reason is that there is already a tool more suitable and tailored for this kind of thing, which is Relay. Making this configurable besides a simple boolean will just mean that at some point another person will came and ask for more configuration, different configuration or whatever else and we don't want to open this Pandora's box.

is the change to redact the headers acceptable?

As I said before, I agree so that's a yes 😃

[soundsgoodsofar]

Updated the unit tests to account for the changes.

Up to you guys, but I would suggest that if the guidance is to use Relay and there is not an intention to build out support for the existing 'send-default-pii' pattern, that pattern should be marked as deprecated, and that information should be added to the documentation about using send-default-pii.

Thanks for your replies!

[ste93cry]

Can you 🙏 both add a CHANGELOG entry and target this PR towards the develop branch?

[soundsgoodsofar]

@ste93cry okay I've rebased onto develop and made changes. I went ahead and moved those headers into options--let me know if you don't like that and I can pull that back out.

Code coverage is failing slightly but I'm not sure what else I can do there.

[ste93cry]

@soundsgoodsofar do you still have plans to finish the work? Otherwise, if it's ok for you I would like to do it for you so that we can merge the PR