Skip to content

SEC-009: Require confirmation for MCP app messages - #280

Draft
budzeg wants to merge 3 commits into
mainfrom
budzeg/sec-009-host-confirmation
Draft

SEC-009: Require confirmation for MCP app messages#280
budzeg wants to merge 3 commits into
mainfrom
budzeg/sec-009-host-confirmation

Conversation

@budzeg

@budzeg budzeg commented Sep 2, 2026

Copy link
Copy Markdown

Summary

Require an explicit host-owned confirmation before an MCP App can submit a user-role message through Berd's ordinary composer path. The approval is bound to the current session, tool call, extension, tool name, exact text, and nonce, with stale, replayed, malformed, cancelled, and concurrent requests rejected.

This preserves the existing composer queue and dispatch behavior after the user explicitly approves the request.

Related issue

N/A — no public issue was opened because the underlying report is security-sensitive. No duplicate public issue or PR was found during the pre-submission search.

Testing

  • bin/just test — 623 files passed; 7,518 tests passed; 1 skipped
  • bin/just check — passed with two pre-existing tests/e2e/table-overflow.spec.ts non-null-assertion warnings
  • git diff --check origin/main...HEAD — passed
  • Independent adversarial security QA — passed against exact diff fingerprint 59f5f1d12352974ac820c5ac72ca90eb06689ef89f39b56e869e7b7476cd2a50
  • UI screenshots are not included because the confirmation behavior is security-sensitive and is covered by the focused interaction tests in this change.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The current implementation can render two focus-trapping dialogs simultaneously (link-safety + message confirmation), which can break overlay state tracking and user focus without additional mutual-exclusion handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces a host-owned confirmation step before an MCP App can submit a user-role message through Berd’s normal composer path, binding approval to the current session/tool-call identity and exact message text to prevent replay, staleness, and concurrent misuse.

Changes:

  • Add an explicit confirmation dialog before dispatching app-authored user messages (with replay/concurrency invalidation).
  • Add i18n strings for the new confirmation UX (EN/ES).
  • Add focused interaction tests covering approval, cancellation, concurrency, and stale/malformed cases.
File summaries
File Description
src/shared/i18n/locales/en/chat.json Adds confirmation dialog copy for MCP app-authored messages.
src/shared/i18n/locales/es/chat.json Adds Spanish translations for the MCP app message confirmation UX.
src/features/chat/ui/McpAppView.tsx Implements pending-message confirmation flow and resolves/rejects requests securely.
src/features/chat/ui/tests/McpAppView.test.tsx Adds tests validating confirmation gating, rejection paths, and concurrency/staleness behavior.
Review details

Suppressed comments (1)

src/features/chat/ui/McpAppView.tsx:416

  • App message confirmation can currently be opened while the link-safety modal is open, resulting in stacked dialogs and unclear user focus. Block app-message requests whenever a link confirmation is already pending.
      if (
        role !== "user" ||
        !Array.isArray(content) ||
        !onSendMessage ||
        pendingAppMessageRef.current ||
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/features/chat/ui/McpAppView.tsx
Comment thread src/features/chat/ui/McpAppView.tsx

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

setMcpActivity now uses a per-message unique sourceId, which can unnecessarily accumulate expiring protection signals and should be stabilized.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/features/chat/ui/McpAppView.tsx
budzeg and others added 3 commits September 2, 2026 15:39
Prevent MCP apps from silently spending user chat authority by requiring a
host-owned confirmation bound to the current app request.

Co-authored-by: Olabode Olaoke <olabode@squareup.com>
Signed-off-by: Olabode Olaoke <olabode@squareup.com>
Keep link and message confirmations mutually exclusive so focus trapping and transcript overlay tracking remain accurate.

Co-authored-by: Olabode Olaoke <olabode@squareup.com>

Signed-off-by: Olabode Olaoke <olabode@squareup.com>
Refresh one expiring row-protection signal after each approved app message instead of accumulating a signal per confirmation.

Signed-off-by: Sleek <93c2629a5f1f93118df6264f931480b8f7b585d5f425aa459e48efd6e883ee14@buzz.block.builderlab.xyz>
@budzeg
budzeg force-pushed the budzeg/sec-009-host-confirmation branch from b1df548 to 6a9d966 Compare September 2, 2026 21:39
@budzeg
budzeg requested a lite review from Copilot September 2, 2026 21:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The confirmation gating and replay/concurrency protections are implemented cohesively and backed by targeted interaction tests.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants