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
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
d37b9b5
Add Response data to Contexts
marandaneto May 22, 2023
e3c79ee
add comments and fix fixture
marandaneto May 23, 2023
f5ed3db
add data tests
marandaneto May 23, 2023
c355585
fix import
marandaneto May 23, 2023
df4ce1f
draft
marandaneto May 24, 2023
2bb2d00
fix
marandaneto May 24, 2023
dba8dd1
scrub data and variables keus
marandaneto May 30, 2023
0ae5e30
fix scrubbing
marandaneto May 31, 2023
19e20f3
ref normalize url encoding
marandaneto May 31, 2023
65bade2
Merge branch 'master' into chore/add-response-data
marandaneto May 31, 2023
a89711e
add more tests
marandaneto May 31, 2023
06d9058
make tests simpler
marandaneto May 31, 2023
f837ad1
add api_target
marandaneto Jun 1, 2023
ed23182
Merge branch 'master' into chore/add-response-data
marandaneto Jun 1, 2023
dda8575
remove comment
marandaneto Jun 1, 2023
b640995
fix rustdoc
marandaneto Jun 1, 2023
28733e3
fix doc
marandaneto Jun 1, 2023
e9ae8a4
add changelog
marandaneto Jun 1, 2023
ac80e06
fixes code review
marandaneto Jun 1, 2023
68b794b
change String to &str
marandaneto Jun 1, 2023
3d2cc5f
remove Option from keys
marandaneto Jun 1, 2023
0ba12b7
fi response body type
marandaneto Jun 1, 2023
27f5018
Merge branch 'master' into chore/add-response-data
marandaneto Jun 1, 2023
85cec17
code review
marandaneto Jun 2, 2023
d7b5af3
fix changelog
marandaneto Jun 2, 2023
054a8e9
Merge branch 'chore/add-response-data' of github.com:getsentry/relay …
marandaneto Jun 2, 2023
f2673b0
Merge branch 'master' into chore/add-response-data
marandaneto Jun 6, 2023
342bba1
delete old snap file
marandaneto Jun 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 127 additions & 2 deletions relay-general/src/pii/processor.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::borrow::Cow;
use std::collections::BTreeMap;
use std::mem;

use once_cell::sync::OnceCell;
Expand All @@ -11,8 +12,10 @@ use crate::pii::{CompiledPiiConfig, Redaction, RuleType};
use crate::processor::{
process_chunked_value, Chunk, Pii, ProcessValue, ProcessingState, Processor, ValueType,
};
use crate::protocol::{AsPair, IpAddr, NativeImagePath, PairList, Replay, User};
use crate::types::{Meta, ProcessingAction, ProcessingResult, Remark, RemarkType, Value};
use crate::protocol::{AsPair, Context, Event, IpAddr, NativeImagePath, PairList, Replay, User};
use crate::types::{
Annotated, Meta, ProcessingAction, ProcessingResult, Remark, RemarkType, Value,
};

/// A processor that performs PII stripping.
pub struct PiiProcessor<'a> {
Expand Down Expand Up @@ -195,6 +198,78 @@ impl<'a> Processor for PiiProcessor<'a> {
replay.process_child_values(self, state)?;
Ok(())
}

fn process_event(
&mut self,
event: &mut Event,
_meta: &mut Meta,
state: &ProcessingState<'_>,
) -> ProcessingResult {
// TODO: make keys lazy init
let mut keys = Vec::new();

// collect the variables keys and scrub them out
if let Some(request) = event.request.value_mut() {
if let Some(Value::Object(data)) = request.data.value_mut() {
if let Some(Annotated(Some(Value::Bool(graph_ql_error)), _)) =
request.other.get("graphql_error")
{
dbg!("graphql_error request {graphql_error}");
if *graph_ql_error {
if let Some(Annotated(Some(Value::Object(variables)), _)) =
data.get_mut("variables")
{
for item in variables.iter_mut() {
keys.push(item.0.to_string());
item.1
.set_value(Some(Value::String("[Filtered]".to_string())));
}
}
}
}
}
}

// scrub PII from the data object if they match the variables keys
if let Some(contexts) = event.contexts.value_mut() {
if let Some(Context::Response(response)) = contexts.get_context_mut("response") {
if let Some(Value::Object(data)) = response.data.value_mut() {
if let Some(Annotated(Some(Value::Bool(graph_ql_error)), _)) =
response.other.get("graphql_error")
{
if *graph_ql_error {
if let Some(Annotated(Some(Value::Object(response_data)), _)) =
data.get_mut("data")
{
scrub_data_from_response(keys.clone(), response_data);
}
}
}
}
}
}

event.process_child_values(self, state)?;

Ok(())
}
}

fn scrub_data_from_response(keys: Vec<String>, data: &mut BTreeMap<String, Annotated<Value>>) {
for item in data.iter_mut() {
match item.1.value_mut() {
Some(Value::Object(item_data)) => {
// TODO: can we avoid cloning every time?
scrub_data_from_response(keys.clone(), item_data);
}
_ => {
if keys.contains(item.0) {
item.1
.set_value(Some(Value::String("[Filtered]".to_string())));
}
}
}
}
}

fn apply_rule_to_value(
Expand Down Expand Up @@ -1281,4 +1356,54 @@ mod tests {
process_value(&mut breadcrumb, &mut pii_processor, ProcessingState::root()).unwrap();
assert_annotated_snapshot!(breadcrumb);
}

#[test]
fn test_scrub_request_data_variables() {
let mut data = Event::from_value(
serde_json::json!({
"event_id": "5b978c77c0344ca1a360acca3da68167",
"request": {
"method": "POST",
"url": "http://absolute.uri/foo",
"query_string": "query=foobar&page=2",
"data": {
"query": "{\n viewer {\n login\n }\n}",
"variables": {
"login": "foo"
}
},
"graphql_error": true
},
"contexts": {
"response": {
"type": "response",
"data": {
"data": {
"viewer": {
"login": "foo"
}
}
},
"status_code": 200,
"graphql_error": true,
}
}
})
.into(),
);

let scrubbing_config = DataScrubbingConfig {
scrub_data: true,
scrub_ip_addresses: true,
scrub_defaults: true,
..Default::default()
};

let pii_config = to_pii_config(&scrubbing_config).unwrap();
let mut pii_processor = PiiProcessor::new(pii_config.compiled());

process_value(&mut data, &mut pii_processor, ProcessingState::root()).unwrap();

assert_debug_snapshot!(&data);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
---
source: relay-general/src/pii/processor.rs
expression: "&data"
---
Event {
id: EventId(
5b978c77-c034-4ca1-a360-acca3da68167,
),
level: ~,
version: ~,
ty: ~,
fingerprint: ~,
culprit: ~,
transaction: ~,
transaction_info: ~,
time_spent: ~,
logentry: ~,
logger: ~,
modules: ~,
platform: ~,
timestamp: ~,
start_timestamp: ~,
received: ~,
server_name: ~,
release: ~,
dist: ~,
environment: ~,
site: ~,
user: ~,
request: Request {
url: "http://absolute.uri/foo",
method: "POST",
data: Object(
{
"query": String(
"{\n viewer {\n login\n }\n}",
),
"variables": Object(
{
"login": String(
"[Filtered]",
),
},
),
},
),
query_string: Query(
PairList(
[
(
"query",
JsonLenientString(
"foobar",
),
),
(
"page",
JsonLenientString(
"2",
),
),
],
),
),
fragment: ~,
cookies: ~,
headers: ~,
body_size: ~,
env: ~,
inferred_content_type: ~,
other: {
"graphql_error": Bool(
true,
),
},
},
contexts: Contexts(
{
"response": ContextInner(
Response(
ResponseContext {
cookies: ~,
headers: ~,
status_code: 200,
body_size: ~,
data: Object(
{
"data": Object(
{
"viewer": Object(
{
"login": String(
"[Filtered]",
),
},
),
},
),
},
),
inferred_content_type: ~,
other: {
"graphql_error": Bool(
true,
),
},
},
),
),
},
),
breadcrumbs: ~,
exceptions: ~,
stacktrace: ~,
template: ~,
threads: ~,
tags: ~,
extra: ~,
debug_meta: ~,
client_sdk: ~,
ingest_path: ~,
errors: ~,
key_id: ~,
project: ~,
grouping_config: ~,
checksum: ~,
csp: ~,
hpkp: ~,
expectct: ~,
expectstaple: ~,
spans: ~,
measurements: ~,
breakdowns: ~,
_metrics: ~,
other: {},
}
25 changes: 25 additions & 0 deletions relay-general/src/protocol/contexts/response.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ use crate::protocol::{Cookies, Headers};
use crate::types::{Annotated, Object, Value};

/// Response interface that contains information on a HTTP response related to the event.
///
/// The data variable should only contain the response body. It can either be
/// a dictionary (for standard HTTP responses) or a raw response body.
#[derive(Clone, Debug, Default, PartialEq, Empty, FromValue, IntoValue, ProcessValue)]
#[cfg_attr(feature = "jsonschema", derive(JsonSchema))]
// #[metastructure(process_func = "process_response")]
pub struct ResponseContext {
/// The cookie values.
///
Expand All @@ -26,6 +30,17 @@ pub struct ResponseContext {
/// HTTP response body size.
pub body_size: Annotated<u64>,

/// Response data in any format that makes sense.
///
/// SDKs should discard large and binary bodies by default. Can be given as string or
/// structural data of any format.
#[metastructure(pii = "true", bag_size = "large")]
pub data: Annotated<Value>,

/// The inferred content type of the response payload.
#[metastructure(skip_serialization = "empty")]
pub inferred_content_type: Annotated<String>,

/// Additional arbitrary fields for forwards compatibility.
/// These fields are retained (`retain = "true"`) to keep supporting the format that the Dio integration sends:
/// <https://github.com/getsentry/sentry-dart/blob/7011abe27ac69bd160bdc6ecf3314974b8340b97/dart/lib/src/protocol/sentry_response.dart#L4-L8>
Expand Down Expand Up @@ -75,6 +90,10 @@ mod tests {
],
"status_code": 500,
"body_size": 1000,
"data": {
"some": 1
},
"inferred_content_type": "application/json",
"arbitrary_field": "arbitrary",
"type": "response"
}"#;
Expand All @@ -90,6 +109,12 @@ mod tests {
headers: Annotated::new(Headers(PairList(headers))),
status_code: Annotated::new(500),
body_size: Annotated::new(1000),
data: {
let mut map = Object::new();
map.insert("some".to_string(), Annotated::new(Value::I64(1)));
Annotated::new(Value::Object(map))
},
inferred_content_type: Annotated::new("application/json".to_string()),
other: {
let mut map = Object::new();
map.insert(
Expand Down
3 changes: 3 additions & 0 deletions relay-general/src/store/normalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -904,6 +904,9 @@ impl<'a> Processor for NormalizeProcessor<'a> {
_meta: &mut Meta,
state: &ProcessingState<'_>,
) -> ProcessingResult {
// TODO: scrubb request.data by trying to convert it to a JSON object and scrubbing
// the "variables" object.

request.process_child_values(self, state)?;

request::normalize_request(request)?;
Expand Down
Loading