Add missing ClientType, ClientId validation checks

.changelog/unreleased/breaking-changes/621-missing-client-identifier-validation.md

@@ -0,0 +1,3 @@
+- Add missing `ClientType` and `ClientId` validation checks and move `ClientType` under the
+  ICS24 section
+  ([#621](https://github.com/cosmos/ibc-rs/issues/621))

crates/ibc/src/clients/ics07_tendermint/mod.rs

@@ -14,5 +14,5 @@ pub mod misbehaviour;
 pub(crate) const TENDERMINT_CLIENT_TYPE: &str = "07-tendermint";
 
 pub fn client_type() -> ClientType {
-    ClientType::new(TENDERMINT_CLIENT_TYPE.to_string())
+    ClientType::from(TENDERMINT_CLIENT_TYPE.to_string())
 }

crates/ibc/src/core/ics02_client/client_type.rs

@@ -1,6 +1,8 @@
 use crate::prelude::*;
 use core::fmt::{Display, Error as FmtError, Formatter};
 
+use crate::core::ics24_host::{error::ValidationError, validate::validate_client_type};
+
 #[cfg_attr(
     feature = "parity-scale-codec",
     derive(
@@ -19,8 +21,11 @@ use core::fmt::{Display, Error as FmtError, Formatter};
 pub struct ClientType(String);
 
 impl ClientType {
-    pub fn new(s: String) -> Self {
-        Self(s)
+    /// Constructs a new `ClientType` from the given `String` if it ends with a valid client identifier.
+    pub fn new(s: String) -> Result<Self, ValidationError> {
+        let s_trim = s.trim();
+        validate_client_type(s_trim)?;
+        Ok(Self(s_trim.to_string()))
     }
 
     /// Yields this identifier as a borrowed `&str`
@@ -29,6 +34,13 @@ impl ClientType {
     }
 }
 
+impl From<String> for ClientType {
+    /// Constructs a new `ClientType` from the given `String` without performing any validation.
+    fn from(value: String) -> Self {
+        Self(value)
+    }
+}
+
 impl Display for ClientType {
     fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> {
         write!(f, "ClientType({})", self.0)

crates/ibc/src/core/ics02_client/events.rs

@@ -429,7 +429,7 @@ mod tests {
             expected_values: Vec<&'static str>,
         }
 
-        let client_type = ClientType::new("07-tendermint".to_string());
+        let client_type = ClientType::from("07-tendermint".to_string());
         let client_id = ClientId::new(client_type.clone(), 0).unwrap();
         let consensus_height = Height::new(0, 5).unwrap();
         let consensus_heights = vec![Height::new(0, 5).unwrap(), Height::new(0, 7).unwrap()];

crates/ibc/src/core/ics03_connection/events.rs

@@ -305,6 +305,7 @@ impl From<OpenConfirm> for abci::Event {
 
 #[cfg(test)]
 mod tests {
+
     use super::*;
     use crate::core::ics02_client::client_type::ClientType;
     use tendermint::abci::Event as AbciEvent;
@@ -318,7 +319,7 @@ mod tests {
             expected_values: Vec<&'static str>,
         }
 
-        let client_type = ClientType::new("07-tendermint".to_string());
+        let client_type = ClientType::from("07-tendermint".to_string());
         let conn_id_on_a = ConnectionId::default();
         let client_id_on_a = ClientId::new(client_type.clone(), 0).unwrap();
         let conn_id_on_b = ConnectionId::new(1);

crates/ibc/src/core/ics24_host/error.rs

@@ -15,10 +15,10 @@ pub enum ValidationError {
     },
     /// identifier `{id}` must only contain alphanumeric characters or `.`, `_`, `+`, `-`, `#`, - `[`, `]`, `<`, `>`
     InvalidCharacter { id: String },
+    /// identifier prefix `{prefix}` is invalid
+    InvalidPrefix { prefix: String },
     /// identifier cannot be empty
     Empty,
-    /// Invalid channel id in counterparty
-    InvalidCounterpartyChannelId,
 }
 
 #[cfg(feature = "std")]

crates/ibc/src/core/ics24_host/identifier.rs

@@ -7,6 +7,8 @@ use derive_more::Into;
 use super::validate::*;
 use crate::clients::ics07_tendermint::client_type as tm_client_type;
 use crate::core::ics02_client::client_type::ClientType;
+use crate::core::ics24_host::validate::validate_client_identifier;
+
 use crate::core::ics24_host::error::ValidationError;
 use crate::prelude::*;
 
@@ -201,12 +203,13 @@ impl ClientId {
     /// ```
     /// # use ibc::core::ics24_host::identifier::ClientId;
     /// # use ibc::core::ics02_client::client_type::ClientType;
-    /// let tm_client_id = ClientId::new(ClientType::new("07-tendermint".to_string()), 0);
+    /// let tm_client_id = ClientId::new(ClientType::from("07-tendermint".to_string()), 0);
     /// assert!(tm_client_id.is_ok());
     /// tm_client_id.map(|id| { assert_eq!(&id, "07-tendermint-0") });
     /// ```
     pub fn new(client_type: ClientType, counter: u64) -> Result<Self, ValidationError> {
-        let prefix = client_type.as_str();
+        let prefix = client_type.as_str().trim();
+        validate_client_type(prefix)?;
         let id = format!("{prefix}-{counter}");
         Self::from_str(id.as_str())
     }

crates/ibc/src/core/ics24_host/validate.rs

@@ -8,8 +8,10 @@ const VALID_SPECIAL_CHARS: &str = "._+-#[]<>";
 
 /// Default validator function for identifiers.
 ///
-/// A valid identifier only contain lowercase alphabetic characters, and be of a given min and max
-/// length.
+/// A valid identifier only contain valid characters, and be of a given min and
+/// max length as specified in the
+/// [`ICS-24`](https://github.com/cosmos/ibc/tree/main/spec/core/ics-024-host-requirements#paths-identifiers-separators)]
+/// spec.
 pub fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), Error> {
     assert!(max >= min);
 
@@ -48,43 +50,76 @@ pub fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), Error
     Ok(())
 }
 
+/// Checks if the prefix can form a valid identifier with the given min/max identifier's length.
+fn validate_prefix_epoch_format(
+    prefix: &str,
+    min_id_length: usize,
+    max_id_length: usize,
+) -> Result<(), Error> {
+    // Checks if the prefix is not blank
+    if prefix.is_empty() {
+        return Err(Error::Empty)?;
+    }
+
+    // Checks if the prefix forms a valid identifier when used with `0`
+    validate_identifier(
+        &format!("{prefix}-{}", u64::MIN),
+        min_id_length,
+        max_id_length,
+    )?;
+
+    // Checks if the prefix forms a valid identifier when used with `u64::MAX`
+    validate_identifier(
+        &format!("{prefix}-{}", u64::MAX),
+        min_id_length,
+        max_id_length,
+    )?;
+
+    Ok(())
+}
+
+/// Default validator function for the Client types.
+pub fn validate_client_type(id: &str) -> Result<(), Error> {
+    validate_prefix_epoch_format(id, 9, 64)
+}
+
 /// Default validator function for Client identifiers.
 ///
-/// A valid identifier must be between 9-64 characters and only contain lowercase
-/// alphabetic characters,
+/// A valid client identifier must be between 9-64 characters as specified in
+/// the ICS-24 spec.
 pub fn validate_client_identifier(id: &str) -> Result<(), Error> {
     validate_identifier(id, 9, 64)
 }
 
 /// Default validator function for Connection identifiers.
 ///
-/// A valid Identifier must be between 10-64 characters and only contain lowercase
-/// alphabetic characters,
+/// A valid connection identifier must be between 10-64 characters as specified
+/// in the ICS-24 spec.
 pub fn validate_connection_identifier(id: &str) -> Result<(), Error> {
     validate_identifier(id, 10, 64)
 }
 
 /// Default validator function for Port identifiers.
 ///
-/// A valid Identifier must be between 2-128 characters and only contain lowercase
-/// alphabetic characters,
+/// A valid port identifier must be between 2-128 characters as specified in the
+/// ICS-24 spec.
 pub fn validate_port_identifier(id: &str) -> Result<(), Error> {
     validate_identifier(id, 2, 128)
 }
 
 /// Default validator function for Channel identifiers.
 ///
-/// A valid identifier must be between 8-64 characters and only contain
-/// alphabetic characters,
+/// A valid channel identifier must be between 8-64 characters as specified in
+/// the ICS-24 spec.
 pub fn validate_channel_identifier(id: &str) -> Result<(), Error> {
     validate_identifier(id, 8, 64)
 }
 
 #[cfg(test)]
 mod tests {
     use crate::core::ics24_host::validate::{
-        validate_channel_identifier, validate_client_identifier, validate_connection_identifier,
-        validate_identifier, validate_port_identifier,
+        validate_channel_identifier, validate_client_identifier, validate_client_type,
+        validate_connection_identifier, validate_identifier, validate_port_identifier,
     };
     use test_log::test;
 
@@ -172,4 +207,22 @@ mod tests {
         let id = validate_identifier("id/1", 1, 10);
         assert!(id.is_err())
     }
+
+    #[test]
+    fn parse_healthy_client_type() {
+        let id = validate_client_type("07-tendermint");
+        assert!(id.is_ok())
+    }
+
+    #[test]
+    fn parse_invalid_short_client_type() {
+        let id = validate_client_type("<7Char");
+        assert!(id.is_err())
+    }
+
+    #[test]
+    fn parse_invalid_lengthy_client_type() {
+        let id = validate_client_type("InvalidClientTypeWithLengthOfClientId>65Char");
+        assert!(id.is_err())
+    }
 }

crates/ibc/src/mock/client_state.rs

@@ -1,3 +1,4 @@
+use crate::core::ics02_client::client_type::ClientType;
 use crate::core::ics24_host::path::{ClientConsensusStatePath, ClientStatePath};
 use crate::prelude::*;
 
@@ -10,7 +11,6 @@ use ibc_proto::ibc::mock::ClientState as RawMockClientState;
 use ibc_proto::protobuf::Protobuf;
 
 use crate::core::ics02_client::client_state::{ClientState, UpdateKind, UpdatedState};
-use crate::core::ics02_client::client_type::ClientType;
 use crate::core::ics02_client::consensus_state::ConsensusState;
 use crate::core::ics02_client::error::ClientError;
 use crate::core::ics23_commitment::commitment::{
@@ -32,7 +32,7 @@ pub const MOCK_CLIENT_STATE_TYPE_URL: &str = "/ibc.mock.ClientState";
 pub const MOCK_CLIENT_TYPE: &str = "9999-mock";
 
 pub fn client_type() -> ClientType {
-    ClientType::new(MOCK_CLIENT_TYPE.to_string())
+    ClientType::from(MOCK_CLIENT_TYPE.to_string())
 }
 
 /// A mock of an IBC client record as it is stored in a mock context.