From 20498937fb24189d55112b8af9f7efc60e22b5ab Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Wed, 8 Nov 2023 18:29:44 +0100 Subject: [PATCH 1/7] MonitorSchedule constructor the validates crontab syntax --- sentry-types/src/crontab_validator.rs | 79 +++++++++++++++++++++++++++ sentry-types/src/lib.rs | 1 + sentry-types/src/protocol/monitor.rs | 17 ++++++ 3 files changed, 97 insertions(+) create mode 100644 sentry-types/src/crontab_validator.rs diff --git a/sentry-types/src/crontab_validator.rs b/sentry-types/src/crontab_validator.rs new file mode 100644 index 00000000..834c6963 --- /dev/null +++ b/sentry-types/src/crontab_validator.rs @@ -0,0 +1,79 @@ +use std::collections::HashSet; + +#[derive(PartialEq, Eq, Hash)] +enum CronToken<'a> { + Numeric(u64), + Alphabetic(&'a str), +} + +const MONTHS: &[&str] = &[ + "jan", "feb", "mar", "apr", "may", "jun", "jul", "aug", "sep", "oct", "nov", "dec", +]; + +const DAYS: &[&str] = &["sun", "mon", "tue", "wed", "thu", "fri", "sat"]; + +fn value_is_allowed(value: &str, allowed_values: &HashSet) -> bool { + match value.parse::() { + Ok(numeric_value) => allowed_values.contains(&CronToken::Numeric(numeric_value)), + Err(_) => allowed_values.contains(&CronToken::Alphabetic(&value.to_lowercase())), + } +} + +fn validate_range(range: &str, allowed_values: &HashSet) -> bool { + range == "*" + || range // TODO: Validate that the last range bound is after the previous one. + .splitn(2, "-") + .all(|bound| value_is_allowed(bound, allowed_values)) +} + +/// A valid step is None or Some positive value +fn validate_step(step: &Option<&str>) -> bool { + match *step { + Some(value) => match value.parse::() { + Ok(value) => value > 0, + Err(_) => false, + }, + None => true, + } +} + +fn validate_steprange(steprange: &str, allowed_values: &HashSet) -> bool { + let mut steprange_split = steprange.splitn(2, "/"); + let range = match steprange_split.next() { + Some(range) => range, + None => { + return false; + } + }; + let range_is_valid = validate_range(range, allowed_values); + let step = steprange_split.next(); + + range_is_valid && validate_step(&step) +} + +fn validate_segment(segment: &str, allowed_values: &HashSet) -> bool { + segment + .split(",") + .all(|steprange| validate_steprange(steprange, &allowed_values)) +} + +pub fn validate(crontab: &str) -> bool { + let allowed_values = vec![ + (0..60).map(CronToken::Numeric).collect(), + (0..24).map(CronToken::Numeric).collect(), + (1..32).map(CronToken::Numeric).collect(), + (1..13) + .map(CronToken::Numeric) + .chain(MONTHS.iter().map(|&month| CronToken::Alphabetic(month))) + .collect(), + (0..8) + .map(CronToken::Numeric) + .chain(DAYS.iter().map(|&day| CronToken::Alphabetic(day))) + .collect(), + ]; + + crontab + .split_whitespace() + .zip(allowed_values) + .all(|(segment, allowed_values)| validate_segment(segment, &allowed_values)) +} diff --git a/sentry-types/src/lib.rs b/sentry-types/src/lib.rs index 093f9850..321e1088 100644 --- a/sentry-types/src/lib.rs +++ b/sentry-types/src/lib.rs @@ -40,6 +40,7 @@ mod macros; mod auth; +mod crontab_validator; mod dsn; mod project_id; pub mod protocol; diff --git a/sentry-types/src/protocol/monitor.rs b/sentry-types/src/protocol/monitor.rs index 89eaff3e..370686f2 100644 --- a/sentry-types/src/protocol/monitor.rs +++ b/sentry-types/src/protocol/monitor.rs @@ -1,6 +1,8 @@ use serde::{Deserialize, Serialize, Serializer}; use uuid::Uuid; +use crate::crontab_validator; + /// Represents the status of the monitor check-in #[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize)] #[serde(rename_all = "snake_case")] @@ -39,6 +41,21 @@ pub enum MonitorSchedule { }, } +impl MonitorSchedule { + /// Attempts to create a MonitorSchedule from a provided crontab_str. If the crontab_str is a + /// valid crontab schedule, we return the MonitorSchedule wrapped in a Some variant. Otherwise, + /// we return None. + pub fn from_crontab(crontab_str: &str) -> Option { + if crontab_validator::validate(crontab_str) { + Some(Self::Crontab { + value: String::from(crontab_str), + }) + } else { + None + } + } +} + /// The unit for the interval schedule type #[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize)] #[serde(rename_all = "snake_case")] From c721d5973e7d58940d38ea083f75683b1782ec5d Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Thu, 9 Nov 2023 14:01:08 +0100 Subject: [PATCH 2/7] Added tests, and made improvemnts --- Cargo.lock | 48 +++++++ sentry-types/Cargo.toml | 3 + sentry-types/src/crontab_validator.rs | 181 +++++++++++++++++++------- 3 files changed, 182 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 71c0a05f..f25fa644 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1314,6 +1314,12 @@ version = "0.3.28" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "76d3d132be6c0e6aa1534069c705a74a5997a356c0dc2f86a47765e5617c5b65" +[[package]] +name = "futures-timer" +version = "3.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e64b03909df88034c26dc1547e8970b91f98bdb65165d6a4e9110d94263dbb2c" + [[package]] name = "futures-util" version = "0.3.28" @@ -1380,6 +1386,12 @@ version = "0.28.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6fb8d784f27acf97159b40fc4db5ecd8aa23b9ad5ef69cdd136d3bc80665f0c0" +[[package]] +name = "glob" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" + [[package]] name = "gloo-timers" version = "0.2.6" @@ -2399,6 +2411,12 @@ version = "0.7.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dbb5fb1acd8a1a18b3dd5be62d25485eb770e05afb408a9627d14d451bae12da" +[[package]] +name = "relative-path" +version = "1.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c707298afce11da2efef2f600116fa93ffa7a032b5d7b628aa17711ec81383ca" + [[package]] name = "reqwest" version = "0.11.20" @@ -2456,6 +2474,35 @@ dependencies = [ "winapi", ] +[[package]] +name = "rstest" +version = "0.18.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "97eeab2f3c0a199bc4be135c36c924b6590b88c377d416494288c14f2db30199" +dependencies = [ + "futures", + "futures-timer", + "rstest_macros", + "rustc_version 0.4.0", +] + +[[package]] +name = "rstest_macros" +version = "0.18.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d428f8247852f894ee1be110b375111b586d4fa431f6c46e64ba5a0dcccbe605" +dependencies = [ + "cfg-if", + "glob", + "proc-macro2", + "quote", + "regex", + "relative-path", + "rustc_version 0.4.0", + "syn 2.0.32", + "unicode-ident", +] + [[package]] name = "rustc-demangle" version = "0.1.23" @@ -2820,6 +2867,7 @@ dependencies = [ "debugid", "hex", "rand 0.8.5", + "rstest", "serde", "serde_json", "thiserror", diff --git a/sentry-types/Cargo.toml b/sentry-types/Cargo.toml index 9638f352..34eb4809 100644 --- a/sentry-types/Cargo.toml +++ b/sentry-types/Cargo.toml @@ -30,3 +30,6 @@ thiserror = "1.0.15" time = { version = "0.3.5", features = ["formatting", "parsing"] } url = { version = "2.1.1", features = ["serde"] } uuid = { version = "1.0.0", features = ["serde"] } + +[dev-dependencies] +rstest = "0.18.2" diff --git a/sentry-types/src/crontab_validator.rs b/sentry-types/src/crontab_validator.rs index 834c6963..11d2895d 100644 --- a/sentry-types/src/crontab_validator.rs +++ b/sentry-types/src/crontab_validator.rs @@ -1,4 +1,6 @@ -use std::collections::HashSet; +use std::fmt::{self, Display}; +use std::ops::RangeInclusive; +use std::{collections::HashSet, error::Error}; #[derive(PartialEq, Eq, Hash)] enum CronToken<'a> { @@ -6,74 +8,153 @@ enum CronToken<'a> { Alphabetic(&'a str), } +struct SegmentAllowedValues<'a> { + /// Range of permitted numeric values + numeric_range: RangeInclusive, + + /// Allowed alphabetic single values + single_values: &'a [&'a str], +} + const MONTHS: &[&str] = &[ "jan", "feb", "mar", "apr", "may", "jun", "jul", "aug", "sep", "oct", "nov", "dec", ]; const DAYS: &[&str] = &["sun", "mon", "tue", "wed", "thu", "fri", "sat"]; -fn value_is_allowed(value: &str, allowed_values: &HashSet) -> bool { - match value.parse::() { - Ok(numeric_value) => allowed_values.contains(&CronToken::Numeric(numeric_value)), - Err(_) => allowed_values.contains(&CronToken::Alphabetic(&value.to_lowercase())), +const ALLOWED_VALUES: &[&SegmentAllowedValues] = &[ + &SegmentAllowedValues { + numeric_range: 0..=59, + single_values: &[], + }, + &SegmentAllowedValues { + numeric_range: 0..=23, + single_values: &[], + }, + &SegmentAllowedValues { + numeric_range: 1..=31, + single_values: &[], + }, + &SegmentAllowedValues { + numeric_range: 1..=12, + single_values: MONTHS, + }, + &SegmentAllowedValues { + numeric_range: 0..=6, + single_values: DAYS, + }, +]; + +fn validate_range(range: &str, allowed_values: &SegmentAllowedValues) -> bool { + if range == "*" { + return true; } -} -fn validate_range(range: &str, allowed_values: &HashSet) -> bool { - range == "*" - || range // TODO: Validate that the last range bound is after the previous one. - .splitn(2, "-") - .all(|bound| value_is_allowed(bound, allowed_values)) + let range_limits: Vec<_> = range.split('-').map(str::parse::).collect(); + + range_limits.len() == 2 + && range_limits.iter().all(|limit| { + limit + .as_ref() + .is_ok_and(|limit| allowed_values.numeric_range.contains(limit)) + }) + && range_limits[0].as_ref().unwrap() <= range_limits[1].as_ref().unwrap() } -/// A valid step is None or Some positive value -fn validate_step(step: &Option<&str>) -> bool { - match *step { - Some(value) => match value.parse::() { - Ok(value) => value > 0, - Err(_) => false, - }, - None => true, +fn validate_step(step: &str) -> bool { + match step.parse::() { + Ok(value) => value > 0, + Err(_) => false, } } -fn validate_steprange(steprange: &str, allowed_values: &HashSet) -> bool { +fn validate_steprange(steprange: &str, allowed_values: &SegmentAllowedValues) -> bool { let mut steprange_split = steprange.splitn(2, "/"); - let range = match steprange_split.next() { - Some(range) => range, - None => { - return false; - } + let range_is_valid = match steprange_split.next() { + Some(range) => validate_range(range, allowed_values), + None => false, }; - let range_is_valid = validate_range(range, allowed_values); - let step = steprange_split.next(); - range_is_valid && validate_step(&step) + range_is_valid + && match steprange_split.next() { + Some(step) => validate_step(step), + None => true, + } } -fn validate_segment(segment: &str, allowed_values: &HashSet) -> bool { - segment - .split(",") - .all(|steprange| validate_steprange(steprange, &allowed_values)) +fn validate_listitem(listitem: &str, allowed_values: &SegmentAllowedValues) -> bool { + match listitem.parse::() { + Ok(value) => allowed_values.numeric_range.contains(&value), + Err(_) => validate_steprange(listitem, allowed_values), + } +} + +fn validate_list(list: &str, allowed_values: &SegmentAllowedValues) -> bool { + list.split(",") + .all(|listitem| validate_listitem(listitem, &allowed_values)) +} + +fn validate_segment(segment: &str, allowed_values: &SegmentAllowedValues) -> bool { + allowed_values + .single_values + .contains(&segment.to_lowercase().as_ref()) + || validate_list(segment, allowed_values) } pub fn validate(crontab: &str) -> bool { - let allowed_values = vec![ - (0..60).map(CronToken::Numeric).collect(), - (0..24).map(CronToken::Numeric).collect(), - (1..32).map(CronToken::Numeric).collect(), - (1..13) - .map(CronToken::Numeric) - .chain(MONTHS.iter().map(|&month| CronToken::Alphabetic(month))) - .collect(), - (0..8) - .map(CronToken::Numeric) - .chain(DAYS.iter().map(|&day| CronToken::Alphabetic(day))) - .collect(), - ]; - - crontab - .split_whitespace() - .zip(allowed_values) - .all(|(segment, allowed_values)| validate_segment(segment, &allowed_values)) + let lists: Vec<_> = crontab.split_whitespace().collect(); + if lists.len() != 5 { + return false; + } + + //let x: Box = Box::new(CrontabParseError {}); + + lists + .iter() + .zip(ALLOWED_VALUES) + .all(|(segment, allowed_values)| validate_segment(segment, allowed_values)) +} + +#[cfg(test)] +mod tests { + use super::*; + use rstest::rstest; + + #[rstest] + #[case("* * * * *", true)] + #[case(" * * * * * ", true)] + #[case("invalid", false)] + #[case("", false)] + #[case("* * * *", false)] + #[case("* * * * * *", false)] + #[case("0 0 1 1 0", true)] + #[case("0 0 0 1 0", false)] + #[case("0 0 1 0 0", false)] + #[case("59 23 31 12 6", true)] + #[case("0 0 1 may sun", true)] + #[case("0 0 1 may sat,sun", false)] + #[case("0 0 1 may,jun sat", false)] + #[case("0 0 1 fri sun", false)] + #[case("0 0 1 JAN WED", true)] + #[case("0,24 5,23,6 1,2,3,31 1,2 5,6", true)] + #[case("0-20 * * * *", true)] + #[case("20-0 * * * *", false)] + #[case("0-20/3 * * * *", true)] + #[case("20/3 * * * *", false)] + #[case("*/3 * * * *", true)] + #[case("*/3,2 * * * *", true)] + #[case("*/foo * * * *", false)] + #[case("1-foo * * * *", false)] + #[case("foo-34 * * * *", false)] + fn test_parse(#[case] crontab: &str, #[case] expected: bool) { + assert_eq!( + validate(crontab), + expected, + "\"{crontab}\" is {}a valid crontab", + match expected { + true => "", + false => "not ", + }, + ); + } } From d17c5b4c8f71ffac151641bb631d1d8ef70c5750 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Thu, 9 Nov 2023 14:02:10 +0100 Subject: [PATCH 3/7] Applied linter fixes --- sentry-types/src/crontab_validator.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/sentry-types/src/crontab_validator.rs b/sentry-types/src/crontab_validator.rs index 11d2895d..1f15d9ab 100644 --- a/sentry-types/src/crontab_validator.rs +++ b/sentry-types/src/crontab_validator.rs @@ -1,12 +1,4 @@ -use std::fmt::{self, Display}; use std::ops::RangeInclusive; -use std::{collections::HashSet, error::Error}; - -#[derive(PartialEq, Eq, Hash)] -enum CronToken<'a> { - Numeric(u64), - Alphabetic(&'a str), -} struct SegmentAllowedValues<'a> { /// Range of permitted numeric values @@ -69,7 +61,7 @@ fn validate_step(step: &str) -> bool { } fn validate_steprange(steprange: &str, allowed_values: &SegmentAllowedValues) -> bool { - let mut steprange_split = steprange.splitn(2, "/"); + let mut steprange_split = steprange.splitn(2, '/'); let range_is_valid = match steprange_split.next() { Some(range) => validate_range(range, allowed_values), None => false, @@ -90,8 +82,8 @@ fn validate_listitem(listitem: &str, allowed_values: &SegmentAllowedValues) -> b } fn validate_list(list: &str, allowed_values: &SegmentAllowedValues) -> bool { - list.split(",") - .all(|listitem| validate_listitem(listitem, &allowed_values)) + list.split(',') + .all(|listitem| validate_listitem(listitem, allowed_values)) } fn validate_segment(segment: &str, allowed_values: &SegmentAllowedValues) -> bool { From b08ec77c66d549145914549472d891e16ede0e22 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Thu, 9 Nov 2023 14:04:48 +0100 Subject: [PATCH 4/7] Deleted commented out code --- sentry-types/src/crontab_validator.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/sentry-types/src/crontab_validator.rs b/sentry-types/src/crontab_validator.rs index 1f15d9ab..5a7cdaae 100644 --- a/sentry-types/src/crontab_validator.rs +++ b/sentry-types/src/crontab_validator.rs @@ -99,8 +99,6 @@ pub fn validate(crontab: &str) -> bool { return false; } - //let x: Box = Box::new(CrontabParseError {}); - lists .iter() .zip(ALLOWED_VALUES) From a216277466ccbbe80adc0e81b1dc18bcf85aaea3 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Thu, 9 Nov 2023 15:03:28 +0100 Subject: [PATCH 5/7] from_crontab now returns Result --- sentry-types/src/protocol/monitor.rs | 68 ++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 5 deletions(-) diff --git a/sentry-types/src/protocol/monitor.rs b/sentry-types/src/protocol/monitor.rs index 370686f2..b2b9c301 100644 --- a/sentry-types/src/protocol/monitor.rs +++ b/sentry-types/src/protocol/monitor.rs @@ -1,8 +1,48 @@ +use std::fmt::Display; + use serde::{Deserialize, Serialize, Serializer}; use uuid::Uuid; use crate::crontab_validator; +/// Error type for errors with parsing a crontab schedule +#[derive(Debug)] +pub struct CrontabParseError { + invalid_crontab: String, +} + +impl Display for CrontabParseError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "\"{}\" is not a valid crontab schedule.\n\t \ + For help determining why this schedule is invalid, you can use this site: \ + https://crontab.guru/#{}", + self.invalid_crontab, + self.invalid_crontab + .split_whitespace() + .collect::>() + .join("_"), + ) + } +} + +impl CrontabParseError { + /// Constructs a new CrontabParseError from a given invalid crontab string + /// + /// ## Example + /// ``` + /// use sentry_types::protocol::v7::CrontabParseError; + /// + /// let error = CrontabParseError::new("* * * *"); + /// ``` + pub fn new(invalid_crontab: &str) -> Self { + Self { + invalid_crontab: String::from(invalid_crontab), + } + } +} + /// Represents the status of the monitor check-in #[derive(Clone, Copy, Debug, PartialEq, Deserialize, Serialize)] #[serde(rename_all = "snake_case")] @@ -43,15 +83,33 @@ pub enum MonitorSchedule { impl MonitorSchedule { /// Attempts to create a MonitorSchedule from a provided crontab_str. If the crontab_str is a - /// valid crontab schedule, we return the MonitorSchedule wrapped in a Some variant. Otherwise, - /// we return None. - pub fn from_crontab(crontab_str: &str) -> Option { + /// valid crontab schedule, we return a Result containing the MonitorSchedule; otherwise, we + /// return a Result containing a CrontabParseError. + /// + /// ## Example with valid crontab + /// ``` + /// use sentry_types::protocol::v7::MonitorSchedule; + /// + /// // Create a crontab that runs every other day of the month at midnight. + /// let result = MonitorSchedule::from_crontab("0 0 */2 * *"); + /// assert!(result.is_ok()) + /// ``` + /// + /// ## Example with an invalid crontab + /// ``` + /// use sentry_types::protocol::v7::MonitorSchedule; + /// + /// // Invalid crontab. + /// let result = MonitorSchedule::from_crontab("invalid"); + /// assert!(result.is_err()); + /// ``` + pub fn from_crontab(crontab_str: &str) -> Result { if crontab_validator::validate(crontab_str) { - Some(Self::Crontab { + Ok(Self::Crontab { value: String::from(crontab_str), }) } else { - None + Err(CrontabParseError::new(crontab_str)) } } } From be5052ff4c9709a41a30958d9441c8e9caa7804c Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Thu, 9 Nov 2023 15:24:06 +0100 Subject: [PATCH 6/7] Implement error for CrontabParseError --- sentry-types/src/protocol/monitor.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sentry-types/src/protocol/monitor.rs b/sentry-types/src/protocol/monitor.rs index b2b9c301..b6d6bf2a 100644 --- a/sentry-types/src/protocol/monitor.rs +++ b/sentry-types/src/protocol/monitor.rs @@ -1,4 +1,4 @@ -use std::fmt::Display; +use std::{error::Error, fmt::Display}; use serde::{Deserialize, Serialize, Serializer}; use uuid::Uuid; @@ -27,6 +27,8 @@ impl Display for CrontabParseError { } } +impl Error for CrontabParseError {} + impl CrontabParseError { /// Constructs a new CrontabParseError from a given invalid crontab string /// From fb65dfdeffc9d15d3396febc5ccc2c2abaa98f03 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Fri, 10 Nov 2023 14:15:27 +0100 Subject: [PATCH 7/7] Removed `is_ok_and` call --- sentry-types/src/crontab_validator.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/sentry-types/src/crontab_validator.rs b/sentry-types/src/crontab_validator.rs index 5a7cdaae..853b49b1 100644 --- a/sentry-types/src/crontab_validator.rs +++ b/sentry-types/src/crontab_validator.rs @@ -45,10 +45,9 @@ fn validate_range(range: &str, allowed_values: &SegmentAllowedValues) -> bool { let range_limits: Vec<_> = range.split('-').map(str::parse::).collect(); range_limits.len() == 2 - && range_limits.iter().all(|limit| { - limit - .as_ref() - .is_ok_and(|limit| allowed_values.numeric_range.contains(limit)) + && range_limits.iter().all(|limit| match limit { + Ok(limit) => allowed_values.numeric_range.contains(limit), + Err(_) => false, }) && range_limits[0].as_ref().unwrap() <= range_limits[1].as_ref().unwrap() }