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 (#2286)
Previously, `is_session_expired_error` fired on any error containing
"401 + unauthorized", causing Discord bot-token failures, BYO-key
provider 401s, and Composio direct-mode errors to clear the user's
app session and force re-authentication.
The fix distinguishes error origins by format:
- OpenHuman backend errors (via `authed_json`) use "{METHOD} /path
failed (401 Unauthorized): {body}" — they start with an HTTP verb.
- Provider errors ("Discord API error: ...", "OpenAI API error ...")
start with a provider name, not an HTTP method.
Changes:
- `is_session_expired_error`: keeps explicit session markers ("session
expired", SESSION_EXPIRED, "no backend session token", "session jwt
required") and the HTTP-method-prefixed 401 check; removes the
bare "invalid token" and generic "401 + unauthorized" matches.
- Adds `is_downstream_provider_auth_error` helper for diagnostic
logging only (no side effects).
- `coreRpcClient.ts`: adds `provider_auth` error kind; tightens
`classifyRpcError` to match backend-path 401s by HTTP-method prefix
and route remaining 401s to `provider_auth` instead of `auth_expired`.
- Tests updated in `jsonrpc_tests.rs` and `coreRpcClient.test.ts`.
Closes #2286
This commit is contained in:
@@ -666,6 +666,14 @@ describe('classifyRpcError', () => {
|
|||||||
undefined,
|
undefined,
|
||||||
'transport',
|
'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'],
|
['some random message', undefined, 'unknown'],
|
||||||
] as const)('%s => %s', (message, status, expected) => {
|
] as const)('%s => %s', (message, status, expected) => {
|
||||||
expect(classifyRpcError(message, status)).toBe(expected);
|
expect(classifyRpcError(message, status)).toBe(expected);
|
||||||
|
|||||||
@@ -71,6 +71,7 @@ let resolvingCoreRpcToken: Promise<string | null> | null = null;
|
|||||||
*/
|
*/
|
||||||
export type CoreRpcErrorKind =
|
export type CoreRpcErrorKind =
|
||||||
| 'auth_expired'
|
| 'auth_expired'
|
||||||
|
| 'provider_auth' // downstream provider 401 — NOT user session expiry
|
||||||
| 'transport'
|
| 'transport'
|
||||||
| 'timeout'
|
| 'timeout'
|
||||||
| 'rate_limited'
|
| 'rate_limited'
|
||||||
@@ -107,13 +108,34 @@ export function classifyRpcError(
|
|||||||
if (isThreadNotFoundRpcData(data)) return 'thread_not_found';
|
if (isThreadNotFoundRpcData(data)) return 'thread_not_found';
|
||||||
if (httpStatus === 401) return 'auth_expired';
|
if (httpStatus === 401) return 'auth_expired';
|
||||||
if (httpStatus === 429) return 'rate_limited';
|
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
|
// 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
|
// frontend may still hold a stale sessionToken from an optimistic post-login
|
||||||
// patch. Treat as auth-expired so `CoreStateProvider` clears the session and
|
// patch. Treat as auth-expired so `CoreStateProvider` clears the session and
|
||||||
// `ProtectedRoute` bounces the user back to `/` (login) instead of trapping
|
// `ProtectedRoute` bounces the user back to `/` (login) instead of trapping
|
||||||
// them on an onboarding step that polls a failing RPC every 5 s.
|
// them on an onboarding step that polls a failing RPC every 5 s.
|
||||||
if (/no backend session token/i.test(message)) return 'auth_expired';
|
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.
|
||||||
|
if (/^(GET|POST|PUT|DELETE|PATCH)\s+\/[^\s].*\(401\b.*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 (/429.*rate.?limit/i.test(message)) return 'rate_limited';
|
||||||
if (/Budget exceeded|Insufficient budget/i.test(message)) return 'budget_exceeded';
|
if (/Budget exceeded|Insufficient budget/i.test(message)) return 'budget_exceeded';
|
||||||
// Local AbortController hit `CORE_RPC_TIMEOUT_MS` — distinct from backend
|
// Local AbortController hit `CORE_RPC_TIMEOUT_MS` — distinct from backend
|
||||||
|
|||||||
+68
-28
@@ -180,7 +180,7 @@ pub async fn invoke_method(state: AppState, method: &str, params: Value) -> Resu
|
|||||||
if let Err(ref msg) = result {
|
if let Err(ref msg) = result {
|
||||||
if is_session_expired_error(msg) {
|
if is_session_expired_error(msg) {
|
||||||
log::warn!(
|
log::warn!(
|
||||||
"[jsonrpc] backend returned 401 for method '{}' — publishing SessionExpired",
|
"[jsonrpc] confirmed session expiry for method='{}' — publishing SessionExpired",
|
||||||
method
|
method
|
||||||
);
|
);
|
||||||
// Scrub before publishing — subscribers log `reason`, and the
|
// Scrub before publishing — subscribers log `reason`, and the
|
||||||
@@ -193,47 +193,87 @@ pub async fn invoke_method(state: AppState, method: &str, params: Value) -> Resu
|
|||||||
reason: crate::openhuman::inference::provider::ops::sanitize_api_error(msg),
|
reason: crate::openhuman::inference::provider::ops::sanitize_api_error(msg),
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
|
} else if is_downstream_provider_auth_error(msg) {
|
||||||
|
log::info!(
|
||||||
|
"[jsonrpc] downstream provider auth failure for method='{}' (not session expiry) — {}",
|
||||||
|
method,
|
||||||
|
msg
|
||||||
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
result
|
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.
|
||||||
///
|
///
|
||||||
/// Deliberately **looser** than
|
/// **Narrower than the previous implementation** (fixed in issue #2286):
|
||||||
/// [`crate::core::observability::is_session_expired_message`]: this
|
|
||||||
/// dispatch-site predicate also matches the generic `"401 + unauthorized"` /
|
|
||||||
/// `"invalid token"` pair so token cleanup +
|
|
||||||
/// `DomainEvent::SessionExpired` publish fire on *any* 401, including
|
|
||||||
/// BYO-key provider failures (which clear the stale local token even if
|
|
||||||
/// the user mis-configured an OpenAI / Anthropic key). The strict
|
|
||||||
/// classifier in `observability` is for the agent / web-channel
|
|
||||||
/// `report_error_or_expected` call sites, where matching too loosely would
|
|
||||||
/// silence actionable BYO-key configuration errors (OPENHUMAN-TAURI-26
|
|
||||||
/// rationale: the agent-layer demote must NOT also swallow generic
|
|
||||||
/// provider 401s).
|
|
||||||
///
|
///
|
||||||
/// "No backend session token" is also treated as a session-expired signal: the
|
/// The old predicate matched ANY `"401 + unauthorized"` pattern, which caused
|
||||||
/// auth profile is missing entirely (the user was never signed in, or their
|
/// downstream provider 401s (Discord bot token failures, BYO-key OpenAI /
|
||||||
/// stored profile was wiped between login and the next RPC). The frontend may
|
/// Anthropic failures, Composio direct-mode errors) to clear the user's session
|
||||||
/// still believe it holds a session token from an optimistic post-login patch,
|
/// and log them out. The fix distinguishes between:
|
||||||
/// 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.
|
|
||||||
///
|
///
|
||||||
/// "session JWT required" covers the case where a prior 401 already cleared the
|
/// - **OpenHuman backend 401s** (`authed_json` in `src/api/rest.rs`): formatted
|
||||||
/// token and the very next RPC call (e.g. `channels_telegram_login_start`) finds
|
/// as `"{METHOD} /path failed (401 Unauthorized): {body}"`, e.g.
|
||||||
/// no JWT in the store. This is the same auth-boundary condition, just surfaced
|
/// `"GET /teams failed (401 Unauthorized): {"success":false}"`. These always
|
||||||
/// as a local guard rather than a backend response.
|
/// 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 {
|
fn is_session_expired_error(msg: &str) -> bool {
|
||||||
let lower = msg.to_lowercase();
|
let lower = msg.to_lowercase();
|
||||||
(lower.contains("401") && lower.contains("unauthorized"))
|
// Explicit session-expired markers from the OpenHuman backend / local guards.
|
||||||
|| lower.contains("invalid token")
|
if lower.contains("session expired")
|
||||||
|| lower.contains("no backend session token")
|
|| lower.contains("no backend session token")
|
||||||
|| lower.contains("session jwt required")
|
|| lower.contains("session jwt required")
|
||||||
|| msg.contains("SESSION_EXPIRED")
|
|| msg.contains("SESSION_EXPIRED")
|
||||||
|
{
|
||||||
|
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.
|
||||||
|
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
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Returns `true` when the error looks like a downstream provider / integration
|
||||||
|
/// 401 that should NOT clear the user's OpenHuman session.
|
||||||
|
///
|
||||||
|
/// 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. Does not drive any side-effects.
|
||||||
|
fn is_downstream_provider_auth_error(msg: &str) -> bool {
|
||||||
|
let lower = msg.to_lowercase();
|
||||||
|
lower.contains("401") && lower.contains("unauthorized")
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Returns `true` when the error message comes from JSON-RPC params validation
|
/// Returns `true` when the error message comes from JSON-RPC params validation
|
||||||
|
|||||||
@@ -565,25 +565,86 @@ fn parse_json_params_reports_error_message() {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn is_session_expired_error_matches_401_unauthorized() {
|
fn is_session_expired_error_matches_401_unauthorized() {
|
||||||
|
// Issue #2286: only OpenHuman backend path 401s (HTTP-method prefix) should
|
||||||
|
// match, not generic 401/Unauthorized strings.
|
||||||
assert!(is_session_expired_error(
|
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"
|
||||||
|
));
|
||||||
|
// Generic 401+unauthorized strings without HTTP-method prefix must NOT match.
|
||||||
|
assert!(!is_session_expired_error(
|
||||||
"backend returned 401 Unauthorized"
|
"backend returned 401 Unauthorized"
|
||||||
));
|
));
|
||||||
assert!(is_session_expired_error("401 UNAUTHORIZED"));
|
assert!(!is_session_expired_error("401 UNAUTHORIZED"));
|
||||||
assert!(is_session_expired_error("got 401 and unauthorized body"));
|
assert!(!is_session_expired_error("got 401 and unauthorized body"));
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn is_session_expired_error_requires_both_401_and_unauthorized() {
|
fn is_session_expired_error_requires_both_401_and_unauthorized() {
|
||||||
// 401 alone is not sufficient — could be HTTP/3.01 nonsense or
|
// 401 alone is not sufficient — could be HTTP/3.01 nonsense or
|
||||||
// unrelated text. We require the string "unauthorized" too.
|
// unrelated text. We require the string "unauthorized" too, plus HTTP-method
|
||||||
|
// prefix for the 401 path.
|
||||||
assert!(!is_session_expired_error("server returned 401"));
|
assert!(!is_session_expired_error("server returned 401"));
|
||||||
assert!(!is_session_expired_error("unauthorized without code"));
|
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]
|
#[test]
|
||||||
fn is_session_expired_error_matches_invalid_token_case_insensitive() {
|
fn is_session_expired_error_matches_invalid_token_case_insensitive() {
|
||||||
assert!(is_session_expired_error("Invalid Token"));
|
// "invalid token" is no longer a session-expiry trigger (issue #2286):
|
||||||
assert!(is_session_expired_error("got an invalid token here"));
|
// it was too broad and caught Discord/OAuth provider token errors.
|
||||||
|
assert!(!is_session_expired_error("Invalid Token"));
|
||||||
|
assert!(!is_session_expired_error("got an invalid token here"));
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
Reference in New Issue
Block a user