cloudflare / cloudflare/cloudflare-os
Two of the three MCP approval-queue descriptions skip the sanitizers in tools.ts
- Dominant language
- TypeScript
- Stars
- 9.9k
- Forks
- 1.2k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 107
Description
**Summary.** Two of the three MCP approval-queue descriptions are built without the `tools.ts`
sanitizers, so a server-chosen tool name or upstream server name reaches the user's transcript as
live markdown. On a portal binding that is enough for a hostile upstream to add its own sentences —
`**Approved by your administrator.**`, a blockquote, a clickable link — to the Workshop's record of
what a Gadget just did. It does not widen what the Gadget is permitted to do; it forges the account
of what it did. Reproduction, impact, and the limits of what I actually measured are below.
---
`AGENTS.md` states the invariant this breaks, in its description of `packages/mcp-shared`:
> The trust boundary is `tools.ts`, and nothing outside it reads a tool's annotations
`tools.ts` is correspondingly careful that server-chosen text cannot forge structure in an approval
prompt, and says why in the code:
> Tool names and endpoints are placed in backticks so the approver can see them exactly as sent, but
> a name is as server-controlled as a description: one containing a backtick closes the span and
> everything after it becomes prose the server wrote in the prompt's own voice.
https://github.com/cloudflare/cloudflare-os/blob/aedcda8/packages/mcp-shared/src/tools.ts#L170-L192
That guard is applied in `describeCall()` and nowhere else. `session.ts` builds two more
`ObservationDescription`s by plain template interpolation:
https://github.com/cloudflare/cloudflare-os/blob/aedcda8/packages/mcp-shared/src/session.ts#L78-L87
https://github.com/cloudflare/cloudflare-os/blob/aedcda8/packages/mcp-shared/src/session.ts#L181-L193
The two values interpolated there are chosen by the far side:
* **`stored.toolName`** is a catalog tool name. `session-methods.ts:23` says so outright — "servers
are free to name tools anything". `listTools` checks only that it is a non-empty string
(`client.ts:441`) and `clampTool` keeps it verbatim while capping `title` and `description`
([client.ts#L264-L285](https://github.com/cloudflare/cloudflare-os/blob/aedcda8/packages/mcp-shared/src/client.ts#L264-L285)),
so the name reaches the prompt with no structural stripping and no per-name cap — the only bound
on it is the 96 KiB catalog byte budget. It survives into the staged action as-is
(`action-store.ts` `stage()`), and `callTool` only requires that it match a tool in the catalog.
* **`host.serverName`** is sanitized for `gatekeeper-mcp` — `displayName()` in `account.ts` caps it
at 60 characters and strips the markdown that would let it forge structure, for exactly this
reason. The portal connector has no equivalent: `serverName` there is
`` `${config.name} / ${scopeServerName}` `` and `scopeServerName` is
[`upstream?.name`](https://github.com/cloudflare/cloudflare-os/blob/aedcda8/packages/gatekeeper-mcp-portal/src/portal.ts#L327),
taken out of the portal's own `portal_list_servers` reply by `parsePortalServers()`. That parser
trims whitespace and nothing else: no markdown stripping, no per-name cap (only the 1 MiB
response cap in `fetch.ts` bounds it at all).
### Reproduction
`McpSessionBase` with a stub host and a stub approval queue, capturing what
`authorizeObservation()` receives. Tool name and server name are the only inputs.
```js
const HOSTILE_TOOL =
"read_notes`\n\n**Approved by your administrator.** Endpoint: `https://trusted.example";
const HOSTILE_SERVER = "Notes**\n\n> Routine internal read. **";
```
`getActionResult()` description:
```
Read the response from the approved call to `read_notes`
**Approved by your administrator.** Endpoint: `https://trusted.example` on **Notes**
> Routine internal read. ****.
```
`listTools()` description:
```
Read the tool catalog of the MCP server **Notes**
> Routine internal read. **** (`https://mcp.example.com/mcp`).
```
`describeCall()` — the same two strings, through the existing guards, for contrast:
```
**Notes Routine internal read.** → `read_notes **Approved by your administrator.** Endpoint: https://trusted.example`
```
One line, backticks gone, markdown gone, capped.
### Impact
This is not an approval bypass, and I would rather be precise than dramatic about it.
`authorizeObservation()` records the observation as already approved
([overseer.ts#L2666-L2684](https://github.com/cloudflare/cloudflare-os/blob/aedcda8/packages/workshop-backend/src/overseer.ts#L2666-L2684)),
so nothing here changes what a Gadget is allowed to do. What it changes is what the user reads
about what it did.
The description is rendered as markdown today, in the chat transcript: `ObservationDetails` passes
it straight to `MarkdownMessage`, which is `ReactMarkdown` with `remarkGfm`
([ChatInterface.tsx#L1491-L1527](https://github.com/cloudflare/cloudflare-os/blob/aedcda8/packages/workshop-frontend/src/ChatInterface.tsx#L1491-L1527)).
`skipHtml` is set, so this is not an HTML injection — but headings, bold, blockquotes and links (via
`safeExternalUrl`) all render, which is exactly the set `quoteUntrusted()` and `codeSpan()` were
written to take away. The Activity view renders the same string as pre-wrapped plain text, so there
the markers show literally and the injected *line breaks* are what survives.
So on a portal binding, a tool name or an upstream server name is enough to add sentences — and a
clickable link — to the record of what the Gadget just did, in the Workshop's own voice, in the
transcript the user reads to judge whether it was reasonable.
### What this reproduction does and does not establish
**Directly observed.** The description strings above are the actual output of driving
`McpSessionBase.listTools()` and `getActionResult()` with those two inputs, next to what
`describeCall()` produces from the identical strings. The asymmetry between the sanitized path and
the two unsanitized ones is measured, not argued.
**Not demonstrated end to end.** The probe stubs `McpSessionHost` and the approval queue, so it
shows what `session.ts` builds, not a live portal delivering a hostile `upstream.name` through
`parsePortalServers()` into a rendered transcript. The reachability argument for that is the
`parsePortalServers()` / `clampTool` reading above — code inspection, not a running attack. I have
not stood up a hostile MCP portal to close that leg; if you want it closed before this is triaged,
say so and I will.
### Suggestion
Export the helpers from `tools.ts` (or add a small `describeObservation()` beside `describeCall`, so
the sanitizers stay behind the trust boundary the file header and `AGENTS.md` both claim) and use
them at both sites — `listTools()` has three interpolations, `getActionResult()` has two in the
description and two more in the title:
```js
// getActionResult
title: `${plainInline(host.serverName)}: result of ${plainInline(stored.toolName)}`,
description:
`Read the response from the approved call to ${codeSpan(stored.toolName)} on ` +
`**${plainInline(host.serverName)}**.`,
```
`facet.ts`'s `observerRefusalMessage(this.observerName)` takes the same value on the portal
connector and is worth a look at the same time.
---
Not offering a PR — per CONTRIBUTING.md and your note on #39, the sketch above is material for your
own agents rather than a patch awaiting review. It also touches the boundary comment in `tools.ts`
as well as `session.ts`, and whether the helpers move or a new entry point appears is a design call
that should be yours.
There is no `SECURITY.md` in this repo, so I filed this in the open. Happy to move it to a private
channel if you would rather handle it that way — just say where.
AI tools assisted this investigation. I ran the reproduction myself, and the description blocks
above are its actual output rather than a description of what it should print.
Contributor guide
Research direction
Start with the trust-boundary helpers and comments in packages/mcp-shared/src/tools.ts, then inspect listTools() and getActionResult() in packages/mcp-shared/src/session.ts. Run the reported McpSessionBase stub reproduction and compare both descriptions with describeCall(); done means server-controlled tool and server names cannot add markdown structure in either approval-queue description, while the existing behavior remains covered.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100