SEC-009: Require confirmation for MCP app messages - #280
Conversation
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
🟡 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
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>
b1df548 to
6a9d966
Compare
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 skippedbin/just check— passed with two pre-existingtests/e2e/table-overflow.spec.tsnon-null-assertion warningsgit diff --check origin/main...HEAD— passed59f5f1d12352974ac820c5ac72ca90eb06689ef89f39b56e869e7b7476cd2a50