Skip to content

Commit

Permalink
fix(spans): Max description length (#3115)
Browse files Browse the repository at this point in the history
Do not attempt to parse very long span description.

This should prevent stack overflows in the SQL parser.

We could also limit the length of the description field by setting a
`bag_size` attribute on it. Unfortunately, the `TrimmingProcessor` runs
after other normalization steps, so it would not help in this case.

ref: getsentry/team-ingest#272
  • Loading branch information
jjbayer authored Feb 15, 2024
1 parent a9baace commit eba85e3
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Fix regression in SQL query scrubbing. ([#3091](https://github.com/getsentry/relay/pull/3091))
- Normalize route in trace context data field. ([#3104](https://github.com/getsentry/relay/pull/3104))
- Limit the length of scrubbed span descriptions. ([#3115](https://github.com/getsentry/relay/pull/3115))

**Features**:

Expand Down
24 changes: 23 additions & 1 deletion relay-event-normalization/src/normalize/span/description/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ use crate::span::tag_extraction::HTTP_METHOD_EXTRACTOR_REGEX;
/// Dummy URL used to parse relative URLs.
static DUMMY_BASE_URL: Lazy<Url> = Lazy::new(|| "http://replace_me".parse().unwrap());

/// Very large SQL queries may cause stack overflows in the parser, so do not attempt to parse these.
const MAX_DESCRIPTION_LENGTH: usize = 10_000;

/// Maximum length of a resource URL segment.
///
/// Segments longer than this are treated as identifiers.
Expand All @@ -38,6 +41,14 @@ pub(crate) fn scrub_span_description(
return (None, None);
};

if description.len() > MAX_DESCRIPTION_LENGTH {
relay_log::error!(
description = description,
"Span description too large to parse"
);
return (None, None);
}

let data = span.data.value();

let db_system = data
Expand Down Expand Up @@ -421,7 +432,7 @@ mod tests {

// Same output and input means the input was already scrubbed.
// An empty output `""` means the input wasn't scrubbed and Relay didn't scrub it.
($name:ident, $description_in:literal, $op_in:literal, $expected:literal) => {
($name:ident, $description_in:expr, $op_in:literal, $expected:literal) => {
#[test]
fn $name() {
let json = format!(
Expand Down Expand Up @@ -911,6 +922,17 @@ mod tests {

span_description_test!(db_prisma, "User find", "db.sql.prisma", "User find");

span_description_test!(
long_description_none,
// Do not attempt to parse very long descriptions.
{
let repeated = "+1".repeat(5000);
&("SELECT 1".to_string() + &repeated)
},
"db.query",
""
);

#[test]
fn informed_sql_parser() {
let json = r#"
Expand Down

0 comments on commit eba85e3

Please sign in to comment.