From d756b41881321b1208f63335e3546393a8099bd5 Mon Sep 17 00:00:00 2001 From: ducnmm <165614309+ducnmm@users.noreply.github.com> Date: Wed, 8 Jul 2026 09:57:17 +0700 Subject: [PATCH] fix(mcp): bind /connect/mcp consent to the local bridge (#368) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/app/src/pages/ConnectMcp.tsx | 258 ++++++++++++++++++++- packages/mcp/src/login.ts | 36 ++- packages/mcp/test/login-handshake.test.mjs | 138 +++++++++++ 3 files changed, 413 insertions(+), 19 deletions(-) create mode 100644 packages/mcp/test/login-handshake.test.mjs diff --git a/apps/app/src/pages/ConnectMcp.tsx b/apps/app/src/pages/ConnectMcp.tsx index 9632d187..5309af41 100644 --- a/apps/app/src/pages/ConnectMcp.tsx +++ b/apps/app/src/pages/ConnectMcp.tsx @@ -60,6 +60,41 @@ function hexToBytes(hex: string): number[] { return out } +/** + * Prove the request came from a Walrus Memory MCP bridge running on THIS + * machine before we register a delegate key on-chain (issue #368). + * + * The bridge mints a single-use `connectState` token, keeps it in memory, and + * only puts it in the `/connect/mcp` URL it opens itself. We hand that token + * back to the bridge's localhost `/handshake`; it answers `{ ok: true }` only + * when the token matches the one it minted. A phishing link opened from an + * email carries an attacker-chosen token and reaches no local bridge (or a + * bridge whose token differs), so this returns false and we refuse to sign. + * + * Any failure — no listener, wrong token, CORS/PNA block, non-bridge process + * on the port — is treated as "not verified". False negatives are safe: they + * only ask a legitimate user to restart the MCP login command. + */ +/** Compare two relayer URLs ignoring trailing slashes and case. */ +function sameRelayer(a: string, b: string): boolean { + const norm = (s: string) => s.replace(/\/+$/, '').toLowerCase() + return norm(a) === norm(b) +} + +async function verifyLocalBridge(port: string, state: string): Promise { + try { + const res = await fetch( + `http://127.0.0.1:${port}/handshake?state=${encodeURIComponent(state)}`, + { method: 'GET' }, + ) + if (!res.ok) return false + const data = (await res.json().catch(() => null)) as { ok?: unknown } | null + return data?.ok === true + } catch { + return false + } +} + async function resolveAccountId( suiClient: ReturnType, ownerAddress: string, @@ -129,7 +164,17 @@ export default function ConnectMcp() { const [walletPickerOpen, setWalletPickerOpen] = useState(false) const [callbackPayload, setCallbackPayload] = useState(null) const [callbackDelivered, setCallbackDelivered] = useState(null) + // Bridge binding (issue #368): null = still checking, true = a local bridge + // confirmed it issued this request, false = could not confirm (a phishing + // link opened from elsewhere, or the local bridge has exited). We refuse to + // sign `add_delegate_key` unless this is true. + const [bridgeVerified, setBridgeVerified] = useState(null) const invalidRequestTrackedRef = useRef(false) + // True once the user actively started the flow (clicked the consent button). + // Gates the wallet-picker auto-proceed effect so a pre-connected wallet does + // NOT skip straight past the consent screen on mount — the user must read + // and click through the disclosure first (issue #368). + const initiatedRef = useRef(false) // Validate query string up-front. const paramsValid = useMemo(() => { @@ -166,6 +211,9 @@ export default function ConnectMcp() { ) const handleConnect = useCallback(async () => { + // Mark the flow as user-initiated so the auto-proceed effect below is + // allowed to run after the wallet picker closes (issue #368). + initiatedRef.current = true if (!paramsValid) { trackEvent('mcp_connect_failed', { error_type: 'invalid_request' }) setErrorMsg('Invalid query parameters from MCP client.') @@ -179,6 +227,30 @@ export default function ConnectMcp() { } trackEvent('mcp_connect_start', { wallet_connected: true }) + + // Bind this on-chain write to the local bridge (issue #368). Never sign + // add_delegate_key for a request we can't prove a bridge on this port + // issued — this is what blocks a consent-phishing link opened from an + // email/chat, where no local bridge (or a bridge with a different token) + // answers the handshake. Re-checked here right before signing even + // though the page already probed on load, in case the bridge exited in + // between. + const bridgeOk = await verifyLocalBridge(port, state) + setBridgeVerified(bridgeOk) + if (!bridgeOk) { + trackEvent('mcp_connect_failed', { error_type: 'bridge_unverified' }) + setErrorMsg( + 'Could not confirm this request came from a Walrus Memory MCP login running on this computer, ' + + 'so nothing was registered on-chain. ' + + 'If you did NOT start this yourself — for example you opened a link from an email or chat message — ' + + 'close this tab: approving would grant someone else ongoing access to all your memories. ' + + 'If you DID start this, make sure you are running the latest MCP client ' + + '(e.g. `npx @mysten-incubation/memwal-mcp@latest login`) and restart the login command, then try again.' + ) + setStep('error') + return + } + setStep('signing') try { // Resolve the user's Walrus Memory account object. @@ -257,6 +329,7 @@ export default function ConnectMcp() { publicKey, delegateAddress, label, + port, state, postCallback, ]) @@ -267,6 +340,22 @@ export default function ConnectMcp() { trackEvent('mcp_connect_failed', { error_type: 'invalid_request' }) }, [paramsValid]) + // Probe the local bridge as soon as the request looks well-formed, so the + // consent screen can warn — and disable Approve — before the user ever + // connects a wallet if this link did not come from a bridge on this machine + // (issue #368). handleConnect re-checks right before signing. + useEffect(() => { + setBridgeVerified(null) + if (!paramsValid) return + let cancelled = false + void verifyLocalBridge(port, state).then((ok) => { + if (!cancelled) setBridgeVerified(ok) + }) + return () => { + cancelled = true + } + }, [paramsValid, port, state]) + // Persist the connect request so it survives the Google OAuth redirect. // Enoki's redirect_uri is pinned to the app root (App.tsx), so signing in // with Google leaves this page and returns to `/` — losing the query @@ -282,8 +371,10 @@ export default function ConnectMcp() { }, [paramsValid, port, publicKey, delegateAddress, label, relayer, state]) // If the wallet popup completes after we asked it to open, auto-proceed. + // Gated on initiatedRef so a wallet that was ALREADY connected on mount does + // not skip the consent screen — the user must click through it first (#368). useEffect(() => { - if (!walletPickerOpen && currentAccount && step === 'consent') { + if (initiatedRef.current && !walletPickerOpen && currentAccount && step === 'consent') { // user picked a wallet — kick off the connect flow. void handleConnect() } @@ -325,8 +416,11 @@ export default function ConnectMcp() { )} @@ -406,33 +500,83 @@ export default function ConnectMcp() { function ConsentCard({ label, delegateAddress, + publicKey, relayer, + relayerIsDefault, wallet, + bridgeVerified, onConnect, }: { label: string delegateAddress: string + publicKey: string relayer: string + relayerIsDefault: boolean wallet: string | null + bridgeVerified: boolean | null onConnect: () => void }) { + // We only enable the on-chain action once a local bridge has confirmed it + // issued this request (issue #368). `null` = still probing. + const blocked = bridgeVerified !== true + + let buttonText: string + if (bridgeVerified === null) buttonText = 'Checking this request…' + else if (bridgeVerified === false) buttonText = 'Request could not be verified' + else buttonText = wallet ? 'Approve in wallet' : 'Connect Sui wallet' + return (

- {label} wants access to your Walrus Memory + Authorize an MCP client to access your Walrus Memory

- Review the permissions below, then connect your Sui wallet to register this delegate key on-chain. + Something is asking to register a delegate key on your Walrus Memory + account. Read what this grants before you approve — the request below is not verified by + Walrus Memory, only by you.

+ {/* Bridge-binding status. This is the anti-phishing signal: a request + that did not originate from a bridge on this machine cannot pass. */} + {bridgeVerified === false && ( +
+ ⚠ This request did not come from an MCP login on this computer. +

+ Walrus Memory could not reach a local Walrus Memory MCP login for this request, so + approving is disabled. If a link was sent to you by someone else,{' '} + close this tab — it may be trying to trick you into granting them + access. If you started this yourself, your MCP client may be out of date — update to + the latest and restart the login command. +

+
+ )} + {bridgeVerified === true && ( +
+ ✓ Verified this request came from a Walrus Memory MCP login running on this computer. +
+ )} +
-

Permissions requested

+

What approving grants

    -
  • ✓ Read your memories (memwal_recall)
    • -
  • ✓ Save new memories (memwal_remember)
    • -
  • ✓ Extract facts from text (memwal_analyze)
    • -
  • ✓ Re-index from Walrus (memwal_restore)
    • +
  • ✓ Read and decrypt every memory in this account — all namespaces
    • +
  • ✓ Create, update, and overwrite memories
    • +
  • ✓ Extract facts from text and re-index from Walrus storage
+

+ This gives a third party persistent read, decrypt, and write access to + everything in this account — through the relayer below — and it lasts until you + revoke the key from your dashboard. It is not limited to one session. +

+ +
+ +

The client identifies itself as

+
{label}
+

+ This name is supplied by the requester and is not verified. Anyone can + claim any name. +

@@ -440,10 +584,20 @@ function ConsentCard({
Relayer {relayer} + {!relayerIsDefault && ( + + ⚠ Non-default relayer — your memories would be served through this host. Only + proceed if you recognize it. + + )} +
+
+ Delegate address (full) + {delegateAddress}
- Delegate address - {delegateAddress.slice(0, 16)}…{delegateAddress.slice(-6)} + Delegate public key (full) + {publicKey}
Connected wallet @@ -453,9 +607,20 @@ function ConsentCard({
+
+ Only continue if you just started this from your own machine. Approving hands + the key above ongoing access to all your memories. +
+
-
@@ -568,3 +733,72 @@ const detailValueStyle: React.CSSProperties = { const errorTextStyle: React.CSSProperties = { color: '#ff6b6b', } + +// Red alert box — shown when the request can't be tied to a local bridge. +const dangerBoxStyle: React.CSSProperties = { + background: 'rgba(239, 68, 68, 0.12)', + border: '1px solid #ef4444', + borderRadius: 8, + padding: '12px 14px', + margin: '0 0 18px', + fontSize: '0.88rem', + color: '#ffb4b4', +} + +// Green confirmation box — shown when the bridge handshake succeeded. +const okBoxStyle: React.CSSProperties = { + background: 'rgba(34, 197, 94, 0.12)', + border: '1px solid #22c55e', + borderRadius: 8, + padding: '10px 14px', + margin: '0 0 18px', + fontSize: '0.85rem', + color: '#86efac', +} + +// The blunt "what you're really granting" paragraph inside the card. +const scaryNoteStyle: React.CSSProperties = { + margin: '14px 0 0', + fontSize: '0.84rem', + lineHeight: 1.5, + color: '#f0a3a3', +} + +// Attacker-controlled label — rendered as untrusted free text, never as a +// verified identity in the heading. +const untrustedLabelStyle: React.CSSProperties = { + fontFamily: 'var(--font-mono)', + fontSize: '0.9rem', + color: '#faf8f5', + background: '#1c1e20', + border: '1px solid #2a2c2e', + borderRadius: 6, + padding: '8px 10px', + wordBreak: 'break-all', +} + +const untrustedHintStyle: React.CSSProperties = { + margin: '6px 0 0', + fontSize: '0.78rem', + color: '#8f9294', +} + +const relayerWarnStyle: React.CSSProperties = { + marginTop: 4, + fontSize: '0.78rem', + color: '#f0a3a3', + lineHeight: 1.45, +} + +const safetyNoteStyle: React.CSSProperties = { + margin: '18px 0 0', + fontSize: '0.82rem', + lineHeight: 1.5, + color: '#c7b48a', + textAlign: 'center', +} + +const disabledBtnStyle: React.CSSProperties = { + opacity: 0.45, + cursor: 'not-allowed', +} diff --git a/packages/mcp/src/login.ts b/packages/mcp/src/login.ts index af6991b6..ea7b2583 100644 --- a/packages/mcp/src/login.ts +++ b/packages/mcp/src/login.ts @@ -4,10 +4,12 @@ * 1. Generate Ed25519 keypair locally. * 2. Start an HTTP listener on a random localhost port. * 3. Open the user's browser at the configured web URL with the public - * key + callback port in the query string. - * 4. Web page asks user to connect Sui wallet → signs `add_delegate_key` - * on-chain → POSTs the resulting {accountId, walletAddress, packageId, - * txDigest, ...} to `http://localhost:/callback`. + * key + callback port + single-use state token in the query string. + * 4. Web page fetches `GET /handshake?state=` first to confirm a real local + * bridge issued this request (binds the on-chain write to this process — + * see issue #368), then asks the user to connect their Sui wallet → signs + * `add_delegate_key` on-chain → POSTs the resulting {accountId, + * walletAddress, packageId, txDigest, ...} to `http://localhost:/callback`. * 5. Listener receives the callback, returns 200 + a friendly success * page, then shuts down. We resolve with `MemWalCredentials`. * @@ -227,10 +229,10 @@ export async function loginFlow(opts: LoginOptions = {}): Promise { - // CORS — only the dashboard origin we control may POST. `*` would - // let any web page on the internet talk to this localhost port. + // CORS — only the dashboard origin we control may talk to us. `*` + // would let any web page on the internet reach this localhost port. res.setHeader("access-control-allow-origin", expectedOrigin); - res.setHeader("access-control-allow-methods", "POST, OPTIONS"); + res.setHeader("access-control-allow-methods", "GET, POST, OPTIONS"); res.setHeader("access-control-allow-headers", "content-type"); res.setHeader("vary", "origin"); if (req.method === "OPTIONS") { @@ -263,6 +265,26 @@ export async function loginFlow(opts: LoginOptions = {}): Promise { + const req = http.request( + { + host: "127.0.0.1", + port, + path: "/callback", + method: "POST", + headers: { + "content-type": "application/json", + "content-length": Buffer.byteLength(body), + origin, + }, + }, + (r) => { + let buf = ""; + r.on("data", (c) => (buf += c)); + r.on("end", () => res({ status: r.statusCode, body: buf })); + }, + ); + req.on("error", rej); + req.end(body); + }); +} + +test("GET /handshake gates the flow on the single-use state token", async (t) => { + const home = mkdtempSync(join(tmpdir(), "memwal-handshake-")); + const prevHome = process.env.HOME; + const prevUserProfile = process.env.USERPROFILE; + // saveCreds writes under HOME on a successful callback — keep it off the + // real ~/.memwal. + process.env.HOME = home; + process.env.USERPROFILE = home; + t.after(() => { + process.env.HOME = prevHome; + process.env.USERPROFILE = prevUserProfile; + rmSync(home, { recursive: true, force: true }); + }); + + let connectUrl = ""; + const flow = loginFlow({ + openBrowser: false, + webUrl: WEB_URL, + relayerUrl: "http://127.0.0.1:1", + label: "Handshake Test", + timeoutMs: 20_000, + onUrl: (u) => { + connectUrl = u; + }, + }); + + // Wait for the listener to be ready and surface its URL. + await new Promise((r) => setTimeout(r, 100)); + assert.ok(connectUrl, "loginFlow should surface the connect URL via onUrl"); + + const parsed = new URL(connectUrl); + const port = parsed.searchParams.get("port"); + const token = parsed.searchParams.get("connectState"); + assert.match(port ?? "", /^\d+$/, "URL should carry a numeric port"); + assert.match(token ?? "", /^[0-9a-f]{64}$/, "URL should carry a 64-hex state token"); + + const handshake = (state) => + fetch(`http://127.0.0.1:${port}/handshake?state=${state}`).then(async (r) => ({ + status: r.status, + body: await r.json().catch(() => null), + })); + + // Probe before asserting so the server is always torn down via the callback + // below even if an expectation fails. + const good = await handshake(token); + const wrong = await handshake("f".repeat(64)); + const missing = await fetch(`http://127.0.0.1:${port}/handshake`).then(async (r) => ({ + status: r.status, + body: await r.json().catch(() => null), + })); + + // Complete the flow with a legitimate callback so loginFlow resolves and + // closes its listener. + const cb = await postCallback(port, { + accountId: ACCOUNT_ID, + walletAddress: WALLET, + packageId: PACKAGE_ID, + state: token, + txDigest: "0x" + "d".repeat(64), + label: "Handshake Test", + }); + const creds = await flow; + + // Correct token → authorized. + assert.equal(good.status, 200, "correct state should return 200"); + assert.equal(good.body?.ok, true, "correct state should return { ok: true }"); + + // Attacker-chosen token → refused (this is what defeats the phishing link). + assert.equal(wrong.status, 403, "wrong state should return 403"); + assert.equal(wrong.body?.ok, false, "wrong state should return { ok: false }"); + + // Missing token → refused. + assert.equal(missing.status, 403, "missing state should return 403"); + assert.equal(missing.body?.ok, false, "missing state should return { ok: false }"); + + // Sanity: the legitimate callback still works end-to-end. + assert.equal(cb.status, 200, "valid callback should return 200"); + assert.equal(creds.accountId, ACCOUNT_ID, "credentials should carry the account id"); +});