Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/default-callback-warn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"agents": patch
---

fix: `addMcpServer` now warns when it falls back to the default OAuth callback URL because no `callbackPath` was provided. The default path embeds the agent instance name and only works when the Worker routes the agents prefix through `routeAgentRequest`; previously the fallback was silent whenever `sendIdentityOnConnect` was `true`, letting OAuth flows hang in `AUTHENTICATING` with no signal.
18 changes: 15 additions & 3 deletions packages/agents/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10779,9 +10779,21 @@ export class Agent<
let callbackUrl: string | undefined;
if (resolvedCallbackHost) {
const normalizedHost = resolvedCallbackHost.replace(/\/$/, "");
callbackUrl = resolvedCallbackPath
? `${normalizedHost}/${resolvedCallbackPath.replace(/^\//, "")}`
: `${normalizedHost}/${resolvedAgentsPrefix}/${camelCaseToKebabCase(this._ParentClass.name)}/${this.name}/callback`;
if (resolvedCallbackPath) {
callbackUrl = `${normalizedHost}/${resolvedCallbackPath.replace(/^\//, "")}`;
} else {
// The sendIdentityOnConnect:false guard above throws for this case;
// every other path that lands here gets a warning instead — the
// default URL leaks the instance name and only works when the Worker
// routes the agents prefix through routeAgentRequest (#1378).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Comment incorrectly claims sendIdentityOnConnect:false guard covers all paths to this branch

The comment at packages/agents/src/index.ts:10785 states "The sendIdentityOnConnect:false guard above throws for this case" — but this is only true when resolvedCallbackHost was explicitly provided by the caller. When the host is auto-derived from the request/connection at packages/agents/src/index.ts:10767-10776, the guard at packages/agents/src/index.ts:10754-10764 has already been passed (because resolvedCallbackHost was still undefined at that point). This means: with sendIdentityOnConnect:false, no explicit callbackHost, and a host auto-derived from the current request, the code reaches this else branch and only warns — the instance name still appears in the default callback URL despite the agent opting out of identity exposure. The underlying enforcement gap is pre-existing (the old ternary had the same control flow), but the new comment gives a false sense that it's fully handled. Consider either extending the guard to also check after auto-derivation, or correcting the comment to note this gap.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the comment over-claimed. Corrected in 53fd1ad: the throw only covers sendIdentityOnConnect: false with an explicit callbackHost; an auto-derived host bypasses it regardless of the option and now lands in the warn branch (which is the behavioral improvement this PR adds — that path was previously silent). Left the enforcement gap as a warn rather than extending the throw to derived hosts, since that would break currently-working setups that do route /agents/* correctly; a maintainer could escalate it in the next major (#844).

callbackUrl = `${normalizedHost}/${resolvedAgentsPrefix}/${camelCaseToKebabCase(this._ParentClass.name)}/${this.name}/callback`;
console.warn(
`addMcpServer("${serverName}"): no callbackPath was provided, falling back to the default OAuth callback URL ${callbackUrl}. ` +
`This exposes the agent instance name in the URL and only works if your Worker routes /${resolvedAgentsPrefix}/* ` +
`through routeAgentRequest. If that route is handled elsewhere (e.g. static assets), the OAuth flow will hang in ` +
`AUTHENTICATING — pass an explicit callbackPath and route it to this agent via getAgentByName.`
);
}
}

const id = requestedId ?? nanoid(8);
Expand Down
39 changes: 39 additions & 0 deletions packages/agents/src/tests/agents/mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1054,6 +1054,45 @@ export class TestHttpMcpDedupAgent extends Agent {
};
}
}

// #1378: capture console.warn during addMcpServer to assert the default
// callback URL warning fires (and stays silent with an explicit path).
private async _captureWarnings(
fn: () => Promise<unknown>
): Promise<string[]> {
const captured: string[] = [];
const originalWarn = console.warn;
console.warn = (...args: unknown[]) => {
captured.push(args.map(String).join(" "));
};
try {
await fn();
} catch {
// connection failures from the mocked transport are expected
} finally {
console.warn = originalWarn;
}
return captured;
}

async testDefaultCallbackUrlWarns() {
const warnings = await this._captureWarnings(() =>
this.addMcpServer("warn-server", "https://mcp.example.com/warn", {
callbackHost: "https://example.com"
})
);
return { warnings };
}

async testExplicitCallbackPathDoesNotWarn() {
const warnings = await this._captureWarnings(() =>
this.addMcpServer("no-warn-server", "https://mcp.example.com/no-warn", {
callbackHost: "https://example.com",
callbackPath: "/auth/mcp-callback"
})
);
return { warnings };
}
}

/**
Expand Down
37 changes: 37 additions & 0 deletions packages/agents/src/tests/mcp/add-mcp-server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,43 @@ describe("addMcpServer callbackPath enforcement", () => {
});
});

describe("addMcpServer default callback URL warning (#1378)", () => {
it("warns when the default callback URL is used without callbackPath", async () => {
const agentStub = await getAgentByName(
env.TestHttpMcpDedupAgent,
"test-default-callback-warn"
);
const { warnings } = (await agentStub.testDefaultCallbackUrlWarns()) as {
warnings: string[];
};
const warning = warnings.find((w) =>
w.includes("falling back to the default OAuth callback URL")
);
expect(warning).toBeDefined();
expect(warning).toContain('addMcpServer("warn-server")');
// Default URL embeds the agents prefix and the instance name
expect(warning).toContain("https://example.com/agents/");
expect(warning).toContain("/test-default-callback-warn/callback");
expect(warning).toContain("callbackPath");
});

it("does not warn when an explicit callbackPath is provided", async () => {
const agentStub = await getAgentByName(
env.TestHttpMcpDedupAgent,
"test-explicit-callback-no-warn"
);
const { warnings } =
(await agentStub.testExplicitCallbackPathDoesNotWarn()) as {
warnings: string[];
};
expect(
warnings.filter((w) =>
w.includes("falling back to the default OAuth callback URL")
)
).toEqual([]);
});
});

describe("addMcpServer API overloads", () => {
describe("new options-based API", () => {
it("should resolve options object with all fields", async () => {
Expand Down
Loading