mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-30 23:14:37 +00:00
fix(auth): narrow SessionExpired to confirmed OpenHuman backend 401s
## Summary
- Narrows `is_session_expired_error` in `src/core/jsonrpc.rs` so `DomainEvent::SessionExpired` only fires for **confirmed OpenHuman session expiry**, not for downstream provider 401s.
- Adds `is_downstream_provider_auth_error` helper for diagnostic logging only (no session side-effects).
- Adds `'provider_auth'` error kind to `CoreRpcErrorKind` in `coreRpcClient.ts`; tightens `classifyRpcError` with the same HTTP-method-prefix logic.
- Fixes Discord card-click logout (issue #2285) as a direct consequence.
## Root Cause
`is_session_expired_error` used a loose `"401 + unauthorized"` string match. Discord bot-token failures arrive as `"Discord API error: Discord list guilds failed (401): Unauthorized"` — which contains both "401" and "unauthorized" — causing the full user session to be cleared on every Discord card interaction.
## Fix
OpenHuman backend errors (from `authed_json` in `src/api/rest.rs`) always use the format `"{HTTP_METHOD} /path failed (401 Unauthorized): {body}"`. Provider errors start with the provider name. The fix keeps the `"401 + unauthorized"` branch only when the message starts with an HTTP method verb, which matches backend paths while excluding Discord, OpenAI, Anthropic, Composio, etc.
## Test plan
- [x] `src/core/jsonrpc_tests.rs` — 10 `is_session_expired_error` tests covering: HTTP-method-prefix matches, Discord exclusion, BYO-key exclusion, Composio exclusion, explicit markers still match
- [x] `app/src/services/__tests__/coreRpcClient.test.ts` — 3 new `test.each` rows: Discord/OpenAI/Anthropic 401 → `provider_auth`; existing `GET /teams failed (401 Unauthorized)` → `auth_expired` preserved
- [x] `cargo test -p openhuman is_session_expired` — 10/10 pass
- [x] `pnpm test:coverage` — full Vitest suite pass
- [x] `pnpm compile` + `cargo check` — clean
- [x] `pnpm format:check` — clean
## Submission Checklist
- [x] Tests added or updated (happy path + at least one failure / edge case)
- [x] Diff coverage ≥ 80% — all new/changed lines in `jsonrpc.rs` and `coreRpcClient.ts` covered by unit tests
- [x] No new external network dependencies introduced
- [x] N/A: Coverage matrix — no new production feature rows
- [x] N/A: Manual smoke checklist — no release surfaces touched
## Related
Closes #2286
Related: #2285 (Discord card-click logout — fixed as a consequence of this change)
---
## AI Authored PR Metadata (required for Codex/Linear PRs)
### Linear Issue
- Key: N/A
- URL: N/A
### Commit & Branch
- Branch: `fix/session-expired-cascade-2286`
- Commit SHA: a0da2423
### Validation Run
- [x] `pnpm --filter openhuman-app compile`
- [x] `pnpm --filter openhuman-app format:check`
- [x] `pnpm --filter openhuman-app lint`
- [x] `cargo check --manifest-path Cargo.toml`
- [x] `pnpm test:coverage` (Vitest full suite)
- [x] `cargo test -p openhuman is_session_expired` (10/10 pass)
### Validation Blocked
- `command:` N/A
- `error:` N/A
- `impact:` N/A
### Behavior Changes
- Intended behavior change: provider-auth 401s (Discord, OpenAI BYO-key, Composio direct-mode) no longer clear the user session
- User-visible effect: clicking the Discord channel card no longer logs the user out; BYO-key misconfiguration no longer forces re-auth
### Parity Contract
- Legacy behavior preserved: OpenHuman backend 401s (`GET /teams failed (401 Unauthorized)`) still trigger session expiry
- Guard/fallback/dispatch parity checks: `api_error` in `inference/provider/ops.rs` still publishes SessionExpired directly for backend auth failures (independent of this fix)
### Duplicate / Superseded PR Handling
- Duplicate PR(s): none
- Canonical PR: this PR
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved error classification to distinguish user session expiry from external provider authentication failures, reducing mistaken session terminations and improving recovery and logging behavior.
* **Tests**
* Expanded and tightened test coverage to ensure confirmed session-expiry signals are detected while external API 401/unauthorized responses do not trigger session-expiry handling.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2356?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: M3gA-Mind <megamind@mahadao.com>
Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
This commit is contained in:
co-authored by
M3gA-Mind
Steven Enamakel
parent
369a39288c
commit
c81fe3dbcc
@@ -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);
|
||||
|
||||
@@ -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'
|
||||
@@ -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
|
||||
|
||||
+64
-23
@@ -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
|
||||
);
|
||||
@@ -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
|
||||
);
|
||||
@@ -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.
|
||||
///
|
||||
/// 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`.
|
||||
/// **Narrower than the previous implementation** (fixed in issue #2286):
|
||||
///
|
||||
/// "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.
|
||||
/// 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:
|
||||
///
|
||||
/// "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.
|
||||
/// - **OpenHuman backend 401s** (`authed_json` in `src/api/rest.rs`): formatted
|
||||
/// as `"{METHOD} /path failed (401 Unauthorized): {body}"`, e.g.
|
||||
/// `"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.
|
||||
///
|
||||
/// **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.
|
||||
///
|
||||
/// **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")
|
||||
|
||||
@@ -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"
|
||||
));
|
||||
@@ -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"
|
||||
));
|
||||
@@ -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"));
|
||||
@@ -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."}"#
|
||||
));
|
||||
|
||||
Reference in New Issue
Block a user