Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/src/services/__tests__/coreRpcClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,14 @@ describe('classifyRpcError', () => {
undefined,
'transport',
],
// Issue #2286: downstream provider 401s must NOT clear the user session.
[
'Discord API error: Discord list guilds failed (401): Unauthorized',
undefined,
'provider_auth',
],
['OpenAI API error (401 Unauthorized): invalid api key', undefined, 'provider_auth'],
['Anthropic API error (401 Unauthorized): auth error', undefined, 'provider_auth'],
['some random message', undefined, 'unknown'],
] as const)('%s => %s', (message, status, expected) => {
expect(classifyRpcError(message, status)).toBe(expected);
Expand Down
31 changes: 30 additions & 1 deletion app/src/services/coreRpcClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ let resolvingCoreRpcToken: Promise<string | null> | null = null;
*/
export type CoreRpcErrorKind =
| 'auth_expired'
| 'provider_auth' // downstream provider 401 — NOT user session expiry
| 'transport'
| 'timeout'
| 'rate_limited'
Expand Down Expand Up @@ -107,13 +108,41 @@ export function classifyRpcError(
if (isThreadNotFoundRpcData(data)) return 'thread_not_found';
if (httpStatus === 401) return 'auth_expired';
if (httpStatus === 429) return 'rate_limited';
if (/\(401\b.*Unauthorized\)|Session expired/i.test(message)) return 'auth_expired';
// Confirmed OpenHuman session expiry — explicit markers from the backend/core.
if (/Session expired|SESSION_EXPIRED/i.test(message)) return 'auth_expired';
// Core-side "no backend session token" → the auth profile is gone but the
// frontend may still hold a stale sessionToken from an optimistic post-login
// patch. Treat as auth-expired so `CoreStateProvider` clears the session and
// `ProtectedRoute` bounces the user back to `/` (login) instead of trapping
// them on an onboarding step that polls a failing RPC every 5 s.
if (/no backend session token/i.test(message)) return 'auth_expired';
// "session JWT required" covers the case where a prior 401 already cleared
// the token and the very next RPC call finds no JWT in the store.
if (/session jwt required/i.test(message)) return 'auth_expired';
// OpenHuman backend path 401s (via authed_json): "{METHOD} /path failed (401 Unauthorized)"
// The HTTP method prefix distinguishes these from downstream provider 401s.
// Fix for issue #2286: only match when the message starts with an HTTP verb
// followed by a path — this excludes "Discord API error:", "OpenAI API error:", etc.
// HEAD and OPTIONS intentionally excluded — authed_json only uses these five verbs.
// Aligned with Rust is_session_expired_error: starts-with-verb check + separate
// contains checks for "401" and "unauthorized" (case-insensitive).
if (
/^(GET|POST|PUT|DELETE|PATCH)\s+\//.test(message) &&
/401/.test(message) &&
/unauthorized/i.test(message)
)
return 'auth_expired';
// Downstream provider/integration 401 — NOT user session expiry.
// e.g. "Discord API error: Discord list guilds failed (401): Unauthorized"
// e.g. "OpenAI API error (401 Unauthorized): invalid api key"
// e.g. "Composio v3 API error: HTTP 401: Unauthorized"
// Note: Discord uses "(401): Unauthorized" format (colon after status, reason outside parens),
// so we test for 401 and "unauthorized" independently rather than requiring both inside parens.
if (
(/401/.test(message) && /unauthorized/i.test(message)) ||
/invalid token|bad token/i.test(message)
)
return 'provider_auth';
if (/429.*rate.?limit/i.test(message)) return 'rate_limited';
if (/Budget exceeded|Insufficient budget/i.test(message)) return 'budget_exceeded';
// Local AbortController hit `CORE_RPC_TIMEOUT_MS` — distinct from backend
Expand Down
87 changes: 64 additions & 23 deletions src/core/jsonrpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ pub async fn invoke_method(state: AppState, method: &str, params: Value) -> Resu
let sanitized_reason = crate::openhuman::inference::provider::ops::sanitize_api_error(msg);
if is_session_expired_error(msg) {
log::warn!(
"[jsonrpc] confirmed session expiry for method '{}' — publishing SessionExpired: {}",
"[jsonrpc] confirmed session expiry for method='{}' — publishing SessionExpired: {}",
method,
sanitized_reason
);
Expand All @@ -196,8 +196,8 @@ pub async fn invoke_method(state: AppState, method: &str, params: Value) -> Resu
},
);
} else if is_unconfirmed_unauthorized_error(msg) {
log::warn!(
"[jsonrpc] unauthorized error for method '{}' did not match OpenHuman session expiry — leaving session intact: {}",
log::info!(
"[jsonrpc] unconfirmed unauthorized error for method='{}' (not session expiry) — leaving session intact: {}",
method,
sanitized_reason
);
Expand All @@ -207,35 +207,76 @@ pub async fn invoke_method(state: AppState, method: &str, params: Value) -> Resu
result
}

/// Helper to determine if an error message indicates an expired or invalid session.
/// Helper to determine if an error message indicates an expired or invalid
/// OpenHuman backend session.
///
/// **Narrower than the previous implementation** (fixed in issue #2286):
///
/// The old predicate matched ANY `"401 + unauthorized"` pattern, which caused
/// downstream provider 401s (Discord bot token failures, BYO-key OpenAI /
/// Anthropic failures, Composio direct-mode errors) to clear the user's session
/// and log them out. The fix distinguishes between:
///
/// Uses the same strict classifier as observability so only explicit
/// OpenHuman auth-session failures clear the app session. A generic
/// `"401 Unauthorized"` or `"invalid token"` can come from BYO-key
/// providers, Composio, channels, or other scoped downstream calls; those
/// must surface as recoverable errors rather than publishing
/// `DomainEvent::SessionExpired`.
/// - **OpenHuman backend 401s** (`authed_json` in `src/api/rest.rs`): formatted
/// as `"{METHOD} /path failed (401 Unauthorized): {body}"`, e.g.
Comment thread
graycyrus marked this conversation as resolved.
/// `"GET /teams failed (401 Unauthorized): {"success":false}"`. These always
/// start with an HTTP method verb followed by a space and a forward slash.
/// - **Provider / downstream 401s** (`api_error` in
/// `src/openhuman/inference/provider/ops.rs`): formatted as
/// `"{ProviderName} API error (401 Unauthorized): {body}"` or
/// `"Discord API error: ... (401): Unauthorized"`. These start with a
/// provider name, NOT an HTTP method verb.
///
/// "No backend session token" is also treated as a session-expired signal: the
/// auth profile is missing entirely (the user was never signed in, or their
/// stored profile was wiped between login and the next RPC). The frontend may
/// still believe it holds a session token from an optimistic post-login patch,
/// so we want the same auto-cleanup + UI-level re-auth path to fire instead of
/// repeatedly reporting this as a hard error to Sentry. See #1465-ish: users
/// stuck on the onboarding `SkillsStep` would spam `composio_list_connections`
/// failures every 5 s without ever being bounced back to the login screen.
/// **What still triggers session expiry:**
/// - `"Session expired"` — explicit body text from the OpenHuman backend.
/// - `"no backend session token"` — pre-flight guard; auth profile is missing.
/// - `"session jwt required"` — local guard; JWT already cleared by a prior 401.
/// - `"SESSION_EXPIRED"` — scheduler-gate sentinel (exact case).
/// - HTTP-method-prefixed 401s (`GET /`, `POST /`, etc.) — backend path format.
///
/// "session JWT required" covers the case where a prior 401 already cleared the
/// token and the very next RPC call (e.g. `channels_telegram_login_start`) finds
/// no JWT in the store. This is the same auth-boundary condition, just surfaced
/// as a local guard rather than a backend response.
/// **What no longer triggers session expiry (fixed in #2286):**
/// - Provider-prefixed 401s (`"Discord API error: ..."`, `"OpenAI API error ..."`)
/// - `"invalid token"` — too broad; also matches Discord / OAuth provider tokens.
///
/// Note: for inference-path OpenHuman backend 401s, `api_error` (in
/// `inference/provider/ops.rs` lines 479–497) ALREADY publishes `SessionExpired`
/// directly, so there is no regression if this predicate misses them — the
/// subscriber is idempotent and a harmless double-publish would still be correct.
fn is_session_expired_error(msg: &str) -> bool {
crate::core::observability::is_session_expired_message(msg)
// Explicit session-expired markers from the OpenHuman backend / local
// guards — delegated to the shared observability classifier so both the
// Sentry expected-error pipeline and the JSON-RPC publish boundary stay
// in lock-step.
if crate::core::observability::is_session_expired_message(msg) {
return true;
}
// OpenHuman backend path 401s via `authed_json`:
// format is "{METHOD} /path failed (401 Unauthorized): {body}"
// The HTTP-method prefix distinguishes these from provider-prefixed errors.
// HEAD and OPTIONS are intentionally excluded — `authed_json` only issues
// the five listed verbs (GET/POST/PUT/DELETE/PATCH) for REST JSON endpoints.
let lower = msg.to_ascii_lowercase();
if (lower.contains("401") && lower.contains("unauthorized"))
&& (msg.starts_with("GET /")
|| msg.starts_with("POST /")
|| msg.starts_with("PUT /")
|| msg.starts_with("DELETE /")
|| msg.starts_with("PATCH /"))
{
return true;
}
false
}

/// Detect auth-looking failures that are not specific enough to clear the
/// OpenHuman session. This is only for diagnostics; it must not feed the
/// `SessionExpired` publish path.
///
/// Matches a generic `401 Unauthorized` OR a bare `"invalid token"` string,
/// either of which can come from BYO-key providers, Composio, channels, or
/// other scoped downstream calls. Used exclusively for diagnostic logging
/// at the `invoke_method` call site so provider auth failures are visible
/// in the logs without being misclassified as session expiry.
fn is_unconfirmed_unauthorized_error(msg: &str) -> bool {
let lower = msg.to_ascii_lowercase();
(lower.contains("401") && lower.contains("unauthorized")) || lower.contains("invalid token")
Expand Down
72 changes: 72 additions & 0 deletions src/core/jsonrpc_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -564,8 +564,24 @@ fn parse_json_params_reports_error_message() {
assert!(err.contains("invalid JSON params"));
}

#[test]
fn is_session_expired_error_matches_backend_path_401() {
// Issue #2286: only OpenHuman backend path 401s (HTTP-method prefix) should
// match, not generic 401/Unauthorized strings.
assert!(is_session_expired_error(
"GET /teams failed (401 Unauthorized): {\"success\":false}"
));
assert!(is_session_expired_error(
"POST /auth/token failed (401 Unauthorized): session expired"
));
assert!(is_session_expired_error(
"DELETE /sessions/abc failed (401 Unauthorized): unauthorized"
));
}

#[test]
fn is_session_expired_error_does_not_match_generic_401_unauthorized() {
// Generic 401+unauthorized strings without HTTP-method prefix must NOT match.
assert!(!is_session_expired_error(
"backend returned 401 Unauthorized"
));
Expand All @@ -575,6 +591,8 @@ fn is_session_expired_error_does_not_match_generic_401_unauthorized() {

#[test]
fn unconfirmed_unauthorized_error_matches_generic_401_for_diagnostics_only() {
// Generic 401+unauthorized text feeds the diagnostic-only branch — never
// SessionExpired publication.
assert!(is_unconfirmed_unauthorized_error(
"backend returned 401 Unauthorized"
));
Expand All @@ -586,12 +604,64 @@ fn unconfirmed_unauthorized_error_matches_generic_401_for_diagnostics_only() {

#[test]
fn is_session_expired_error_does_not_match_partial_auth_text() {
// 401 alone is not sufficient — could be HTTP/3.01 nonsense or
// unrelated text. We require the string "unauthorized" too, plus an
// HTTP-method prefix for the 401 path.
assert!(!is_session_expired_error("server returned 401"));
assert!(!is_session_expired_error("unauthorized without code"));
}

#[test]
fn is_session_expired_error_matches_openhuman_backend_path_401() {
// OpenHuman backend calls via authed_json use the format:
// "{METHOD} /path failed (401 Unauthorized): {body}"
assert!(is_session_expired_error(
"GET /teams failed (401 Unauthorized): {\"success\":false}"
));
assert!(is_session_expired_error(
"POST /auth/token failed (401 Unauthorized): session expired"
));
assert!(is_session_expired_error(
"GET /teams/me/usage failed (401 Unauthorized): unauthorized"
));
assert!(is_session_expired_error(
"PUT /profile failed (401 Unauthorized): token expired"
));
assert!(is_session_expired_error(
"PATCH /settings failed (401 Unauthorized): unauthorized"
));
}

#[test]
fn is_session_expired_error_does_not_match_discord_api_error() {
// Issue #2286: Discord bot token 401 must not clear the user session.
assert!(!is_session_expired_error(
"Discord API error: Discord list guilds failed (401): Unauthorized"
));
assert!(!is_session_expired_error(
"Discord API error: Discord get bot user failed (401): bad token"
));
}

#[test]
fn is_session_expired_error_does_not_match_byo_key_provider_401() {
// BYO-key provider 401 should not clear the user session.
assert!(!is_session_expired_error(
"OpenAI API error (401 Unauthorized): invalid api key"
));
assert!(!is_session_expired_error(
"Anthropic API error (401 Unauthorized): authentication error"
));
assert!(!is_session_expired_error(
"Composio v3 API error: HTTP 401: Unauthorized"
));
}

#[test]
fn is_session_expired_error_does_not_match_invalid_token_case_insensitive() {
// "invalid token" is no longer a session-expiry trigger (issue #2286):
// it was too broad and caught Discord/OAuth provider token errors. It is
// still surfaced via the diagnostic-only `is_unconfirmed_unauthorized_error`.
assert!(!is_session_expired_error("Invalid Token"));
assert!(!is_session_expired_error("got an invalid token here"));
assert!(is_unconfirmed_unauthorized_error("Invalid Token"));
Expand All @@ -602,6 +672,8 @@ fn is_session_expired_error_does_not_match_invalid_token_case_insensitive() {

#[test]
fn is_session_expired_error_matches_openhuman_session_expired_body() {
// Even without an HTTP-method prefix, an explicit "Session expired" body
// text triggers session expiry via the shared observability classifier.
assert!(is_session_expired_error(
r#"OpenHuman API error (401 Unauthorized): {"success":false,"error":"Session expired. Please log in again."}"#
));
Expand Down
Loading