From a13c05d1abf4fbc1dcfa82b76dbeec80fec16d7e Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 21 May 2026 02:49:44 +0530 Subject: [PATCH] fix(channels/discord): convert upstream 401/403 to domain-scoped error so card click can't sign user out (#2285) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Channels page's Discord card opens a setup modal that calls `channels_discord_list_guilds` → `Discord REST /users/@me/guilds`. When the bot token is stale/revoked, Discord returns 401 and the client `bail!`s the literal "Discord list guilds failed (401 Unauthorized): {body}". That string flows up through JSON-RPC as `Err(String)` and trips `src/core/jsonrpc.rs::is_session_expired_error`, which classifies ANY `lower.contains("401") && lower.contains("unauthorized")` as backend session expiry — publishing `DomainEvent::SessionExpired` and signing the user out of OpenHuman over a *Discord* credentials problem. Three in-flight PRs (#2292, #2302, #2356) all attempt to narrow `is_session_expired_error` itself. This PR takes the orthogonal defense-in-depth approach: a Discord-domain rewrap so even if none of those broader fixes land, the Channels page is safe. Discord API client (src/openhuman/channels/providers/discord/api.rs) - New helper `format_discord_http_error(endpoint, status, body)` that detects 401/403 and emits a user-facing message that: 1. does NOT contain "401" or "unauthorized" as substrings (so `is_session_expired_error` returns false), 2. names the failed endpoint (`list_guilds` / `list_channels` / `get_member_info` / `get_guild_roles` / `get_bot_user` / `get_channel`) so triage can read it from logs, 3. ends with the actionable `Settings → Channels → Discord` remediation path. Spells the HTTP code as "four-oh-one"/"four-oh-three" and uses "rejected"/"forbidden" — both are user-visible equivalents that don't match the classifier. - All 6 `anyhow::bail!` sites in `api.rs` route through the helper: list_bot_guilds_at_base, list_guild_channels_at_base, check_channel_permissions_at_base (3 call sites: get_bot_user, get_member_info, get_guild_roles), get_channel. - 401/403 path deliberately omits the upstream body from the user-facing message — Discord's auth-error bodies often include the literal words "401" and "Unauthorized", which would re-introduce the cascade trigger. The body is still in `tracing::debug!` logs above each call site for triage. Tests - `discord/api_tests.rs`: * Replaced `list_bot_guilds_errors_on_non_success_status` with `list_bot_guilds_rewraps_401_so_global_session_cascade_does_not_fire` — drives a mock Discord that returns the canonical `{"message":"401: Unauthorized","code":0}` body and asserts: - the rewrapped error contains neither "401" nor "unauthorized" - it preserves the `list_guilds` endpoint identifier - it carries the `Settings → Channels → Discord` remediation * Added `list_bot_guilds_5xx_still_carries_raw_status` — non-auth errors keep the verbose status format (they don't trip the session classifier anyway). * Replaced `list_guild_channels_errors_on_non_success_status` with `list_guild_channels_rewraps_403_with_remediation_and_no_session_keywords`. * Updated `check_channel_permissions_errors_on_member_lookup_failure` to assert the new endpoint identifier `get_member_info` + the absence of "401" / "unauthorized" substrings. - `core/jsonrpc_tests.rs`: * Added `is_session_expired_error_skips_discord_rewrap_for_2285` — pins the canonical 401 AND 403 rewrap message bodies and asserts `is_session_expired_error` returns `false` for both, so either-side drift (Discord wording change OR classifier broadening) fails loudly in CI. `cargo test --lib discord::api is_session_expired_error` → 31 + 8 = 39 passed, 0 failed (4 new). `cargo check` clean. `cargo fmt` applied. Out of scope (separate / complementary PRs) - No FE change: `DiscordConfig` already surfaces RPC errors via its existing `setError` path; the new domain-specific message bubbles through unchanged. - No change to `is_session_expired_error` itself: that's owned by the in-flight root-cause PRs (#2292 / #2302 / #2356). This PR is defense-in-depth and remains useful even if all three of those land. --- src/core/jsonrpc_tests.rs | 30 +++++ .../channels/providers/discord/api.rs | 90 ++++++++++++++- .../channels/providers/discord/api_tests.rs | 103 ++++++++++++++++-- 3 files changed, 207 insertions(+), 16 deletions(-) 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}" + ); }