fix(mcp): bind /connect/mcp consent to the local bridge (#368)#370
Open
ducnmm wants to merge 1 commit into
Open
fix(mcp): bind /connect/mcp consent to the local bridge (#368)#370ducnmm wants to merge 1 commit into
ducnmm wants to merge 1 commit into
Conversation
The /connect/mcp consent page signed add_delegate_key purely from
query-string params, with no proof the request came from the local MCP
bridge it claims to serve. An attacker could send a victim a link on the
trusted memory.walrus.xyz domain and, on approval, register the attacker's
own delegate key with read/decrypt/write access to all the victim's
memories until manually revoked.
Bind the on-chain write to the local bridge, reusing the existing
single-use state token:
- bridge: add `GET /handshake?state=` that returns { ok: true } only when
the state matches the token it minted (constant-time); allow GET in CORS.
- app: verify the bridge on load and again right before signing; refuse to
sign unless it confirms. A phishing link reaches no local bridge (or one
whose token differs) and fails the handshake.
Make the consent screen honest (report recommendations 2-4):
- show the full delegate address and public key (were truncated / hidden);
- render the client label as untrusted free text, not a verified identity;
- state plainly that approving grants persistent read + decrypt + write to
all memories across all namespaces until revoked; warn on a non-default
relayer;
- don't auto-skip the consent screen when a wallet is already connected.
Fail-closed: an older bridge without /handshake is rejected, matching the
existing "force a bridge upgrade" behaviour of the connectState param, so
publish the updated bridge before deploying the app. The Move contract is
unchanged — the owner check already passes for a victim-signed tx, and the
public_key/sui_address desync is not what enables this attack.
Adds packages/mcp/test/login-handshake.test.mjs.
harrymove-ctrl
approved these changes
Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the consent-phishing surface reported in #368. The
/connect/mcppage registered a delegate key on the connected wallet's Walrus Memory account based entirely on query-string parameters, with no proof the request came from the local MCP bridge it claims to serve. An attacker could send a victim a link on the trustedmemory.walrus.xyzdomain — "Cursor wants access to your Walrus Memory" — and, on approval, permanently grant the attacker's own delegate key read / decrypt / write access to all the victim's memories until manually revoked.This is an app-level consent-phishing issue, not a wallet- or contract-level one: the branded first-party consent screen lent credibility to a fully attacker-defined request. The Move contract is unchanged — the owner check already passes for a victim-signed tx, and (per the report) the
public_key/sui_addressdesync is not what enables the attack.What changed
1. Bind the on-chain write to the local bridge (core fix). Reuses the existing single-use
connectStatetoken, lifting it from guarding only the callback to also gating the signature:packages/mcp/src/login.ts: addGET /handshake?state=that returns{ ok: true }only when the state matches the token the bridge minted (constant-time compare, behind the same Host / Origin / PNA guards as the callback). AllowGETin CORS.apps/app/src/pages/ConnectMcp.tsx: probe the bridge on load and re-verify immediately before signing; refuse to signadd_delegate_keyunless it confirms. A phishing link reaches no local bridge — or one whose token differs — and fails the handshake, so the page never signs.2. Make the consent screen honest (report recommendations 2–4).
labelas untrusted free text, not a verified "<label> wants access" identity.Testing
packages/mcp— newtest/login-handshake.test.mjsdrives the real localhost listener: correct token →200 {ok:true}, wrong / missing token →403 {ok:false}, and a valid callback still resolves credentials. Full suite: 9/9 pass.apps/app—tsc -bclean,eslintclean.Deploy ordering (fail-closed — please read)
The app now rejects an older bridge that lacks
/handshake. This is intentional and matches the existing "force a bridge upgrade" behaviour of theconnectStateparam. Publish the updated@mysten-incubation/memwal-mcpbridge before deploying the app so legitimate users aren't blocked. The error screen guides users to update the client (npx @mysten-incubation/memwal-mcp@latest login).Fixes #368.