fix: security & correctness hardening across the auth flows - #434
fix: security & correctness hardening across the auth flows#434LautaroPetaccio wants to merge 2 commits into
Conversation
Fixes surfaced by a full review of the auth flows, spanning the signing surface, login/callback, connection state, and onboarding. Security - signMethodGuard: hex-decode personal_sign params before the sign-in impersonation check so a hex-encoded Decentraland login payload can no longer bypass the guard (added regression tests). Signing / RequestPage - Never report a broadcast transaction as failed: once the wallet returns a hash/signature, a later outcome-delivery or tip-notification failure shows completion instead of sending a failed outcome (prevents the Explorer prompting a double-submit). - Bound the outcome-delivery POSTs with a timeout. - decodeNftTransferData: only decode single-token transfers; batch variants (uint256[]) no longer produce a bogus tokenId. - Remove unreachable branch in onContinueInApp and the unused walletInfo.chainId. Login / callback - AutoLoginRedirect resolves Magic test-mode from the feature flag (and waits for flags to initialize) so both halves of the social OAuth handshake use the same Magic key. - Cancelling a login (auto-login cancel / connection-modal close) now aborts the in-flight flow so a late-approved wallet prompt can't redirect or push the user into setup. - Clock-sync "Continue" reuses the existing connection instead of reconnecting (no longer tears down the WalletConnect session). - Email-login retry re-runs the post-verification steps instead of re-asking for an already-consumed OTP code (desktop + mobile). - CallbackPage threads the freshly generated identity through instead of re-reading localStorage; MobileCallbackPage preserves the mobile session ids (u/s) on retry. - Explorer-redirect detection matches on the URL pathname, not a substring. Connection state - ConnectionProvider: a stale identity signature can no longer clobber a newer account (guarded by an account-change version + last-event account, so a same-account re-emit still publishes). - getCachedIdentity swallows the malformed-address throw. Onboarding / profile - Newsletter subscription now respects the marketing consent checkbox for inherited emails. - Avatar deploy failure hides the preview overlay so the error/retry is visible. - Timeouts on newsletter, referral, default-profile and fallback-redeploy fetches so a hung server can't strand the deploying spinner. - Validate the fetched default profile before use. - isProfileComplete requires a non-empty name. - Catalyst rotation retries 429/408; markReturningUser lowercases the wallet identifier; guest link uses set (not append) and flushes its analytics event before navigating.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
decentraland-bot
left a comment
There was a problem hiding this comment.
Thanks for the broad hardening pass. I found two correctness issues that should be addressed before merging.
Findings
-
P1 — A canceled login attempt can resume after a later attempt resets the shared cancellation flag (
src/components/Pages/LoginPage/LoginPage.tsx:240-280).handleOnCloseConnectionModal()setsconnectCancelledRef.current = true, but every newhandleOnConnect()sets the same ref back tofalse. If the user closes the modal while wallet/signature flow A is still pending, then starts flow B before A resolves, A's continuation will seefalseat the post-awaitcheckpoints and can still track success / redirect / run setup with the stale approval. Please use a per-attempt token/version orAbortControllercaptured locally by each invocation, and only continue when the captured attempt is still current. -
P1 — Successful off-chain signatures can be lost while the UI reports completion (
src/components/Pages/RequestPage/RequestPage.tsx:1095-1110). The newif (result)catch path is correct for broadcast transactions, where the tx hash is recoverable on-chain, but it also treatspersonal_sign/ typed-data signatures as complete whensendSuccessfulOutcome()times out or fails. For signatures, the outcome POST is the delivery channel; if it fails, the requester never receives the signature and the user cannot retry. Please distinguish transaction broadcasts from off-chain signatures: only show completion on post-delivery failures for broadcast txs, and keep/signature flows in a recoverable retry/error state that can resend the capturedresult.
Other checks
- PR title and branch follow the expected semantic
fix/...convention. - No public API route/schema changes found; this is client-side behavior over the existing outcome API.
- Security pass: no secrets, injection, or auth bypass issues found in the changed code beyond the signature-delivery integrity issue above.
- CI: required checks are passing; e2e checks were still pending when I reviewed.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
…n cancellation Two P1 correctness issues from the PR review: - RequestPage: the post-execution success path treated off-chain signatures the same as broadcast transactions. For a broadcast tx the hash is recoverable on-chain, but for a signature the outcome POST is the only delivery channel, so a delivery failure silently lost the signature while showing completion. Now the successful outcome is delivered with retries (resending the captured result), and on ultimate failure only broadcast txs report completion; signatures surface a (non-completing) error and never send a failed outcome, since the signature itself succeeded. Added regression tests for both paths. - LoginPage: cancellation used a shared boolean that the next connect attempt reset to false, so a still-pending earlier attempt could resume and redirect after the user closed the modal and started another. Replaced it with a per-attempt token captured by each invocation; closing the modal or starting a new attempt invalidates the older one.
What
Fixes surfaced by a full review of the auth flows, spanning the signing surface, login/callback, connection state, and onboarding. No behavioral changes beyond the fixes below.
Highlights
Security
signMethodGuard: the sign-in impersonation guard only inspected plaintext, butpersonal_signparams arrive hex-encoded. A hex-encoded Decentraland login payload could pass the guard and then be signed as plaintext by the wallet, yielding an impersonating auth chain. The guard now hex-decodes params before the structural check (the siblingextractSignaturePayloadalready decoded the same param). Added regression tests.Signing / RequestPage
decodeNftTransferDataonly decodes single-token transfers; batch variants (uint256[]) no longer yield a bogustokenId.onContinueInAppand the unusedwalletInfo.chainId.Login / callback
AutoLoginRedirectresolves Magic test-mode from the feature flag (and waits for flags to initialize), so both halves of the social OAuth handshake use the same Magic key.CallbackPagethreads the freshly generated identity through instead of re-reading localStorage;MobileCallbackPagepreserves the mobile session ids (u/s) on retry.Connection state
ConnectionProvider: a stale identity signature can no longer clobber a newer account (guarded by an account-change version + last-event account, so a same-account re-emit still publishes).getCachedIdentityswallows the malformed-address throw.Onboarding / profile
isProfileCompleterequires a non-empty name; catalyst rotation retries 429/408;markReturningUserlowercases the wallet identifier; the guest link usesset(notappend) and flushes its analytics event before navigating.Testing
tsc --noEmitclean,eslintclean.postIdentityevent gating, the outcome-POST timeouts, the newsletter opt-in, andmarkReturningUser.userEvent-heavy suites are timing-sensitive and can hit the 15s Jest timeout under heavy machine load; they pass reliably in isolation.🤖 Generated with Claude Code