Skip to content
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

Data scrubbing should filter sensitive headers such as Set-Cookie #1501

Closed
marandaneto opened this issue Sep 27, 2022 · 8 comments
Closed

Data scrubbing should filter sensitive headers such as Set-Cookie #1501

marandaneto opened this issue Sep 27, 2022 · 8 comments

Comments

@marandaneto
Copy link
Contributor

marandaneto commented Sep 27, 2022

OkHttp defines its own sensitive headers.
Some of them are filtered by Relay already, but not all of them.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cookie

Not all HTTP clients respect that Set-Cookie should not be accessible.

Cookie is just removed from the headers list instead of being [Filtered], most likely because there's a high-level property Request#cookies already.

Screenshot 2022-09-27 at 10 46 02

@untitaker
Copy link
Member

the request interface of an event is for http request headers, set-cookie is a response header. generally we are filtering header names iirc

@marandaneto
Copy link
Contributor Author

We're trying to use the request property for all the merged headers (request, response), but since there's this limitation plus a few other ones, I will reconsider that.
I'll leave that up to you if you still wanna run scrubbing or not, there's also the inconsistency of Cookie being removed instead of filtered.

@jjbayer
Copy link
Member

jjbayer commented Sep 29, 2022

I suggest we put this hold until we've completed the RFC on where to put response data in the event payload.

@untitaker
Copy link
Member

We're trying to use the request property for all the merged headers (request, response), but since there's this limitation plus a few other ones, I will reconsider that.

I would definitely not do that. Which RFC are we talking about?

@marandaneto
Copy link
Contributor Author

@untitaker I'm going to write an RFC that adds Event.response or Event.contexts.response, data structure similar to event.request but with additional fields such as status_code, etc...
Data scrubbing would need to give a pass in the headers field, etc.

@untitaker
Copy link
Member

@marandaneto sounds good. there is potential for a lot of pii there so i think we should think about designing new default scrubbing rules in the same vein.

@marandaneto
Copy link
Contributor Author

@marandaneto sounds good. there is potential for a lot of pii there so i think we should think about designing new default scrubbing rules in the same vein.

getsentry/rfcs#22

@marandaneto
Copy link
Contributor Author

Closing in favor of getsentry/rfcs#22
Request #headers[Cookie] is removed from the list because it's set to the high-level field cookies if found.
Set-Cookie isn't scrubbed because it's a response header and it should not be set in the first place.
Response headers go to the Response interface -> getsentry/rfcs#22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants