Skip to content

fix(self-mod): render full MCP server payload on the approval card#2998

Open
glifocat wants to merge 3 commits into
mainfrom
fix/mcp-approval-full-payload
Open

fix(self-mod): render full MCP server payload on the approval card#2998
glifocat wants to merge 3 commits into
mainfrom
fix/mcp-approval-full-payload

Conversation

@glifocat

@glifocat glifocat commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Type of Change

  • Feature skill - adds a channel or integration (source code changes + SKILL.md)
  • Utility skill - adds a standalone tool (code files in .claude/skills/<name>/, no source changes)
  • Operational/container skill - adds a workflow or agent skill (SKILL.md only, no source changes)
  • Fix - bug fix or security fix to source code
  • Simplification - reduces or simplifies source code
  • Documentation - docs, README, or CONTRIBUTING changes only

Description

Closes #2762. Closes #2827 (same finding filed twice).

What this does

The add_mcp_server approval card now shows every field that gets applied, not just the name and command. The payload renders in a code fence, one JSON-encoded line per field. Bad payloads are rejected before the approval row exists.

Example card:

Agent "research" is attempting to add a new MCP server:
```
name: "docs"
command: "npx"
args: ["-y","some-mcp","--flag"]
env: {"FOO":"bar","GITHUB_TOKEN":"<redacted: 40 bytes, sha256 a1b2c3d4>"}
```

The details:

  • Backticks and invisible characters (bidi, zero-width, controls) render as visible \uXXXX escapes. Payload content cannot close the fence or add lines to the card.
  • Secret-shaped values (keys like GITHUB_TOKEN, values like sk-...) are redacted on the card only. The real value is applied unchanged. The fingerprint lets an approver check the value against a copy they already have.
  • Keys like NODE_OPTIONS are not secret-shaped and stay fully visible.
  • Limits, checked before the row is created: 32 args, 32 env vars, 16 KB payload, 1500-byte rendered card. The card cap keeps cards under every channel's send limit. The Telegram adapter silently truncates at 4096, which would hide trailing fields again.
  • A failed card delivery now deletes the pending approval row instead of leaving it orphaned (primitive.ts, all approval types).

Why

The handler stores name, command, args, and env in the approval payload, but the card only rendered name and command. On approve, all four fields go into the group's MCP server config. So the approver authorized args and env they never saw. #2762 and #2827 report this, with repro payloads.

How I verified it

  • New src/modules/self-mod/request.test.ts, 15 tests, the first for this module: card contents (fails on unfixed main), fence integrity against markdown and backtick payloads, visible escapes for bidi/zero-width/BOM, redaction fingerprints, boundaries at 32/33 args and env vars and at the exact byte limits, type validation, and the card as delivered through the chat-sdk bridge.
  • New src/modules/approvals/primitive.test.ts: delivery failure removes the row (fails on unfixed primitive.ts), success keeps it.
  • Full host suite: 75 files, 701 tests passed. tsc --noEmit, eslint, and prettier clean on the changed files.
  • Verified against main @ 0c0f4c2.

Left out on purpose: a digest binding the approved payload to the approval row. Apply already reads the stored payload directly. Possible follow-up.

Assisted by Claude; reviewed and verified by a human before submission.

@github-actions github-actions Bot added core-team PR opened by a core team member follows-guidelines PR was created using the current contributing template PR: Fix Bug fix labels Jul 9, 2026
The add_mcp_server approval card showed only the server name and
command, but approval applied name, command, args, and env. The
approver authorized fields they never saw.

Render every field JSON encoded, so boundaries are exact and an
embedded newline shows as a visible \n escape instead of adding fake
lines to the card. Validate args/env types before the approval is
created. apply.ts is unchanged, it already applies exactly the stored
payload.
@glifocat glifocat force-pushed the fix/mcp-approval-full-payload branch from 3f5b41e to 874ee63 Compare July 9, 2026 22:51
Render the payload in a code fence with invisibles and backticks
escaped as visible \uXXXX, redact secret-shaped values to a byte count
plus sha256 fingerprint (card only, the verbatim value is still
applied), and reject oversized payloads before the approval row exists
(32 args, 32 env vars, 16 KB payload, 1500-byte rendered card). A
failed card delivery now deletes the pending approval row instead of
leaving it orphaned.
@glifocat glifocat force-pushed the fix/mcp-approval-full-payload branch from b94005c to 8a03d1d Compare July 9, 2026 23:25
@glifocat glifocat marked this pull request as ready for review July 9, 2026 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core-team PR opened by a core team member follows-guidelines PR was created using the current contributing template PR: Fix Bug fix

Projects

None yet

1 participant