diff --git a/src/core/jsonrpc_tests.rs b/src/core/jsonrpc_tests.rs index 3eed02cf0f..7562e7afaa 100644 --- a/src/core/jsonrpc_tests.rs +++ b/src/core/jsonrpc_tests.rs @@ -600,6 +600,36 @@ fn is_session_expired_error_does_not_match_unrelated_errors() { assert!(!is_session_expired_error("")); } +#[test] +fn is_session_expired_error_skips_discord_rewrap_for_2285() { + // Cross-module regression guard for #2285: the Discord domain + // controller intentionally formats its upstream-auth failures so + // they do NOT match this dispatch-time classifier. If anyone + // changes the wording on either side back into a string that + // contains both "401" and "unauthorized", a connected-Discord + // card click would once again log the user out of OpenHuman. + // + // We pin the exact substrings the Discord rewrap was designed + // to avoid, plus the canonical post-rewrap message body, so + // either-side drift fails loudly. + let canonical_rewrap = "Discord API error: Discord list_guilds: bot token was rejected \ + (upstream HTTP four-oh-one). Open Settings → Channels → Discord \ + and rotate / reconnect the bot token."; + assert!( + !is_session_expired_error(canonical_rewrap), + "Discord rewrap must NOT trip the session-expired classifier: {canonical_rewrap}" + ); + // Defensive: also pin the 403 variant. Same rewrap path, same + // requirement — neither '403' nor 'forbidden' is part of the + // session classifier today, but locking the message in keeps a + // future regression visible. + let canonical_rewrap_403 = + "Discord API error: Discord list_channels: bot token lacks required Discord permissions \ + (upstream HTTP four-oh-three). Open Settings → Channels → Discord \ + and rotate / reconnect the bot token."; + assert!(!is_session_expired_error(canonical_rewrap_403)); +} + #[test] fn is_param_validation_error_matches_the_three_validator_shapes() { // Regression guard for OPENHUMAN-TAURI-20: pre-#1467 cores rejected diff --git a/src/openhuman/channels/providers/discord/api.rs b/src/openhuman/channels/providers/discord/api.rs index 2d2807a06a..a59d057952 100644 --- a/src/openhuman/channels/providers/discord/api.rs +++ b/src/openhuman/channels/providers/discord/api.rs @@ -48,6 +48,66 @@ fn auth_header(token: &str) -> String { format!("Bot {token}") } +/// Format a non-2xx response from the Discord REST API as a string +/// suitable for a JSON-RPC error result. +/// +/// **Load-bearing for issue #2285**: the global JSON-RPC dispatcher +/// at `src/core/jsonrpc.rs::is_session_expired_error` matches any +/// error string that contains `"401"` AND `"unauthorized"` as a +/// signal that the OpenHuman backend session has expired, and +/// publishes a `DomainEvent::SessionExpired` event that signs the +/// user out. A raw upstream Discord 401 (revoked bot token) would +/// previously trip that classifier — opening the connected-Discord +/// card on the Channels page logged the user out of OpenHuman over +/// a *Discord* credentials problem. +/// +/// The fix is to convert auth failures here into a Discord-domain +/// message that: +/// +/// 1. Does NOT contain both `"401"` and `"unauthorized"` as a pair +/// (so the global classifier ignores it). +/// 2. Tells the user the actual remediation: rotate the bot token +/// at `Settings → Channels → Discord`. +/// 3. Preserves the originating endpoint identifier in the message +/// so triage can still see WHICH Discord call failed without +/// plumbing a separate error code. +/// +/// Other non-2xx statuses (400 / 404 / 5xx) pass through with a +/// `Discord API error` prefix — they don't match the +/// `is_session_expired_error` predicate even when verbatim. +fn format_discord_http_error(endpoint: &str, status: reqwest::StatusCode, body: &str) -> String { + if status == reqwest::StatusCode::UNAUTHORIZED || status == reqwest::StatusCode::FORBIDDEN { + let kind = if status == reqwest::StatusCode::UNAUTHORIZED { + "bot token was rejected" + } else { + "bot token lacks required Discord permissions" + }; + // Spell out the HTTP code so `lower.contains("401")` does NOT + // match — see #2285 rationale on this helper. Also avoid the + // word `unauthorized` for the same reason; "rejected"/"forbidden" + // are the user-visible equivalents. + let code_word = if status == reqwest::StatusCode::UNAUTHORIZED { + "four-oh-one" + } else { + "four-oh-three" + }; + // Deliberately do NOT splice the upstream body into this + // user-facing message — Discord's auth-error bodies often + // include the literal words "401" and "Unauthorized", which + // would smuggle the cascade trigger back in. The body is + // still in `tracing::debug!` logs above the call site for + // triage; the user-facing message only needs the remediation. + let _ = body; + format!( + "Discord {endpoint}: {kind} (upstream HTTP {code_word}). \ + Open Settings → Channels → Discord and rotate / reconnect the bot \ + token." + ) + } else { + format!("Discord {endpoint} failed ({status}): {body}") + } +} + /// List all guilds (servers) the bot is a member of. pub async fn list_bot_guilds(token: &str) -> anyhow::Result> { list_bot_guilds_at_base(DISCORD_API_BASE, token).await @@ -77,7 +137,10 @@ async fn list_bot_guilds_at_base(base: &str, token: &str) -> anyhow::Result = resp.json().await?; @@ -120,7 +183,10 @@ async fn list_guild_channels_at_base( body = %body, "[discord-api] non-success response" ); - anyhow::bail!("Discord list channels failed ({status}): {body}"); + anyhow::bail!( + "{}", + format_discord_http_error("list_channels", status, &body) + ); } let all_channels: Vec = resp.json().await?; @@ -183,7 +249,10 @@ async fn check_channel_permissions_at_base( body = %body, "[discord-api] non-success response" ); - anyhow::bail!("Discord get bot user failed ({status}): {body}"); + anyhow::bail!( + "{}", + format_discord_http_error("get_bot_user", status, &body) + ); } let me: serde_json::Value = me_resp.json().await?; let bot_user_id = me.get("id").and_then(|i| i.as_str()).unwrap_or("").trim(); @@ -212,7 +281,10 @@ async fn check_channel_permissions_at_base( body = %body, "[discord-api] non-success response" ); - anyhow::bail!("Discord get member info failed ({status}): {body}"); + anyhow::bail!( + "{}", + format_discord_http_error("get_member_info", status, &body) + ); } let member: serde_json::Value = member_resp.json().await?; @@ -237,7 +309,10 @@ async fn check_channel_permissions_at_base( body = %body, "[discord-api] non-success response" ); - anyhow::bail!("Discord get guild roles failed ({status}): {body}"); + anyhow::bail!( + "{}", + format_discord_http_error("get_guild_roles", status, &body) + ); } let guild_roles: Vec = roles_resp.json().await?; @@ -295,7 +370,10 @@ async fn check_channel_permissions_at_base( body = %body, "[discord-api] non-success response" ); - anyhow::bail!("Discord get channel failed ({status}): {body}"); + anyhow::bail!( + "{}", + format_discord_http_error("get_channel", status, &body) + ); } let channel_data: serde_json::Value = ch_resp.json().await?; if let Some(overwrites) = channel_data diff --git a/src/openhuman/channels/providers/discord/api_tests.rs b/src/openhuman/channels/providers/discord/api_tests.rs index 9e9d727832..b222e15f08 100644 --- a/src/openhuman/channels/providers/discord/api_tests.rs +++ b/src/openhuman/channels/providers/discord/api_tests.rs @@ -151,18 +151,72 @@ async fn list_bot_guilds_parses_discord_response() { } #[tokio::test] -async fn list_bot_guilds_errors_on_non_success_status() { +async fn list_bot_guilds_rewraps_401_so_global_session_cascade_does_not_fire() { + // Upstream returns 401 with the canonical Discord auth-error body. + // BEFORE #2285 the error string flowed up to JSON-RPC as + // "Discord list guilds failed (401 Unauthorized): {\"message\": + // \"401: Unauthorized\",\"code\":0}" — that pair tripped + // `jsonrpc::is_session_expired_error` ("401" + "unauthorized") + // and logged the user out of OpenHuman over a *Discord* + // credentials problem. + // + // After the fix the user-facing message: + // - does NOT contain "401" or "unauthorized" as substrings + // (so `is_session_expired_error` returns false), AND + // - names the endpoint + the actionable Settings → Channels → + // Discord remediation path. let app = Router::new().route( "/users/@me/guilds", - get(|| async { (StatusCode::UNAUTHORIZED, "bad token") }), + get(|| async { + ( + StatusCode::UNAUTHORIZED, + r#"{"message":"401: Unauthorized","code":0}"#, + ) + }), + ); + let base = spawn_mock(app).await; + let err = list_bot_guilds_at_base(&base, "t") + .await + .unwrap_err() + .to_string(); + let lower = err.to_ascii_lowercase(); + assert!( + !lower.contains("401"), + "must NOT contain '401' substring: {err}" + ); + assert!( + !lower.contains("unauthorized"), + "must NOT contain 'unauthorized' substring: {err}" + ); + assert!( + err.contains("list_guilds"), + "endpoint identifier preserved for triage: {err}" + ); + assert!( + err.contains("Settings → Channels → Discord"), + "remediation path present: {err}" + ); +} + +#[tokio::test] +async fn list_bot_guilds_5xx_still_carries_raw_status() { + // Non-auth errors fall through to the legacy verbose format — + // those don't match `is_session_expired_error` even verbatim, so + // surfacing the raw status code helps the user / triage. + let app = Router::new().route( + "/users/@me/guilds", + get(|| async { (StatusCode::INTERNAL_SERVER_ERROR, "discord melting") }), ); let base = spawn_mock(app).await; let err = list_bot_guilds_at_base(&base, "t") .await .unwrap_err() .to_string(); - assert!(err.contains("list guilds failed")); - assert!(err.contains("401")); + assert!( + err.contains("500"), + "5xx must surface verbatim status: {err}" + ); + assert!(err.contains("list_guilds")); } #[tokio::test] @@ -188,18 +242,36 @@ async fn list_guild_channels_filters_text_channels_and_sorts_by_position() { } #[tokio::test] -async fn list_guild_channels_errors_on_non_success_status() { +async fn list_guild_channels_rewraps_403_with_remediation_and_no_session_keywords() { + // 403 follows the same rewrap path as 401 (#2285) — both can + // happen on a stale/disabled bot token AND both share enough + // substrings with `is_session_expired_error` to be a problem if + // raw upstream text reaches the JSON-RPC layer. The user-facing + // message must use the safer wording. let app = Router::new().route( "/guilds/{guild_id}/channels", - get(|| async { (StatusCode::FORBIDDEN, "nope") }), + get(|| async { + ( + StatusCode::FORBIDDEN, + r#"{"message":"Missing Access","code":50001}"#, + ) + }), ); let base = spawn_mock(app).await; let err = list_guild_channels_at_base(&base, "t", "g1") .await .unwrap_err() .to_string(); - assert!(err.contains("list channels failed")); - assert!(err.contains("403")); + let lower = err.to_ascii_lowercase(); + assert!(!lower.contains("403"), "raw 403 must not leak: {err}"); + assert!( + err.contains("list_channels"), + "endpoint identifier preserved: {err}" + ); + assert!( + err.contains("Settings → Channels → Discord"), + "remediation path present: {err}" + ); } #[tokio::test] @@ -363,6 +435,17 @@ async fn check_channel_permissions_errors_on_member_lookup_failure() { .await .unwrap_err() .to_string(); - assert!(err.contains("member info failed")); - assert!(err.contains("401")); + // Endpoint identifier preserved in the rewrap (#2285), and the + // 401 path keeps the substrings "401"/"unauthorized" out of the + // user-facing message so the JSON-RPC session-expired classifier + // ignores it. + assert!(err.contains("get_member_info")); + assert!( + !err.to_ascii_lowercase().contains("401"), + "rewrapped message must not contain '401': {err}" + ); + assert!( + !err.to_ascii_lowercase().contains("unauthorized"), + "rewrapped message must not contain 'unauthorized': {err}" + ); }