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.

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

Jump to bottom

Graphql Scrubbing can't be disabled via the Data Scrubber toggle option #2666

Closed
Fwang36 opened this issue Oct 27, 2023 · 3 comments · Fixed by #2689
Closed

Graphql Scrubbing can't be disabled via the Data Scrubber toggle option #2666

Fwang36 opened this issue Oct 27, 2023 · 3 comments · Fixed by #2689
Assignees
@TBS1996

Comments

@Fwang36
Copy link
Member

Fwang36 commented Oct 27, 2023

Steps to Reproduce

  1. Disable the Data Scrubber in the project settings.
  2. Send an event with a Graphql request body.
  3. See variables still scrubbed like the image in this PR
    Specifically, I believe it is this rule in the repo.

Expected Result

Disabling the Data Scrubber should disable the scrubbing.

Actual Result

Disabling the Data Scrubber does not disable the scrubbing.
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Oct 27, 2023
@TBS1996 TBS1996 self-assigned this Oct 30, 2023
@getsantry getsantry bot removed the status in GitHub Issues with 👀 Oct 31, 2023
@jjbayer
Copy link
Member

jjbayer commented Oct 31, 2023

@Fwang36 we're looking into it!

@TBS1996
Copy link
Contributor

TBS1996 commented Nov 2, 2023

Looked a bit into this:

It should only scrub graphql if it can create a piiconfig from datascrubbingconfig, which would be when to_pii_config returns Ok(Some()).

So for the graphql to not be scrubbed, that function must return Ok(None) in [this line].(

relay/relay-pii/src/convert.rs

Line 125 in 08362f2

if applied_rules.is_empty() && applications.is_empty() {
)

For that to happen, both datascrubbing have to be disabled, and ip address scrubbing must be disabled as well.

Next step for me is to verify this, then think about any potential changes in behaviour.

@TBS1996 TBS1996 mentioned this issue Nov 6, 2023
Merged
@TBS1996 TBS1996 linked a pull request Nov 6, 2023 that will close this issue
feat(server): Disable graphql scrubbing when datascrubbing is disabled #2689
Merged
TBS1996 added a commit that referenced this issue Nov 7, 2023
@TBS1996
feat(server): Disable graphql scrubbing when datascrubbing is disabled (
5970ee9 
#2689)

issue: #2666 

Previously the graphql scrubbing would run any time there is a pii
config, which would always be the case even if datascrubbing is disabled
as it would simply be a config without rules.

The expected behaviour would be that if you disable datascrubbing,
graphql scrubbing will also be disabled.

this PR moves the graphql scrubbing outside of the processor, and simply
checks the datascrubbing flag for whether to scrub graphql or not.
Moving it out because we don't have access to the datascrubbing config
in the processor.

in the future, it would be better to have a separate graphql option for
the user to configure in sentry and the graphql rules could be part of
the normal datascrubbing rules.
@TBS1996
Copy link
Contributor

TBS1996 commented Nov 7, 2023

problem is fixed now, deployed and I've verified that it works as expected. graphql scrubbing now depends on whether datascrubbing is enabled, and if it's not, it still respects the "prevent storing ip address" option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@TBS1996 TBS1996

Labels
None yet
Projects
GitHub Issues with 👀
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(server): Disable graphql scrubbing when datascrubbing is disabled getsentry/relay
3 participants
@jjbayer @TBS1996 @Fwang36