fix(auth): surface the silent /auth/me store-failure that bounces login to signin#3734
Conversation
…in to signin When store_session's GET /auth/me validation fails (esp. a timeout / gateway 5xx), the auth profile is never persisted, so a 'successful' OAuth bounces the user straight back to the signin page on the next snapshot refresh. This was invisible across all layers (FE: error caught, no console-capture integration; core: timeout dropped as transient by observability before_send; backend: only status==500 pages Sentry), making it very hard to diagnose. - FE: capture the store failure in handleAuthDeepLink with a PII-free 'kind' tag (auth_me_timeout/unauthorized/gateway/network) + stable fingerprint so it groups/alerts in Sentry (consent-gated like every other error). - Core: add a WARN at the /auth/me store gate so the app log records it even when Sentry is off/unreachable. - Tests: injection #1 (store failure -> no session applied, no SESSION_TOKEN_ UPDATED, no /home nav -> stays on signin) + classifyAuthStoreFailure units.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe backend now returns a structured ChangesAuth/me failure observability
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01c923ca8e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // the classification on a TAG (survives) + a stable fingerprint so every | ||
| // session-store failure groups together and is alertable. The exception | ||
| // message ("Session validation failed (GET /auth/me): …") carries no token. | ||
| Sentry.captureException(error instanceof Error ? error : new Error(rawMessage), { |
There was a problem hiding this comment.
When /auth/me returns a non-2xx response, BackendOAuthClient::fetch_current_user includes the raw response body in the propagated error (GET /auth/me failed ({status}): {text}), and this catch now sends that original exception message to Sentry. In any backend failure that includes user/account details or request diagnostics in the body, beforeSend strips extras and request data but does not redact exception messages, so the new capture can violate the app's no-PII Sentry contract. Capture a constant/sanitized error keyed by kind instead of the raw exception.
Useful? React with 👍 / 👎.
| // the classification on a TAG (survives) + a stable fingerprint so every | ||
| // session-store failure groups together and is alertable. The exception | ||
| // message ("Session validation failed (GET /auth/me): …") carries no token. | ||
| Sentry.captureException(error instanceof Error ? error : new Error(rawMessage), { |
There was a problem hiding this comment.
When openhuman.auth_store_session exceeds the default 30s RPC budget, storeSession has no timeout override and callCoreRpc throws a CoreRpcError with kind='timeout'; analytics.ts then drops exactly those originalException shapes in beforeSend before the new tags/fingerprint matter. That means the /auth/me hang case this capture is meant to make alertable still never reaches Sentry. Capture a sanitized non-CoreRpcError for this diagnostic or update the filter to exempt the deep-link-auth-store event.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/utils/desktopDeepLinkListener.ts (2)
278-279: ⚡ Quick winUse the project’s namespaced
debuglogger instead ofconsole.warnin this new path.Please switch this new warning to the frontend debug utility for consistency with repo logging standards.
As per coding guidelines, “Use namespaced
debugfunction in frontend logging; never log secrets or full PII.”🤖 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/utils/desktopDeepLinkListener.ts` around lines 278 - 279, The console.warn call in the desktopDeepLinkListener.ts file violates the project's logging standards by using console.warn instead of the namespaced debug logger. Replace the console.warn statement on line 278 with the project's debug utility, maintaining the same message and format parameters. Import the debug logger at the top of the file if not already present, and ensure the namespace follows the project's convention for deep link authentication logging.Source: Coding guidelines
293-313: ⚡ Quick winExtract
classifyAuthStoreFailureinto a focused utility module.This file is now ~540 lines; moving this helper to a small utility keeps
desktopDeepLinkListener.tswithin the size target and easier to maintain.As per coding guidelines, “Keep frontend file size to ≤ ~500 lines; break larger components into smaller, focused modules.”
🤖 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/utils/desktopDeepLinkListener.ts` around lines 293 - 313, Extract the classifyAuthStoreFailure function from desktopDeepLinkListener.ts into a new focused utility module. Create a new utility file dedicated to authentication classification helpers, move the entire classifyAuthStoreFailure function definition (including its JSDoc comment) to this new utility file with a proper export statement, and then import and re-export it in desktopDeepLinkListener.ts to maintain the existing API while reducing the file size.Source: Coding guidelines
🤖 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.
Inline comments:
In `@app/src/utils/desktopDeepLinkListener.ts`:
- Around line 268-277: The generic catch block in handleAuthDeepLink currently
sends all errors to Sentry via the Sentry.captureException call, including
potentially sensitive information from upstream Rust errors. Add a filtering
check before the Sentry.captureException call that validates the error matches
the known `/auth/me` validation failure pattern (as determined by
classifyAuthStoreFailure). Only send the error to Sentry if it matches this
whitelisted pattern, otherwise allow it to fail silently or handle it
differently, ensuring that unfiltered error messages with sensitive information
from consumeLoginToken or storeSession do not reach Sentry.
In `@src/openhuman/credentials/ops.rs`:
- Around line 154-171: The error message prefix "Session validation failed (GET
/auth/me):" returned by store_session when fetch_current_user fails is a
contract with the frontend JavaScript code that classifies these errors. Add a
test to ops_tests.rs that mocks the fetch_current_user method to return an error
and verifies that the error returned by store_session starts with exactly
"Session validation failed (GET /auth/me):" to ensure this cross-layer contract
remains stable and prevents accidental breakage.
---
Nitpick comments:
In `@app/src/utils/desktopDeepLinkListener.ts`:
- Around line 278-279: The console.warn call in the desktopDeepLinkListener.ts
file violates the project's logging standards by using console.warn instead of
the namespaced debug logger. Replace the console.warn statement on line 278 with
the project's debug utility, maintaining the same message and format parameters.
Import the debug logger at the top of the file if not already present, and
ensure the namespace follows the project's convention for deep link
authentication logging.
- Around line 293-313: Extract the classifyAuthStoreFailure function from
desktopDeepLinkListener.ts into a new focused utility module. Create a new
utility file dedicated to authentication classification helpers, move the entire
classifyAuthStoreFailure function definition (including its JSDoc comment) to
this new utility file with a proper export statement, and then import and
re-export it in desktopDeepLinkListener.ts to maintain the existing API while
reducing the file size.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26ed7ec3-3129-480e-a73c-456a7e7180b9
📒 Files selected for processing (3)
app/src/utils/__tests__/desktopDeepLinkListener.test.tsapp/src/utils/desktopDeepLinkListener.tssrc/openhuman/credentials/ops.rs
… for the WARN gate) Adds a focused integration test that points config.api_url at a local server returning HTTP 500 on /auth/me, asserts store_session returns the 'Session validation failed (GET /auth/me)' Err (so the profile is never persisted and the user bounces to signin). Exercises the WARN/Err branch added in the prior commit.
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Pure-observability PR for the "OAuth succeeds but app bounces back to signin" case. Adds Sentry captureException (PII-free kind tag + fingerprint) in the store-failure branch of handleAuthDeepLink, a grep-friendly tracing::warn! at the /auth/me gate in credentials/ops.rs, plus FE + Rust tests. No control-flow/UX change.
Good shape: classifier keeps the message PII-free in intent, stable fingerprint, no behavior change, both new tests assert the right contracts. The instinct to surface this silent failure is correct.
Requesting changes — two issues center on the same fix:
Blocker — raw /auth/me body leaks to Sentry. The captured exception carries error.message, which on a non-2xx /auth/me is built at rest.rs:416 as "GET /auth/me failed ({status}): {text}" where {text} is the verbatim backend body (email/account fields, possibly token-adjacent). beforeSend (analytics.ts:173-176) scrubs extra/breadcrumbs/request body but not event.exception.values[].value — so the body ships. Don't scrub the message; sever it. Capture a synthetic error keyed by kind:
Sentry.captureException(new Error(`auth store failed: ${kind}`), {
level: 'error',
tags: { auth_store_failure: kind },
fingerprint: ['deep-link-auth', 'session-store-failed', kind],
});Major (same fix) — timeout case is dropped by beforeSend. The doc-comment names timeout as the lead cause, but a hang surfaces as CoreRpcError(kind='timeout'), which beforeSend returns null for (analytics.ts:173-176) before the new tag/fingerprint apply — the most important shape never reaches Sentry. The synthetic new Error(...) above also fixes this: once originalException is a plain Error, the timeout drop no longer matches. Land both together.
Major — deferred RCA needs a tracked issue, not a doc-comment. This ships defense-in-depth but defers the real fix (why does /auth/me time out / 5xx at store time; should store retry transient failures instead of hard-bouncing?). Per S3.5.d, open a tracking issue and link it in the PR body so the lead doesn't rot.
Minor — classifier has no contract test to its source. classifyAuthStoreFailure matches substrings of the Rust-produced string; if ops.rs / rest.rs reword, arms silently degrade to other. Add an FE test that pins the actual Rust prefix to a non-other kind so wording drift fails CI.
…er timeout shape
- Capture a synthetic Error keyed only by 'kind' instead of the original error,
so the verbatim /auth/me response body (rest.rs 'GET /auth/me failed
({status}): {text}') never ships to Sentry (beforeSend does not scrub
exception.values[].value) — P1/blocker (codex, coderabbit, oxoxDev).
- Same change makes hint.originalException a plain Error, so beforeSend's
isCoreRpcTimeoutError drop no longer swallows the timeout shape — the lead
cause now reaches Sentry (P2 — codex, oxoxDev).
- Add a classifier contract test pinning the real Rust store_session strings
(gateway/timeout/bare prefix) so wording drift in ops.rs/rest.rs fails CI
instead of degrading to 'other' (minor — coderabbit, oxoxDev).
Deferred RCA tracked in tinyhumansai#3737 (linked in PR body).
|
Thanks @oxoxDev @coderabbitai (+codex) — addressed in b7eda86:
All FE tests (23) + the Rust gate test pass locally; typecheck + fmt clean. |
|
Tip For best results, initiate chat on the files or code changes.
Looks good to me — thanks for the thorough write-up! 🎉 （(\ /) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/app_state_credentials_raw_coverage_e2e.rs`:
- Around line 229-253: The test
`store_session_auth_me_failure_returns_err_and_does_not_persist` currently only
verifies the error message returned, but does not explicitly assert that the
session was not persisted to storage. Add an explicit assertion after the error
message check to verify that no session/auth state was persisted in the config
or that the session remains in an unauthenticated state (for example, by
checking that harness.config() shows no valid session after the failed
store_session call). This ensures the "does_not_persist" contract mentioned in
the test name is actually validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e7eaf68-ce38-4c0f-8a6b-cab2197488de
📒 Files selected for processing (3)
app/src/utils/__tests__/desktopDeepLinkListener.test.tsapp/src/utils/desktopDeepLinkListener.tstests/app_state_credentials_raw_coverage_e2e.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/utils/tests/desktopDeepLinkListener.test.ts
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Re-review of b7eda86 — all four prior items resolved. Approving.
BLOCKER (PII via raw body) — RESOLVED
desktopDeepLinkListener.ts:281 now captures a synthetic new Error("auth store failed: " + kind). The raw error / verbatim /auth/me backend body never reaches Sentry.captureException. rawMessage (:259) is used only for local classification + isDecryptionFailure — neither forwards to Sentry. exception.values[].value is now PII-free, so the beforeSend scrub gap no longer matters.
MAJOR (timeout dropped by beforeSend) — RESOLVED
originalException is now a plain Error, not CoreRpcError(kind='timeout'), so isCoreRpcTimeoutError(originalException) no longer matches and drops it. The hang case the PR targets now reaches Sentry tagged auth_me_timeout (:283-284).
MAJOR (deferred RCA in prose only) — RESOLVED
Remediation is now a tracked issue (#3737, OPEN) linked in the PR body ## Related — no longer just comments in ops.rs.
minor (classifier string drift) — RESOLVED
New contract test (desktopDeepLinkListener.test.ts:379-394) pins real Rust store_session strings (Session validation failed (GET /auth/me): … + nested rest.rs body) to auth_me_gateway / auth_me_timeout / auth_me_other, and asserts the bare prefix is not other — a Rust prefix reword now fails CI.
No remaining concerns. LGTM.
…e failure Per CodeRabbit review: the test name claims 'does_not_persist' but only checked the Err text. Add explicit assertions that, after a failed store_session, the app_state snapshot reads back unauthenticated with no session_token — so a partial regression that wrote a profile fails CI.
|
@coderabbitai addressed in 1b3996c:
Test passes locally ( |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Re-review of 1b3996c55 (test-only) — APPROVE.
The does_not_persist contract is now genuinely asserted. Previously the test only checked the Err text; the new commit reads the post-failure snapshot back and asserts:
!snap.auth.is_authenticatedsnap.session_token.is_none()
snapshot() derives both from the persisted app-session auth profile, so a partial-write regression that wrote a profile/token before the /auth/me gate fails would now fail CI — not slip through on the Err check alone. Assertions are non-vacuous.
Scope: one test file, no product code changed. Prior fixes intact (PII redaction, snapshot timeout, RCA tracking #3737, classifier contract test).
Nice tightening. LGTM.
coderabbitai
Bot
added
bug
rust-core
|
@coderabbitai review — the prior CHANGES_REQUESTED predates 1b3996c, which adds the explicit does-not-persist assertions (snapshot → unauthenticated + no session_token) and locks the Rust /auth/me failure string. Please re-review the latest commit. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the latest PR diff and changed files
gh pr view 3734 --repo tinyhumansai/openhuman --json files,commits,state,reviews 2>/dev/null | jq '{state,commits: [.commits[-3:]],files: [.files[].path],reviews: [.reviews[-3:] | {state,author: .author.login}]}'Length of output: 575 🏁 Script executed: #!/bin/bash
# Get the latest diff for the Rust e2e test file
gh pr diff 3734 --repo tinyhumansai/openhuman 2>/dev/null | grep -A 200 'tests/app_state_credentials_raw_coverage_e2e.rs'Length of output: 4445 🏁 Script executed: #!/bin/bash
# Get the latest diff for the frontend files
gh pr diff 3734 --repo tinyhumansai/openhuman 2>/dev/null | grep -A 120 'desktopDeepLinkListener'Length of output: 11699
Rust test (
All four concerns from the prior [approve] |
…prefix Per CodeRabbit: lock the cross-layer error-string contract (store_session → frontend classifyAuthStoreFailure) with starts_with (not contains), so a reword fails CI instead of silently degrading the FE classifier to 'other'.
|
@coderabbitai review — addressed the error-string contract in 06725de (exact-prefix |
|
I could not push directly to the PR head fork (GitHub returned 403 for sanil-23/openhuman), so I opened a focused helper PR with the remaining CodeRabbit fix: https://github.com/sanil-23/openhuman/pull/17\n\nIt adds only the requested Rust contract test in \src/openhuman/credentials/ops_tests.rs\ for the unresolved thread #3734 (comment), asserting the \Session validation failed (GET /auth/me):\ prefix used by the frontend classifier.\n\nLocal validation on the helper branch:\n- \cargo fmt --manifest-path Cargo.toml --check\n- \cargo test --manifest-path Cargo.toml --lib store_session_auth_me_failure_keeps_error_prefix_contract\n\nNote: I pushed the helper branch with --no-verify\ because the local pre-push hook reached the Tauri CEF CMake build and failed in this Windows environment with an MSVC path tokenization issue, unrelated to this Rust unit-test-only change. |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Full re-review @ 06725de — observability for the "OAuth succeeds but app bounces to signin" store-failure.
Verdict: APPROVE. PII blocker genuinely severed; no remaining blocker. Re-verified all 4 changed files end-to-end against current code.
Security (PII) — primary axis — CLEAN
desktopDeepLinkListener.ts:284capturesnew Error("auth store failed: " + kind). The raw error / verbatim/auth/mebody (rest.rs:416GET /auth/me failed ({status}): {text}) is NOT passed tocaptureException.rawMessageonly feeds the classifier.- Tags + fingerprint carry only the enum
kind(classifyAuthStoreFailurereturns fixed strings: timeout/unauthorized/gateway/network/auth_me_other/other). No URL/token/body. - Synthetic
Errorhas no.cause, solinkedErrorsIntegrationcannot chain the raw message back in. Confirmed severed.
Correctness — CLEAN
- Timeout reach-through verified: a plain
Errormakeshint.originalExceptionnon-CoreRpcError, sobeforeSend'sisCoreRpcTimeoutErrordrop (analytics.ts:175) no longer fires — the lead cause finally reaches Sentry. Matches the PR claim. ops.rs:151+keeps theErrcontract prefixSession validation failed (GET /auth/me):and WARNs before returning; no profile persisted on failure.
Testing — non-vacuous, contracts pinned
- Rust
store_session_auth_me_failure_returns_err_and_does_not_persisthits a real local 500 server, assertserr.starts_with(prefix)(reword → CI fail), AND reads the snapshot back:!is_authenticated+session_token.is_none(). Genuinely asserts does-not-persist, not just Err text. - FE
classifyAuthStoreFailuretable + the "pins real Rust strings" test couple the classifier to the Rust-produced message; a prose drift fails CI rather than degrading toother.
Design / RCA
- Observability-only; RCA deferred to a tracked issue (per PR). Acceptable: this surfaces a previously-silent bounce on a non-data path.
Prior items
- (1) raw body → Sentry: RESOLVED (ts:284, synthetic Error).
- (2) timeout dropped by beforeSend: RESOLVED (plain Error bypasses the timeout filter).
- (3) deferred RCA tracked: present per PR/issue link.
- (4) classifier↔Rust contract test: RESOLVED (FE pin + Rust
starts_with). - (5) does_not_persist now snapshot-asserted: RESOLVED (e2e:273+).
Open (non-blocking)
- CodeRabbit nit: new
console.warn(ts:285) vs namespaceddebuglogger. The whole file already usesconsole.*— consistent with local convention; cosmetic, not a merge blocker.
Verified good: PII path, timeout reach-through, both contract tests, no-persist snapshot.
3 participants
Summary
/auth/mestore-failure that bounces a "successful" OAuth straight back to the signin page.Sentry.captureExceptionof a synthetic Error keyed only bykind(PII-free; severs the raw /auth/me body) + stable fingerprint, inhandleAuthDeepLinks store-failure branch.WARNat the/auth/mestore gate so the app log records it even when Sentry is off/unreachable.classifyAuthStoreFailureunits, and a Rust integration test covering the store-time/auth/mefailureErrgate.Problem
store_sessionvalidates the session JWT viaGET /auth/mebefore persisting the profile (src/openhuman/credentials/ops.rs). If that call fails (timeout / gateway 5xx / 401), nothing is persisted, so the next snapshot refresh readssessionToken=nullandProtectedRoute/DefaultRedirectsend the user back to signin — even though the browser showed OAuth success.This failure was invisible on every layer, which is why it's been hard to diagnose:
handleAuthDeepLink; there's noCaptureConsoleintegration, so theconsole.errornever reaches Sentry./auth/metimeout is classified transient and dropped byobservability.rsbefore_send.setupExpressErrorHandleronly pagesstatus === 500; 502/503/504/timeouts are excluded (BACKEND-ALPHAHUMAN-40).Solution
extra, whichbeforeSenddeletes) so the kind survives, groups, and is alertable; consent-gated exactly like every other error (no bypass). Message carries no token.WARN([credentials][auth-store] GET /auth/me validation FAILED …) gives a local app-log signal independent of Sentry/consent.store_sessionstill returns the sameErr; the FE catch still callsfailDeepLinkAuthProcessing.Submission Checklist
injection #1FE failure-path test +classifyAuthStoreFailureunits + Ruststore_session_auth_me_failure_returns_err_and_does_not_persist.WARN/Errgate is covered by the new Rust integration test (local/auth/meserver returning 500 → assertsstore_sessionerrors).## Related— N/A: no feature row affected.Closes #NNN— N/A: adds observability for an open failure mode; not a full fix (see Related).Impact
Related
{}for the user.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/auth-me-store-failure-observabilityValidation Run
pnpm --filter openhuman-app format:check(prettier clean on changed files;cargo fmt --checkclean)pnpm typecheckdesktopDeepLinkListener.test.ts(injection Feat/gitbooks #1 + classifier) — 22 passing; Rustapp_state_credentials_raw_coverage_e2e::store_session_auth_me_failure_returns_err_and_does_not_persist— passingcargo fmt --checkclean;cargo check+ pre-pushrust:checkpassValidation Blocked
Behavior Changes
Parity Contract
store_sessionreturns the sameErr; catch path unchangedfailDeepLinkAuthProcessing; decryption/readiness branches untouchedDuplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests
/auth/mesession-validation failures (including non-persistence behavior).