Skip to content

fix(security): migrate MCP pairing token from storage.local to storage.session #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hoainho merged 1 commit into from
Jun 7, 2026
Merged

fix(security): migrate MCP pairing token from storage.local to storage.session #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 56 additions & 18 deletions src/__tests__/MCPPairingPanel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,54 +1,92 @@
import { describe, it, expect } from "vitest";
import { beforeEach, describe, expect, it, vi } from "vitest";
import { validatePairingParams } from "../panel/components/MCPPairingPanel";

const VALID_TOKEN = "a".repeat(64);
const VALID_HASH = `#token=${VALID_TOKEN}&port=65000`;

const mockSessionStorage: Record<string, unknown> = {};
const mockLocalStorage: Record<string, unknown> = {};

vi.stubGlobal("chrome", {
storage: {
session: {
set: vi.fn(async (data: Record<string, unknown>) => {
Object.assign(mockSessionStorage, data);
}),
get: vi.fn(async (keys: string[]) => {
return Object.fromEntries(keys.map((k) => [k, mockSessionStorage[k]]));
}),
},
local: {
set: vi.fn(async (data: Record<string, unknown>) => {
Object.assign(mockLocalStorage, data);
}),
get: vi.fn(async (keys: string[]) => {
return Object.fromEntries(keys.map((k) => [k, mockLocalStorage[k]]));
}),
remove: vi.fn(async (keys: string[]) => {
keys.forEach((k) => delete mockLocalStorage[k]);
}),
},
},
});

beforeEach(() => {
Object.keys(mockSessionStorage).forEach((k) => delete mockSessionStorage[k]);
Object.keys(mockLocalStorage).forEach((k) => delete mockLocalStorage[k]);
});

describe("validatePairingParams", () => {
it("returns no errors for a valid deep-link", () => {
const errors = validatePairingParams(VALID_HASH);
expect(errors).toHaveLength(0);
expect(validatePairingParams(VALID_HASH)).toHaveLength(0);
});

it("returns token-malformed when token is too short", () => {
const hash = `#token=short&port=65000`;
const codes = validatePairingParams(hash).map((e) => e.code);
const codes = validatePairingParams(`#token=short&port=65000`).map((e) => e.code);
expect(codes).toContain("token-malformed");
});

it("returns port-out-of-range when port is 99999", () => {
const hash = `#token=${VALID_TOKEN}&port=99999`;
const codes = validatePairingParams(hash).map((e) => e.code);
const codes = validatePairingParams(`#token=${VALID_TOKEN}&port=99999`).map((e) => e.code);
expect(codes).toContain("port-out-of-range");
});

it("returns both token-malformed AND port-out-of-range", () => {
const hash = `#token=short&port=99999`;
const codes = validatePairingParams(hash).map((e) => e.code);
const codes = validatePairingParams(`#token=short&port=99999`).map((e) => e.code);
expect(codes).toContain("token-malformed");
expect(codes).toContain("port-out-of-range");
});

it("returns missing-params when both are absent", () => {
const errors = validatePairingParams("#");
expect(errors[0].code).toBe("missing-params");
expect(validatePairingParams("#")[0].code).toBe("missing-params");
});

it("rejects decimal ports like 1024.5", () => {
const hash = `#token=${VALID_TOKEN}&port=1024.5`;
const codes = validatePairingParams(hash).map((e) => e.code);
const codes = validatePairingParams(`#token=${VALID_TOKEN}&port=1024.5`).map((e) => e.code);
expect(codes).toContain("port-out-of-range");
});

it("rejects hex ports like 0x2000", () => {
const hash = `#token=${VALID_TOKEN}&port=0x2000`;
const codes = validatePairingParams(hash).map((e) => e.code);
const codes = validatePairingParams(`#token=${VALID_TOKEN}&port=0x2000`).map((e) => e.code);
expect(codes).toContain("port-out-of-range");
});

it("rejects ports with leading spaces", () => {
const hash = `#token=${VALID_TOKEN}&port= 8080`;
const codes = validatePairingParams(hash).map((e) => e.code);
const codes = validatePairingParams(`#token=${VALID_TOKEN}&port= 8080`).map((e) => e.code);
expect(codes).toContain("port-out-of-range");
});
});
});

describe("storage migration", () => {
it("token does not appear in chrome.storage.local after migration", async () => {
mockLocalStorage["mcp_pairing_token_v1"] = VALID_TOKEN;
mockLocalStorage["mcp_pairing_port_v1"] = "65000";

const { migrateLegacyStorage } = await import("../panel/components/MCPPairingPanel");
await migrateLegacyStorage();

expect(mockLocalStorage["mcp_pairing_token_v1"]).toBeUndefined();
expect(mockLocalStorage["mcp_pairing_port_v1"]).toBeUndefined();
expect(mockSessionStorage["mcp_pairing_token_v1"]).toBe(VALID_TOKEN);
});
Comment on lines +81 to +91

@gemini-code-assist gemini-code-assist Bot Jun 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test sets up mockLocalStorage using the new keys ("mcp_pairing_token_v1" and "mcp_pairing_port_v1") instead of the actual legacy keys ("mcpToken" and "mcpPort"). This masked the bug in the migration function.

We should update the test to use the actual legacy keys to ensure the migration is tested correctly.

Suggested change
it("token does not appear in chrome.storage.local after migration", async () => {
mockLocalStorage["mcp_pairing_token_v1"] = VALID_TOKEN;
mockLocalStorage["mcp_pairing_port_v1"] = "65000";
const { migrateLegacyStorage } = await import("../panel/components/MCPPairingPanel");
await migrateLegacyStorage();
expect(mockLocalStorage["mcp_pairing_token_v1"]).toBeUndefined();
expect(mockLocalStorage["mcp_pairing_port_v1"]).toBeUndefined();
expect(mockSessionStorage["mcp_pairing_token_v1"]).toBe(VALID_TOKEN);
});
it("token does not appear in chrome.storage.local after migration", async () => {
mockLocalStorage["mcpToken"] = VALID_TOKEN;
mockLocalStorage["mcpPort"] = "65000";
const { migrateLegacyStorage } = await import("../panel/components/MCPPairingPanel");
await migrateLegacyStorage();
expect(mockLocalStorage["mcpToken"]).toBeUndefined();
expect(mockLocalStorage["mcpPort"]).toBeUndefined();
expect(mockSessionStorage["mcp_pairing_token_v1"]).toBe(VALID_TOKEN);
});

});
51 changes: 43 additions & 8 deletions src/panel/components/MCPPairingPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useEffect, useState } from "react";

type ErrorCode = "token-malformed" | "port-out-of-range" | "missing-params";
type ErrorCode = "token-malformed" | "port-out-of-range" | "missing-params" | "storage-failed";

interface PairingError {
code: ErrorCode;
Expand Down Expand Up @@ -57,29 +57,64 @@ export function validatePairingParams(hash: string): PairingError[] {
return errors;
}

const TOKEN_KEY = "mcp_pairing_token_v1";
const PORT_KEY = "mcp_pairing_port_v1";

export async function migrateLegacyStorage() {
if (typeof chrome === "undefined" || !chrome.storage) return;
const local = await chrome.storage.local.get([TOKEN_KEY, PORT_KEY]);
if (local[TOKEN_KEY] || local[PORT_KEY]) {
await chrome.storage.session.set({
[TOKEN_KEY]: local[TOKEN_KEY],
[PORT_KEY]: local[PORT_KEY],
});
await chrome.storage.local.remove([TOKEN_KEY, PORT_KEY]);
}
}
Comment on lines +63 to +73

@gemini-code-assist gemini-code-assist Bot Jun 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The migration function migrateLegacyStorage is attempting to read TOKEN_KEY ("mcp_pairing_token_v1") and PORT_KEY ("mcp_pairing_port_v1") from chrome.storage.local. However, in the legacy implementation, these values were stored under the keys "mcpToken" and "mcpPort".

As a result, the migration will never find the legacy pairing token and port, and existing paired users will lose their pairing state.

Additionally, if chrome.storage.session is undefined (e.g., in environments where it is not supported or not enabled), calling chrome.storage.session.set will throw a TypeError, causing the promise to reject and preventing performPairing() from running.

We should:

  1. Read the correct legacy keys ("mcpToken" and "mcpPort") from chrome.storage.local.
  2. Add a check for chrome.storage.session to avoid runtime errors.
  3. Wrap the migration in a try/catch block to ensure that any storage errors do not block the pairing process.
export async function migrateLegacyStorage() {
  if (typeof chrome === "undefined" || !chrome.storage || !chrome.storage.local || !chrome.storage.session) return;
  try {
    const LEGACY_TOKEN_KEY = "mcpToken";
    const LEGACY_PORT_KEY = "mcpPort";
    const local = await chrome.storage.local.get([LEGACY_TOKEN_KEY, LEGACY_PORT_KEY]);
    if (local[LEGACY_TOKEN_KEY] || local[LEGACY_PORT_KEY]) {
      await chrome.storage.session.set({
        [TOKEN_KEY]: local[LEGACY_TOKEN_KEY],
        [PORT_KEY]: local[LEGACY_PORT_KEY],
      });
      await chrome.storage.local.remove([LEGACY_TOKEN_KEY, LEGACY_PORT_KEY]);
    }
  } catch (err) {
    console.error("Failed to migrate legacy MCP pairing storage:", err);
  }
}


async function savePairing(token: string, port: string) {
await chrome.storage.session.set({
[TOKEN_KEY]: token,
[PORT_KEY]: port,
});
}

export function MCPPairingPanel() {
const [errors, setErrors] = useState<PairingError[]>([]);
const [paired, setPaired] = useState(false);

const performPairing = () => {
const performPairing = async () => {
const hash = window.location.hash;
const found = validatePairingParams(hash);

if (found.length === 0) {
const { token, port } = parseHash(hash);
if (typeof chrome !== "undefined" && chrome.storage?.local) {
chrome.storage.local.set({ mcpToken: token, mcpPort: port });
try {
if (typeof chrome !== "undefined" && chrome.storage?.session) {
await savePairing(token!, port!);
}
window.history.replaceState(null, "", window.location.pathname);
setPaired(true);
setErrors([]);
} catch (err) {
setErrors([{
code: "storage-failed",
message: "Could not save pairing.",
fix: (err as Error).message,
}]);
}
Comment on lines +92 to 105

@gemini-code-assist gemini-code-assist Bot Jun 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If chrome is defined (meaning we are in an extension context) but chrome.storage.session is undefined (e.g., in older browser versions or unsupported environments), the code will silently skip saving the pairing token but still set paired to true and clear the hash. This leads to a silent failure where the user thinks pairing succeeded but the token was never saved.

We should throw an error if chrome.storage.session is missing in an extension context so that the failure is captured and displayed to the user.

Suggested change
try {
if (typeof chrome !== "undefined" && chrome.storage?.session) {
await savePairing(token!, port!);
}
window.history.replaceState(null, "", window.location.pathname);
setPaired(true);
setErrors([]);
} catch (err) {
setErrors([{
code: "storage-failed",
message: "Could not save pairing.",
fix: (err as Error).message,
}]);
}
try {
if (typeof chrome !== "undefined") {
if (!chrome.storage?.session) {
throw new Error("chrome.storage.session is not supported in this browser version.");
}
await savePairing(token!, port!);
}
window.history.replaceState(null, "", window.location.pathname);
setPaired(true);
setErrors([]);
} catch (err) {
setErrors([{
code: "storage-failed",
message: "Could not save pairing.",
fix: (err as Error).message,
}]);
}

setPaired(true);
setErrors([]);
} else {
setErrors(found);
setPaired(false);
}
};

useEffect(() => {
performPairing();
if (typeof chrome !== "undefined" && chrome.storage) {
migrateLegacyStorage().then(() => performPairing());
} else {
performPairing();
}
}, []);

const handleRetry = () => {
Expand Down Expand Up @@ -110,4 +145,4 @@ export function MCPPairingPanel() {
<button onClick={handleRetry}>Retry</button>
</div>
);
}
}
Loading