From 2838aa4f5c512f20c55d3170422e148e29597c69 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Mon, 13 Nov 2023 19:36:36 +0100 Subject: [PATCH] refactor: simplify identifier length validation (#960) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: simplify identifier length validation Firstly, remove asserts from the validate_identifier_length and validate_prefix_length functions. Instead, let the regular checks handle cases where min and max constraints don’t allow for a valid prefix to exist. Secondly, ensure minimum length constraints is at least one. This makes sure that the validation functions will check for empty identifiers and prefixes. This makes empty identifier check in validate_channel_identifier as well as Error::Empty variant unnecessary so get rid of those too. Lastly, collapse all checks in validate_prefix_length into a single call to validate_identifier_length with correctly adjusted min and max constraints. Issue: https://github.com/cosmos/ibc-rs/issues/959 * log * Update 960-simplify-length-validation.md Signed-off-by: Michal Nazarewicz * tests * Update and rename 960-simplify-length-validation.md to 961-simplify-length-validation.md Signed-off-by: Michal Nazarewicz * update changelog entry * revert error variant deletion * refactor id validation tests * use numeral consistently --------- Signed-off-by: Michal Nazarewicz Co-authored-by: Ranadeep Biswas --- .../961-refactor-identifier-validation.md | 2 + .../core/ics24_host/identifier/validate.rs | 90 ++++++++----------- 2 files changed, 38 insertions(+), 54 deletions(-) create mode 100644 .changelog/unreleased/breaking-changes/961-refactor-identifier-validation.md diff --git a/.changelog/unreleased/breaking-changes/961-refactor-identifier-validation.md b/.changelog/unreleased/breaking-changes/961-refactor-identifier-validation.md new file mode 100644 index 000000000..ca8ca1d0e --- /dev/null +++ b/.changelog/unreleased/breaking-changes/961-refactor-identifier-validation.md @@ -0,0 +1,2 @@ +- Simplify and refactor ICS-24 identifier validation logic. + ([\#961](https://github.com/cosmos/ibc-rs/issues/961)) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 7e0f2acf3..c16fcdf12 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -9,11 +9,6 @@ const VALID_SPECIAL_CHARS: &str = "._+-#[]<>"; /// [`ICS-24`](https://github.com/cosmos/ibc/tree/main/spec/core/ics-024-host-requirements#paths-identifiers-separators)] /// spec. pub fn validate_identifier_chars(id: &str) -> Result<(), Error> { - // Check identifier is not empty - if id.is_empty() { - return Err(Error::Empty); - } - // Check identifier does not contain path separators if id.contains(PATH_SEPARATOR) { return Err(Error::ContainSeparator { id: id.into() }); @@ -38,19 +33,19 @@ pub fn validate_identifier_chars(id: &str) -> Result<(), Error> { /// [`ICS-24`](https://github.com/cosmos/ibc/tree/main/spec/core/ics-024-host-requirements#paths-identifiers-separators)] /// spec. pub fn validate_identifier_length(id: &str, min: u64, max: u64) -> Result<(), Error> { - assert!(max >= min); - - // Check identifier length is between given min/max - if (id.len() as u64) < min || id.len() as u64 > max { - return Err(Error::InvalidLength { + // Make sure min is at least one so we reject empty identifiers. + let min = min.max(1); + let length = id.len() as u64; + if (min..=max).contains(&length) { + Ok(()) + } else { + Err(Error::InvalidLength { id: id.into(), - length: id.len() as u64, + length, min, max, - }); + }) } - - Ok(()) } /// Checks if a prefix forms a valid identifier with the given min/max identifier's length. @@ -61,42 +56,16 @@ pub fn validate_prefix_length( min_id_length: u64, max_id_length: u64, ) -> Result<(), Error> { - assert!(max_id_length >= min_id_length); - - if prefix.is_empty() { - return Err(Error::InvalidPrefix { - prefix: prefix.into(), - }); - } - - // Statically checks if the prefix forms a valid identifier length when constructed with `u64::MAX` - // len(prefix + '-' + u64::MAX) <= max_id_length (minimum prefix length is 1) - if max_id_length < 22 { - return Err(Error::InvalidLength { - id: prefix.into(), - length: prefix.len() as u64, - min: 0, - max: 0, - }); - } - - // Checks if the prefix forms a valid identifier length when constructed with `u64::MIN` - // len('-' + u64::MIN) = 2 - validate_identifier_length( - prefix, - min_id_length.saturating_sub(2), - max_id_length.saturating_sub(2), - )?; - - // Checks if the prefix forms a valid identifier length when constructed with `u64::MAX` - // len('-' + u64::MAX) = 21 - validate_identifier_length( - prefix, - min_id_length.saturating_sub(21), - max_id_length.saturating_sub(21), - )?; - - Ok(()) + // Prefix must be at least `min_id_length - 2` characters long since the + // shortest identifier we can construct is `{prefix}-0` which extends prefix + // by 2 characters. + let min = min_id_length.saturating_sub(2); + // Prefix must be at most `max_id_length - 21` characters long since the + // longest identifier we can construct is `{prefix}-{u64::MAX}` which + // extends prefix by 21 characters. + let max = max_id_length.saturating_sub(21); + + validate_identifier_length(prefix, min, max) } /// Default validator function for the Client types. @@ -220,10 +189,21 @@ mod tests { } #[test] - fn parse_invalid_id_empty() { - // invalid id empty - let id = validate_identifier_chars(""); - assert!(id.is_err()) + fn validate_chars_empty_id() { + // validate_identifier_chars allows empty identifiers + assert!(validate_identifier_chars("").is_ok()); + } + + #[test] + fn validate_length_empty_id() { + // validate_identifier_length does not allow empty identifiers + assert!(validate_identifier_length("", 0, 64).is_err()); + } + + #[test] + fn validate_min_gt_max_constraints() { + // validate_identifier_length rejects the id if min > max. + assert!(validate_identifier_length("foobar", 5, 3).is_err()); } #[test] @@ -252,8 +232,10 @@ mod tests { } #[rstest] + #[case::zero_min_length("", 0, 64, false)] #[case::empty_prefix("", 1, 64, false)] #[case::max_is_low("a", 1, 10, false)] + #[case::min_greater_than_max("foobar", 5, 3, false)] #[case::u64_max_is_too_big("a", 3, 21, false)] #[case::u64_min_is_too_small("a", 4, 22, false)] #[case::u64_min_max_boundary("a", 3, 22, true)]