fix: enforce identity and local-only commit in OneKey ID auth state#12539
fix: enforce identity and local-only commit in OneKey ID auth state#12539sidmorizon wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2708a7df0e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (slotTokenSub !== inFlightTokenSub) { | ||
| defaultLogger.prime.subscription.onekeyIdSessionPersistFailed({ | ||
| reason: `${callerName}: keyless session slot was replaced by a different account, skip server login`, | ||
| }); | ||
| throw new OneKeyLocalError( | ||
| `${callerName} ERROR: keyless session slot was replaced by a different account`, |
There was a problem hiding this comment.
Avoid clearing the replacement keyless session
When this new identity check detects the exact concurrent popup/expand-tab race described above, the slot now contains another valid keyless session, but the thrown OneKeyLocalError is not considered transient by isTransientNetworkLikeError. The callers I checked (PrimeLoginOAuthDialog, keyless create, and legacy bind) clear the OAuth/keyless session on any non-transient apiOAuthLogin/bind failure, so the losing flow will wipe the winning flow's replacement slot and can make both concurrent logins fail instead of allowing the replacement session to continue. This race-specific mismatch needs a non-destructive/error classification path for callers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 36b7ade. The sub-mismatch branch now throws a typed OneKeyErrorOneKeyIdKeylessSessionSlotReplaced (className survives the bg→main bridge, same mechanism as OneKeyErrorOneKeyIdOAuthIdentityAlreadyBound), and every teardown-on-definitive-failure site (PrimeLoginOAuthDialog, startKeylessCreateWithOAuthProvider, both OneKeyIdLegacyOAuthBind paths) skips session teardown for that className — the losing flow still fails with a toast, but the winning flow's replacement session stays intact. Empty/corrupt/undecodable slot states keep the plain definitive error since their slot is unusable and cleanup is harmless. Covered by a unit test asserting the typed class and that the POST is never reached.
Generated by Claude Code
| // outside the lock. | ||
| const slot = | ||
| await readPersistedAccessTokenBySessionSourceStrict(authSessionSource); | ||
| if (slot.status !== 'ok') { |
There was a problem hiding this comment.
🟠 P1 提交段只校验槽位占用、未复核身份,guard 的身份检查在 commit 处失效
问题
assertKeylessSessionPersistedBeforeLogin 在 POST 之前比对了 slot 的 JWT sub 与 in-flight accessToken 的 sub,但真正写入登录身份的提交段 commitAuthSessionSourceAndPrimeAtom 只做了占用检查:
const slot =
await readPersistedAccessTokenBySessionSourceStrict(authSessionSource);
if (slot.status !== 'ok') { ... throw ... }persistKeylessOAuthSession 运行在 main runtime 且不持有 bg 的 loginMutex。因此在 guard(身份检查) 与 commit(占用检查) 之间的窗口内(含中间的网络 POST,窗口并不短),另一并发流程(ext popup vs expand tab)可把共享 keyless 槽位改写成 另一个账号 B 的 session。commit 只看到 status === 'ok' 便照常提交 authSessionSource=KeylessOAuth + atom(来自账号 A 的服务器响应),完全不会复核 sub。
这正是 guard 注释(slotTokenSub !== inFlightTokenSub)声称要阻止的 “atom 属于 A、槽位属于 B” 局面——只是把它从 POST 前挪到了 commit 期间重新打开。
影响
结果是持久化出 atom = 账号 A、slot = 账号 B 的不一致对:后续所有走 getActiveAuthToken() -> getAuthTokenBySessionSource(KeylessOAuth) 的 OneKey ID 已认证请求都会用 B 的 token,而 UI/atom 显示为 A,即跨账号身份/令牌混淆。
触发需要 B 的 persistKeylessOAuthSession 恰好落在 guard 与 commit 之间,且 B 随后不再完成自身 apiOAuthLogin 提交(否则会被 B 的登录自愈)——概率窗口窄,但一旦发生即为持久化的错账号状态,且正是本 PR 宣称要消除的失效模式。现有测试只覆盖 “guard 之前被替换”,未覆盖此窗口。
建议
把身份不变量下推到 mutex 内:对 KeylessOAuth 源,在 commit 的严格读之后追加一次 sub 复核(把期望 sub 或 accessToken 传入 commitAuthSessionSourceAndPrimeAtom),mismatch 时走与空槽相同的 rollback(clearAuthTokens + setPrimePersistAtomNotLoggedIn)。legacy 源无 keyless 身份,可跳过该复核。
if (expectedSub && (decodeJWT(slot.accessToken)?.sub || '') !== expectedSub) {
throw new OneKeyLocalError(`${callerName} ERROR: slot replaced by a different account`);
}Generated by Claude Code
There was a problem hiding this comment.
Fixed in 393b976, exactly as suggested — the identity invariant is now re-asserted inside the commit lock. assertKeylessSessionPersistedBeforeLogin returns the guard-verified in-flight sub; apiOAuthLogin and apiBindLegacyOneKeyIdOAuth pass it into commitAuthSessionSourceAndPrimeAtom as expectedSlotTokenSub, and after the strict in-lock slot read the commit compares the slot token's sub against it. Mismatch → the typed OneKeyErrorOneKeyIdKeylessSessionSlotReplaced (so main-runtime teardown-skip applies — the slot holds the winning flow's session) → the existing rollback (clearAuthTokens + setPrimePersistAtomNotLoggedIn, slot untouched) → rethrow. Undecodable slot payload aborts like a corrupt slot. Legacy-realm apiLogin omits the param as suggested. New unit test covers the guard→POST→commit replacement window (POST fired, typed error, rollback called, atom=A/slot=B state impossible); the rotated-same-identity test now also covers the commit-side read. 32/32 passing.
Generated by Claude Code
Summary
Second batch from the OneKey ID / Keyless OAuth unification review — the two login-state-commit-core defects deliberately excluded from #12538, plus the onboarding-cache ordering fix. Runtime scope:
ServicePrimechanges are bg-runtime;useKeylessWalletis main-runtime UI.1. Identity check in the pre-login keyless slot guard (TOCTOU)
assertKeylessSessionPersistedBeforeLoginpreviously only proved that a session occupies the shared keyless slot. SincepersistKeylessOAuthSessionruns in the main runtime outsideloginMutex, a concurrent flow (ext popup vs expand tab) could overwrite the slot with another account's session between persist and commit — the POST would authenticate account A while the committed local state served account B's slot; on the bind path the server-side identity bind is irreversible.The guard now takes the in-flight
accessTokenand compares JWTsubclaims (payload decode via existingstringUtils.decodeJWT, no signature/expiry verification — the server validates the POSTed token) between the slot session and the in-flight token. Sub mismatch or undecodable payload → definitive abort before the server POST. Raw-byte comparison is deliberately not used: bg auto-refresh legitimately rotates tokens for the same identity. Applied at both call sites (apiOAuthLogin,apiBindLegacyOneKeyIdOAuth).2. Local-only, rollback-on-failure login commit (half-committed state)
commitAuthSessionSourceBeforeAtomUpdateperformed a network-capablegetActiveAuthToken()(SDKgetSession, can trigger a network token refresh or throw transient sealed-storage errors) insideauthStateWriteMutex, violating the file's own lock policy; a throw aftersetAuthSessionSourceescaped with the source committed but the atom not updated — server logged in, source persisted, UI logged out, device-limit slot consumed.Now renamed
commitAuthSessionSourceAndPrimeAtom:readPersistedAccessTokenBySessionSourceStrict, verified to support both realms) — no network I/O inside the lock. Callers don't need a refresh at this point: their token was just accepted by the server; steady-state reads still refresh outside the lock viagetActiveAuthToken.clearAuthTokens+setPrimePersistAtomNotLoggedIn) before rethrowing. No path can exit with source committed but atom not updated. Bind-path rollback to logged-out mirrors the documented empty-slot precedent (re-login recovers).getSessionexception is now scoped to the cleanup guards only.3. Onboarding cache: validate before caching, clear on mismatch
In ResetPin/VerifyPinOnly modes,
checkKeylessWalletCreatedOnServerwrote the OAuth token into the module-level 8-minute onboarding LRU beforevalidateTokenMatchesKeylessWallet, and a mismatch (wrong Google/Apple account picked) left the wrong account's token cached as "the onboarding token". Now the cache is written only after validation succeeds (key-wisedelete('socialLoginToken')on mismatch — unrelated entries like the same-email status survive), while the default create/restore path keeps its original write timing (all downstream consumers traced; they run post-navigation).Test
ServicePrime.test.ts: 31/31 passing, including 7 new cases — commit success asserts strict-local read (and thatgetActiveAuthTokenis never called in-lock), empty-slot / transient-read / atom-update-failure rollbacks, identity-guard abort before POST on sub mismatch, rotated-same-identity acceptance, undecodable-token definitive abort.yarn agent:check --profile commitpasses (lint, format, tsc).Notes for reviewers
This touches the login-state commit core hardened in #12526 — the residual risks called out there (slot occupancy-only check, in-lock
getSession) are exactly what this PR closes. Recommend review by whoever owns #12526.Generated by Claude Code