Self-hosted: have the signer report which wallet actually signed - #1366
Conversation
Closes #1361's residual case, the one #1360 and #1362 could not reach because it is not a hardcoded string. signBufferWithExtension takes a preferred wallet id, and when that extension has been uninstalled mid-session it falls through to Keeper-first detection rather than failing. The caller knew only the id it passed in, so a message about the prompt the user had just seen named a wallet they no longer had installed. It now returns the id it actually used. The chosen fix is the additive one the issue named: nothing about which wallet is asked to sign changes, only what the caller is told afterwards. Resolution reports its own id rather than mapping the instance back to one. Keeper aliases itself onto window.hive_keychain, so comparing object identity against getKeychainInstance() would label Keeper "Keychain" on a Keeper-only browser, which is the confusion this line of work exists to remove. Login records the wallet that signed instead of the one it asked for, so a stale preference is not written in the first place. That is where the mismatch starts. Tests drive the real function against a stubbed window and a real localStorage, rather than the preference path silently short-circuiting through its own try/catch and passing for the wrong reason. The call site is asserted separately: every behaviour test here passed with getHostingToken reverted to the stale user.extension, since the pieces being right is not the property, the caller using them is.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe authentication flow now tracks the extension that actually signs a challenge. Signing returns this extension, and login persistence and cancellation messages use it. Tests cover preferred signing, fallback wallets, aliases, missing extensions, and cancellation labels. ChangesSigner identity tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AuthActions
participant signBufferWithExtension
participant resolveSigningExtension
participant WalletExtension
AuthActions->>signBufferWithExtension: sign challenge
signBufferWithExtension->>resolveSigningExtension: resolve preferred or fallback wallet
resolveSigningExtension->>WalletExtension: request signature
WalletExtension-->>signBufferWithExtension: return signature
signBufferWithExtension-->>AuthActions: return signature and signed.extension
AuthActions->>AuthActions: save signed.extension
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/self-hosted/src/features/auth/utils/extension-labels.test.ts (1)
395-420: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the source assertion with a behavioral
getHostingTokentest.This test parses
hosting-token.tsand requires one exact argument expression. A valid refactor can break the test without changing behavior.Mock the challenge request and signing result. Assert that
getHostingTokenrejects with a cancellation message forsigned.extension.As per coding guidelines: “test user-visible behavior rather than implementation details.”
🤖 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 `@apps/self-hosted/src/features/auth/utils/extension-labels.test.ts` around lines 395 - 420, Replace the AST/source inspection test in the “passes signed.extension to the cancellation message” case with a behavioral test of getHostingToken. Mock the challenge request and signing result so the signing result contains signed.extension, then assert that getHostingToken rejects with the corresponding cancellation message. Remove the exact extensionCancelledMessage argument assertion and preserve coverage of the user-visible rejection behavior.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.
Nitpick comments:
In `@apps/self-hosted/src/features/auth/utils/extension-labels.test.ts`:
- Around line 395-420: Replace the AST/source inspection test in the “passes
signed.extension to the cancellation message” case with a behavioral test of
getHostingToken. Mock the challenge request and signing result so the signing
result contains signed.extension, then assert that getHostingToken rejects with
the corresponding cancellation message. Remove the exact
extensionCancelledMessage argument assertion and preserve coverage of the
user-visible rejection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f859df67-b1e4-48cc-b246-6606b9bbfcac
📒 Files selected for processing (4)
apps/self-hosted/src/features/auth/auth-actions.tsapps/self-hosted/src/features/auth/utils/extension-labels.test.tsapps/self-hosted/src/features/auth/utils/hive-extensions.tsapps/self-hosted/src/features/auth/utils/hosting-token.ts
… the tree Review point. resolveKeychainLikeDetected reproduced resolveKeychainLikeInstance's whole decision tree, the three preference branches and the Keeper-first fallback, and the old function stays live in sign-payment.ts and broadcastWithExtension. Two hand-kept copies of one resolution order is the lockstep-drift class this series keeps naming, and a divergence would send those callers to a different wallet than the signing path names. The instance-only function now returns the delegate's instance. Semantics are identical, including peakvault resolving to null, which the reviewer checked and so did I. The test that came with this was replaced. Comparing the two functions is comparing one with itself once delegation is in place, which asserts nothing. Delegation makes divergence impossible by construction, so what can actually regress is someone re-inlining the tree, and that is what is pinned: the body must call the delegate and must not name the branches. Confirmed by re-inlining it and watching only that case fail.
1 participant
Closes the residual case in #1361, the one #1360 and #1362 could not reach because it is not a hardcoded string.
signBufferWithExtensiontakes a preferred wallet id, and when that extension has been uninstalled mid-session it deliberately does not clear the stored preference and falls through to Keeper-first detection:So Keychain is recorded, Keychain is uninstalled, Keeper prompts, the user cancels, and the message named Keychain — the right name for the recorded preference, the wrong name for the wallet that actually appeared.
The fix
The additive option the issue named, not the behaviour-changing one. Nothing about which wallet gets asked to sign changes; only what the caller is told afterwards.
signBufferWithExtensionreturnsSignedByExtension, the signing result plus the id it actually used.getHostingTokennamessigned.extensioninstead of the session's storeduser.extension.Resolution reports its own id rather than mapping the resolved instance back to one. Keeper aliases itself onto
window.hive_keychain, so comparing object identity againstgetKeychainInstance()would label Keeper "Keychain" on a Keeper-only browser, reintroducing the exact confusion this work removes.Tests
Driven against a stubbed
windowand a reallocalStorage. The preference store's own try/catch would swallow a missinglocalStorageand returnnull, which happens to be the value these cases want, so the tests would have passed for a reason unrelated to what they assert.Cases: the asked-for wallet when present; the mid-session uninstall naming Keeper; Keeper's alias not being reported as Keychain; rejection when nothing is installed; and the cancellation message agreeing with the signer.
The call site is asserted separately, and here is why
Every behaviour test above passed with
getHostingTokenreverted touser.extension. The pieces being correct is not the property under test; the caller using them is.getHostingTokencannot be driven here because it opens a real socket to the hosting API, so the argument it passes is asserted through the AST.Mutations run, each failing only its own case: reverting the call site to
user.extension; reportingextId ?? resolved.idin the fallback, which is the bug restated; and reordering resolution to prefer Keychain over Keeper.Verification
799 passed, 59 files. Typecheck clean apart from the pre-existing gitignored
config.jsonerror.Summary by CodeRabbit
Bug Fixes
Tests