feat(desktop): redesign Nostr bind verification flow#1850
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf1f5a2c4d
ℹ️ 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".
wesbillman
left a comment
There was a problem hiding this comment.
Two blockers before this identity-signing flow is safe to ship:
-
Show the requesting origin before signing. This PR removes the only disclosure of
payload.origin, yet the signing command accepts any syntactically valid HTTPS origin and embeds it in the signed identity proof. The six-digit check does not authenticate the caller: both the code and origin come from the same attacker-controllable deep link, so a phishing page can launch Buzz with its own origin, show its matching code, and obtain a valid proof for that origin without Buzz ever telling the user what they are authorizing. Clipboard return does not fix that—the user can paste the response back into the phishing page. Keep the focused flow, but display the exact origin prominently before Continue (and ideally the identity being bound). This is consent context, not a claim of OS attestation. -
Add the claimed focused tests to the PR. This is a 644-line behavioral rewrite of a security-sensitive signing surface, but the only changed file is the component; there is no committed Playwright/unit coverage for OTP entry/paste/navigation, mismatch behavior, reduced motion, expiry, signing, copy failure, or origin disclosure. Existing green desktop CI therefore does not exercise the new flow. Please add the focused coverage described in the PR body, including a test proving the requesting origin is visible before signing and that no signing call occurs for incomplete/mismatched/expired input.
wesbillman
left a comment
There was a problem hiding this comment.
Two blockers remain at bf1f5a2c4:
- The signing confirmation still omits
payload.origin, so users cannot see which requesting origin they are authorizing before signing. That origin is self-asserted, but it is still essential consent context and must be displayed. - The PR description claims focused verification tests, but the branch changes only the component and commits no tests for the rewritten bind flow. Please add focused committed coverage for the request details and approve/reject behavior.
The branch also no longer contains current main; update and rerun CI after addressing these findings.
Summary
Testing
just ciScreenshots
Enter verification code
Six portrait code slots with Continue gated until the browser code matches.
Code mismatch
Persistent, layout-stable mismatch feedback using the specified error treatment and shake motion.
Finish verification
The signed response is shown in a readable code container with an explicit copy action.