Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
👁️ Cyclops Security Review✅ Reviewed 08877bf (latest) · Coverage: partial ·
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/commands/login.ts— Users or agents who start a login with--idget retry commands without it, so a retry after an expiry or denial opens the passkey page.finishLoginbuildsloginwithcontinuationCommand(["wallet", "login"], network), and the E_LOGIN_EXPIRED, E_LOGIN_DENIED and logout-cancelled errors return that command. The pending login does not record that--idwas chosen. Fix: save the choice inPendingLogin(or readtempo_idback from the savedverification_uri) and add--idto everywallet loginretry commandfinishLoginbuilds, while bare logins keep the command they get today.Why this was flagged
An agent runs
tempo wallet login --id --no-wait.startLogin(src/commands/login.ts:168-170) addstempo_id=trueonly to the URLs in the saved registration and stores no flag. The user lets the code expire, andlogin statusreachesfinishLogin. The retry command at src/commands/login.ts:196 istempo wallet login --network <net>with no--id. It is returned as the next command for E_LOGIN_EXPIRED (line 238-243), E_LOGIN_DENIED (line 232-237) and the logout-cancelled E_USAGE (line 215-220). An agent that runs the returned command, as the_nextcontract invites, starts a passkey login, so the Tempo ID choice is silently lost. The base branch has no--id, so this only appears once the PR adds the option. No safeguard carries the choice forward:loginStatusHandleronly getsnetworkand the poll options, andPendingLoginhas no field for it.Verification: nit. Trigger: a login started with
--idends denied, expired or cancelled by logout, and the user or agent runs the returned next command. Nothing breaks on the base branch, but the new flag's choice is dropped on retry.parsePendingLoginat src/commands/login.ts:171-181 saves no flag, and line 196 buildscontinuationCommand(["wallet", "login"], network)without--id.
| const cached = await cachedLoginOutput(state, network); | ||
| if (cached) return cached; |
There was a problem hiding this comment.
Informational: login --id is silently ignored when a usable login is cached
loginHandler returns the cached whoami output before it reads options.id. With an unexpired access key on the target network, tempo wallet login --id exits 0 without registering a device code, printing a Tempo ID link, or telling the user the flag was dropped. First reported at 57c0ead; recheck at this head: still present.
Valid? React with 👍 / 👎.
| if (options.id) | ||
| for (const key of ["verification_uri", "verification_uri_complete"]) | ||
| if (key in registration) registration[key] = withTempoId(registration[key]); |
There was a problem hiding this comment.
Medium (exposure and trigger unconfirmed): refresh cannot request Tempo ID sign-in after login --id
tempo wallet login --id adds tempo_id=true only by rewriting the saved registration inside startLogin. tempo wallet refresh sends the same wallet_connect request but prints and opens the wallet's link unmodified and rejects --id, so a wallet logged in with Tempo ID gets a passkey approval link when it renews its access key.
Full finding
tempo wallet login --id adds tempo_id=true only by rewriting the saved registration inside startLogin. tempo wallet refresh sends the same wallet_connect request but prints and opens the wallet's link unmodified and rejects --id, so a wallet logged in with Tempo ID gets a passkey approval link when it renews its access key.
First reported at 57c0ead; recheck at this head: still present.
Valid? React with 👍 / 👎.
Problem
tempo wallet loginalways approves with a passkey in the wallet app. There is no way to sign in with Tempo ID from the CLI on mainnet or testnet.Change
tempo wallet login --idaddstempo_id=trueto the approval link the CLI prints and opens. The wallet app's approval page then offers Tempo ID's sign-in instead of a passkey. Thewallet_connectrequest itself is unchanged, on whichever network--networkselects.--idis a newloginoption (src/schemas/login.ts), passed tostartLogin, which rewritesverification_uriandverification_uri_completein the registration before it is saved. So--no-wait,login statusand the printedContinue at:line all show the same link.docs/commands.mdregenerated; changelog entry in.changelog/login-tempo-id.md.The name is
tempo_id, notid, because on the wallet's/remoteapproval pagesidis the JSON-RPC request id and the page redirects it away.Needs the wallet app to accept Tempo ID approvals of CLI logins on mainnet and testnet: https://github.com/tempoxyz/wallet/pull/2718. Until that ships, the page ignores
tempo_idand approves with a passkey as before.Validation
test/login.test.tscases on mainnet and testnet: the pending login'sverification_url, the printed line and the saved registration carrytempo_id=true, the registered request names the right chain, andlogin statusfinishes the login.pnpm typecheck,pnpm test:typesandvp checkpass.vitest run: 343 failures here, the same count as on unchangedmain(sessions and sqlite tests, nosqlite3on this machine). One of them islogin status > returns an approved login when its pending login cannot be cleared, which also fails onmainhere.tempo wallet login --id --no-wait --no-browser --network testnetprintedhttps://wallet.tempo.xyz/remote/rpc/wallet_connect?user_code=…&tempo_id=true, and that page renders (200) without dropping the parameter.