feat: improve MCPPairingPanel error messages for invalid token/port (#7)

[iMindCap]

Closes #7

Changes

• Added MCPPairingPanel.tsx with three distinct error messages:
  • token-malformed: when token is not 64 hex characters
  • port-out-of-range: when port is outside 1024–65535
  • missing-params: when both params are absent
• Each error includes a "How to fix" hint
• Errors are dismissable via a Retry button (no auto-redirect)
• Added Vitest tests covering all acceptance criteria

Tests

All Vitest tests passing ✅

[gemini-code-assist]

Code Review

This pull request introduces the MCPPairingPanel component and its corresponding unit tests to handle MCP pairing via URL hash parameters. It validates the token and port, saves successful pairings to Chrome local storage, and displays error messages with retry capabilities. The review feedback recommends refactoring the pairing logic into a shared helper function to eliminate code duplication between the initial load and retry handler, while also safely checking for the global chrome object to avoid runtime errors in non-extension environments.

[gemini-code-assist]

There is code duplication between the useEffect hook and the handleRetry handler. Additionally, directly accessing the global chrome object without checking if it is defined (e.g., typeof chrome !== 'undefined') can lead to a ReferenceError in non-extension environments (such as unit tests or standard browser tabs). Extracting this logic into a single helper function resolves both issues.

  const performPairing = () => {
    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 });
      }
      setPaired(true);
      setErrors([]);
    } else {
      setErrors(found);
      setPaired(false);
    }
  };

  useEffect(() => {
    performPairing();
  }, []);

  const handleRetry = () => {
    performPairing();
  };

[hoainho]

Thanks @iMindCap — really nice first-PR work. The validator regex is clean (anchored, no ReDoS surface), all 5 acceptance-criteria scenarios from #7 are tested, and the discriminated ErrorCode union is a sharp design choice.

I ran a multi-agent review (security, code quality, best-practices) and found 5 issues that should be fixed before merge. They're all subtle — not careless. Most come from a part of the MCP threat model that issue #7 didn't fully convey; that's on us, not you.

🔴 Critical (block merge)

C-1. Storage schema doesn't match what the rest of MCP v1 expects.
You write chrome.storage.local.set({ mcpToken: token, mcpPort: port }) — flat keys. The OpenSpec at specs/extension-mcp-client/spec.md (search for "Extension SHALL handle MCP pairing deep-link") requires:

await chrome.storage.local.set({
  'mcp.pairing': { port: Number(port), token, pairedAt: Date.now() }
});

The Service Worker MCP client (task 2.11) will look up chrome.storage.local.get('mcp.pairing') and find undefined — pairing will silently fail downstream. Existing repo convention matches: see src/background/index.ts#L56 (debugger_enabled_${tabId}) and src/services/ai-client.ts#L232 (ai_config).

C-2. Token persisted in browser history (security).
After a successful pair, window.location.hash still contains #token=<64-hex>&port=.... That token is now recoverable from browser history, session restore on crash, bookmarks, and the DevTools URL pane. Fix: call window.history.replaceState(null, '', window.location.pathname) right after the storage write resolves. (Violates OWASP ASVS 3.1.1 — session tokens must not live in URL state.)

C-3. No Origin check — anyone who can set the hash can silently re-pair you.
useEffect(() => performPairing(), []) auto-fires on every mount, regardless of who navigated here. A malicious page that can navigate the options page with a crafted hash could silently overwrite an active pairing with attacker-controlled values. Fix:

1. Verify location.protocol === 'chrome-extension:' and location.host === chrome.runtime.id before processing.
2. Require an explicit user click to confirm pairing — don't auto-write on mount.
3. If existing pairing is present, prompt "Replace existing pairing with bridge on port X?" before overwriting.

C-4. Component is unreachable — options_page missing from manifest.
Verified directly: public/manifest.json has no options_page field and public/options.html doesn't exist. The component lives at src/panel/components/MCPPairingPanel.tsx but there's no host page to mount it. Either bundle the manifest change + options.html in this PR, or explicitly mark this PR "blocked by #X" and wait for the manifest wiring.

C-5. chrome.storage.local.set() errors silently swallowed.
The write is fire-and-forget. If it fails (quota exceeded, permissions error), the UI still says "MCP pairing successful!" — false positive. Per Chrome docs, wrap in await + try/catch:

try {
  await chrome.storage.local.set({ 'mcp.pairing': {...} });
  setPaired(true);
} catch (err) {
  setErrors([{ code: 'storage-failed', message: 'Could not save pairing.', fix: err.message }]);
}

🟡 Warnings (please also fix)

• Port validation accepts decimals/hex/whitespace. Number('1024.5') → 1024.5, Number('0x2000') → 8192, Number(' 8080 ') → 8080 all pass your range check. Add /^\d+$/.test(port) first.
• Test file in wrong directory + wrong extension. Repo convention (verified: 11 existing tests) is src/__tests__/*.test.tsx. Move to src/__tests__/MCPPairingPanel.test.tsx.
• Inline styles violate the repo's CSS architecture. All existing components use class names from src/panel/styles/panel.css; style={} is reserved for dynamic computed values only. Move structural styles to new classes like .mcp-pairing-panel.
• React.FC is a repo-wide antipattern (zero existing usage in src/panel/**). Use export function MCPPairingPanel(): React.JSX.Element { ... } with an explicit props interface.
• import React, { ... } from 'react' is redundant under "jsx": "react-jsx" (tsconfig). Drop the default import; noUnusedLocals will error once React.FC is removed.

💡 Improvements (nice-to-have)

• Extract validatePairingParams + parseHash to src/utils/validate-mcp-pairing.ts so tests don't need to import a React component file. Matches the sanitize.ts ↔ sanitize.test.ts pattern.
• Add a useRef guard around the useEffect so React 18 StrictMode's double-mount doesn't cause issues.
• Add edge-case tests: decimal/hex/leading-zero ports, duplicate hash keys, URL-encoded tokens, missing #.
• Trailing newline on both new files (POSIX).

What you got right

✅ All 5 issue-#7 acceptance-criteria scenarios tested · ✅ Regex ^[0-9a-fA-F]{64}$ correctly anchored — no ReDoS · ✅ Pure validator function is easy to test · ✅ Conventional Commits title · ✅ Closes #7 reference

Recommended next steps

1. Read the extension-mcp-client spec — this is your integration contract.
2. Address C-1 through C-5 first (security + integration before polish).
3. Then the warnings.
4. Push to the same branch — CI will re-run.
5. Ping me (@hoainho) for re-review.

Full review report on disk at .opencode/reviews/PR_17_2026-05-31.md — read if you want more depth on any finding.

Genuinely good first contribution. Don't be discouraged by the issue count — the bugs are non-obvious and you wrote the parts that are hardest (clean validator design, good tests, good error UX).

[hoainho]

👋 Hey @iMindCap — checking in. No pressure at all, just wanted to make sure the review wasn't overwhelming. The 5 critical issues are a lot to process for a first PR, and most of them come from a part of the MCP threat model that issue #7 didn't fully explain.

If any of the findings are unclear, happy to:

• Pair on a specific one (drop a comment with "explain C-X" and I'll go deeper)
• Or scope the PR down — we can merge just the validator improvements (the regex + error taxonomy is genuinely solid work) and split the security/integration fixes (history.replaceState, Origin check, storage schema) into follow-up PRs. That way your good work doesn't get blocked on the harder stuff.

What works for you? Take your time — there's no SLA on first PRs.

[iMindCap]

Hi @hoainho Thank you for the thorough review and for being so encouraging it really helps as a first-time contributor.
I'd love to go with the scoped approach: merge the validator + error taxonomy now, and tackle C-1 through C-5 in follow-up PRs. Before that, I can clean up the warnings that don't depend on the MCP integration (test directory, inline styles, React.FC) right now in this branch if that works for you.
Let me know and I'll get started!

[hoainho]

Awesome @iMindCap — that's exactly the right call. Green light on both fronts:

✅ Confirmed plan

This PR (now)

Keep scope to: token format validator + error taxonomy + the test directory/inline-styles/React.FC cleanups. Those are independent of the MCP integration so they're safe to land first.

Follow-up PRs (later)

• C-1 through C-5 each get their own PR once mcp-server-v1 lands M3 (~Jul 28). I'll open tracking issues for each so they're visible:
  • C-1: token storage (chrome.storage.session)
  • C-2: timing-safe token comparison
  • C-3: pairing handshake state machine
  • C-4: token expiration + rotation
  • C-5: rate limit on pair attempts

I'll create those tracking issues this week so they're queued up. No work needed from you on them right now — just Closes #N in each follow-up PR when the time comes.

🛠 What to do here

When you push the cleanup commits:

1. Test directory: move MCPPairingPanel.test.ts into src/__tests__/ so it's picked up by vitest's include pattern (existing pattern: see src/__tests__/IssueCard.test.tsx)
2. Inline styles: extract the style={{ ... }} props on lines 84-92 (pairing-panel-*) into panel.css — pick class names like .pairing-panel-container, .pairing-panel-form, etc. Follow the BEM-ish pattern used elsewhere in the file
3. React.FC → typed arrow: most components in src/panel/* use function ComponentName({ ... }: Props) { ... } rather than const ComponentName: React.FC<Props> = .... Match the local convention.

After those 3, request another review and I'll re-check + merge.

👏 Re first-time-contributor note

You're handling this exceptionally well for a first PR. Most experienced contributors don't read a 5-finding security review this carefully. The scope-discipline alone (asking which path to take instead of guessing) is something most maintainers wish for. Take your time — there's no rush.

— Hoài

[hoainho]

@iMindCap — quick follow-up: the 5 tracking issues for C-1 through C-5 are now open. Reference these in your PR description (or in follow-up PRs after this one merges):

#	Issue	Severity
C-1	Move pairing token from chrome.storage.local to .session	Critical
C-2	Timing-safe comparison for token	High
C-3	Replace imperative flow with state machine	High
C-4	Token expiration + rotation	High
C-5	Rate-limit pairing attempts	Medium-High

All 5 are labeled security + help wanted so they're claimable when you (or anyone) is ready. They're explicitly noted as blocked on mcp-server-v1 M3 (~Jul 28, 2026) — there's no rush.

Take whatever time you need on the cleanup commits in this PR. No deadline.

🙏

[iMindCap]

Hi @hoainho! I've addressed all 3 cleanups:

Moved the test file to src/tests/MCPPairingPanel.test.tsx
Extracted all inline styles to panel.css using .pairing-panel-container, .pairing-panel-error, and .pairing-panel-error-fix
Replaced React.FC with a plain export function MCPPairingPanel() matching the local convention

Also took the opportunity to fix the port validation to reject decimals, hex, and whitespace as you noted in the warnings.
All tests passing. Pushing the commits now — ready for re-review when you get a chance! 🙏

[hoainho]

@iMindCap Thanks for the careful refactor — 31645af addresses all 3 cleanups from the previous review cleanly:

✅ Fixed correctly:

• React.FC → function declaration
• Inline styles → .pairing-panel-container / .pairing-panel-error / .pairing-panel-error-fix CSS classes
• Stricter port validation: /^\\d+$/ regex rejects decimals, hex, leading spaces
• New tests in src/__tests__/MCPPairingPanel.test.tsx cover the new regex edge cases (decimals, hex, leading spaces) — nice extension

⛔ One blocking issue:

The OLD test file src/panel/components/MCPPairingPanel.test.ts was NOT deleted when you moved the tests to src/__tests__/MCPPairingPanel.test.tsx. Both files now exist on the branch:

src/panel/components/MCPPairingPanel.test.ts  (old, 5 cases)
src/__tests__/MCPPairingPanel.test.tsx        (new, 8 cases — superset)

The new file is a strict superset of the old (same 5 cases + 3 new edge cases). Vitest will discover both and run all 13 tests = duplicate runs of 5 identical cases.

To fix: git rm src/panel/components/MCPPairingPanel.test.ts && git commit -m "refactor: remove duplicate test file (moved to src/__tests__/)" && git push

Once that's in, this PR is approved + ready to merge. Everything else looks great.

[iMindCap]

Hi @hoainho! All done:

Removed the duplicate test file with git rm
Resolved the merge conflict in panel.css (kept both changes)

Ready for re-review whenever you get a chance! 🙏