fix: Handle SshError(Disconnect) during auth for password fallback (issue #113) (#114)

When SSH key authentication fails, russh may disconnect the connection
before returning the authentication failure result, resulting in
SshError(Disconnect) instead of KeyAuthFailed. This prevented the
password fallback mechanism from triggering.

Changes:
- Update is_auth_error_for_password_fallback() to recognize
  SshError(Disconnect) and SshError(RecvError) as auth failures
- Refactor establish_connection() to use the helper function
  for consistent error classification
- Add comprehensive tests for the new error handling
- Add integration tests for issue #113 scenarios

Author: inureyes

src/commands/interactive/connection.rs

@@ -72,16 +72,16 @@ impl InteractiveCommand {
 
         // Check if authentication failed and password fallback is allowed
         // This matches SSH key failures as well as SSH agent authentication failures
+        // Also handles the case where russh disconnects during authentication failure
+        // (which returns SshError(Disconnect) instead of KeyAuthFailed)
         let result = match result {
-            Err(
-                SshError::KeyAuthFailed
-                | SshError::AgentAuthenticationFailed
-                | SshError::AgentNoIdentities
-                | SshError::AgentConnectionFailed
-                | SshError::AgentRequestIdentitiesFailed,
-            ) if allow_password_fallback && atty::is(atty::Stream::Stdin) => {
+            Err(ref err)
+                if allow_password_fallback
+                    && atty::is(atty::Stream::Stdin)
+                    && is_auth_error_for_password_fallback(err) =>
+            {
                 tracing::debug!(
-                    "SSH authentication failed for {username}@{host}:{port}, attempting password fallback"
+                    "SSH authentication failed for {username}@{host}:{port} ({err}), attempting password fallback"
                 );
 
                 // Prompt for password (matching OpenSSH behavior)
@@ -473,18 +473,56 @@ impl InteractiveCommand {
 /// - SSH agent has no identities loaded
 /// - SSH agent connection fails
 /// - SSH agent identity request fails
+/// - SSH server disconnects during authentication (russh::Error::Disconnect)
+///   This is a common behavior when the server rejects key authentication
+///   and the russh library drops the connection before returning the auth result.
 ///
 /// These are all cases where falling back to password authentication makes sense,
 /// matching OpenSSH's behavior.
+///
+/// # Important
+/// The SshError(Disconnect) case is particularly important because russh may
+/// disconnect the connection before returning the authentication failure result.
+/// The log flow in this case is:
+/// ```text
+/// userauth_failure -> drop handle -> disconnected SshError(Disconnect)
+/// ```
+/// Without handling this case, password fallback would never be triggered when
+/// key authentication fails on servers that disconnect after auth failure.
 pub fn is_auth_error_for_password_fallback(error: &SshError) -> bool {
-    matches!(
-        error,
+    match error {
+        // Explicit authentication failures
         SshError::KeyAuthFailed
-            | SshError::AgentAuthenticationFailed
-            | SshError::AgentNoIdentities
-            | SshError::AgentConnectionFailed
-            | SshError::AgentRequestIdentitiesFailed
-    )
+        | SshError::AgentAuthenticationFailed
+        | SshError::AgentNoIdentities
+        | SshError::AgentConnectionFailed
+        | SshError::AgentRequestIdentitiesFailed => true,
+
+        // russh may disconnect after auth failure, which manifests as these errors
+        // This is a key fix for GitHub issue #113: the server may disconnect
+        // during authentication, and we should treat this as an auth failure
+        // that can be retried with password.
+        SshError::SshError(russh::Error::Disconnect) => {
+            tracing::debug!(
+                "Treating SshError(Disconnect) as auth failure - server likely \
+                 disconnected after key authentication rejection"
+            );
+            true
+        }
+
+        // RecvError can occur when the server closes the channel during auth
+        SshError::SshError(russh::Error::RecvError) => {
+            tracing::debug!(
+                "Treating SshError(RecvError) as auth failure - server likely \
+                 closed connection during authentication"
+            );
+            true
+        }
+
+        // All other errors should not trigger password fallback
+        // This includes: PasswordWrong, ServerCheckFailed, IoError, etc.
+        _ => false,
+    }
 }
 
 #[cfg(test)]
@@ -574,4 +612,57 @@ mod tests {
             "KeyboardInteractiveAuthFailed should NOT trigger password fallback"
         );
     }
+
+    // Tests for issue #113: Handle SshError(Disconnect) during authentication
+    #[test]
+    fn test_ssh_disconnect_triggers_password_fallback() {
+        let error = SshError::SshError(russh::Error::Disconnect);
+        assert!(
+            is_auth_error_for_password_fallback(&error),
+            "SshError(Disconnect) should trigger password fallback - \
+             server may disconnect after key auth rejection"
+        );
+    }
+
+    #[test]
+    fn test_ssh_recv_error_triggers_password_fallback() {
+        let error = SshError::SshError(russh::Error::RecvError);
+        assert!(
+            is_auth_error_for_password_fallback(&error),
+            "SshError(RecvError) should trigger password fallback - \
+             server may close connection during authentication"
+        );
+    }
+
+    #[test]
+    fn test_ssh_hup_does_not_trigger_fallback() {
+        // HUP is a different type of disconnect that happens during normal operation
+        let error = SshError::SshError(russh::Error::HUP);
+        assert!(
+            !is_auth_error_for_password_fallback(&error),
+            "SshError(HUP) should NOT trigger password fallback - \
+             this indicates remote closed connection, not auth failure"
+        );
+    }
+
+    #[test]
+    fn test_ssh_connection_timeout_does_not_trigger_fallback() {
+        let error = SshError::SshError(russh::Error::ConnectionTimeout);
+        assert!(
+            !is_auth_error_for_password_fallback(&error),
+            "SshError(ConnectionTimeout) should NOT trigger password fallback - \
+             this is a network issue, not auth failure"
+        );
+    }
+
+    #[test]
+    fn test_ssh_not_authenticated_does_not_trigger_fallback() {
+        // NotAuthenticated means we haven't tried auth yet, not that auth failed
+        let error = SshError::SshError(russh::Error::NotAuthenticated);
+        assert!(
+            !is_auth_error_for_password_fallback(&error),
+            "SshError(NotAuthenticated) should NOT trigger password fallback - \
+             this means auth hasn't been attempted yet"
+        );
+    }
 }

tests/password_fallback_test.rs

@@ -105,3 +105,59 @@ fn test_host_key_verification_not_bypassed() {
         "ServerCheckFailed must NOT trigger password fallback - host key verification is a security feature"
     );
 }
+
+// Tests for issue #113: Handle SshError(Disconnect) during authentication
+// The russh library may disconnect the connection before returning authentication failure,
+// which manifests as SshError(Disconnect) instead of KeyAuthFailed.
+
+/// Test that SshError(Disconnect) triggers password fallback.
+/// This is the key fix for GitHub issue #113.
+#[test]
+fn test_ssh_disconnect_during_auth_triggers_password_fallback() {
+    let error = SshError::SshError(russh::Error::Disconnect);
+    assert!(
+        is_auth_error_for_password_fallback(&error),
+        "SshError(Disconnect) should trigger password fallback - \
+         server may disconnect after key authentication rejection (issue #113)"
+    );
+}
+
+/// Test that SshError(RecvError) triggers password fallback.
+/// The server may close the channel during authentication, resulting in RecvError.
+#[test]
+fn test_ssh_recv_error_during_auth_triggers_password_fallback() {
+    let error = SshError::SshError(russh::Error::RecvError);
+    assert!(
+        is_auth_error_for_password_fallback(&error),
+        "SshError(RecvError) should trigger password fallback - \
+         server may close connection during authentication"
+    );
+}
+
+/// Test that other SSH errors (HUP, ConnectionTimeout) do NOT trigger password fallback.
+/// These indicate network issues, not authentication failures.
+#[test]
+fn test_other_ssh_errors_do_not_trigger_fallback() {
+    let non_auth_ssh_errors: Vec<(SshError, &str)> = vec![
+        (
+            SshError::SshError(russh::Error::HUP),
+            "HUP - remote closed connection",
+        ),
+        (
+            SshError::SshError(russh::Error::ConnectionTimeout),
+            "ConnectionTimeout - network issue",
+        ),
+        (
+            SshError::SshError(russh::Error::NotAuthenticated),
+            "NotAuthenticated - auth not attempted yet",
+        ),
+    ];
+
+    for (error, desc) in non_auth_ssh_errors {
+        assert!(
+            !is_auth_error_for_password_fallback(&error),
+            "{} should NOT trigger password fallback",
+            desc
+        );
+    }
+}