From ec3547ab1c6bc4398635daf1370a5ee43c84c279 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 11 Feb 2025 12:22:41 -0500 Subject: [PATCH 1/3] Remove leading 0s before parsing into `serde_json::Number`. We previously decided to allow leading 0s, per this discussion: https://github.com/apollographql/router/pull/5762#discussion_r1711807550 However, since we use the parsing logic of `serde_json::Number` to produce the internal representation, and that logic fails given leading zeros, we need to normalize the input `&str` a bit before calling `number.parse()`. We already perform some similar normalizations, e.g. ensuring the fractional part is at least a 0 (not empty after the `.`), so there's precedent. --- .../connect/json_selection/lit_expr.rs | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/apollo-federation/src/sources/connect/json_selection/lit_expr.rs b/apollo-federation/src/sources/connect/json_selection/lit_expr.rs index 517dafafc0..528216ea0f 100644 --- a/apollo-federation/src/sources/connect/json_selection/lit_expr.rs +++ b/apollo-federation/src/sources/connect/json_selection/lit_expr.rs @@ -96,7 +96,14 @@ impl LitExpr { ); let mut s = String::new(); - s.push_str(int.fragment()); + let int_str = int.fragment(); + // Remove leading zeros to avoid failing the stricter + // number.parse() below, but allow a single zero. + if let Some(first_non_zero) = int_str.chars().position(|c| c != '0') { + s.push_str(&int_str[first_non_zero..]); + } else { + s.push('0'); + } let full_range = if let Some((_, dot, _, frac)) = frac { let frac_range = merge_ranges( @@ -277,6 +284,7 @@ mod tests { use crate::sources::connect::json_selection::location::new_span; use crate::sources::connect::json_selection::PathList; + #[track_caller] fn check_parse(input: &str, expected: LitExpr) { match LitExpr::parse(new_span(input)) { Ok((remainder, parsed)) => { @@ -317,6 +325,24 @@ mod tests { "-123.", LitExpr::Number(serde_json::Number::from_f64(-123.0).unwrap()), ); + check_parse("00", LitExpr::Number(serde_json::Number::from(0))); + check_parse( + "-00", + LitExpr::Number(serde_json::Number::from_f64(-0.0).unwrap()), + ); + check_parse("0", LitExpr::Number(serde_json::Number::from(0))); + check_parse( + "-0", + LitExpr::Number(serde_json::Number::from_f64(-0.0).unwrap()), + ); + check_parse(" 00 ", LitExpr::Number(serde_json::Number::from(0))); + check_parse(" 0 ", LitExpr::Number(serde_json::Number::from(0))); + check_parse( + " - 0 ", + LitExpr::Number(serde_json::Number::from_f64(-0.0).unwrap()), + ); + check_parse("001", LitExpr::Number(serde_json::Number::from(1))); + check_parse("0010", LitExpr::Number(serde_json::Number::from(10))); check_parse("true", LitExpr::Bool(true)); check_parse(" true ", LitExpr::Bool(true)); From 7a76bec3797289656c07f2dbc7aae9be91d36d3d Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 11 Feb 2025 12:32:47 -0500 Subject: [PATCH 2/3] Create changeset entry for leading 0s fix. Part of #6766. --- .changesets/fix_sushi_caboose_police_agency.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changesets/fix_sushi_caboose_police_agency.md diff --git a/.changesets/fix_sushi_caboose_police_agency.md b/.changesets/fix_sushi_caboose_police_agency.md new file mode 100644 index 0000000000..fb9839b0e8 --- /dev/null +++ b/.changesets/fix_sushi_caboose_police_agency.md @@ -0,0 +1,9 @@ +### Remove leading 0s before parsing into `serde_json::Number` ([PR #6766](https://github.com/apollographql/router/pull/6766)) + +We previously decided to allow leading 0s, per [this discussion](https://github.com/apollographql/router/pull/5762#discussion_r1711807550). + +However, since we use the parsing logic of `serde_json::Number` to produce the internal representation, and that logic fails given leading zeros, we need to normalize the input `&str` a bit before calling `number.parse()`. We already perform some similar normalizations, e.g. ensuring the fractional part is at least a 0 (not empty after the `.`), so there's precedent. + +Thanks to @nicholascioli for finding this problem using fuzz testing! + +By [@benjamn](https://github.com/benjamn) in https://github.com/apollographql/router/pull/6766 \ No newline at end of file From bfccf2a3a0f8fa7ebfbc495a4c8e616ddb3443c5 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 11 Feb 2025 15:24:07 -0500 Subject: [PATCH 3/3] Address review feedback. https://github.com/apollographql/router/pull/6766#discussion_r1951468922 https://github.com/apollographql/router/pull/6766#discussion_r1951470757 --- .../connect/json_selection/lit_expr.rs | 35 +++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/apollo-federation/src/sources/connect/json_selection/lit_expr.rs b/apollo-federation/src/sources/connect/json_selection/lit_expr.rs index 528216ea0f..e753626ffa 100644 --- a/apollo-federation/src/sources/connect/json_selection/lit_expr.rs +++ b/apollo-federation/src/sources/connect/json_selection/lit_expr.rs @@ -96,11 +96,14 @@ impl LitExpr { ); let mut s = String::new(); - let int_str = int.fragment(); + // Remove leading zeros to avoid failing the stricter // number.parse() below, but allow a single zero. - if let Some(first_non_zero) = int_str.chars().position(|c| c != '0') { - s.push_str(&int_str[first_non_zero..]); + let mut int_chars_without_leading_zeros = + int.fragment().chars().skip_while(|c| *c == '0'); + if let Some(first_non_zero) = int_chars_without_leading_zeros.next() { + s.push(first_non_zero); + s.extend(int_chars_without_leading_zeros); } else { s.push('0'); } @@ -342,7 +345,33 @@ mod tests { LitExpr::Number(serde_json::Number::from_f64(-0.0).unwrap()), ); check_parse("001", LitExpr::Number(serde_json::Number::from(1))); + check_parse( + "00.1", + LitExpr::Number(serde_json::Number::from_f64(0.1).unwrap()), + ); check_parse("0010", LitExpr::Number(serde_json::Number::from(10))); + check_parse( + "00.10", + LitExpr::Number(serde_json::Number::from_f64(0.1).unwrap()), + ); + check_parse("-001 ", LitExpr::Number(serde_json::Number::from(-1))); + check_parse( + "-00.1", + LitExpr::Number(serde_json::Number::from_f64(-0.1).unwrap()), + ); + check_parse(" - 0010 ", LitExpr::Number(serde_json::Number::from(-10))); + check_parse( + "- 00.10", + LitExpr::Number(serde_json::Number::from_f64(-0.1).unwrap()), + ); + check_parse( + "007.", + LitExpr::Number(serde_json::Number::from_f64(7.0).unwrap()), + ); + check_parse( + "-007.", + LitExpr::Number(serde_json::Number::from_f64(-7.0).unwrap()), + ); check_parse("true", LitExpr::Bool(true)); check_parse(" true ", LitExpr::Bool(true));