Merge branch 'master' into fix/normalize-tags

* master:
  ref(pii): Use external utf16string crate (#807)
  feat(pii): Merge padding and masking characters (#810)
  fix(server): Rate limit outcomes emited only for events on "fast-path" (#809)
  fix(server): Rate limit outcomes emited only for events (#806)

Author: jan-auer

CHANGELOG.md

@@ -7,6 +7,7 @@
 - Add support for measurement ingestion. ([#724](https://github.com/getsentry/relay/pull/724), [#785](https://github.com/getsentry/relay/pull/785))
 - Add support for scrubbing UTF-16 data in attachments ([#742](https://github.com/getsentry/relay/pull/742), [#784](https://github.com/getsentry/relay/pull/784), [#787](https://github.com/getsentry/relay/pull/787))
 - Add upstream request metric. ([#793](https://github.com/getsentry/relay/pull/793))
+- The padding character in attachment scrubbing has been changed to match the masking character, there is no usability benefit from them being different. ([#810](https://github.com/getsentry/relay/pull/810))
 
 **Bug Fixes**:
 
@@ -18,6 +19,7 @@
 
 - Project states are now cached separately per DSN public key instead of per project ID. This means that there will be multiple separate cache entries for projects with more than one DSN. ([#778](https://github.com/getsentry/relay/pull/778))
 - Relay no longer uses the Sentry endpoint to resolve project IDs by public key. Ingestion for the legacy store endpoint has been refactored to rely on key-based caches only. As a result, the legacy endpoint is supported only on managed Relays. ([#800](https://github.com/getsentry/relay/pull/800))
+- Fix rate limit outcomes, now emitted only for error events but not transactions. ([#806](https://github.com/getsentry/relay/pull/806), [#809](https://github.com/getsentry/relay/pull/809))
 
 ## 20.9.0
 

Cargo.lock

@@ -120,6 +120,11 @@ impl DataCategory {
             Self::Unknown => "unknown",
         }
     }
+
+    /// Returns true if the DataCategory refers to an error (i.e an error event).
+    pub fn is_error(self) -> bool {
+        matches!(self, Self::Error | Self::Default | Self::Security)
+    }
 }
 
 impl fmt::Display for DataCategory {

relay-common/src/constants.rs

@@ -27,7 +27,6 @@ pest_derive = "2.1.0"
 regex = "1.3.9"
 relay-common = { path = "../relay-common" }
 relay-general-derive = { path = "derive" }
-relay-wstring = { path = "../relay-wstring" }
 schemars = { version = "0.8.0-alpha-4", features = ["uuid", "chrono"], optional = true }
 scroll = "0.9.0"
 serde = { version = "1.0.114", features = ["derive"] }
@@ -37,6 +36,7 @@ sha-1 = "0.8.1"
 smallvec = { version = "1.4.0", features = ["serde"] }
 uaparser = { version = "0.3.3", optional = true }
 url = "2.1.1"
+utf16string = "0.2.0"
 uuid = { version = "0.8.1", features = ["v4", "serde"] }
 
 [dev-dependencies]

relay-general/Cargo.toml

@@ -4,8 +4,7 @@ use std::iter::FusedIterator;
 use regex::bytes::RegexBuilder as BytesRegexBuilder;
 use regex::{Match, Regex};
 use smallvec::SmallVec;
-
-use relay_wstring::WStr;
+use utf16string::{LittleEndian, WStr};
 
 use crate::pii::compiledconfig::RuleRef;
 use crate::pii::regexes::{get_regex_for_rule_type, ReplaceBehavior};
@@ -106,7 +105,11 @@ fn apply_regex_to_utf16le_bytes(
 }
 
 /// Extract the matching encoded slice from the encoded string.
-fn get_wstr_match<'a>(all_text: &str, re_match: Match, all_encoded: &'a mut WStr) -> &'a mut WStr {
+fn get_wstr_match<'a>(
+    all_text: &str,
+    re_match: Match,
+    all_encoded: &'a mut WStr<LittleEndian>,
+) -> &'a mut WStr<LittleEndian> {
     let mut encoded_start = 0;
     let mut encoded_end = all_encoded.len();
 
@@ -151,7 +154,7 @@ trait StringMods: AsRef<[u8]> {
 
     /// Apply a PII scrubbing redaction to this string slice.
     fn apply_redaction(&mut self, redaction: &Redaction) {
-        const PADDING: char = 'x';
+        const PADDING: char = '*';
         const MASK: char = '*';
 
         match redaction {
@@ -172,7 +175,7 @@ trait StringMods: AsRef<[u8]> {
     }
 }
 
-impl StringMods for WStr {
+impl StringMods for WStr<LittleEndian> {
     fn fill_content(&mut self, fill_char: char) {
         // If fill_char is too wide, fill_char.encode_utf16() will panic, fulfilling the
         // trait's contract that we must panic if fill_char is too wide.
@@ -325,7 +328,7 @@ impl<'a> FusedIterator for WStrSegmentIter<'a> {}
 /// longer match.
 struct WStrSegment<'a> {
     /// The raw bytes of this segment.
-    encoded: &'a mut WStr,
+    encoded: &'a mut WStr<LittleEndian>,
     /// The decoded string of this segment.
     decoded: String,
 }
@@ -469,7 +472,11 @@ impl<'a> PiiAttachmentsProcessor<'a> {
     }
 
     /// Scrub a filepath, preserving the basename.
-    pub fn scrub_utf16_filepath(&self, path: &mut WStr, state: &ProcessingState<'_>) -> bool {
+    pub fn scrub_utf16_filepath(
+        &self,
+        path: &mut WStr<LittleEndian>,
+        state: &ProcessingState<'_>,
+    ) -> bool {
         let index =
             path.char_indices().rev().find_map(
                 |(i, c)| {
@@ -600,7 +607,7 @@ mod tests {
             filename: "foo.txt",
             value_type: ValueType::Binary,
             input: b"before 127.0.0.1 after",
-            output: b"before [ip]xxxxx after",
+            output: b"before [ip]***** after",
             changed: true,
         }
         .run();
@@ -614,7 +621,7 @@ mod tests {
             filename: "foo.txt",
             value_type: ValueType::Binary,
             input: utf16le("before 127.0.0.1 after").as_slice(),
-            output: utf16le("before [ip]xxxxx after").as_slice(),
+            output: utf16le("before [ip]***** after").as_slice(),
             changed: true,
         }
         .run();
@@ -684,7 +691,7 @@ mod tests {
             filename: "foo.txt",
             value_type: ValueType::Binary,
             input: b"before 127.0.0.1 after",
-            output: b"before xxxxxxxxx after",
+            output: b"before ********* after",
             changed: true,
         }
         .run();
@@ -698,7 +705,7 @@ mod tests {
             filename: "foo.txt",
             value_type: ValueType::Binary,
             input: utf16le("before 127.0.0.1 after").as_slice(),
-            output: utf16le("before xxxxxxxxx after").as_slice(),
+            output: utf16le("before ********* after").as_slice(),
             changed: true,
         }
         .run();
@@ -734,7 +741,7 @@ mod tests {
             filename: "foo.txt",
             value_type: ValueType::Binary,
             input: (0..255 as u8).collect::<Vec<_>>().as_slice(),
-            output: &[b'x'; 255],
+            output: &[b'*'; 255],
             changed: true,
         }
         .run();
@@ -766,7 +773,7 @@ mod tests {
                 filename: "foo.txt",
                 value_type: ValueType::Binary,
                 input: bytes,
-                output: &vec![b'x'; bytes.len()],
+                output: &vec![b'*'; bytes.len()],
                 changed: true,
             }
             .run()

relay-general/src/pii/attachments.rs

@@ -15,8 +15,7 @@ use minidump::{
 };
 use num_traits::FromPrimitive;
 use scroll::Endian;
-
-use relay_wstring::{Utf16Error, WStr};
+use utf16string::{Utf16Error, WStr};
 
 use crate::pii::{PiiAttachmentsProcessor, ScrubEncodings};
 use crate::processor::{FieldAttrs, Pii, ValueType};

relay-general/src/pii/minidumps.rs

@@ -286,6 +286,21 @@ impl RateLimits {
     pub fn longest(&self) -> Option<&RateLimit> {
         self.iter().max_by_key(|limit| limit.retry_after)
     }
+
+    /// Returns the longest rate limit that is error releated.
+    ///
+    /// The most relevant rate limit from the point of view of an error generating an outcome
+    /// is the longest rate limit for error messages.
+    pub fn longest_error(&self) -> Option<&RateLimit> {
+        let is_event_related = |rate_limit: &&RateLimit| {
+            rate_limit.categories.is_empty()
+                || rate_limit.categories.iter().any(|cat| cat.is_error())
+        };
+
+        self.iter()
+            .filter(is_event_related)
+            .max_by_key(|limit| limit.retry_after)
+    }
 }
 
 /// Immutable rate limits iterator.
@@ -651,6 +666,68 @@ mod tests {
         "###);
     }
 
+    #[test]
+    fn test_rate_limits_longest_error_none() {
+        let mut rate_limits = RateLimits::new();
+
+        rate_limits.add(RateLimit {
+            categories: smallvec![DataCategory::Transaction],
+            scope: RateLimitScope::Organization(42),
+            reason_code: None,
+            retry_after: RetryAfter::from_secs(1),
+        });
+        rate_limits.add(RateLimit {
+            categories: smallvec![DataCategory::Attachment],
+            scope: RateLimitScope::Organization(42),
+            reason_code: None,
+            retry_after: RetryAfter::from_secs(1),
+        });
+        rate_limits.add(RateLimit {
+            categories: smallvec![DataCategory::Session],
+            scope: RateLimitScope::Organization(42),
+            reason_code: None,
+            retry_after: RetryAfter::from_secs(1),
+        });
+
+        // only non event rate limits so nothing relevant
+        assert_eq!(rate_limits.longest_error(), None)
+    }
+
+    #[test]
+    fn test_rate_limits_longest_error() {
+        let mut rate_limits = RateLimits::new();
+        rate_limits.add(RateLimit {
+            categories: smallvec![DataCategory::Transaction],
+            scope: RateLimitScope::Organization(40),
+            reason_code: None,
+            retry_after: RetryAfter::from_secs(100),
+        });
+        rate_limits.add(RateLimit {
+            categories: smallvec![DataCategory::Error],
+            scope: RateLimitScope::Organization(41),
+            reason_code: None,
+            retry_after: RetryAfter::from_secs(5),
+        });
+        rate_limits.add(RateLimit {
+            categories: smallvec![DataCategory::Error],
+            scope: RateLimitScope::Organization(42),
+            reason_code: None,
+            retry_after: RetryAfter::from_secs(7),
+        });
+
+        let rate_limit = rate_limits.longest().unwrap();
+        insta::assert_ron_snapshot!(rate_limit, @r###"
+        RateLimit(
+          categories: [
+            transaction,
+          ],
+          scope: Organization(40),
+          reason_code: None,
+          retry_after: RetryAfter(100),
+        )
+        "###);
+    }
+
     #[test]
     fn test_rate_limits_clean_expired() {
         let mut rate_limits = RateLimits::new();

relay-quotas/src/rate_limit.rs

@@ -143,7 +143,7 @@ impl ProcessingError {
             Self::DuplicateItem(_) => Some(Outcome::Invalid(DiscardReason::DuplicateItem)),
             Self::NoEventPayload => Some(Outcome::Invalid(DiscardReason::NoEventPayload)),
             Self::RateLimited(ref rate_limits) => rate_limits
-                .longest()
+                .longest_error()
                 .map(|r| Outcome::RateLimited(r.reason_code.clone())),
 
             // Processing-only outcomes (Sentry-internal Relays)

relay-server/src/actors/events.rs

@@ -79,8 +79,8 @@ pub enum BadStoreRequest {
 }
 
 impl BadStoreRequest {
-    fn to_outcome(&self) -> Outcome {
-        match self {
+    fn to_outcome(&self) -> Option<Outcome> {
+        Some(match self {
             BadStoreRequest::UnsupportedProtocolVersion(_) => {
                 Outcome::Invalid(DiscardReason::AuthVersion)
             }
@@ -119,16 +119,14 @@ impl BadStoreRequest {
             }
 
             BadStoreRequest::RateLimited(rate_limits) => {
-                let reason_code = rate_limits
-                    .longest()
-                    .and_then(|limit| limit.reason_code.clone());
-
-                Outcome::RateLimited(reason_code)
+                return rate_limits
+                    .longest_error()
+                    .map(|r| Outcome::RateLimited(r.reason_code.clone()));
             }
 
             // should actually never create an outcome
             BadStoreRequest::InvalidEventId => Outcome::Invalid(DiscardReason::Internal),
-        }
+        })
     }
 }
 
@@ -457,13 +455,15 @@ where
             metric!(counter(RelayCounters::EnvelopeRejected) += 1);
 
             if is_event {
-                outcome_producer.do_send(TrackOutcome {
-                    timestamp: start_time,
-                    scoping: *scoping.borrow(),
-                    outcome: error.to_outcome(),
-                    event_id: *event_id.borrow(),
-                    remote_addr,
-                });
+                if let Some(outcome) = error.to_outcome() {
+                    outcome_producer.do_send(TrackOutcome {
+                        timestamp: start_time,
+                        scoping: *scoping.borrow(),
+                        outcome,
+                        event_id: *event_id.borrow(),
+                        remote_addr,
+                    });
+                }
             }
 
             if !emit_rate_limit && matches!(error, BadStoreRequest::RateLimited(_)) {