feat(pii): Scrub arrays of 2 elements as key-value pairs #3639

iambriccardo · 2024-05-23T11:59:18Z

This PR adds support for treating an array of arrays of length two as an object. This was done since headers and other pieces of data are commonly encoded in [["key", "value"], ["key", "value"]] and our previous logic was scrubbing each string value individually.

The algorithm works by figuring out if given an array, it is composed of multiple arrays of length two where each first component is a string and then fakes entering the first element of the array as the key of an object.

Closes: #2567

iambriccardo · 2024-05-27T06:46:15Z

relay-pii/src/processor.rs

+        let mut processor = PiiProcessor::new(config.compiled());
+        process_value(&mut event, &mut processor, ProcessingState::root()).unwrap();
+
+        let vars = event


Is there a better way?

iambriccardo · 2024-05-27T07:25:16Z

relay-pii/src/processor.rs

+                        process_value(&mut value[1], self, &entered)?;
+                        // We check whether meta has changed from empty to non-empty in order to
+                        // understand if some rules matched downstream.
+                        if previous_meta.is_empty() && !value[1].meta().is_empty() {


A better impl here would check for structural equivalence but we might gain some performance if we assume that metadata won't be there on an incoming annotated value. Maybe this assumption is totally wrong.

iambriccardo · 2024-05-27T07:25:46Z

relay-pii/src/processor.rs

+            r##"
+                {
+                    "applications": {
+                        "$string": ["@password:remove"]


This rule @password:remove works by matching a key and then scrubbing the value, this is why it was put here.

jjbayer

We should try to get rid of the .clone().into_value() calls as they are potentially expensive. We should aim for a structure like this:

        match try_as_pairlist(value) {
            Ok(map) => self.process_map(value, meta, state),
            Err(_) => value.process_child_values(self, state)
        }

where try_as_pairlist would invoke another visitor i.e. processor that attempts to assemble a borrowing map view, and process_map is a generalization of the existing process_object that can handle any kind of map-like structure.

jjbayer · 2024-05-28T14:39:21Z

relay-pii/src/processor.rs

+                        let index_state = state.enter_index(index, state.inner_attrs(), value_type);
+                        // We enter the key of the first element of the array, since we treat it
+                        // as a pair.
+                        let key_state = index_state.enter_borrowed(


Why do we need to enter both by index and by key? That seems semantically incorrect, because we want the same semantics for [["a", 1], ["b": 2]] as for {"a": 1, "b": 2}. With the current code, we treat the list like [{"a": 1}, {"b": 2}].

See also

relay/relay-pii/src/utils.rs

Lines 28 to 29 in d812b63

let entered = state.enter_borrowed(key_name, state.inner_attrs(), value_type);

processor::process_value(value, slf, &entered)?;

Oh yeah, that's actually a good point. I mistakenly implemented it before. Good catch

Dav1dde · 2024-05-28T14:41:36Z

relay-pii/src/processor.rs

+    where
+        T: ProcessValue,
+    {
+        if is_pairlist(array) {


I prefer a design where the check already returns you a prepared datastructure to visit:

if let Some(pairlist) = Pairlist::try_from(array) { pairlist.visit(self) } else { visit_children }

We would just pipe through the array since this check is not doing anything with the data structure itself.

Same here, this was the original design, but I didn't manage to create a lightweight (i.e. borrowing) pairlist from the visitor because the visitor (process_string) gets a string reference without a lifetime parameter. I'm still open to going that route though, either by changing the Processor framework to allow such a thing, or by simply accepting the cost and creating an owned PairList tentatively.

Dav1dde · 2024-05-28T14:44:12Z

relay-pii/src/processor.rs

@@ -489,6 +597,30 @@ mod tests {
        rv
    }

+    fn extract_vars(event: Option<&Event>) -> &FrameVars {


Thought I've seen a helper macro for this already somewhere 🤔

get_value!

relay-pii/src/processor.rs

Co-authored-by: David Herberth <david.herberth@sentry.io>

…lay into riccardo/feat/add-scrubbing

iambriccardo · 2024-05-29T06:21:02Z

relay-pii/src/snapshots/relay_pii__convert__tests__bearer_tokens_scrubbed.snap

@@ -13,7 +13,7 @@ expression: data
                "request": {
                  "headers": [
                    [
-                      "[Filtered]",
+                      "AuthToken",


The first element is now kept since we treat is as a key.

jjbayer · 2024-05-29T15:25:41Z

relay-pii/src/convert.rs

+                                                    [
+                                                        "passwd",
+                                                        "my_password"
+                                                    ]


Can we split this up to test both the key-detection and the value detection?

Suggested change

[

"passwd",

"my_password"

]

[

"passwd",

"asdf123"

],

[

"something_else",

"my_password"

]

jjbayer · 2024-06-05T10:18:00Z

relay-pii/src/processor.rs

+    {
+        self.is_pair = state.depth() == 0 && value.len() == 2;
+        if self.is_pair {
+            let value_type = ValueType::for_field(&value[0]);


Suggested change

let value_type = ValueType::for_field(&value[0]);

let key_type = ValueType::for_field(&value[0]);

CHANGELOG.md

jjbayer · 2024-06-05T12:10:16Z

relay-pii/src/processor.rs

@@ -202,6 +244,69 @@ impl<'a> Processor for PiiProcessor<'a> {
    }
 }

+#[derive(Default)]
+struct PairListProcessor {


Suggested change

struct PairListProcessor {

/// Checks whether an array is a pair with a string key.

struct PairCheck {

Co-authored-by: Joris Bayer <joris.bayer@sentry.io>

iambriccardo added 4 commits May 23, 2024 13:59

feat(pii): Scrub key value pairs

0ef6862

feat(pii): Scrub key value pairs

b5c2d3e

Add code

7aedc7b

Fix

fd9a5b2

iambriccardo commented May 27, 2024

View reviewed changes

Fix

d1acc1c

iambriccardo changed the title ~~feat(pii): Scrub key value pairs~~ feat(pii): Scrub arrays of 2 elements as key-value pairs May 27, 2024

iambriccardo added 2 commits May 27, 2024 09:22

Fix

0967300

Fix

0672c9c

iambriccardo commented May 27, 2024

View reviewed changes

Update snapshot

4902859

olksdr assigned iambriccardo May 27, 2024

iambriccardo added 3 commits May 27, 2024 13:32

Change

901545f

Change

e13348d

Change

8b44603

iambriccardo marked this pull request as ready for review May 27, 2024 12:24

iambriccardo requested a review from a team as a code owner May 27, 2024 12:24

jjbayer reviewed May 28, 2024

View reviewed changes

jjbayer and others added 5 commits May 28, 2024 11:27

wip

a99c1ae

wip

399814f

test

a2d7868

Fix

8bfc141

Fix

7a2c98e

jjbayer reviewed May 28, 2024

View reviewed changes

Fix

0d40b4c

Dav1dde reviewed May 28, 2024

View reviewed changes

iambriccardo and others added 3 commits May 28, 2024 16:49

Fix

b3194c9

Fix

53e68be

Update relay-pii/src/processor.rs

684b095

Co-authored-by: David Herberth <david.herberth@sentry.io>

iambriccardo added 3 commits May 28, 2024 17:01

Fix

48aef40

Merge branch 'riccardo/feat/add-scrubbing' of github.com:getsentry/re…

b96d3d7

…lay into riccardo/feat/add-scrubbing

Fix

123f6be

iambriccardo requested review from Dav1dde and jjbayer May 29, 2024 06:02

iambriccardo commented May 29, 2024

View reviewed changes

iambriccardo force-pushed the riccardo/feat/add-scrubbing branch from cd1cc8b to 123f6be Compare June 5, 2024 08:23

iambriccardo added 2 commits June 5, 2024 10:25

Fix

24d772b

Merge

d776b99

jjbayer approved these changes Jun 6, 2024

View reviewed changes

iambriccardo and others added 5 commits June 6, 2024 16:04

Update CHANGELOG.md

815a230

Co-authored-by: Joris Bayer <joris.bayer@sentry.io>

Fix

26842af

Fix

eff37fe

Fix

685cf83

Fix

d4af9b6

iambriccardo merged commit 962c91f into master Jun 10, 2024
22 checks passed

iambriccardo deleted the riccardo/feat/add-scrubbing branch June 10, 2024 06:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pii): Scrub arrays of 2 elements as key-value pairs #3639

feat(pii): Scrub arrays of 2 elements as key-value pairs #3639

iambriccardo commented May 23, 2024 •

edited

Loading

iambriccardo May 27, 2024

iambriccardo May 27, 2024

iambriccardo May 27, 2024

jjbayer left a comment

jjbayer May 28, 2024

iambriccardo May 28, 2024

Dav1dde May 28, 2024

iambriccardo May 28, 2024

jjbayer May 28, 2024

Dav1dde May 28, 2024

jjbayer May 28, 2024

iambriccardo May 28, 2024

iambriccardo May 29, 2024

jjbayer May 29, 2024

jjbayer Jun 5, 2024

jjbayer Jun 5, 2024

	let entered = state.enter_borrowed(key_name, state.inner_attrs(), value_type);
	processor::process_value(value, slf, &entered)?;

	let value_type = ValueType::for_field(&value[0]);
	let key_type = ValueType::for_field(&value[0]);

	struct PairListProcessor {
	/// Checks whether an array is a pair with a string key.
	struct PairCheck {

feat(pii): Scrub arrays of 2 elements as key-value pairs #3639

feat(pii): Scrub arrays of 2 elements as key-value pairs #3639

Conversation

iambriccardo commented May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambriccardo commented May 23, 2024 •

edited

Loading