fix(auth): narrow SessionExpired to confirmed OpenHuman backend 401s#2356
Conversation
…inyhumansai#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 tinyhumansai#2286
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR narrows SessionExpired publication to confirmed OpenHuman user-session expiry by introducing a ChangesSession Expiry vs. Provider Auth Distinction
Sequence Diagram(s)sequenceDiagram
participant invoke_method
participant is_session_expired_error
participant is_downstream_provider_auth_error
participant SessionExpiredEvent
invoke_method->>is_session_expired_error: check error text
is_session_expired_error-->>invoke_method: true (explicit session expiry)
invoke_method->>SessionExpiredEvent: publish SessionExpired (sanitized reason)
invoke_method->>is_downstream_provider_auth_error: else check provider 401 patterns
is_downstream_provider_auth_error-->>invoke_method: true (provider 401)
invoke_method-->>invoke_method: log info (provider auth, no teardown)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/jsonrpc.rs (1)
196-201: 💤 Low valueConsider sanitizing the provider auth failure log message.
The session-expired path (line 193) sanitizes the error message via
sanitize_api_error, but the provider auth log on line 200 logsmsgdirectly. While provider error messages typically don't contain secrets, they can include user-supplied paths or tokens that were rejected. For consistency with the session-expired path, consider sanitizing:} else if is_downstream_provider_auth_error(msg) { + let redacted = crate::openhuman::inference::provider::ops::sanitize_api_error(msg); log::info!( "[jsonrpc] downstream provider auth failure for method='{}' (not session expiry) — {}", method, - msg + redacted ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/jsonrpc.rs` around lines 196 - 201, The provider auth failure log in the is_downstream_provider_auth_error branch currently logs msg directly; change the log to use the same sanitizer as the session-expired path by passing the error through sanitize_api_error before logging (i.e., use sanitize_api_error(msg) in the log::info call), keeping the log message and method variable unchanged so provider errors are consistently sanitized.app/src/services/coreRpcClient.ts (1)
111-138: 💤 Low valueVerify regex alignment with Rust
is_session_expired_error.The TypeScript regex on line 126 uses
^(GET|POST|PUT|DELETE|PATCH)\s+\/[^\s].*\(401\b.*Unauthorized\)/iwhile the Rust implementation usesstarts_with("GET /")etc. with separatecontains("401")andcontains("unauthorized")checks.The TypeScript regex requires:
\(401\b.*Unauthorized\)— both "401" and "Unauthorized" must appear with "401" followed by a word boundary and eventually "Unauthorized" (in any order relative to other text, but both present)The Rust version checks:
msg.starts_with("GET /")(note: no space between method and slash)lower.contains("401") && lower.contains("unauthorized")The TypeScript regex expects
GET /path(space then slash) while Rust expectsGET /(method immediately followed by space-slash). Both should work for the actual backend format"GET /teams failed (401 Unauthorized)", but the subtle difference in the regex pattern is worth noting for future maintenance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/services/coreRpcClient.ts` around lines 111 - 138, The HTTP-verb 401 check in coreRpcClient.ts (the if using the regex /^(GET|POST|PUT|DELETE|PATCH)\s+\/[^\s].*\(401\b.*Unauthorized\)/i) is stricter than the Rust is_session_expired_error logic; update the condition so it only requires the message to start with an HTTP verb + space + '/' and to contain both "401" and "unauthorized" (case-insensitive) rather than requiring the specific "(401 ... Unauthorized)" sequence—i.e., replace the single complex regex check on message with a starts-with-verb check plus separate contains checks for "401" and "unauthorized" (use the same message variable and keep the same verb list GET/POST/PUT/DELETE/PATCH).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/services/coreRpcClient.ts`:
- Around line 111-138: The HTTP-verb 401 check in coreRpcClient.ts (the if using
the regex /^(GET|POST|PUT|DELETE|PATCH)\s+\/[^\s].*\(401\b.*Unauthorized\)/i) is
stricter than the Rust is_session_expired_error logic; update the condition so
it only requires the message to start with an HTTP verb + space + '/' and to
contain both "401" and "unauthorized" (case-insensitive) rather than requiring
the specific "(401 ... Unauthorized)" sequence—i.e., replace the single complex
regex check on message with a starts-with-verb check plus separate contains
checks for "401" and "unauthorized" (use the same message variable and keep the
same verb list GET/POST/PUT/DELETE/PATCH).
In `@src/core/jsonrpc.rs`:
- Around line 196-201: The provider auth failure log in the
is_downstream_provider_auth_error branch currently logs msg directly; change the
log to use the same sanitizer as the session-expired path by passing the error
through sanitize_api_error before logging (i.e., use sanitize_api_error(msg) in
the log::info call), keeping the log message and method variable unchanged so
provider errors are consistently sanitized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84a6abb7-3377-40d9-9d0a-6b23352bd735
📒 Files selected for processing (4)
app/src/services/__tests__/coreRpcClient.test.tsapp/src/services/coreRpcClient.tssrc/core/jsonrpc.rssrc/core/jsonrpc_tests.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — fix(auth): narrow SessionExpired to confirmed OpenHuman backend 401s
Solid, well-scoped fix for a high-severity session-stability bug. The root cause analysis is excellent — the old is_session_expired_error matched any "401 + unauthorized" substring, which meant provider-originating 401s (Discord bot tokens, BYO-key OpenAI/Anthropic, Composio) were nuking the user's session. The fix correctly uses the HTTP-method prefix (GET /, POST /, etc.) as a discriminator since authed_json in rest.rs always formats errors with that prefix while provider errors start with the provider name.
What's good
- Clean separation: explicit markers (
Session expired,SESSION_EXPIRED,no backend session token,session jwt required) checked first, then the HTTP-method-prefix 401 path. is_downstream_provider_auth_erroris properly scoped to diagnostic logging only — no side effects.- The
else ifat theinvoke_methodcall site means backend 401s never double-log as provider errors. - Rust and TS classification logic mirror each other correctly.
- 10 Rust tests + 3 new TS test rows covering the key cases (backend 401, Discord, BYO-key, Composio).
- All CI green, including coverage gate.
Issue alignment
- #2286 — all acceptance criteria met: session expiry narrowed, downstream 401s typed as recoverable, no token wipe on unrelated 401, logging improved with method/source context, Discord regression protected, tests comprehensive.
- #2285 — fixed as a consequence (Discord card-click no longer triggers session expiry).
| Area | Files | Verdict |
|---|---|---|
| Rust core | jsonrpc.rs, jsonrpc_tests.rs |
Clean |
| Frontend | coreRpcClient.ts, coreRpcClient.test.ts |
Clean |
One minor note inline. No critical or major issues found.
…nyhumansai#2356 - Sanitize provider auth error msg via sanitize_api_error before logging (addresses @coderabbitai on src/core/jsonrpc.rs:196-201) - Add comment noting HEAD/OPTIONS intentionally excluded from HTTP method allowlist (addresses @graycyrus on src/core/jsonrpc.rs:219) - Align TS classifyRpcError HTTP-verb 401 check with Rust is_session_expired_error: use starts-with-verb check + separate /401/ and /unauthorized/i tests instead of strict /(401.*Unauthorized)/ regex (addresses @coderabbitai on app/src/services/coreRpcClient.ts:111-138)
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Continuation review — all prior feedback addressed.
The follow-up commit adds the HEAD/OPTIONS scope comment I flagged, and also hardens the downstream provider auth log by redacting through sanitize_api_error — good improvement.
The regex restructuring in coreRpcClient.ts (splitting the single pattern into three separate checks aligned with the Rust side) is cleaner and easier to reason about.
No new issues found. Previous [minor] resolved. This PR is ready for merge.
# Conflicts: # src/core/jsonrpc.rs # src/core/jsonrpc_tests.rs
|
huge thanks @M3gA-Mind 🙌 love how surgical this fix is, scoping sessionexpired to only confirmed openhuman 401s (and keeping provider auth stuff as diagnostic-only) is exactly the kind of nuance that saves so much debugging pain later. always a treat seeing your prs land 🚀 |
|
4 participants
Summary
is_session_expired_errorinsrc/core/jsonrpc.rssoDomainEvent::SessionExpiredonly fires for confirmed OpenHuman session expiry, not for downstream provider 401s.is_downstream_provider_auth_errorhelper for diagnostic logging only (no session side-effects).'provider_auth'error kind toCoreRpcErrorKindincoreRpcClient.ts; tightensclassifyRpcErrorwith the same HTTP-method-prefix logic.Root Cause
is_session_expired_errorused 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_jsoninsrc/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
src/core/jsonrpc_tests.rs— 10is_session_expired_errortests covering: HTTP-method-prefix matches, Discord exclusion, BYO-key exclusion, Composio exclusion, explicit markers still matchapp/src/services/__tests__/coreRpcClient.test.ts— 3 newtest.eachrows: Discord/OpenAI/Anthropic 401 →provider_auth; existingGET /teams failed (401 Unauthorized)→auth_expiredpreservedcargo test -p openhuman is_session_expired— 10/10 passpnpm test:coverage— full Vitest suite passpnpm compile+cargo check— cleanpnpm format:check— cleanSubmission Checklist
jsonrpc.rsandcoreRpcClient.tscovered by unit testsRelated
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
Commit & Branch
fix/session-expired-cascade-2286Validation Run
pnpm --filter openhuman-app compilepnpm --filter openhuman-app format:checkpnpm --filter openhuman-app lintcargo check --manifest-path Cargo.tomlpnpm test:coverage(Vitest full suite)cargo test -p openhuman is_session_expired(10/10 pass)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
GET /teams failed (401 Unauthorized)) still trigger session expiryapi_errorininference/provider/ops.rsstill publishes SessionExpired directly for backend auth failures (independent of this fix)Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests