From 3c7754b98e09d965973bf6580d48dcce9216e022 Mon Sep 17 00:00:00 2001
From: Shon Feder <shon@informal.systems>
Date: Tue, 23 Feb 2021 10:40:43 -0500
Subject: [PATCH] Property based tests of Time (#815)

* Proptest regressions for inversion of Time serde

* Test property that Time's from_value inverts its to_value

* Add parse time PBT failure

* Use prop_assert! for cleaner results

* Test that Time can parse RFC3339 timestamps

* Remove spurious test failure

* Only generate stamps for valid times

And don't test dates before the epoch

* Reduce range of input params in Time PBT

* Add regression cases

* Make timestamp serialization safe

Without this change, serde of timestamps would result in a panic if the
stamp was before the UNIX epoch. In discussion with @thanethomson, we
agreed this opened up a potential attack vector, since a node could be
sent messages causing it to crash on deserialization.

It also violated the documented contract implied by the `From` trait:

> Note: This trait must not fail. If the conversion can fail, use TryFrom.

https://doc.rust-lang.org/std/convert/trait.From.html

The fix is simply to use `impl From(DateTime<Utc>) for SystemTime`
supplied by chrono.

* Cleanup and comment

* More succinct parameter in safe timestamps test

* Clippy fixes
---
 tendermint/Cargo.toml                    |   4 +
 tendermint/proptest-regressions/time.txt |   9 ++
 tendermint/src/time.rs                   | 195 +++++++++++++++++++++--
 3 files changed, 191 insertions(+), 17 deletions(-)
 create mode 100644 tendermint/proptest-regressions/time.txt

diff --git a/tendermint/Cargo.toml b/tendermint/Cargo.toml
index 9caa6eaf1..1eb6fd84e 100644
--- a/tendermint/Cargo.toml
+++ b/tendermint/Cargo.toml
@@ -64,3 +64,7 @@ ripemd160 = { version = "0.9", optional = true }
 
 [features]
 secp256k1 = ["k256", "ripemd160"]
+
+[dev-dependencies]
+
+proptest = "0.10.1"
diff --git a/tendermint/proptest-regressions/time.txt b/tendermint/proptest-regressions/time.txt
new file mode 100644
index 000000000..affc86d67
--- /dev/null
+++ b/tendermint/proptest-regressions/time.txt
@@ -0,0 +1,9 @@
+# Seeds for failure cases proptest has generated in the past. It is
+# automatically read and these particular cases re-run before any
+# novel cases are generated.
+#
+# It is recommended to check this file in to source control so that
+# everyone who runs the test benefits from these saved cases.
+cc 7dcafc2d86a88d57fde70fe8b6dfbe9359a4d470b04fcd4bf3a7b0f05c2274a0 # shrinks to time = Time(1970-01-01T00:00:01Z)
+cc 9f588cd1ad2e3715e8f3cae1b2e2738bef53f9f75c582e4e9e0f2b887328e6c2 # shrinks to time = Time(2262-04-11T23:47:17Z)
+cc 6e17ffd49bafd6f5e8ddabc01784b27eea9c5213235aaca28f78607b1906f819 # shrinks to stamp = "0000-01-01T00:00:00-00:00"
diff --git a/tendermint/src/time.rs b/tendermint/src/time.rs
index 06ec79e38..43e7c2aed 100644
--- a/tendermint/src/time.rs
+++ b/tendermint/src/time.rs
@@ -41,7 +41,7 @@ impl From<Time> for Timestamp {
     fn from(value: Time) -> Self {
         // prost_types::Timestamp has a SystemTime converter but
         // tendermint_proto::Timestamp can be JSON-encoded
-        let prost_value = prost_types::Timestamp::from(value.to_system_time().unwrap());
+        let prost_value = prost_types::Timestamp::from(SystemTime::from(value));
         Timestamp {
             seconds: prost_value.seconds,
             nanos: prost_value.nanos,
@@ -78,12 +78,6 @@ impl Time {
     pub fn to_rfc3339(&self) -> String {
         timestamp::to_rfc3339_nanos(&self.0)
     }
-
-    /// Convert [`Time`] to [`SystemTime`]
-    pub fn to_system_time(&self) -> Result<SystemTime, Error> {
-        let duration_since_epoch = self.duration_since(Self::unix_epoch())?;
-        Ok(UNIX_EPOCH + duration_since_epoch)
-    }
 }
 
 impl fmt::Display for Time {
@@ -120,7 +114,7 @@ impl From<SystemTime> for Time {
 
 impl From<Time> for SystemTime {
     fn from(t: Time) -> SystemTime {
-        t.to_system_time().unwrap()
+        t.0.into()
     }
 }
 
@@ -150,11 +144,145 @@ pub trait ParseTimestamp {
 
 #[cfg(test)]
 mod tests {
+    use std::convert::TryInto;
+
     use super::*;
 
-    #[test]
-    fn serde_roundtrip() {
-        const DATES: &[&str] = &[
+    // TODO(shon) Extract arbitrary generators into own library
+
+    use chrono::{DateTime, NaiveDate, TimeZone, Timelike, Utc};
+    use proptest::{prelude::*, sample::select};
+
+    // Any higher, and we're at seconds
+    const MAX_NANO_SECS: u32 = 999_999_999u32;
+
+    // With values larger or smaller then these, chrono produces invalid rfc3339
+    // timestamps. See https://github.com/chronotope/chrono/issues/537
+    fn min_time() -> DateTime<Utc> {
+        Utc.timestamp(-9999999999, 0)
+    }
+
+    fn max_time() -> DateTime<Utc> {
+        Utc.timestamp(99999999999, 0)
+    }
+
+    fn num_days_in_month(year: i32, month: u32) -> u32 {
+        // Using chrono, we get the duration beteween this month and the next,
+        // then count the number of days in that duration.  See
+        // https://stackoverflow.com/a/58188385/1187277
+        let given_month = NaiveDate::from_ymd(year, month, 1);
+        let next_month = NaiveDate::from_ymd(
+            if month == 12 { year + 1 } else { year },
+            if month == 12 { 1 } else { month + 1 },
+            1,
+        );
+        next_month
+            .signed_duration_since(given_month)
+            .num_days()
+            .try_into()
+            .unwrap()
+    }
+
+    prop_compose! {
+        /// An abitrary `chrono::DateTime` that is between `min` and `max`
+        /// DateTimes.
+        fn arb_datetime_in_range(min: DateTime<Utc>, max: DateTime<Utc>)(
+            secs in min.timestamp()..max.timestamp()
+        )(
+            // min mano secods is only relevant if we happen to hit the minimum
+            // seconds on the nose.
+            nano in (if secs == min.timestamp() { min.nanosecond() } else { 0 })..MAX_NANO_SECS,
+            // Make secs in scope
+            secs in Just(secs),
+        ) -> DateTime<Utc> {
+            println!(">> Secs {:?}", secs);
+            Utc.timestamp(secs, nano)
+        }
+    }
+
+    prop_compose! {
+        /// An abitrary `chrono::DateTime`
+        fn arb_datetime()
+            (
+                d in arb_datetime_in_range(min_time(), max_time())
+            ) -> DateTime<Utc> {
+                d
+            }
+    }
+
+    prop_compose! {
+        fn arb_rfc339_time_offset()(
+            sign in "[+-]",
+            hour in 0..23u8,
+            min in 0..59u8,
+        ) -> String {
+            format!("{:}{:0>2}:{:0>2}", sign, hour, min)
+        }
+    }
+
+    fn arb_rfc3339_offset() -> impl Strategy<Value = String> {
+        prop_oneof![arb_rfc339_time_offset(), Just("Z".to_owned())]
+    }
+
+    prop_compose! {
+        fn arb_rfc3339_partial_time()(
+            hour in 0..23u8,
+            min in 0..59u8,
+            sec in 0..59u8,
+            secfrac in proptest::option::of(0..u64::MAX),
+        ) -> String {
+            let frac = match secfrac {
+                None => "".to_owned(),
+                Some(frac) => format!(".{:}", frac)
+            };
+            format!("{:0>2}:{:0>2}:{:0>2}{:}", hour, min, sec, frac)
+        }
+    }
+
+    prop_compose! {
+        fn arb_rfc3339_full_time()(
+            time in arb_rfc3339_partial_time(),
+            offset in arb_rfc3339_offset()
+        ) -> String {
+            format!("{:}{:}", time, offset)
+        }
+    }
+
+    prop_compose! {
+        fn arb_rfc3339_day_of_year_and_month(year: i32, month: u32)
+            (
+                d in 1..num_days_in_month(year, month)
+            ) -> u32 {
+            d
+        }
+    }
+
+    prop_compose! {
+        fn arb_rfc3339_full_date()(year in 0..9999i32, month in 1..12u32)
+            (
+                day in arb_rfc3339_day_of_year_and_month(year, month),
+                year in Just(year),
+                month in Just(month),
+            ) -> String {
+                format!("{:0>4}-{:0>2}-{:0>2}", year, month, day)
+            }
+    }
+
+    prop_compose! {
+        /// An aribtrary rfc3339 timestamp
+        ///
+        /// Follows https://tools.ietf.org/html/rfc3339#section-5.6
+        fn arb_rfc3339_timestamp()(
+            date in arb_rfc3339_full_date(),
+            time in arb_rfc3339_full_time()
+        ) -> String {
+            format!("{:}T{:}", date, time)
+        }
+    }
+
+    // We want to make sure that these timestamps specifically get tested.
+    fn particular_rfc3339_timestamps() -> impl Strategy<Value = String> {
+        let strs: Vec<String> = vec![
             "2020-09-14T16:33:54.21191421Z",
             "2020-09-14T16:33:00Z",
             "2020-09-14T16:33:00.1Z",
@@ -171,14 +299,47 @@ mod tests {
             "2021-01-07T20:26:12.078268Z",
             "2021-01-07T20:26:13.08074100Z",
             "2021-01-07T20:26:15.079663000Z",
-        ];
+        ]
+        .into_iter()
+        .map(String::from)
+        .collect();
 
-        for input in DATES {
-            let initial_time: Time = input.parse().unwrap();
-            let encoded_time = serde_json::to_value(&initial_time).unwrap();
-            let decoded_time = serde_json::from_value(encoded_time.clone()).unwrap();
+        select(strs)
+    }
+
+    proptest! {
+        #[test]
+        fn can_parse_rfc3339_timestamps(stamp in arb_rfc3339_timestamp()) {
+            prop_assert!(stamp.parse::<Time>().is_ok())
+        }
+
+        #[test]
+        fn serde_from_value_is_the_inverse_of_to_value_within_reasonable_time_range(
+            datetime in arb_datetime()
+        ) {
+            // If `from_value` is the inverse of `to_value`, then it will always
+            // map the JSON `encoded_time` to back to the inital `time`.
+            let time: Time = datetime.into();
+            let json_encoded_time = serde_json::to_value(&time).unwrap();
+            let decoded_time: Time = serde_json::from_value(json_encoded_time).unwrap();
+            prop_assert_eq!(time, decoded_time);
+        }
 
-            assert_eq!(initial_time, decoded_time);
+        #[test]
+        fn serde_of_rfc3339_timestamps_is_safe(
+            stamp in prop_oneof![
+                arb_rfc3339_timestamp(),
+                particular_rfc3339_timestamps(),
+            ]
+        ) {
+            // ser/de of rfc3339 timestamps is safe if it never panics.
+            // This differes from the the inverse test in that we are testing on
+            // arbitrarily generated textual timestamps, rather than times in a
+            // range. Tho we do incidentally test the inversion as well.
+            let time: Time = stamp.parse().unwrap();
+            let json_encoded_time = serde_json::to_value(&time).unwrap();
+            let decoded_time: Time = serde_json::from_value(json_encoded_time).unwrap();
+            prop_assert_eq!(time, decoded_time);
         }
     }
 }