cloudflare / cloudflare/cloudflare-os

Two of the three MCP approval-queue descriptions skip the sanitizers in tools.ts

Open
#42 0 comments 1 reaction 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.