Repository: osa1/tiny (language: rust)

Issue:
Don't rely on RPL_YOURHOST parsing for connectivity checking
I discovered this while reviewing #237 (see [my comment][1]). We currently use the server name when sending PINGs to a server to check connectivity, but we can't do that if we don't know the server name, which we currently parse from the RPL_YOURHOST message. When parsing fails `servername` stays `None`, and `send_ping` just returns:

https://github.com/osa1/tiny/blob/efcb30310e0113c51e27cb21bbbd8e6783f064a7/libtiny_client/src/state.rs#L29-L31

This breaks connectivity checks when we can't parse RPL_YOURHOST for some reason. One example of this happening is when connecting to [irc.gitter.im][2]. Gitter's IRC server is not following the standards too closely and caused problems before (#214, #211). Currently RPL_YOURHOST from Gitter looks like this:

```
Msg { pfx: Some(Server("irc.gitter.im")), cmd: Reply { num: 2, params: ["osa1", " 1.10.0"] } }
```

This is quite different than how RFC 2812 defines it:

```
       002    RPL_YOURHOST
              "Your host is <servername>, running version <ver>"
```

**What to do when we can't parse RPL_YOURHOST?** I see a few options:

- Send an empty PING message. The error response will reset the connectivity timer. The problem with this is that we'll see error responses in the server tab. Example: `helix.oftc.net: osa1 PING Not enough parameters`
- Send a PING to yourself. This won't work if we're stuck in nick selection (not sure if this can happen). In that case we can fall back to (1). I just tried this on OFTC and it seems to work.
- Send a HELP message to the server, which doesn't require arguments so will always work. (Note: HELP is ignored by irc.gitter.im)
- (Other ideas?)

Note that (1) and (2) works on irc.gitter.im, which doesn't seem care about the PING argument at all and always responds with a PONG immediately. (3) doesn't work as it doesn't seem to support HELP message.

cc @trevarj 

[1]: https://github.com/osa1/tiny/pull/237#discussion_r471390983
[2]: https://irc.gitter.im/


Discussion / hints:
@osa1 I found this on https://www.irc.com/dev/docs/refs/numerics/002.html

> The \<servername\> may also include other information depending on the server software, for example irc.example.com[203.0.113.15/6667], so we instead recommend relying on the prefix or the RPL_MYINFO (004) numeric if knowing the current server hostname is desired.


Maybe to just rewrite parse_servername to use the prefix?
Good idea @trevarj . Would be good to compare values parsed currently vs. the prefix, and whether using prefix would work for pinging. I just tried irc.gitter.im and `PING irc.gitter.im` worked fine.
Another idea is to use the prefix as a fallback, when current parser fails.
This appears to work for all my servers, including gitter
```
                // First try to use prefix, which should have the server name
                if let Some(Pfx::Server(server_name)) = pfx {
                    self.servername = Some(server_name.to_owned())
                } else {
                    // Try to parse message for server name
                    match parse_servername(params) {
                        None => {
                            error!("Could not parse server name in 002 RPL_YOURHOST message.");
                        }
                        Some(servername) => {
                            self.servername = Some(servername);
                        }
                    }
                }
```
@trevarj sounds good. Some suggestions:

- Let's do all of this parsing in parse_servername (it should take a full message as argument) and test it toroughly using responses of some of the widely used servers. I can add some tests based on servers I'm connecting to if you're going to submit a PR.

- Let's use "Your host is ..." parsing as we do today first, and use prefix if that fails. Reason: it's battle-tested and we know it works well on standard-conforming servers.

Fix patch:
diff --git a/libtiny_client/src/state.rs b/libtiny_client/src/state.rs
index 41e7810e..720e24d7 100644
--- a/libtiny_client/src/state.rs
+++ b/libtiny_client/src/state.rs
@@ -480,7 +480,7 @@ impl StateInner {
 
                 // An example <servername>: cherryh.freenode.net[149.56.134.238/8001]
 
-                match parse_servername(params) {
+                match parse_servername(pfx.as_ref(), params) {
                     None => {
                         error!("Could not parse server name in 002 RPL_YOURHOST message.");
                     }
@@ -723,8 +723,8 @@ async fn retry_channel_join(
 const SERVERNAME_PREFIX: &str = "Your host is ";
 const SERVERNAME_PREFIX_LEN: usize = SERVERNAME_PREFIX.len();
 
-/// Try to parse servername in a 002 RPL_YOURHOST reply
-fn parse_servername(params: &[String]) -> Option<String> {
+/// Try to parse servername in a 002 RPL_YOURHOST reply params.
+fn parse_yourhost_msg(params: &[String]) -> Option<String> {
     let msg = params.get(1).or_else(|| params.get(0))?;
     if msg.len() >= SERVERNAME_PREFIX_LEN && &msg[..SERVERNAME_PREFIX_LEN] == SERVERNAME_PREFIX {
         let slice1 = &msg[SERVERNAME_PREFIX_LEN..];
@@ -735,6 +735,21 @@ fn parse_servername(params: &[String]) -> Option<String> {
     }
 }
 
+/// Get the server name from a prefix.
+fn parse_server_pfx(pfx: Option<&Pfx>) -> Option<String> {
+    if let Some(Pfx::Server(server_name)) = pfx {
+        Some(server_name.to_owned())
+    } else {
+        None
+    }
+}
+
+/// Parse server name from RPL_YOURHOST reply or fallback to using the server name inside
+/// Pfx::Server. See https://www.irc.com/dev/docs/refs/numerics/002.html for more info.
+fn parse_servername(pfx: Option<&Pfx>, params: &[String]) -> Option<String> {
+    parse_yourhost_msg(&params).or_else(|| parse_server_pfx(pfx))
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 
 #[cfg(test)]
@@ -743,27 +758,48 @@ mod tests {
 
     #[test]
     fn test_parse_servername_1() {
-        let args = vec![
-            "tiny_test".to_owned(),
-            "Your host is adams.freenode.net[94.125.182.252/8001], \
-             running version ircd-seven-1.1.4"
-                .to_owned(),
+        // IRC standard
+        let prefix = Some(Pfx::Server("card.freenode.net".to_string()));
+        let params = vec![
+            "nickname".to_string(),
+            "Your host is card.freenode.net[38.229.70.22/6697], running version ircd-seven-1.1.9"
+                .to_string(),
+        ];
+        assert_eq!(
+            parse_servername(prefix.as_ref(), &params),
+            Some("card.freenode.net".to_owned())
+        );
+
+        let prefix = Some(Pfx::Server("coulomb.oftc.net".to_string()));
+        let params = vec![
+            "nickname".to_string(),
+            "Your host is coulomb.oftc.net[109.74.200.93/6697], running version hybrid-7.2.2+oftc1.7.3".to_string(),
+        ];
+        assert_eq!(
+            parse_servername(prefix.as_ref(), &params),
+            Some("coulomb.oftc.net".to_owned())
+        );
+
+        let prefix = Some(Pfx::Server("irc.eagle.y.se".to_string()));
+        let params = vec![
+            "nickname".to_string(),
+            "Your host is irc.eagle.y.se, running version UnrealIRCd-4.0.18".to_string(),
         ];
         assert_eq!(
-            parse_servername(&args),
-            Some("adams.freenode.net".to_owned())
+            parse_servername(prefix.as_ref(), &params),
+            Some("irc.eagle.y.se".to_owned())
         );
     }
 
     #[test]
     fn test_parse_servername_2() {
-        let args = vec![
-            "tiny_test".to_owned(),
-            "Your host is belew.mozilla.org, running version InspIRCd-2.0".to_owned(),
-        ];
+        // Gitter variation
+        // Msg { pfx: Some(Server("irc.gitter.im")), cmd: Reply { num: 2, params: ["nickname", " 1.10.0"] } }
+        let prefix = Some(Pfx::Server("irc.gitter.im".to_string()));
+        let params = vec!["nickname".to_string(), " 1.10.0".to_string()];
         assert_eq!(
-            parse_servername(&args),
-            Some("belew.mozilla.org".to_owned())
+            parse_servername(prefix.as_ref(), &params),
+            Some("irc.gitter.im".to_owned())
         );
     }
 }


Test patch: