cloudflare / cloudflare/agents

addMcpServer: default callback URL silently breaks when callers don't set `callbackPath` + `sendIdentityOnConnect: true`

Open
#1,378 1 comment 0 reactions 1 assignee Claimed by @mattzcarey View on GitHub
bug mcp
Dominant language
TypeScript
Stars
5.6k
Forks
711
Avg merge
1d 20h
Merged PRs (30d)
53

Description

## Summary

`addMcpServer(name, url)` with no explicit `callbackPath` falls back to a default callback URL of the form:

```
${host}/${agentsPrefix}/${kebab(ParentClass.name)}/${this.name}/callback
```

e.g. `https://example.com/agents/my-assistant/octocat/callback`.

There is an existing guard in [`packages/agents/src/index.ts`](https://github.com/cloudflare/agents/blob/main/packages/agents/src/index.ts) (~line 5495) that refuses this default and throws a helpful error:

```ts
if (
!this._resolvedOptions.sendIdentityOnConnect &&
resolvedCallbackHost &&
!resolvedCallbackPath
) {
throw new Error(
"callbackPath is required in addMcpServer options when sendIdentityOnConnect is false — " +
"the default callback URL would expose the instance name. " +
"Provide a callbackPath and route the callback request to this agent via getAgentByName."
);
}
```

But the guard is bypassed whenever `sendIdentityOnConnect: true` is set. That option is about something different (broadcasting the DO's identity over the WS handshake), and yet turning it on silently disables a piece of safety scaffolding that has nothing to do with identity broadcasting.

## Why it bit us

Discovered while reviewing #1374. In `examples/assistant`:

- `MyAssistant` sets `static options = { sendIdentityOnConnect: true }` so the browser can learn the server-assigned DO name.
- The Worker narrows `run_worker_first` to `/auth/*` and `/chat*` (so random clients can't reach `/agents/my-assistant/` directly — an auth-bypass fix).
- The example's `addServer` RPC calls `this.addMcpServer(name, url)` with no options.

Because `sendIdentityOnConnect` is true, no error is thrown. The default callback URL `https://.../agents/my-assistant//callback` is silently generated and persisted. When the user goes through OAuth:

1. The provider redirects the browser to that URL.
2. `/agents/*` is not in `run_worker_first`, so the SPA asset handler serves `index.html` with a 200.
3. The Durable Object never sees the `code`.
4. The MCP server hangs in \`AUTHENTICATING\` forever, with no server-side error.

The fix on the example side was trivial (one option: `callbackPath: \"chat/mcp-callback\"`), but the reason it was even possible to misconfigure without a warning is the guard bypass above.

## Proposal

Make \`sendIdentityOnConnect: true\` not disable the callback-path enforcement. Options, in order of least-disruptive first:

1. **Always throw/warn when the default callback URL is used**, regardless of \`sendIdentityOnConnect\`. The default URL leaks \`this.name\` into the path either way, and it's a foot-gun for any Worker that doesn't route \`/agents/*\` to the framework. If it's considered a breaking change, start with a \`console.warn\`.
2. Require an explicit opt-in (e.g. \`allowDefaultCallbackUrl: true\`) when the caller actually wants the legacy behavior.
3. If there's a reason \`sendIdentityOnConnect\` was coupled to the guard, document it prominently — right now the coupling is non-obvious and the failure mode is silent.

## Acceptance criteria

- Calling \`addMcpServer(name, url)\` with no explicit \`callbackPath\` in a \`sendIdentityOnConnect: true\` agent either throws the existing helpful error or logs a \`console.warn\` that surfaces the misconfiguration at development time.
- The existing tests in \`packages/agents/src/tests/mcp/add-mcp-server.test.ts\` are extended to cover the \`sendIdentityOnConnect: true\` branch.
- \`addMcpServer\` docs call out that the default callback URL only works if you route \`/agents/*\` to the framework's \`routeAgentRequest\`.

## Workaround (today)

Always pass an explicit \`callbackPath\` that lands on a Worker route you control, and route OAuth callbacks through the same authenticated path as the rest of your app:

\`\`\`ts
await this.addMcpServer(name, url, { callbackPath: "chat/mcp-callback" });
\`\`\`

## Related

- #1374 (context / reproduction)
- \`packages/agents/src/index.ts\` around line 5495 (the guard) and 5520-5527 (the default URL construction)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.