Height::from_str accepts invalid heights

[benluelo]

Bug Summary

Height::from_str accepts invalid heights. The implementation should use split_once instead of split:
https://github.com/cosmos/ibc-rs/blob/main/crates/ibc/src/core/ics02_client/height.rs#L174-L197

Details

let invalid_height_string = "1-1-1";

let roundtrip = invalid_height_string
    .parse::<Height>()
    .unwrap()
    .to_string();

assert_eq!(invalid_height_string, roundtrip);

Version

latest main.

[DaviRain-Su]

I believe you are intentionally creating errors, or in other words, you think there might be incorrect usage in the code.

[KaiserKarel]

I believe you are intentionally creating errors, or in other words, you think there might be incorrect usage in the code.

yeah, which is why parse returns an error; it should only return Ok if the provided string is valid; otherwise it is unusable for untrusted inputs, which parse is ToStr is intended for.

[benluelo]

Also, the use of indexing causes a panic if there is no - in the string:

"1".parse::<ibc::Height>();
"".parse::<ibc::Height>();

thread 'height' panicked at 'index out of bounds: the len is 1 but the index is 1'

[DaviRain-Su]

I want to know where this input can be generated.

[benluelo]

This is public api in a published library. Where the input is generated is irrelevant. TryFrom is not supposed to panic on invalid input - it returns a Result for a reason, as was previously stated. Honestly, this could be seen as a security vulnerability; a caller would expect any conversion errors to be returned in an Err, and would therefore think it would be safe to call in security sensitive code.

[Farhad-Shabani]

@benluelo much appreciated for bringing this up. This is very true!