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

Add data and api_target fields to ResponseContext and scrub graphql bodies. #2141

Merged
merged 28 commits into from
Jun 6, 2023

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented May 22, 2023

PR for https://github.com/getsentry/rfcs/blob/choregraphql-errors/text/0087-graphql-errors.md

  • This PR adds a few fields to the event protocol.
    • Request#api_target which is the API target used for advanced data scrubbing, a marker of the type of the request.
      Response#data is similar to Request#data.
      Response#inferred_content_type is similar to Request#inferred_content_type.
  • This PR implements the process_event for the PII processor scrubbing variables from a GraphQL error based on the Request#api_target.

@jjbayer
Copy link
Member

jjbayer commented May 23, 2023

  • Do I need to normalize similarly to request?

Only if we allow SDKs to send multiple formats that have to be normalized into one. When in doubt, I would say no.

  • Do I need to process_response similarly to request? I believe so if the one above is also a yes.

This would be the function to hook into. It already takes care of normalizing cookies:

fn normalize_response(response: &mut ResponseContext) {
let headers = match response.headers.value_mut() {
Some(headers) => headers,
None => return,
};
if response.cookies.value().is_some() {
headers.remove("Set-Cookie");
return;
}
let cookie_header = match headers.get_header("Set-Cookie") {
Some(header) => header,
None => return,
};
if let Ok(new_cookies) = crate::protocol::Cookies::parse(cookie_header) {
response.cookies = Annotated::from(new_cookies);
headers.remove("Set-Cookie");
}
}

  • Where response.data data scrubbing would go? Probably PiiProcessor?

Default scrubbing runs automatically because of pii=true. Any custom scrubbing would have to go into PiiProcessor, yes.

  • Is the new field auto-generated in test_fixtures__event_schema.snap or should I add it manually?

Use cargo insta review to review the changed snapshots:

cargo install cargo-insta
cargo insta test
cargo insta review

@marandaneto
Copy link
Contributor Author

@jjbayer this PR should be good to go, I've updated the PR description.
I'll test these changes locally with Client SDKs, so it's fine if we don't merge it right away.

@marandaneto marandaneto marked this pull request as ready for review June 1, 2023 08:48
@marandaneto marandaneto requested a review from a team June 1, 2023 08:48
@marandaneto marandaneto changed the title Add Response data to Contexts Add data and api_target fields to Response Context and scrub graphql bodies Jun 1, 2023
@marandaneto marandaneto changed the title Add data and api_target fields to Response Context and scrub graphql bodies Add data and api_target fields to ResponseContext and scrub graphql bodies. Jun 1, 2023
@marandaneto marandaneto requested a review from jjbayer June 1, 2023 12:03
@marandaneto marandaneto enabled auto-merge (squash) June 6, 2023 12:22
@marandaneto marandaneto merged commit 26c7291 into master Jun 6, 2023
@marandaneto marandaneto deleted the chore/add-response-data branch June 6, 2023 12:41
malwilley added a commit to getsentry/sentry that referenced this pull request Jun 8, 2023
`api_target` was added in getsentry/relay#2141,
this ensures that it gets picked up by the event serializer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants