fix(oauth): tighten legacy state validation and fallback handling (#1701)

* fix(oauth): tighten legacy state validation and fallback handling

* style: fix formatting

* refactor: separate validation checks for clearer error messages

src/cli/oauth_defaults.rs

@@ -569,6 +569,42 @@ pub async fn sweep_expired_flows(registry: &PendingOAuthRegistry) {
 const HOSTED_STATE_PREFIX: &str = "ic2";
 const HOSTED_STATE_CHECKSUM_BYTES: usize = 12;
 
+/// Maximum length for a legacy flow ID or instance name.
+const LEGACY_STATE_MAX_LEN: usize = 128;
+/// Minimum length for a legacy flow ID.
+const LEGACY_STATE_MIN_LEN: usize = 8;
+
+/// Validate that a legacy state component (flow_id or instance_name) contains
+/// only safe characters: alphanumeric, dash, underscore.
+fn is_valid_legacy_state_component(s: &str) -> bool {
+    !s.is_empty()
+        && s.len() <= LEGACY_STATE_MAX_LEN
+        && s.bytes()
+            .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_')
+}
+
+fn validate_legacy_flow_id(flow_id: &str) -> Result<(), String> {
+    if flow_id.len() < LEGACY_STATE_MIN_LEN {
+        return Err(format!(
+            "Legacy OAuth flow_id too short ({} chars, minimum {LEGACY_STATE_MIN_LEN})",
+            flow_id.len()
+        ));
+    }
+    if flow_id.len() > LEGACY_STATE_MAX_LEN {
+        return Err(format!(
+            "Legacy OAuth flow_id too long ({} chars, maximum {LEGACY_STATE_MAX_LEN})",
+            flow_id.len()
+        ));
+    }
+    if !flow_id
+        .bytes()
+        .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_')
+    {
+        return Err("Legacy OAuth flow_id contains invalid characters".to_string());
+    }
+    Ok(())
+}
+
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct DecodedHostedOAuthState {
     pub flow_id: String,
@@ -653,6 +689,17 @@ pub fn decode_hosted_oauth_state(state: &str) -> Result<DecodedHostedOAuthState,
         if flow_id.is_empty() {
             return Err("Hosted OAuth legacy state is missing flow_id".to_string());
         }
+        validate_legacy_flow_id(flow_id)?;
+        if !instance_name.is_empty() && !is_valid_legacy_state_component(instance_name) {
+            return Err(format!(
+                "Legacy OAuth instance name contains invalid characters or exceeds max length ({LEGACY_STATE_MAX_LEN})"
+            ));
+        }
+        tracing::debug!(
+            flow_id,
+            instance_name,
+            "Decoded legacy prefixed OAuth state"
+        );
         return Ok(DecodedHostedOAuthState {
             flow_id: flow_id.to_string(),
             instance_name: if instance_name.is_empty() {
@@ -668,6 +715,9 @@ pub fn decode_hosted_oauth_state(state: &str) -> Result<DecodedHostedOAuthState,
         return Err("Hosted OAuth state is empty".to_string());
     }
 
+    validate_legacy_flow_id(state)?;
+    tracing::debug!(flow_id = state, "Decoded legacy raw OAuth state");
+
     Ok(DecodedHostedOAuthState {
         flow_id: state.to_string(),
         instance_name: None,
@@ -1734,13 +1784,13 @@ mod tests {
     fn test_decode_hosted_oauth_state_accepts_legacy_formats() {
         use crate::cli::oauth_defaults::decode_hosted_oauth_state;
 
-        let decoded = decode_hosted_oauth_state("kind-deer:abc123").expect("legacy prefixed");
-        assert_eq!(decoded.flow_id, "abc123");
+        let decoded = decode_hosted_oauth_state("kind-deer:abc12345").expect("legacy prefixed");
+        assert_eq!(decoded.flow_id, "abc12345");
         assert_eq!(decoded.instance_name.as_deref(), Some("kind-deer"));
         assert!(decoded.is_legacy);
 
-        let decoded = decode_hosted_oauth_state("abc123").expect("legacy raw");
-        assert_eq!(decoded.flow_id, "abc123");
+        let decoded = decode_hosted_oauth_state("abc12345").expect("legacy raw");
+        assert_eq!(decoded.flow_id, "abc12345");
         assert_eq!(decoded.instance_name, None);
         assert!(decoded.is_legacy);
     }
@@ -1864,4 +1914,72 @@ mod tests {
         assert_eq!(decoded_no_instance.instance_name, None);
         assert!(!decoded_no_instance.is_legacy);
     }
+
+    /// Legacy flow IDs that are too short must be rejected (#1443).
+    #[test]
+    fn test_legacy_state_rejects_short_flow_id() {
+        use crate::cli::oauth_defaults::decode_hosted_oauth_state;
+
+        let err = decode_hosted_oauth_state("abc").expect_err("short raw flow_id");
+        assert!(err.contains("too short"), "unexpected error: {err}");
+
+        let err = decode_hosted_oauth_state("inst:abc").expect_err("short prefixed flow_id");
+        assert!(err.contains("too short"), "unexpected error: {err}");
+    }
+
+    /// Legacy flow IDs with invalid characters must be rejected (#1443).
+    #[test]
+    fn test_legacy_state_rejects_invalid_characters() {
+        use crate::cli::oauth_defaults::decode_hosted_oauth_state;
+
+        let err = decode_hosted_oauth_state("flow id with spaces!").expect_err("spaces in flow_id");
+        assert!(
+            err.contains("invalid characters"),
+            "unexpected error: {err}"
+        );
+
+        let err = decode_hosted_oauth_state("inst:flow/id?bad=yes")
+            .expect_err("special chars in prefixed flow_id");
+        assert!(
+            err.contains("invalid characters"),
+            "unexpected error: {err}"
+        );
+    }
+
+    /// Legacy instance names with invalid characters must be rejected (#1444).
+    #[test]
+    fn test_legacy_state_rejects_invalid_instance_name() {
+        use crate::cli::oauth_defaults::decode_hosted_oauth_state;
+
+        let err = decode_hosted_oauth_state("bad instance!:valid-flow-id-12345")
+            .expect_err("invalid instance name");
+        assert!(err.contains("instance name"), "unexpected error: {err}");
+    }
+
+    /// Excessively long legacy flow IDs must be rejected (#1443).
+    #[test]
+    fn test_legacy_state_rejects_oversized_flow_id() {
+        use crate::cli::oauth_defaults::decode_hosted_oauth_state;
+
+        let long_id = "a".repeat(200);
+        let err = decode_hosted_oauth_state(&long_id).expect_err("oversized flow_id");
+        assert!(err.contains("too long"), "unexpected error: {err}");
+    }
+
+    /// Valid legacy flow IDs at boundary lengths are accepted.
+    #[test]
+    fn test_legacy_state_accepts_boundary_lengths() {
+        use crate::cli::oauth_defaults::decode_hosted_oauth_state;
+
+        // Exactly 8 chars (minimum)
+        let decoded = decode_hosted_oauth_state("abcd1234").expect("8-char flow_id");
+        assert_eq!(decoded.flow_id, "abcd1234");
+        assert!(decoded.is_legacy);
+
+        // Exactly 128 chars (maximum)
+        let max_id = "a".repeat(128);
+        let decoded = decode_hosted_oauth_state(&max_id).expect("128-char flow_id");
+        assert_eq!(decoded.flow_id, max_id);
+        assert!(decoded.is_legacy);
+    }
 }