Refactor converters to numeric types for aws_smithy_types::Number (#…

…1274)

Currently, conversions from `aws_smithy_types::Number` into numeric Rust
types (`{i,u}{8, 16, 32, 64}` and `f{32, 64}`) are always lossy, because
they use the `as` Rust keyword to cast into the target type. This means
that clients and servers are accepting lossy data: for example, if an
operation is modeled to take in a 32-bit integer as input, and a client
incorrectly sends an integer number that does not fit in 32 bits, the
server will silently accept the truncated input. There are malformed
request protocol tests that verify that servers must reject these
requests.

This commit removes the lossy `to_*` methods on `Number` and instead
implements `TryFrom<$typ> for Number` for the target numeric type
`$typ`. These converters will attempt their best to perform the
conversion safely, and fail if it is lossy.

The code-generated JSON parsers will now fail with
`aws_smithy_json::deserialize::ErrorReason::InvalidNumber` if the number
in the JSON document cannot be converted into the modeled integer type
without losing precision. For floating point target types, lossy
conversions are still performed, via `Number::to_f32_lossy` and
`Number::to_f64_lossy`.

CHANGELOG.next.toml

@@ -117,3 +117,63 @@ There is a canonical and easier way to run smithy-rs on Lambda [see example].
 references = ["smithy-rs#1551"]
 meta = { "breaking" = false, "tada" = true, "bug" = false, "target" = "server" }
 author = "hugobast"
+
+[[smithy-rs]]
+message = """
+Lossy converters into integer types for `aws_smithy_types::Number` have been
+removed. Lossy converters into floating point types for
+`aws_smithy_types::Number` have been suffixed with `_lossy`. If you were
+directly using the integer lossy converters, we recommend you use the safe
+converters.
+_Before:_
+```rust
+fn f1(n: aws_smithy_types::Number) {
+    let foo: f32 = n.to_f32(); // Lossy conversion!
+    let bar: u32 = n.to_u32(); // Lossy conversion!
+}
+```
+_After:_
+```rust
+fn f1(n: aws_smithy_types::Number) {
+    use std::convert::TryInto; // Unnecessary import if you're using Rust 2021 edition.
+    let foo: f32 = n.try_into().expect("lossy conversion detected"); // Or handle the error instead of panicking.
+    // You can still do lossy conversions, but only into floating point types.
+    let foo: f32 = n.to_f32_lossy();
+    // To lossily convert into integer types, use an `as` cast directly.
+    let bar: u32 = n as u32; // Lossy conversion!
+}
+```
+"""
+references = ["smithy-rs#1274"]
+meta = { "breaking" = true, "tada" = false, "bug" = true, "target" = "all" }
+author = "david-perez"
+
+[[aws-sdk-rust]]
+message = """
+Lossy converters into integer types for `aws_smithy_types::Number` have been
+removed. Lossy converters into floating point types for
+`aws_smithy_types::Number` have been suffixed with `_lossy`. If you were
+directly using the integer lossy converters, we recommend you use the safe
+converters.
+_Before:_
+```rust
+fn f1(n: aws_smithy_types::Number) {
+    let foo: f32 = n.to_f32(); // Lossy conversion!
+    let bar: u32 = n.to_u32(); // Lossy conversion!
+}
+```
+_After:_
+```rust
+fn f1(n: aws_smithy_types::Number) {
+    use std::convert::TryInto; // Unnecessary import if you're using Rust 2021 edition.
+    let foo: f32 = n.try_into().expect("lossy conversion detected"); // Or handle the error instead of panicking.
+    // You can still do lossy conversions, but only into floating point types.
+    let foo: f32 = n.to_f32_lossy();
+    // To lossily convert into integer types, use an `as` cast directly.
+    let bar: u32 = n as u32; // Lossy conversion!
+}
+```
+"""
+references = ["smithy-rs#1274"]
+meta = { "breaking" = true, "tada" = false, "bug" = true }
+author = "david-perez"

aws/rust-runtime/aws-config/src/credential_process.rs

@@ -194,7 +194,12 @@ pub(crate) fn parse_credential_process_json_credentials(
              "Expiration": "2022-05-02T18:36:00+00:00"
             */
             (key, Token::ValueNumber { value, .. }) if key.eq_ignore_ascii_case("Version") => {
-                version = Some(value.to_i32())
+                version = Some(i32::try_from(*value).map_err(|err| {
+                    InvalidJsonCredentials::InvalidField {
+                        field: "Version",
+                        err: err.into(),
+                    }
+                })?);
             }
             (key, Token::ValueString { value, .. }) if key.eq_ignore_ascii_case("AccessKeyId") => {
                 access_key_id = Some(value.to_unescaped()?)

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt

@@ -661,40 +661,14 @@ class ServerProtocolTestGenerator(
             FailingTest(RestJson, "RestJsonWithPayloadExpectsImpliedAccept", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonBodyMalformedBlobInvalidBase64_case1", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonBodyMalformedBlobInvalidBase64_case2", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyByteMalformedValueRejected_case2", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyByteMalformedValueRejected_case6", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyByteMalformedValueRejected_case8", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyByteMalformedValueRejected_case10", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyByteUnderflowOverflow_case0", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyByteUnderflowOverflow_case1", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyByteUnderflowOverflow_case2", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyByteUnderflowOverflow_case3", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonWithBodyExpectsApplicationJsonContentType", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonWithPayloadExpectsImpliedContentType", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonWithPayloadExpectsModeledContentType", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonWithoutBodyExpectsEmptyContentType", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyIntegerMalformedValueRejected_case2", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyIntegerMalformedValueRejected_case6", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyIntegerMalformedValueRejected_case8", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyIntegerMalformedValueRejected_case10", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyIntegerUnderflowOverflow_case0", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyIntegerUnderflowOverflow_case1", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonBodyMalformedListNullItem", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonBodyMalformedMapNullValue", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyLongMalformedValueRejected_case2", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyLongMalformedValueRejected_case6", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyLongMalformedValueRejected_case8", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyLongMalformedValueRejected_case10", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonMalformedSetDuplicateItems", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonMalformedSetNullItem", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyShortMalformedValueRejected_case2", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyShortMalformedValueRejected_case6", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyShortMalformedValueRejected_case8", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyShortMalformedValueRejected_case10", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyShortUnderflowOverflow_case0", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyShortUnderflowOverflow_case1", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyShortUnderflowOverflow_case2", TestType.MalformedRequest),
-            FailingTest(RestJson, "RestJsonBodyShortUnderflowOverflow_case3", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonHeaderMalformedStringInvalidBase64MediaType_case1", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonBodyTimestampDefaultRejectsMalformedEpochSeconds_case5", TestType.MalformedRequest),
             FailingTest(RestJson, "RestJsonBodyTimestampDefaultRejectsMalformedEpochSeconds_case7", TestType.MalformedRequest),

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt

@@ -278,8 +278,20 @@ class JsonParserGenerator(
     }
 
     private fun RustWriter.deserializeNumber(target: NumberShape) {
-        val symbol = symbolProvider.toSymbol(target)
-        rustTemplate("#{expect_number_or_null}(tokens.next())?.map(|v| v.to_#{T}())", "T" to symbol, *codegenScope)
+        if (target.isFloatShape) {
+            rustTemplate("#{expect_number_or_null}(tokens.next())?.map(|v| v.to_f32_lossy())", *codegenScope)
+        } else if (target.isDoubleShape) {
+            rustTemplate("#{expect_number_or_null}(tokens.next())?.map(|v| v.to_f64_lossy())", *codegenScope)
+        } else {
+            rustTemplate(
+                """
+                #{expect_number_or_null}(tokens.next())?
+                    .map(|v| v.try_into())
+                    .transpose()?
+                """,
+                *codegenScope,
+            )
+        }
     }
 
     private fun RustWriter.deserializeTimestamp(shape: TimestampShape, member: MemberShape) {

rust-runtime/aws-smithy-json/src/deserialize.rs

@@ -282,7 +282,7 @@ impl<'a> JsonTokenIterator<'a> {
     /// returns `(start_index, end_index, negative, floating)`, with `start_index`
     /// and `end_index` representing the slice of the stream that is the number,
     /// `negative` whether or not it is a negative number, and `floating` whether or not
-    /// it is a floating point number.
+    /// the number contains a decimal point and/or an exponent.
     fn scan_number(&mut self) -> (usize, usize, bool, bool) {
         let start_index = self.index;
         let negative = if self.peek_byte() == Some(b'-') {
@@ -338,7 +338,7 @@ impl<'a> JsonTokenIterator<'a> {
                 if negative > 0 {
                     Number::Float(-(positive as f64))
                 } else {
-                    Number::NegInt(negative as i64)
+                    Number::NegInt(negative)
                 }
             } else {
                 Number::PosInt(

rust-runtime/aws-smithy-json/src/deserialize/error.rs

@@ -82,3 +82,12 @@ impl From<EscapeError> for Error {
         }
     }
 }
+
+impl From<aws_smithy_types::TryFromNumberError> for Error {
+    fn from(_: aws_smithy_types::TryFromNumberError) -> Self {
+        Error {
+            reason: ErrorReason::InvalidNumber,
+            offset: None,
+        }
+    }
+}

rust-runtime/aws-smithy-json/src/deserialize/token.rs

@@ -217,7 +217,7 @@ pub fn expect_timestamp_or_null(
 ) -> Result<Option<DateTime>, Error> {
     Ok(match timestamp_format {
         Format::EpochSeconds => {
-            expect_number_or_null(token)?.map(|v| DateTime::from_secs_f64(v.to_f64()))
+            expect_number_or_null(token)?.map(|v| DateTime::from_secs_f64(v.to_f64_lossy()))
         }
         Format::DateTime | Format::HttpDate => expect_string_or_null(token)?
             .map(|v| DateTime::from_str(v.as_escaped_str(), timestamp_format))