Add `data` and `api_target` fields to `ResponseContext` and scrub `graphql` bodies. by marandaneto · Pull Request #2141 · getsentry/relay

marandaneto · 2023-05-22T13:58:51Z

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 · 2023-05-23T13:11:18Z

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:

relay/relay-general/src/store/normalize/contexts.rs

Lines 180 to 200 in 5d764fc

    
           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 · 2023-06-01T08:48:56Z

@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.

…into chore/add-response-data

`api_target` was added in getsentry/relay#2141, this ensures that it gets picked up by the event serializer.

Add Response data to Contexts

d37b9b5

marandaneto added 7 commits May 23, 2023 15:34

add comments and fix fixture

e3c79ee

add data tests

f5ed3db

fix import

c355585

draft

df4ce1f

fix

2bb2d00

scrub data and variables keus

dba8dd1

fix scrubbing

0ae5e30