diff --git a/docs/02-architecture/adr/ADR-0020-mcp-server-url-validity.md b/docs/02-architecture/adr/ADR-0020-mcp-server-url-validity.md new file mode 100644 index 00000000000..c5e43368561 --- /dev/null +++ b/docs/02-architecture/adr/ADR-0020-mcp-server-url-validity.md @@ -0,0 +1,166 @@ +# ADR-0020 — MCP server URL validity: when to check, and whether to block + +- **Status:** Accepted +- **Date:** 2026-07-27 +- **Source:** [`mcp-remote-servers.md` §3](../external-dependencies/mcp-remote-servers.md#3-open-questions) + +## Context + +Forced by an external fact. When a user supplies a remote MCP server URL, a +common mistake is to paste the host or a parent path instead of the endpoint +— `https://mcp.notion.com/` instead of `https://mcp.notion.com/mcp`. The +toolkit currently accepts such a URL, scaffolds a project around it, and the +mistake surfaces only at provision time or not at all. + +[`mcp-remote-servers.md` §1.3–§1.4](../external-dependencies/mcp-remote-servers.md#13-measured-behavior--documented-endpoint-urls) +records what nine real endpoints and fourteen truncated forms return. Four +measured facts drive this decision: + +- No valid endpoint returned `404`. A `404` from an `initialize` `POST` is + therefore a zero-false-positive negative signal on the sample measured. +- It is not a complete signal: it caught six of the fourteen truncated URLs. + Widening to the other negative shapes measured — `403`, `405`, and `2xx` + without a JSON-RPC envelope — raises that to ten of fourteen, still without + misclassifying any of the nine valid endpoints. +- Those wider shapes are weaker evidence. Both measured `403`s came from a WAF + intercepting ahead of the application, and a WAF fronting a *valid* endpoint + could reject the probe the same way. `405` came from a valid endpoint too, + when addressed with the wrong method. +- `5xx` and transport failures mean the server is unreachable, not that the + URL is wrong, and must be distinguished from both of the above. + +One valid server that *can* answer `404` is absent from the sample rather than +nonexistent. The transport spec's backwards-compatibility rule has a client +read `404` or `405` from the `initialize` `POST` as "this may be a deprecated +HTTP+SSE server" and disambiguate with a `GET` that opens an event stream. +No such server was reachable to measure — every `/sse` path probed is either a +streamable-HTTP endpoint or authorization-gated ahead of transport dispatch — +but the requirement is normative, so the probe performs that `GET` before +forming a `notEndpoint` verdict (fact page §1.2). The zero-false-positive +property is preserved by construction rather than by the sample. + +A fifth fact constrains how success is recognized: `https://learn.microsoft.com/api/mcp` +requires no authorization at all and answers `initialize` with `200`, while +`https://substrate-sdf.office.com/` answers the same request with `200` and +HTML. The status is not the signal; the JSON-RPC envelope in the body is. +Recognizing it also means a confirmed endpoint stops being indistinguishable +from a probe that learned nothing. + +The scaffold flows also differ in what backstop they already have. The paths +that fetch tools detect a wrong URL independently, because a wrong URL yields +zero tools. The dynamic-tool-discovery path performs no tool fetch by design +and has no such backstop, which is why the `404` signal currently surfaces +there as a post-scaffold warning. + +The question this ADR settles is *where* the check belongs and *how hard* it +should push back — not whether `404` is meaningful, which §1 establishes. + +## Options considered + +- **A — Post-scaffold warning only (current behavior).** The probe already + runs during scaffolding; a `404` becomes a warning in the scaffolding + summary. Cheapest, and cannot break input flows. But it tells the user after + the project exists, and the remedy is to recreate it. + +- **B — Non-blocking hint at input time.** Move the signal to the MCP server + URL question so the user sees it while the field is still editable, but + allow them to proceed. Requires the URL question to probe on accept, which + it does not currently do on the VS Code path. Costs a network round-trip in + the question walk and must degrade silently when offline. + +- **C — Blocking validation at input time.** Same probe, but reject the URL. + The measured precision supports it, but the sample is nine endpoints, a + gateway mid-deploy can legitimately `404`, and a hard block on a network + probe makes the create flow fail closed when the network does. + +- **D — Positive validation instead of negative.** Require evidence the URL + *is* an MCP endpoint — a successful `initialize`, or a `401` challenge — + rather than looking for evidence it is not. Strictly stronger: it would also + catch the `200`-returning wrong URLs that `404` misses. But it inverts the + failure mode, rejecting valid endpoints whenever the probe cannot complete, + and the path-prefix `401` case (§1.4) would still pass. + +- **E — B plus telemetry, then reconsider C.** Ship the hint, measure how + often it fires and how often users correct the URL afterwards, and use that + to decide whether blocking is justified. Defers the precision question to + data instead of a nine-endpoint sample. + +- **F — C for the certain signal, A for the weaker ones.** Block at input time + on `404` alone; let the other negative shapes surface as a post-scaffold + warning. Two tiers to implement and reason about, but the strength of the + pushback then matches the strength of the evidence. + +## Decision + +**F — block at input time on `404`; warn after scaffolding on the weaker +negative shapes.** + +The URL question rejects a value if, and only if, an unauthenticated +`initialize` `POST` to it completes and the server answers `404`. Every other +outcome accepts: a `401`, a successful `initialize`, `403`, `405`, any other +status, a `5xx`, a timeout, a DNS or TLS failure. The user must have received a +positive statement from the server that nothing is routed at that path before +they are stopped. + +The probe classifies the URL into three states rather than reporting only +whether authorization is required: + +| State | Produced by | Effect | +|---|---|---| +| `confirmed` | `401`, or `2xx` carrying a JSON-RPC envelope | none | +| `notEndpoint` | `403`, `404`, `405`, or `2xx` without a JSON-RPC envelope, and no HTTP+SSE stream at the URL | `404` blocks at input; the rest warn after scaffolding | +| `undetermined` | `5xx`, other statuses, timeouts, transport failures | none | + +Blocking on `404` is chosen over the non-blocking hint (B) because the measured +precision is 100% across nine endpoints and the failure it prevents is +expensive: the project is scaffolded around the wrong URL, authorization +discovery then resolves that *host's* authorization server and writes +plausible-but-wrong endpoints, and on the dynamic-tool-discovery path nothing +else in the flow disagrees. A warning the user can click past does not prevent +that, and the remedy after the fact is to recreate the project. + +The weaker shapes are not promoted to blocking because both measured `403`s +came from a WAF rather than the application. Wrongly blocking a legitimate URL +is worse than missing a wrong one — the user has no way past a blocking +validator, whereas a warning still leaves a usable project. + +It is chosen over pure positive validation (D) for the same reason in stronger +form: D rejects valid endpoints whenever the probe cannot complete, which +breaks offline and restricted-network scaffolding. The rule adopted here fails +open on every ambiguous outcome by construction — `undetermined` never +surfaces at all. + +The negative statuses are enumerated (`403`, `404`, `405`) rather than +generalized to "any 4xx", because statuses such as `429` and `408` are +transient and would slander a URL that is in fact correct. `400` is excluded +for a stronger reason: the transport spec *requires* a server to answer `400` +when it rejects the `MCP-Protocol-Version`, so a valid endpoint can produce +one. + +The check is not a substitute for the tool-fetch signal on the paths that have +one, and does not claim completeness — §1.4 of the fact page documents a +truncated URL that answers `401` and therefore passes. + +## Consequences + +- New constraints §2.8 – §2.13 on + [`mcp-remote-servers.md`](../external-dependencies/mcp-remote-servers.md#2-constraints-derived-from-these-facts). +- Scaffolding gains a network round-trip in the question walk on every + platform. Previously the VS Code path performed no probe at this point. +- A URL that is going to be rejected costs a second round-trip, the HTTP+SSE + `GET`. Only the `notEndpoint` path pays it; a confirmed or undetermined URL + never does. +- A valid MCP server that is mid-deploy behind a gateway returning `404` will + block the create flow with no override. This is the accepted cost of the + decision; the residual risk is bounded because a deploying gateway more + commonly returns `5xx`, which accepts. +- The rejection message must name the measured cause and the likely fix, since + the user has no way past it. +- The post-scaffold warning on the dynamic-tool-discovery path now fires for + every `notEndpoint` shape, not only `404`. Its wording can no longer name a + status code. +- The `404` half of that warning becomes unreachable through the interactive + create flow. It is retained because that path is also reached through entry + points that do not walk the URL question. +- `confirmed` is recorded but not rendered anywhere. Whether to show positive + feedback for it is left open on the fact page §3. diff --git a/docs/02-architecture/adr/README.md b/docs/02-architecture/adr/README.md index f1153915517..77255db5355 100644 --- a/docs/02-architecture/adr/README.md +++ b/docs/02-architecture/adr/README.md @@ -38,6 +38,7 @@ decision. The format for a new ADR is defined inline below under | ADR-0017 | [Named pipeline + step / actionTemplate whitelist](ADR-0017-named-pipeline-step-whitelist.md) | Accepted | [`scaffolding.create.proposal.md` §14](../scaffolding.create.proposal.md#14-adrs-this-proposal-will-be-decomposed-into) | | ADR-0018 | [`ScaffoldRuntime` + T1/T2/T3 test pyramid + design-first `descriptor.spec` gate (application of ADR-0013, not a competing tiering)](ADR-0018-scaffold-runtime-test-pyramid.md) | Accepted | [`scaffolding.create.proposal.md` §14](../scaffolding.create.proposal.md#14-adrs-this-proposal-will-be-decomposed-into) | | ADR-0019 | [Dual-stream scaffold telemetry (v3 verbatim + parallel `scaffold-v4-*` family)](ADR-0019-dual-stream-scaffold-telemetry.md) | Accepted | [`scaffolding.create.proposal.md` §14](../scaffolding.create.proposal.md#14-adrs-this-proposal-will-be-decomposed-into) | +| ADR-0020 | [MCP server URL validity: when to check, and whether to block](ADR-0020-mcp-server-url-validity.md) | Accepted | [`mcp-remote-servers.md` §3](../external-dependencies/mcp-remote-servers.md#3-open-questions) | > **Reading order vs. numeric order.** The Index is in **numeric order** — > the immutable, reservation-ordered ADR id is the anchor for every forward diff --git a/docs/02-architecture/external-dependencies/README.md b/docs/02-architecture/external-dependencies/README.md index 85ee48382fd..f9ee5ed0d54 100644 --- a/docs/02-architecture/external-dependencies/README.md +++ b/docs/02-architecture/external-dependencies/README.md @@ -72,6 +72,7 @@ Every fact in §1 of the fact page must have a row in the code map. If a | Office Add-in tooling (`office-addin-project` / `office-addin-manifest`, `convertProject` precondition, MetaOS schema pins) | [`office-addin-tooling.md`](office-addin-tooling.md) | [`office-addin-tooling.code-map.md`](office-addin-tooling.code-map.md) | | Microsoft Graph DriveItem resolution (`/sites`, `/shares` endpoints, share-URL encoding, stable identifiers) | [`graph-driveitem-resolution.md`](graph-driveitem-resolution.md) | [`graph-driveitem-resolution.code-map.md`](graph-driveitem-resolution.code-map.md) | | Experimentation (Azure ExP / TAS) (`vscode/ab` endpoint, `clientid` assignment unit, `vscode` config namespace, `getTreatmentVariableAsync` feature-flag contract) | [`experimentation-exp.md`](experimentation-exp.md) | [`experimentation-exp.code-map.md`](experimentation-exp.code-map.md) | +| Remote MCP servers (`initialize` probe contract, 2025-03-26 vs 2025-06-18 authorization discovery, endpoint-validity signals) | [`mcp-remote-servers.md`](mcp-remote-servers.md) | [`mcp-remote-servers.code-map.md`](mcp-remote-servers.code-map.md) | ## What does NOT live here diff --git a/docs/02-architecture/external-dependencies/mcp-remote-servers.code-map.md b/docs/02-architecture/external-dependencies/mcp-remote-servers.code-map.md new file mode 100644 index 00000000000..1b763f2adf5 --- /dev/null +++ b/docs/02-architecture/external-dependencies/mcp-remote-servers.code-map.md @@ -0,0 +1,34 @@ +# Remote MCP servers — Code Map + +Navigation aid for refactor work on the remote MCP server binding. Maps each +fact in [`mcp-remote-servers.md`](mcp-remote-servers.md) to its current +location in source. + +> **This file is not part of the contract.** It is expected to churn as code +> moves. Constraints live in +> [`mcp-remote-servers.md`](mcp-remote-servers.md#2-constraints-derived-from-these-facts); +> updates here do not require an ADR. + +| Fact (from `mcp-remote-servers.md` §1) | File(s) | +|---|---| +| §1.1 Both auth generations — discovery entry point | `packages/fx-core/src/common/mcpToolFetcher.ts` (`resolveMCPOAuthMetadata`) | +| §1.1 `resource_metadata` chain (2025-06-18) | `packages/fx-core/src/common/mcpToolFetcher.ts` (`candidatesFromProtectedResourceMetadata`) | +| §1.6 Protected-resource document derived from the server URL | `packages/fx-core/src/common/mcpToolFetcher.ts` (`buildProtectedResourceCandidates`, `candidatesFromDerivedProtectedResource`) | +| §1.1 Origin-root fallback (2025-03-26) | `packages/fx-core/src/common/mcpToolFetcher.ts` (`buildMCPServerWellKnownCandidates`) | +| §1.1 Issuer-derived candidates, no origin-root forms | `packages/fx-core/src/common/mcpToolFetcher.ts` (`buildWellKnownCandidates`) | +| §1.2 `initialize` POST probe | `packages/fx-core/src/common/mcpToolFetcher.ts` (`probeMCPServerAuth`) | +| §1.2 Streamable HTTP / SSE transport selection for tool listing | `packages/fx-core/src/common/mcpToolFetcher.ts` (`fetchMCPTools`) | +| §1.2 Shared re-export consumed by v3 call sites | `packages/fx-core/src/component/utils/mcpToolFetcher.ts` | +| §1.2 JSON-RPC envelope check, tolerant of SSE framing | `packages/fx-core/src/common/mcpToolFetcher.ts` (`carriesJSONRPCEnvelope`) | +| §1.3 / §1.4 Three-state URL verdict | `packages/fx-core/src/common/mcpToolFetcher.ts` (`MCPEndpointStatus`, `MCPAuthProbeResult.endpointStatus`) | +| §1.4 Negative statuses treated as "not an MCP endpoint" | `packages/fx-core/src/common/mcpToolFetcher.ts` (`NOT_AN_ENDPOINT_STATUSES`) | +| §1.2 Deprecated HTTP+SSE ruled out before a 4xx becomes a verdict | `packages/fx-core/src/common/mcpToolFetcher.ts` (`servesLegacySSETransport`, `announcesLegacyMessageEndpoint`) | +| §1.4 Warning raised on the dynamic-tool-discovery scaffold path | `packages/fx-core/src/component/generator/declarativeAgent/helper.ts` (`generateForMCPForDAWithAuth`, warning type `mcpServerUrlNotAnEndpoint`) | +| §1.4 Warning text | `packages/fx-core/resource/package.nls.json` (`core.MCPForDA.mcpServerUrlNotAnEndpoint`) | +| §1.4 Tool-fetch based signal on the non-DT paths | `packages/fx-core/src/component/generator/declarativeAgent/helper.ts` (`generateForMCPForDA`), `packages/fx-core/src/core/FxCore.declarativeAgent.ts` (warning type `mcpNoToolsFetched`) | +| §1.1 / §1.4 v3 endpoint resolution and placeholder fallback | `packages/fx-core/src/component/utils/mcpAuthScaffolder.ts` (`resolveMCPAuthEndpoints`) | +| §1.1 / §1.4 v4 endpoint resolution and placeholder fallback | `packages/fx-core/src/v4/mcp/mcpAuthScaffold.ts` (`resolveEndpoints`), `packages/fx-core/src/v4/mcp/mcpAuthAction.ts` | +| §1.4 v4 tool fetch on the static path | `packages/fx-core/src/v4/runtime/steps/mcpStatic.ts` | +| §2.10 MCP server URL question (input-time blocking validation site) | `packages/fx-core/src/question/scaffold/vsc/teamsProjectTypeNode.ts` (`MCPForDAServerUrlNode`, `additionalValidationOnAccept`) | +| §2.10 Rejection message | `packages/fx-core/resource/package.nls.json` (`core.createProjectQuestion.mcpForDa.ServerUrl.notAnMcpEndpoint`) | +| §1.3 / §1.4 Measured behavior regression tests | `packages/fx-core/tests/component/utils/mcpToolFetcher.test.ts`, `packages/fx-core/tests/component/generator/declarativeAgentGenerator.test.ts` | diff --git a/docs/02-architecture/external-dependencies/mcp-remote-servers.md b/docs/02-architecture/external-dependencies/mcp-remote-servers.md new file mode 100644 index 00000000000..9109ad4bb73 --- /dev/null +++ b/docs/02-architecture/external-dependencies/mcp-remote-servers.md @@ -0,0 +1,305 @@ +# Remote MCP servers + +External-dependency fact page. Captures the **non-negotiable** HTTP behavior +the Microsoft 365 Agents Toolkit binds to when it talks to a remote MCP server +the user supplies by URL — tool discovery, authorization discovery, and +deciding whether the URL is an MCP endpoint at all. + +Remote MCP servers are third-party services owned entirely outside this +codebase. This page records only observed, reproducible wire behavior. How the +toolkit composes that behavior into scaffold flows is an internal concern and +belongs in an ADR under [`../adr/`](../adr/README.md). + +All measurements in §1.3–§1.5 were taken 2026-07-27, those in §1.6 on 2026-07-28, +with an unauthenticated `initialize` request; every row was re-verified 2026-07-28 +and all are reproducible with the recipe in §1.7. + +## 1. Facts the toolkit is bound to + +### 1.1 Authorization spec generations + +Two generations are in the wild and a client must handle both. + +| Generation | Discovery chain | 401 challenge | +|---|---|---| +| 2025-06-18 (RFC 9728) | `WWW-Authenticate` carries `resource_metadata="…"` → fetch that document → `authorization_servers[0]` → RFC 8414 / OIDC well-known on the **issuer** | includes `resource_metadata` | +| 2025-03-26 | The MCP server **is** its own authorization server; RFC 8414 metadata sits at the **origin root**. No protected-resource document exists | carries only `realm` / `error` | + +Every production server measured in §1.3 implements 2025-06-18. The only +2025-03-26 server observed is an internal eval server. A client that +implements only the 2025-06-18 chain dead-ends on the latter. + +A challenge is not the only way the first chain starts. A server may publish a +protected-resource document and still never challenge, because it defers +authorization to the individual tool calls rather than to `initialize` (§1.6). +The document's location is not a secret the server has to reveal: RFC 9728 §3.1 +derives it from the resource URL by inserting `/.well-known/oauth-protected-resource` +between the host and the resource path. Discovery driven only by +`resource_metadata` loses this class entirely. + +### 1.2 Transport and method + +- Streamable HTTP is the current transport; SSE (`/sse`) endpoints are still + deployed and reachable. +- An MCP endpoint answers an unauthenticated `initialize` **POST** with either + a JSON-RPC result, or a `401` challenge. It does not answer `404`. +- A `2xx` status alone is not proof. `https://substrate-sdf.office.com/` answers + `200` with HTML to the same `POST` (§1.4). The **JSON-RPC envelope in the body** + is the proof, and it survives the `text/event-stream` framing a streamable-HTTP + server replies with, because `jsonrpc` sits inside the `data:` payload: + + ``` + event: message + data: {"result":{"protocolVersion":"2025-03-26",…},"id":1,"jsonrpc":"2.0"} + ``` + +- Authorization is **not** universal. `https://learn.microsoft.com/api/mcp` + answers `initialize` with `200` and a full result, and serves its three tools, + with no credentials at all (§1.3). +- `GET` on the same URL is **not** a substitute. Two of the servers measured + return `200` with an HTML or JSON page on `GET` at a non-endpoint path while + returning `404` to `POST` at that same path, and one **valid** endpoint + returns `405` to `GET` (§1.3). +- A `4xx` to the `initialize` `POST` does not settle the question on its own. + The transport spec's backwards-compatibility rule has a client read `405` or + `404` there as "this may be a 2024-11-05 HTTP+SSE server" and disambiguate + with a `GET`, which that transport answers `text/event-stream`, opening by + naming where messages go: + + ``` + event: endpoint + data: /messages?sessionId=… + ``` + + No such server appears in these measurements — every `/sse` path probed is + either a streamable-HTTP endpoint or gated by authorization ahead of + transport dispatch (§1.3, §1.4). This is a spec-mandated constraint, not a + measured one, and it is the only known way a **valid** server produces the + `404` that §2.10 blocks on. +- `400` carries no signal about the URL. The spec **requires** a server to + answer `400` when it rejects the `MCP-Protocol-Version`, and Google's servers + answer any unroutable path with `400` and an HTML error page (§1.4). + +### 1.3 Measured behavior — documented endpoint URLs + +| Server | URL | POST | GET | Challenge | +|---|---|---|---|---| +| GitHub Copilot | `https://api.githubcopilot.com/mcp/` | 401 | 401 | `resource_metadata=".../oauth-protected-resource/mcp/"` | +| Monday | `https://mcp.monday.com/sse` | 401 | 401 | `realm="OAuth"`, `resource_metadata=".../oauth-protected-resource/sse"` | +| HubSpot | `https://mcp.hubspot.com/` | 401 | 401 | `resource_metadata=".../oauth-protected-resource"` | +| Notion | `https://mcp.notion.com/mcp` | 401 | 401 | `realm="OAuth"`, `resource_metadata=".../oauth-protected-resource/mcp"` | +| Canva | `https://mcp.canva.com/mcp` | 401 | 401 | `realm="OAuth"`, `resource_metadata=".../oauth-protected-resource/mcp"` | +| Moody's | `https://api.moodys.com/genai-ready-data/m1/mcp` | 401 | 401 | `resource_metadata="https://api.moodys.com/genai-ready-data/.well-known/oauth-protected-resource/m1/mcp"` | +| LSEG | `https://api.analytics.lseg.com/lfa/mcp` | 401 | 401 | `realm="MCP Server"`, `resource_metadata=".../oauth-protected-resource/lfa/mcp"` | +| Microsoft Learn | `https://learn.microsoft.com/api/mcp` | **200**, `text/event-stream`, JSON-RPC result | **405** (HTML) | — (no authorization required) | +| Office SDF | `https://substrate-sdf.office.com/exmigd2sapp/mcp` | 503 | 503 | — (Envoy `upstream connect error`; ring was down) | + +**No valid MCP endpoint returned `404`.** Nine reachable valid endpoints were +measured (the eight above plus the eval server in §1.5); eight answered `401` +and one answered `200`. The Office SDF ring was unreachable and returned `503` +on every path, valid or not. + +The Learn row also shows `405` coming from a **valid** endpoint: the server +exists and rejects the method, answering `GET` with +`"This is an MCP server endpoint and cannot be accessed directly via a browser +or unsupported transports like SSE."` `405` therefore only carries a signal for +the `initialize` `POST`, never for `GET`. + +Note that HubSpot's endpoint **is** the origin root. A bare origin is a +legitimate MCP endpoint URL, so URL *shape* carries no signal. + +### 1.4 Measured behavior — the same URLs with the final segment removed + +This is the mistake being detected: the user pastes the host or a parent path +instead of the endpoint. + +| Truncated URL | POST | GET | Detected by POST 404? | +|---|---|---|---| +| `https://api.githubcopilot.com/` | 404 | 404 | yes | +| `https://mcp.monday.com/` | 404 | 404 | yes | +| `https://mcp.notion.com/` | 404 | 404 | yes | +| `https://mcp.canva.com/` | **404** | **200** (HTML) | yes — `GET` would have cleared it | +| `https://api.analytics.lseg.com/lfa/` | 404 | 404 | yes (Kong `no Route matched`) | +| `https://taskmaster-mcp-server.azurewebsites.net/` | **404** | **200** (JSON) | yes | +| `https://learn.microsoft.com/api/` | **405** | 404 (`API not handled`) | **no** | +| `https://learn.microsoft.com/api` | **403** (Akamai) | 404 | **no** | +| `https://learn.microsoft.com/` | **403** (Akamai) | 200 (HTML) | **no** | +| `https://api.moodys.com/genai-ready-data/m1/` | **401** | 401 | **no** | +| `https://drivemcp.googleapis.com/` | **400** (HTML) | not measured | **no** | +| `https://drivemcp.googleapis.com/mcp/` | **400** (HTML) | not measured | **no** | +| `https://substrate-sdf.office.com/exmigd2sapp/` | 503 | 503 | no (ring down) | +| `https://substrate-sdf.office.com/` | 200 (HTML) | 200 (HTML) | **no** | + +HubSpot has no truncated form — its endpoint is already the origin root. + +A `404`-only rule catches 6 of these 14. Widening the negative signal to the +other shapes the measurements produced — `403`, `405`, and `2xx` without a +JSON-RPC envelope — raises that to 10 of 14 while still misclassifying none of +the nine valid endpoints in §1.3 and §1.5. + +The two `403`s are **not** the application answering: Akamai intercepts ahead +of it and rejects the probe outright. A WAF fronting a *valid* endpoint could +do the same to a request carrying no ordinary browser `User-Agent`, so `403` is +weaker evidence than `404` — it can mean "wrong URL" or "the edge disliked the +request". + +The Moody's row is the significant miss: the truncated URL is answered by the +API gateway's auth layer, which is mounted on the **path prefix**, so it +returns a well-formed `401` carrying a `resource_metadata` pointing at +`.../oauth-protected-resource/m1/`. Authorization discovery therefore +**succeeds** and yields plausible-but-wrong endpoints. No status-code rule can +catch this. + +### 1.5 Reference: a 2025-03-26 server + +`taskmaster-mcp-server.azurewebsites.net` (internal eval server): + +| Path | POST | GET | +|---|---|---| +| `/mcp` | 401, `realm="taskmaster-mcp"`, **no** `resource_metadata` | 401 | +| `/` | 404 | **200** (JSON self-description) | +| `/.well-known/oauth-authorization-server` | — | 200 (RFC 8414 document) | +| `/.well-known/oauth-protected-resource` | — | 404 | + +Its auth middleware is mounted on `/mcp*`, so `/mcp/.well-known/…` returns +`401` rather than `404`. + +### 1.6 Reference: servers that publish metadata but never challenge + +Google's MCP servers authorize the individual tool calls, not `initialize`. +An unauthenticated `initialize` `POST` is answered `200` with a complete +JSON-RPC result and **no** `WWW-Authenticate` header: + +``` +{"id":1,"jsonrpc":"2.0","result":{"capabilities":{…}, + "protocolVersion":"2025-03-26", + "serverInfo":{"name":"StatelessServer","version":"ESF"}}} +``` + +Each nevertheless publishes an RFC 9728 document, reachable only at the §3.1 +insertion location: + +| URL | Status | +|---|---| +| `https://drivemcp.googleapis.com/.well-known/oauth-protected-resource/mcp/v1` | **200** | +| `https://gmailmcp.googleapis.com/.well-known/oauth-protected-resource/mcp/v1` | **200** | +| `https://calendarmcp.googleapis.com/.well-known/oauth-protected-resource/mcp/v1` | **200** | +| `https://drivemcp.googleapis.com/.well-known/oauth-protected-resource` | 404 | +| `https://drivemcp.googleapis.com/mcp/v1/.well-known/oauth-protected-resource` | 404 | +| every `oauth-authorization-server` / `openid-configuration` form on the server host | 404 | + +All three documents name the same issuer: + +``` +{"authorization_servers":["https://accounts.google.com/"], + "resource":"https://drivemcp.googleapis.com/mcp/v1", + "bearer_methods_supported":["header"],"scopes_supported":[…]} +``` + +and `https://accounts.google.com/.well-known/oauth-authorization-server` +returns the endpoints. + +The insertion form is the one that generalizes. Where both were measured, only +it was always present: + +| URL | Status | +|---|---| +| `https://mcp.notion.com/.well-known/oauth-protected-resource/mcp` | 200 | +| `https://mcp.notion.com/.well-known/oauth-protected-resource` | 200 | +| `https://api.githubcopilot.com/.well-known/oauth-protected-resource/mcp` | 200 | +| `https://api.githubcopilot.com/.well-known/oauth-protected-resource` | **404** | + +These three servers also add to the valid-endpoint sample of §1.3 as the only +measured production endpoints that answer `initialize` with neither a challenge +nor a requirement for credentials at that stage. + +The document is served for **any** path prefix on those hosts, echoing the +requested path back in `resource`: + +``` +GET https://drivemcp.googleapis.com/.well-known/oauth-protected-resource/mcp +200 {"authorization_servers":["https://accounts.google.com/"], + "resource":"https://drivemcp.googleapis.com/mcp", …} +``` + +so a wrong-but-plausible Google path still resolves to real endpoints, and RFC +9728 §3.3's `resource` check cannot catch it — the same class of miss as the +Moody's row in §1.4. + +The reverse also occurs, and is caught. `https://api.githubcopilot.com/mcp/sse` +is not an endpoint, yet answers `401` because the auth gate sits ahead of +routing, and it advertises +`resource_metadata=".../oauth-protected-resource/mcp/sse"`. That document URL +itself answers `401`, no derived or well-known candidate resolves, and +discovery fails — so the wrong URL is caught one stage after the probe rather +than by it. + +### 1.7 Reproducing these measurements + +``` +POST +Content-Type: application/json +Accept: application/json, text/event-stream + +{"jsonrpc":"2.0","id":1,"method":"initialize", + "params":{"protocolVersion":"2025-06-18","capabilities":{}, + "clientInfo":{"name":"atk-probe","version":"1.0.0"}}} +``` + +Record the status, the full `WWW-Authenticate` header, and the body. Repeat +with `GET` and with the final path segment removed. + +## 2. Constraints derived from these facts + +1. Authorization discovery must attempt **both** spec generations, and must not + depend on being challenged: the advertised `resource_metadata` chain, then + the RFC 9728 protected-resource document **derived** from the MCP server URL, + and — when no such document exists — RFC 8414 / OIDC well-known candidates + derived from the MCP server URL including its origin root (§1.1, §1.6). +2. Well-known candidates derived from an **issuer** must not include + origin-root forms: for a tenant-scoped issuer the root form can return a + valid-but-wrong-tenant document, which is worse than failing (§1.1). +3. Reachability and endpoint-validity probes must use `POST`; a `GET` result + must not be treated as evidence that a URL is an MCP endpoint (§1.2, §1.4). +4. `404` from an `initialize` `POST` means no MCP endpoint is routed at that + URL — but only once the deprecated HTTP+SSE transport has been ruled out + with the `GET` the spec prescribes, since that transport answers the `POST` + with `404` or `405` (§1.2, §1.3). +5. A non-`404` response must not be treated as proof the URL is correct — a + path-prefix auth gateway answers `401` for parent paths (§1.4). +6. `5xx` and transport failures indicate an unreachable server, not a wrong + URL, and must not be reported as one (§1.3, §1.4). +7. URL shape carries no signal: a bare origin is a legitimate endpoint, and + `/sse` endpoints are still deployed (§1.2, §1.3). +8. A successful `initialize` must be recognized by the **JSON-RPC envelope in + the body**, not by the `2xx` status, and the check must tolerate + `text/event-stream` framing (§1.2). +9. Confirmation that a URL is an MCP endpoint must be tracked separately from + the absence of an authorization challenge: a server needing no + authorization at all is legitimate (§1.2), so "no auth" cannot stand in for + "could not tell" (§1.2, §1.3). +10. A user-supplied MCP server URL must be rejected at input time when an + `initialize` `POST` returns `404`, and only then + ([ADR-0020](../adr/ADR-0020-mcp-server-url-validity.md)). +11. The weaker negative signals — `403`, `405`, and `2xx` without a JSON-RPC + envelope — must warn rather than block, because a `403` can come from a WAF + in front of a valid endpoint (§1.4) + ([ADR-0020](../adr/ADR-0020-mcp-server-url-validity.md)). +12. Accepting a URL must never be presented as confirmation that it serves + tools — §1.4 documents a wrong URL that answers `401` + ([ADR-0020](../adr/ADR-0020-mcp-server-url-validity.md)). +13. `400` must never be treated as a wrong-URL signal: the spec requires it for + a rejected `MCP-Protocol-Version`, so a valid endpoint can produce one + (§1.2, §1.4). + +## 3. Open questions + +- The dynamic-tool-discovery scaffold path performs no tool fetch by design, + so it has no independent check that the URL serves tools. §2.10 and §2.11 + cover the status-code cases; the §1.4 Moody's case remains uncovered, as does + a URL whose host was unreachable at scaffold time. +- Whether a confirmed endpoint should be surfaced to the user as positive + feedback at input time, and through which affordance, is undecided. The + probe now distinguishes the state; nothing renders it. +- When a URL is known-wrong, should dynamic client registration still be + allowed to register an OAuth client against that host during provision? diff --git a/packages/fx-core/resource/package.nls.json b/packages/fx-core/resource/package.nls.json index fb64c782cdc..c68b0ffdf57 100644 --- a/packages/fx-core/resource/package.nls.json +++ b/packages/fx-core/resource/package.nls.json @@ -1126,6 +1126,8 @@ "error.kiotaClient.EmptyResult": "Get empty result when parser OpenAPI description file.", "core.createProjectQuestion.mcpForDa.ServerUrl.title": "MCP Server URL", "core.createProjectQuestion.mcpForDa.ServerUrl.placeholder": "Enter your MCP server URL(e.g. https://example-mcp.com)", + "core.createProjectQuestion.mcpForDa.ServerUrl.notAnMcpEndpoint": "Couldn't reach an MCP server at this URL (returned 404 to the MCP 'initialize' request). Verify the URL, or confirm the server is running.", + "core.createProjectQuestion.mcpForDa.ServerUrl.invalidUrl": "Enter an absolute URL that starts with https:// or http://.", "core.createProjectQuestion.mcpForDa.PreFetchTools.title": "Select Operation(s) Copilot can interact with", "core.createProjectQuestion.mcpForDa.AuthType.title": "Select Authentication Type", "core.createProjectQuestion.mcpForDa.Auth.OAuth": "OAuth (with static registration)", @@ -1165,11 +1167,12 @@ "core.createProjectQuestion.mcpLocalServer.validationHelp": "At least one MCP server must be selected", "core.MCPForDA.updatePluginManifest": "The operations selected from your MCP server are successfully added for Copilot to interact with. You can go to the '%s' to check on details. Now you are able to provision your declarative agent to continue.", "core.MCPForDA.mcpAuthMetadataMissingError": "Unable to find the authentication metadata in the MCP server. Please make sure your MCP server is configured correctly and try again. Details: %s", - "core.MCPForDA.mcpAuthDcrPlaceholderWarning": "Unable to discover the OAuth well-known authorization server URL for MCP server '%s'. The 'dcr/register' action was added to m365agents.yml with a placeholder — replace '' with the correct URL before provisioning.", + "core.MCPForDA.mcpAuthDcrPlaceholderWarning": "Scaffolding succeeded, but the OAuth endpoints couldn't be discovered automatically. Your MCP server is reachable, but it doesn't advertise its OAuth endpoints — ATK checks the server's OAuth metadata ('.well-known/oauth-authorization-server') and found 'none'. Replace the 'wellKnownAuthorizationServer' placeholder in 'm365agents.yml' with the right value before provisioning.", + "core.MCPForDA.mcpAuthOAuthPlaceholderWarning": "Scaffolding succeeded, but the OAuth endpoints couldn't be discovered automatically. Your MCP server is reachable, but it doesn't advertise its OAuth endpoints — ATK checks the server's OAuth metadata ('.well-known/oauth-authorization-server') and found 'none'. Replace the 'authorizationUrl' and 'tokenUrl' placeholders in 'm365agents.yml' with the right values before provisioning.", "core.v4.scaffold.existingFileSkipped": "'%s' exists and was not overwritten. Delete or rename it to rebuild the file.", "core.MCPForDA.mcpAuthMetadataUrlNotFound": "Unable to find the authentication metadata from property \"resource_metadata\" from response of the MCP server.", - "core.MCPForDA.mcpServerMetadataUrlNotFound": "Unable to find the server metadata from property \"authorization_servers\" from repsonse of the Protected Resource Metadata of the MCP server.", - "core.MCPForDA.authUrlNotFound": "Unable to find the authentication URL(s) from response of Authorization Server Metadata of the MCP server.", + "core.MCPForDA.mcpServerMetadataUrlNotFound": "Unable to find the server metadata from property \"authorization_servers\" from response of the Protected Resource Metadata of the MCP server.", + "core.MCPForDA.authUrlNotFound": "Unable to find the authentication URL(s) from response of Authorization Server Metadata of the MCP server. Attempted: %s", "core.MCPForDA.toolsFileNotFound": "MCP tools file not found: %s", "core.MCPForDA.toolsFileInvalidFormat": "Invalid MCP tools file format. Expected %s or a JSON array of tools: %s", @@ -1183,5 +1186,7 @@ "core.MCPForDA.toolsFileReadError": "Failed to read MCP tools file: %s. To add tools later, run:\n %s", "core.MCPForDA.authRequired": "The MCP server at %s requires authentication. Tools could not be fetched automatically. To add tools, run:\n %s", "core.MCPForDA.noToolsFetched": "No tools were discovered from the MCP server at %s. To add tools later, run:\n %s", + "core.MCPForDA.mcpServerUrlNotAnEndpoint": "%s answered an MCP initialize request with HTTP %s, so it may not be an MCP server URL. Verify the URL before provisioning.", + "core.MCPForDA.openYmlFile": "Open m365agents.yml", "core.MCPForDA.fetchError": "Failed to fetch tools from the MCP server at %s. To add tools later, run:\n %s" } diff --git a/packages/fx-core/src/common/mcpToolFetcher.ts b/packages/fx-core/src/common/mcpToolFetcher.ts index 168cb155f85..f73dca9d26f 100644 --- a/packages/fx-core/src/common/mcpToolFetcher.ts +++ b/packages/fx-core/src/common/mcpToolFetcher.ts @@ -3,6 +3,7 @@ import axios from "axios"; import fs from "fs-extra"; +import { Readable } from "stream"; import { getLocalizedString } from "./localizeUtils"; export interface MCPTool { @@ -129,12 +130,122 @@ export async function readMCPToolsFromFile(filePath: string): Promise }); } +/** + * What a probe learned about the URL itself, independently of whether authorization is needed. + * + * - `confirmed`: something MCP-shaped answered — either a successful `initialize` or an OAuth + * challenge. The URL is right. + * - `notEndpoint`: the server answered definitively and the answer was not MCP. The URL is + * most likely wrong. + * - `undetermined`: nothing was learned (server error, timeout, DNS or transport failure). + * Says nothing about the URL and must never be reported as a problem with it. + */ +export type MCPEndpointStatus = "confirmed" | "notEndpoint" | "undetermined"; + +/** HTTP statuses treated as proof that no MCP endpoint is routed at the URL. + * + * Deliberately enumerated rather than "any 4xx": measurements across nine live servers only ever + * produced these three for a wrong URL (404 from routing, 405 from an endpoint that exists but + * rejects the method, 403 from a WAF in front of the app). Statuses such as 429 or 408 are + * transient and would slander a URL that is in fact correct. 400 is excluded on purpose: the + * transport spec requires a server to answer 400 when it rejects the `MCP-Protocol-Version`, + * so a valid endpoint may well produce one for this probe. */ +const NOT_AN_ENDPOINT_STATUSES = [403, 404, 405]; + +/** How long to wait for a deprecated HTTP+SSE server to name its message endpoint. */ +const LEGACY_SSE_TIMEOUT_MS = 5000; + +/** Stop buffering an event stream that is not going to name an endpoint. */ +const LEGACY_SSE_MAX_BYTES = 8192; + +/** + * A 2xx alone proves nothing: a truncated URL on a host that serves a landing page answers 200 + * with HTML. The JSON-RPC envelope in the payload is the actual proof. The body text is searched + * instead of the frames being parsed because that works for both `application/json` and the + * `text/event-stream` framing streamable-HTTP servers use — `jsonrpc` sits inside the `data:` + * payload either way. The quotes keep an HTML page that merely mentions the word from matching. + */ +function carriesJSONRPCEnvelope(body: unknown): boolean { + if (typeof body === "string") { + return body.includes('"jsonrpc"'); + } + if (body && typeof body === "object") { + return "jsonrpc" in body; + } + return false; +} + export interface MCPAuthProbeResult { requiresAuth: boolean; authMetadataUrl?: string; + /** + * Whether the URL was confirmed to be an MCP endpoint. Reported separately from `requiresAuth` + * because it says whether the URL is right, not whether authorization is missing. A mistyped + * URL commonly still serves an ordinary page on GET — the host's landing page, say — which + * makes it look reachable; only the `initialize` POST exposes that nothing MCP is there. + */ + endpointStatus: MCPEndpointStatus; + /** The HTTP status behind a `notEndpoint` verdict. Absent for the other two states. */ + responseStatus?: number; +} + +/** + * Read an event stream only as far as the first `endpoint` event, which is how a 2024-11-05 + * server announces where its messages go. Anything else — the stream ending, an error, a + * stream that just keeps talking, a server that never answers — means no such announcement. + */ +function announcesLegacyMessageEndpoint(stream: Readable): Promise { + return new Promise((resolve) => { + let buffered = ""; + const settle = (found: boolean) => { + clearTimeout(timer); + stream.destroy(); + resolve(found); + }; + const timer = setTimeout(() => settle(false), LEGACY_SSE_TIMEOUT_MS); + stream.on("data", (chunk: Buffer | string) => { + buffered += chunk.toString(); + if (/^event:\s*endpoint\s*$/m.test(buffered)) { + settle(true); + } else if (buffered.length > LEGACY_SSE_MAX_BYTES) { + settle(false); + } + }); + stream.on("end", () => settle(false)); + stream.on("error", () => settle(false)); + }); +} + +/** + * Ask the URL, the way the MCP transport spec's backwards-compatibility rule prescribes, + * whether a deprecated HTTP+SSE server is listening there. + * + * A 2024-11-05 server has no streamable-HTTP endpoint to POST to, so it answers the + * `initialize` POST with a 4xx — the spec names 405 and 404 explicitly. Reading that 4xx as + * proof of a wrong URL would therefore condemn a whole generation of valid servers. The spec's + * own disambiguation is to open an SSE stream with GET: an HTTP+SSE server replies + * `text/event-stream` and names its message endpoint in the first event. + */ +async function servesLegacySSETransport(serverUrl: string): Promise { + try { + const response = await axios.get(serverUrl, { + timeout: LEGACY_SSE_TIMEOUT_MS, + responseType: "stream", + headers: { Accept: "text/event-stream" }, + }); + const contentType = String(response.headers?.["content-type"] ?? ""); + if (!contentType.includes("text/event-stream")) { + response.data?.destroy?.(); + return false; + } + return await announcesLegacyMessageEndpoint(response.data); + } catch { + // A GET that fails says only that the old transport is not there either. + return false; + } } -/** Probe an MCP streamable-HTTP endpoint for an OAuth challenge. */ +/** Probe an MCP streamable-HTTP endpoint for an OAuth challenge and for the URL's validity. */ export async function probeMCPServerAuth(serverUrl: string): Promise { const initializeBody = { jsonrpc: "2.0", @@ -147,16 +258,21 @@ export async function probeMCPServerAuth(serverUrl: string): Promise { + let response; + try { + response = await axios.get(authMetadataUrl); + } catch (error) { + // A server advertising a document it cannot serve is broken, but the MCP server URL may + // still lead to the authorization server, so let the caller try that before failing. + if (canFallBack) { + return []; + } + throw error; + } + const issuers = response.data?.authorization_servers; + if (response.status === 200 && Array.isArray(issuers) && issuers.length > 0) { + return buildWellKnownCandidates(issuers[0]); + } + return []; +} + +/** + * Look for a protected-resource document at the locations RFC 9728 derives from the MCP server + * URL. The first document naming an authorization server wins; a location that is absent only + * means this server publishes at the other one. + */ +async function candidatesFromDerivedProtectedResource(mcpServerUrl: string): Promise { + for (const candidate of buildProtectedResourceCandidates(mcpServerUrl)) { + const candidates = await candidatesFromProtectedResourceMetadata(candidate, true); + if (candidates.length > 0) { + return candidates; + } + } + return []; +} + /** Resolve OAuth endpoints from MCP resource or authorization-server metadata. */ export async function resolveMCPOAuthMetadata( authMetadataUrl?: string, - wellKnownUrl?: string + wellKnownUrl?: string, + mcpServerUrl?: string ): Promise { - let resolvedWellKnownUrl = wellKnownUrl; + let candidates: string[]; + + if (wellKnownUrl) { + // An explicitly configured URL is authoritative — never substitute a guess for it. + candidates = [wellKnownUrl]; + } else { + candidates = authMetadataUrl + ? await candidatesFromProtectedResourceMetadata(authMetadataUrl, !!mcpServerUrl) + : []; - if (!resolvedWellKnownUrl) { - if (!authMetadataUrl) { - throw new Error(getLocalizedString("core.MCPForDA.mcpAuthMetadataUrlNotFound")); + if (candidates.length === 0 && mcpServerUrl) { + // The authoritative answer, when the server publishes one, is its protected-resource + // document — so try that before assuming the server is its own authorization server. + candidates = await candidatesFromDerivedProtectedResource(mcpServerUrl); } - const response = await axios.get(authMetadataUrl); - if ( - response.status === 200 && - response.data && - response.data.authorization_servers && - response.data.authorization_servers.length > 0 - ) { - const mcpServerMetadataUrl = response.data.authorization_servers[0]; - const serverUrl = new URL(mcpServerMetadataUrl); - const serverPath = serverUrl.pathname === "/" ? "" : serverUrl.pathname; - resolvedWellKnownUrl = `${serverUrl.protocol}//${serverUrl.host}/.well-known/oauth-authorization-server${serverPath}`; - } else { - throw new Error(getLocalizedString("core.MCPForDA.mcpServerMetadataUrlNotFound")); + if (candidates.length === 0 && mcpServerUrl) { + candidates = buildMCPServerWellKnownCandidates(mcpServerUrl); } - } - const metadataResponse = await axios.get(resolvedWellKnownUrl); - const authorizationUrl = metadataResponse.data?.authorization_endpoint; - const tokenUrl = metadataResponse.data?.token_endpoint; - const refreshUrl = metadataResponse.data?.refresh_endpoint; + if (candidates.length === 0) { + throw new Error( + getLocalizedString( + authMetadataUrl + ? "core.MCPForDA.mcpServerMetadataUrlNotFound" + : "core.MCPForDA.mcpAuthMetadataUrlNotFound" + ) + ); + } + } - if (!authorizationUrl || !tokenUrl) { - throw new Error(getLocalizedString("core.MCPForDA.authUrlNotFound")); + for (const candidate of candidates) { + try { + const metadataResponse = await axios.get(candidate); + const authorizationUrl = metadataResponse.data?.authorization_endpoint; + const tokenUrl = metadataResponse.data?.token_endpoint; + const refreshUrl = metadataResponse.data?.refresh_endpoint; + if (authorizationUrl && tokenUrl) { + return { authorizationUrl, tokenUrl, refreshUrl, wellKnownUrl: candidate }; + } + } catch { + // A 404 / unreachable candidate just means this provider uses a different + // discovery form; keep probing and report every attempt if all of them fail. + } } - return { authorizationUrl, tokenUrl, refreshUrl, wellKnownUrl: resolvedWellKnownUrl }; + throw new Error(getLocalizedString("core.MCPForDA.authUrlNotFound", candidates.join(", "))); } diff --git a/packages/fx-core/src/component/generator/declarativeAgent/helper.ts b/packages/fx-core/src/component/generator/declarativeAgent/helper.ts index 8562c5266ed..58157072772 100644 --- a/packages/fx-core/src/component/generator/declarativeAgent/helper.ts +++ b/packages/fx-core/src/component/generator/declarativeAgent/helper.ts @@ -728,10 +728,13 @@ export async function generateForMCPForDA( if (injectResult.wellKnownUrlPlaceholderUsed) { warnings.push({ type: "mcpAuthDcrWellKnownUrlPlaceholder", - content: getLocalizedString( - "core.MCPForDA.mcpAuthDcrPlaceholderWarning", - mcpServerUrl - ), + content: getLocalizedString("core.MCPForDA.mcpAuthDcrPlaceholderWarning"), + }); + } + if (injectResult.oauthUrlPlaceholderUsed) { + warnings.push({ + type: "mcpAuthOAuthUrlPlaceholder", + content: getLocalizedString("core.MCPForDA.mcpAuthOAuthPlaceholderWarning"), }); } } @@ -821,6 +824,21 @@ async function generateForMCPForDAWithAuth( if (authProbe.authMetadataUrl) { inputs[QuestionNames.MCPForDAAuthMetadataUrl] = authProbe.authMetadataUrl; } + // This branch never fetches tools, so a mistyped server URL would otherwise leave no + // trace at all: endpoint discovery falls back to the host and happily returns its + // authorization server, and the scaffold looks complete. Warns on every `notEndpoint` + // shape, not just 404 — this is advisory, so it can be broader than the blocking rule + // applied when the URL is first entered. + if (authProbe.endpointStatus === "notEndpoint") { + warnings.push({ + type: "mcpServerUrlNotAnEndpoint", + content: getLocalizedString( + "core.MCPForDA.mcpServerUrlNotAnEndpoint", + mcpServerUrl, + String(authProbe.responseStatus) + ), + }); + } } catch { // Probe failed — continue; endpoint resolution below will best-effort // and yml injection will still run with undefined endpoints. @@ -859,10 +877,13 @@ async function generateForMCPForDAWithAuth( if (injectResult.wellKnownUrlPlaceholderUsed) { warnings.push({ type: "mcpAuthDcrWellKnownUrlPlaceholder", - content: getLocalizedString( - "core.MCPForDA.mcpAuthDcrPlaceholderWarning", - mcpServerUrl - ), + content: getLocalizedString("core.MCPForDA.mcpAuthDcrPlaceholderWarning"), + }); + } + if (injectResult.oauthUrlPlaceholderUsed) { + warnings.push({ + type: "mcpAuthOAuthUrlPlaceholder", + content: getLocalizedString("core.MCPForDA.mcpAuthOAuthPlaceholderWarning"), }); } } diff --git a/packages/fx-core/src/component/generator/openApiSpec/helper.ts b/packages/fx-core/src/component/generator/openApiSpec/helper.ts index d9ff000fa44..310b948e33d 100644 --- a/packages/fx-core/src/component/generator/openApiSpec/helper.ts +++ b/packages/fx-core/src/component/generator/openApiSpec/helper.ts @@ -72,6 +72,7 @@ import { import { manifestUtils } from "../../driver/teamsApp/utils/ManifestUtils"; import { pluginManifestUtils } from "../../driver/teamsApp/utils/PluginManifestUtils"; import { generatePlugin, listAPIInfo, validateOpenAPISpec } from "../../../common/daSpecParser"; +import { isMCPScaffoldWarning } from "../../utils/mcpAuthScaffolder"; import { pathUtils } from "../../utils/pathUtils"; const enum telemetryProperties { @@ -727,10 +728,19 @@ export async function generateScaffoldingSummary( }); } + // MCP scaffolding warnings are not spec-parser warnings, but they report a project that + // cannot provision until the developer edits it (unresolved auth placeholders, tools that + // could not be fetched), so they must reach the summary too. Declarative-agent projects + // route through this function, which would otherwise drop them silently. + const mcpWarningMessage = warnings.filter(isMCPScaffoldWarning).map((warn) => { + return `${SummaryConstant.NotExecuted} ${warn.content}`; + }); + if ( apiSpecWarningMessage.length || manifestWarningMessage.length || - pluginWarningMessage.length + pluginWarningMessage.length || + mcpWarningMessage.length ) { let details = ""; if (apiSpecWarningMessage.length) { @@ -745,6 +755,10 @@ export async function generateScaffoldingSummary( details += EOL + pluginWarningMessage.join(EOL); } + if (mcpWarningMessage.length) { + details += EOL + mcpWarningMessage.join(EOL); + } + return getLocalizedString("core.copilotPlugin.scaffold.summary", details); } else { return ""; diff --git a/packages/fx-core/src/component/utils/mcpAuthScaffolder.ts b/packages/fx-core/src/component/utils/mcpAuthScaffolder.ts index 218454f0304..eb0261553c7 100644 --- a/packages/fx-core/src/component/utils/mcpAuthScaffolder.ts +++ b/packages/fx-core/src/component/utils/mcpAuthScaffolder.ts @@ -69,7 +69,8 @@ export async function resolveMCPAuthEndpoints( } const metadata = await mcpAuthScaffolderDeps.resolveMCPOAuthMetadata( inputs[QuestionNames.MCPForDAAuthMetadataUrl], - inputs[QuestionNames.MCPForDAAuthWellKnownUrl] + inputs[QuestionNames.MCPForDAAuthWellKnownUrl], + inputs[QuestionNames.MCPForDAServerUrl] ); return { authorizationUrl: metadata.authorizationUrl, @@ -88,10 +89,34 @@ export async function resolveMCPAuthEndpoints( export const MCP_DCR_WELL_KNOWN_URL_PLACEHOLDER = ""; +/** + * Placeholders written to `oauth/register` when endpoint discovery can't produce the + * authorization / token URLs for a static `oauth` (`identityProvider: Custom`) action. + * Omitting the fields instead would hide the gap entirely — the action looks complete but + * can never provision — so the placeholders make the missing values visible and editable, + * mirroring the `dcr/register` contract above. + */ +export const MCP_OAUTH_AUTHORIZATION_URL_PLACEHOLDER = ""; +export const MCP_OAUTH_TOKEN_URL_PLACEHOLDER = ""; + +/** + * Every MCP scaffolding warning type is camelCased with this prefix (`mcpAuthRequired`, + * `mcpNoToolsFetched`, `mcpAuthDcrWellKnownUrlPlaceholder`, ...), while spec-parser warning + * types are kebab-cased (`operationid-missing`, `generate-card-failed`, ...). Surfaces use + * this predicate to let MCP warnings through the scaffolding summary without also leaking + * spec-parser warnings that the API-plugin flows deliberately suppress. + */ +export function isMCPScaffoldWarning(warning: { type: string }): boolean { + return warning.type.startsWith("mcp"); +} + export interface InjectMCPAuthActionResult { /** True when `oauth-dynamic` was injected with the placeholder URL because * `endpoints.wellKnownUrl` was missing. */ wellKnownUrlPlaceholderUsed?: boolean; + /** True when `oauth` was injected with placeholder authorization / token URLs + * because endpoint discovery didn't return them. */ + oauthUrlPlaceholderUsed?: boolean; } /** @@ -157,18 +182,25 @@ export async function injectMCPAuthActionToYml(args: { }; } } + // Only static `oauth` emits these endpoints; `entra-sso` resolves them from Entra. + const oauthUrlPlaceholderUsed = + args.authType === "oauth" && (!args.endpoints.authorizationUrl || !args.endpoints.tokenUrl); await ActionInjector.injectCreateOAuthActionForMCP( args.ymlPath, args.authType, args.authName, args.registrationId, args.mcpServerUrl, - args.endpoints.authorizationUrl, - args.endpoints.tokenUrl, + args.authType === "oauth" + ? (args.endpoints.authorizationUrl ?? MCP_OAUTH_AUTHORIZATION_URL_PLACEHOLDER) + : args.endpoints.authorizationUrl, + args.authType === "oauth" + ? (args.endpoints.tokenUrl ?? MCP_OAUTH_TOKEN_URL_PLACEHOLDER) + : args.endpoints.tokenUrl, args.endpoints.refreshUrl, credentialEnvNames ); - return {}; + return oauthUrlPlaceholderUsed ? { oauthUrlPlaceholderUsed: true } : {}; } /** diff --git a/packages/fx-core/src/core/FxCore.declarativeAgent.ts b/packages/fx-core/src/core/FxCore.declarativeAgent.ts index ad584275d93..21f391aba51 100644 --- a/packages/fx-core/src/core/FxCore.declarativeAgent.ts +++ b/packages/fx-core/src/core/FxCore.declarativeAgent.ts @@ -349,7 +349,7 @@ export class FxCoreDeclarativeAgentPart { if (mcpAuth === "OAuthPluginVault" && !!registrationId) { // insert oauth info in teamsapp.yaml - await injectMCPAuthActionToYml({ + const injectResult = await injectMCPAuthActionToYml({ ymlPath: pathUtils.getYmlFilePath(projectPath) as string, authType, authName: serverName, @@ -357,6 +357,22 @@ export class FxCoreDeclarativeAgentPart { mcpServerUrl, endpoints: mcpAuthEndpoints, }); + // This flow has no scaffolding summary to fall back on, so surface the placeholders + // directly — otherwise provision fails later with an opaque error. + if (injectResult.wellKnownUrlPlaceholderUsed) { + void context.userInteraction.showMessage( + "warn", + getLocalizedString("core.MCPForDA.mcpAuthDcrPlaceholderWarning"), + false + ); + } + if (injectResult.oauthUrlPlaceholderUsed) { + void context.userInteraction.showMessage( + "warn", + getLocalizedString("core.MCPForDA.mcpAuthOAuthPlaceholderWarning"), + false + ); + } } void context.userInteraction .showMessage( @@ -750,6 +766,20 @@ export class FxCoreDeclarativeAgentPart { if (authProbe.authMetadataUrl) { inputs[QuestionNames.MCPForDAAuthMetadataUrl] = authProbe.authMetadataUrl; } + // Same advisory warning the create flow raises on its DT branch: this branch + // never fetches tools, so a URL that is not an MCP endpoint would otherwise + // leave no trace — endpoint discovery falls back to the host and the action + // looks complete. + if (authProbe.endpointStatus === "notEndpoint") { + mcpWarnings.push({ + type: "mcpServerUrlNotAnEndpoint", + content: getLocalizedString( + "core.MCPForDA.mcpServerUrlNotAnEndpoint", + mcpServerUrl, + String(authProbe.responseStatus) + ), + }); + } } catch { // best-effort probe; endpoint resolution below tolerates undefined } @@ -773,7 +803,7 @@ export class FxCoreDeclarativeAgentPart { try { const ymlPath = pathUtils.getYmlFilePath(inputs.projectPath); if (ymlPath) { - await injectMCPAuthActionToYml({ + const injectResult = await injectMCPAuthActionToYml({ ymlPath, authType, authName: namespace, @@ -784,6 +814,18 @@ export class FxCoreDeclarativeAgentPart { serverName, scopes: inputs[QuestionNames.MCPForDAScopes], }); + if (injectResult.wellKnownUrlPlaceholderUsed) { + mcpWarnings.push({ + type: "mcpAuthDcrWellKnownUrlPlaceholder", + content: getLocalizedString("core.MCPForDA.mcpAuthDcrPlaceholderWarning"), + }); + } + if (injectResult.oauthUrlPlaceholderUsed) { + mcpWarnings.push({ + type: "mcpAuthOAuthUrlPlaceholder", + content: getLocalizedString("core.MCPForDA.mcpAuthOAuthPlaceholderWarning"), + }); + } } } catch (error: any) { mcpWarnings.push({ @@ -1005,7 +1047,7 @@ export class FxCoreDeclarativeAgentPart { const endpoints = await resolveMCPAuthEndpoints(authType, inputs); const ymlPath = pathUtils.getYmlFilePath(inputs.projectPath); if (ymlPath) { - await injectMCPAuthActionToYml({ + const injectResult = await injectMCPAuthActionToYml({ ymlPath, authType, authName: actionId, @@ -1013,6 +1055,18 @@ export class FxCoreDeclarativeAgentPart { mcpServerUrl, endpoints, }); + if (injectResult.wellKnownUrlPlaceholderUsed) { + mcpWarnings.push({ + type: "mcpAuthDcrWellKnownUrlPlaceholder", + content: getLocalizedString("core.MCPForDA.mcpAuthDcrPlaceholderWarning"), + }); + } + if (injectResult.oauthUrlPlaceholderUsed) { + mcpWarnings.push({ + type: "mcpAuthOAuthUrlPlaceholder", + content: getLocalizedString("core.MCPForDA.mcpAuthOAuthPlaceholderWarning"), + }); + } } } catch (error: any) { mcpWarnings.push({ @@ -1045,6 +1099,26 @@ export class FxCoreDeclarativeAgentPart { } } } + + // An unreplaced placeholder guarantees a provision failure later, and the output channel + // is easy to miss — more so here, where the action is about to report success. Raise this + // one case to a notification, as the create flow does. + const placeholderWarning = mcpWarnings.find( + (warning) => + warning.type === "mcpAuthDcrWellKnownUrlPlaceholder" || + warning.type === "mcpAuthOAuthUrlPlaceholder" + ); + if (placeholderWarning && inputs.platform === Platform.VSCode) { + const openYml = getLocalizedString("core.MCPForDA.openYmlFile"); + const ymlPath = pathUtils.getYmlFilePath(inputs.projectPath); + void context.userInteraction + .showMessage("warn", placeholderWarning.content, false, openYml) + .then((userRes) => { + if (userRes.isOk() && userRes.value === openYml && ymlPath) { + void TOOLS?.ui?.openFile?.(ymlPath); + } + }); + } } else { const addPluginRes = await declarativeAgentHelperModule.addExistingPlugin( declarativeCopilotManifestPath, diff --git a/packages/fx-core/src/index.ts b/packages/fx-core/src/index.ts index 29c7c947e17..ea3bd75ba8c 100644 --- a/packages/fx-core/src/index.ts +++ b/packages/fx-core/src/index.ts @@ -113,7 +113,11 @@ export * from "./component/middleware/actionExecutionMW"; export { outputScaffoldingWarningMessage } from "./component/utils/common"; export { DotenvOutput, envUtil } from "./component/utils/envUtil"; export { metadataUtil } from "./component/utils/metadataUtil"; -export { MCPAuthProbeResult, probeMCPServerAuth } from "./component/utils/mcpToolFetcher"; +export { + MCPAuthProbeResult, + MCPEndpointStatus, + probeMCPServerAuth, +} from "./component/utils/mcpToolFetcher"; export { ODRTool, ODRServer, ODRProvider } from "./component/utils/odrProvider"; export { pathUtils } from "./component/utils/pathUtils"; export { newResourceGroupOption, resourceGroupHelper } from "./component/utils/ResourceGroupHelper"; diff --git a/packages/fx-core/src/question/other.ts b/packages/fx-core/src/question/other.ts index 6d4a8361c55..f52919c55bd 100644 --- a/packages/fx-core/src/question/other.ts +++ b/packages/fx-core/src/question/other.ts @@ -65,6 +65,7 @@ import { inputOrSearchAPISpecNode } from "./scaffold/commonNodes"; import { MCPForDAAuthCredentialNodes, MCPForDAAuthTypeStaticOptions, + validateMCPServerUrl, } from "./scaffold/vsc/teamsProjectTypeNode"; export function convertAadToNewSchemaQuestionNode(): IQTreeNode { @@ -717,6 +718,14 @@ export function addPluginQuestionNode(): IQTreeNode { placeholder: getLocalizedString( "core.createProjectQuestion.mcpForDa.ServerUrl.placeholder" ), + // Same rejection rule as the create flow — a URL that is not an MCP endpoint is + // no more usable when added to an existing project than when creating a new one. + additionalValidationOnAccept: { + validFunc: async (value: string): Promise => { + if (!value) return undefined; + return await validateMCPServerUrl(value); + }, + }, }, children: [ // MCP tools file input (CLI only — VS Code DT-on uses dynamic discovery) diff --git a/packages/fx-core/src/question/scaffold/vsc/teamsProjectTypeNode.ts b/packages/fx-core/src/question/scaffold/vsc/teamsProjectTypeNode.ts index 72000b31a9a..88e11d48ec0 100644 --- a/packages/fx-core/src/question/scaffold/vsc/teamsProjectTypeNode.ts +++ b/packages/fx-core/src/question/scaffold/vsc/teamsProjectTypeNode.ts @@ -14,7 +14,11 @@ import * as fs from "fs-extra"; import path from "path"; import { featureFlagManager, FeatureFlags } from "../../../common/featureFlags"; import { getLocalizedString } from "../../../common/localizeUtils"; -import { fetchMCPTools, readMCPToolsFromFile } from "../../../component/utils/mcpToolFetcher"; +import { + fetchMCPTools, + readMCPToolsFromFile, + probeMCPServerAuth, +} from "../../../component/utils/mcpToolFetcher"; import { ODRProvider, ODRServer } from "../../../component/utils/odrProvider"; import { SPFxFrameworkQuestion, @@ -44,6 +48,7 @@ export const teamsProjectTypeDeps = { readJSON: fs.readJSON, fetchMCPTools, readMCPToolsFromFile, + probeMCPServerAuth, }; /** @@ -612,6 +617,39 @@ export function MCPForDAAuthCredentialNodes(): IQTreeNode[] { return [clientIdNode, clientSecretNode, scopesNode]; } +/** + * Reject an MCP server URL that cannot be one. + * + * A value with no scheme never reaches the network: axios cannot build a request from it, so + * the probe throws, reports no status, and would be classified `undetermined` — letting a + * plainly malformed URL through. Rule it out syntactically first. + * + * ADR-0020: of the answers a server actually gives, block only on 404, the one no valid MCP + * endpoint was ever measured giving. The other `notEndpoint` shapes (403, 405, 200 without a + * JSON-RPC envelope) only warn after scaffolding, because a 403 can come from a WAF sitting in + * front of a perfectly good endpoint and wrongly blocking a legitimate URL is worse than + * missing a wrong one. `undetermined` — 5xx, timeouts, unreachable hosts — never blocks, so + * scaffolding does not fail closed because the network did. + * + * Shared by the create flow and the "add MCP server" action flow so both reject the same URLs. + */ +export async function validateMCPServerUrl(value: string): Promise { + let parsed: URL; + try { + parsed = new URL(value); + } catch { + return getLocalizedString("core.createProjectQuestion.mcpForDa.ServerUrl.invalidUrl"); + } + if (parsed.protocol !== "https:" && parsed.protocol !== "http:") { + return getLocalizedString("core.createProjectQuestion.mcpForDa.ServerUrl.invalidUrl"); + } + const probe = await teamsProjectTypeDeps.probeMCPServerAuth(value); + if (probe.endpointStatus === "notEndpoint" && probe.responseStatus === 404) { + return getLocalizedString("core.createProjectQuestion.mcpForDa.ServerUrl.notAnMcpEndpoint"); + } + return undefined; +} + export function MCPForDAServerUrlNode(): IQTreeNode { return { condition: { equals: ActionStartOptions.mcp().id }, @@ -623,6 +661,10 @@ export function MCPForDAServerUrlNode(): IQTreeNode { additionalValidationOnAccept: { validFunc: async (value: string, inputs?: Inputs): Promise => { if (!value || !inputs) return undefined; + const rejection = await validateMCPServerUrl(value); + if (rejection) { + return rejection; + } // For CLI: attempt to auto-fetch tools from the server if (inputs.platform !== Platform.VSCode) { try { diff --git a/packages/fx-core/src/v4/mcp/mcpAuthAction.ts b/packages/fx-core/src/v4/mcp/mcpAuthAction.ts index 5eed7cbc265..e2cdcad3643 100644 --- a/packages/fx-core/src/v4/mcp/mcpAuthAction.ts +++ b/packages/fx-core/src/v4/mcp/mcpAuthAction.ts @@ -12,6 +12,14 @@ const SUPPORTED_AUTH_TYPES = new Set(["none", "oauth", "entra-sso", "oauth-dynam export const MCP_DCR_WELL_KNOWN_URL_PLACEHOLDER = ""; +/** + * Placeholders for a static `oauth` action whose authorization / token URLs could not be + * discovered. `identityProvider: Custom` is unusable without both, and omitting the fields + * hides the gap; the placeholders make it visible and editable. + */ +export const MCP_OAUTH_AUTHORIZATION_URL_PLACEHOLDER = ""; +export const MCP_OAUTH_TOKEN_URL_PLACEHOLDER = ""; + export interface ResolvedMCPAuthEndpoints { authorizationUrl?: string; tokenUrl?: string; @@ -35,6 +43,7 @@ export interface MCPAuthActionArgs { export interface MCPAuthActionResult { yaml: string; wellKnownUrlPlaceholderUsed: boolean; + oauthUrlPlaceholderUsed: boolean; } function failure(message: string): Result { @@ -102,14 +111,13 @@ function ensureMinimumVersion(current: unknown, minimum: string): string | undef function oauthAction(args: MCPAuthActionArgs, appIdEnvName: string): Record { const endpointFields: Record = {}; - if (args.endpoints.authorizationUrl) { - endpointFields.authorizationUrl = args.endpoints.authorizationUrl; - } - if (args.endpoints.tokenUrl) { - endpointFields.tokenUrl = args.endpoints.tokenUrl; - } - if (args.endpoints.refreshUrl) { - endpointFields.refreshUrl = args.endpoints.refreshUrl; + if (args.authType === "oauth") { + endpointFields.authorizationUrl = + args.endpoints.authorizationUrl ?? MCP_OAUTH_AUTHORIZATION_URL_PLACEHOLDER; + endpointFields.tokenUrl = args.endpoints.tokenUrl ?? MCP_OAUTH_TOKEN_URL_PLACEHOLDER; + if (args.endpoints.refreshUrl) { + endpointFields.refreshUrl = args.endpoints.refreshUrl; + } } return { uses: "oauth/register", @@ -165,7 +173,7 @@ export function injectMcpAuthActionYaml( return failure(`Unsupported MCP auth type '${args.authType}'.`); } if (args.authType === "none") { - return ok({ yaml, wellKnownUrlPlaceholderUsed: false }); + return ok({ yaml, wellKnownUrlPlaceholderUsed: false, oauthUrlPlaceholderUsed: false }); } const document = parseDocument(yaml); @@ -182,7 +190,11 @@ export function injectMcpAuthActionYaml( (item) => uses(item) === actionUses && registrationId(item) === args.registrationId ); if (alreadyRegistered) { - return ok({ yaml: document.toString(), wellKnownUrlPlaceholderUsed: false }); + return ok({ + yaml: document.toString(), + wellKnownUrlPlaceholderUsed: false, + oauthUrlPlaceholderUsed: false, + }); } provision.items = provision.items.filter((item) => uses(item) !== undefined); @@ -193,6 +205,8 @@ export function injectMcpAuthActionYaml( } const placeholderUsed = args.authType === "oauth-dynamic" && !args.endpoints.wellKnownUrl; + const oauthUrlPlaceholderUsed = + args.authType === "oauth" && (!args.endpoints.authorizationUrl || !args.endpoints.tokenUrl); const action = args.authType === "oauth-dynamic" ? dcrAction( @@ -210,5 +224,9 @@ export function injectMcpAuthActionYaml( } } - return ok({ yaml: document.toString(), wellKnownUrlPlaceholderUsed: placeholderUsed }); + return ok({ + yaml: document.toString(), + wellKnownUrlPlaceholderUsed: placeholderUsed, + oauthUrlPlaceholderUsed, + }); } diff --git a/packages/fx-core/src/v4/mcp/mcpAuthScaffold.ts b/packages/fx-core/src/v4/mcp/mcpAuthScaffold.ts index ba0de43f490..81d4741946f 100644 --- a/packages/fx-core/src/v4/mcp/mcpAuthScaffold.ts +++ b/packages/fx-core/src/v4/mcp/mcpAuthScaffold.ts @@ -76,7 +76,8 @@ async function resolveEndpoints( const probe = await mcpAuthScaffoldDeps.probeMCPServerAuth(mcpServerUrl); const metadata = await mcpAuthScaffoldDeps.resolveMCPOAuthMetadata( probe.authMetadataUrl, - undefined + undefined, + mcpServerUrl ); return { authorizationUrl: metadata.authorizationUrl, @@ -140,7 +141,10 @@ export async function injectMcpAuthAction( return err(injectResult.error); } if (injectResult.value.wellKnownUrlPlaceholderUsed) { - ctx.warn?.(getLocalizedString("core.MCPForDA.mcpAuthDcrPlaceholderWarning", args.mcpServerUrl)); + ctx.warn?.(getLocalizedString("core.MCPForDA.mcpAuthDcrPlaceholderWarning")); + } + if (injectResult.value.oauthUrlPlaceholderUsed) { + ctx.warn?.(getLocalizedString("core.MCPForDA.mcpAuthOAuthPlaceholderWarning")); } ctx.write(args.ymlPath, Buffer.from(injectResult.value.yaml, "utf8")); return ok(undefined); diff --git a/packages/fx-core/tests/component/generator/declarativeAgentGenerator.test.ts b/packages/fx-core/tests/component/generator/declarativeAgentGenerator.test.ts index aa462ea7302..e5ad61ddcf5 100644 --- a/packages/fx-core/tests/component/generator/declarativeAgentGenerator.test.ts +++ b/packages/fx-core/tests/component/generator/declarativeAgentGenerator.test.ts @@ -2003,6 +2003,127 @@ describe("helper", async () => { ); }); + it("DT auth: warns when the MCP server url returns 404", async () => { + // The DT path never fetches tools, so this warning is the only thing standing between a + // mistyped url and a scaffold that looks complete but points nowhere. + vi.spyOn(featureFlagManager, "getBooleanValue").mockReturnValue(true); + vi.spyOn(fs, "pathExists").mockResolvedValue(true); + vi.spyOn(fs, "readJSON").mockResolvedValue({ + schema_version: "v1", + functions: [], + runtimes: [], + }); + vi.spyOn(fs, "writeJSON").mockResolvedValue(); + vi.spyOn(fs, "readFile").mockResolvedValue( + "provision:\n - uses: teamsApp/create\n writeToEnvironmentFile:\n teamsAppId: TEAMS_APP_ID\n" as any + ); + vi.spyOn(fs, "writeFile").mockResolvedValue(); + + vi.spyOn(mcpToolFetcher, "probeMCPServerAuth").mockResolvedValue({ + requiresAuth: false, + endpointStatus: "notEndpoint", + responseStatus: 404, + }); + vi.spyOn( + mcpAuthScaffolderDeps.mcpAuthScaffolderDeps, + "resolveMCPOAuthMetadata" + ).mockResolvedValue({ + authorizationUrl: "https://taskmaster.example.com/oauth/authorize", + tokenUrl: "https://taskmaster.example.com/oauth/token", + refreshUrl: "https://taskmaster.example.com/oauth/token", + wellKnownUrl: "https://taskmaster.example.com/.well-known/oauth-authorization-server", + }); + + const inputs: Inputs = { + platform: Platform.VSCode, + // Missing the trailing /mcp: the host answers GET, so only the probe exposes it. + [QuestionNames.MCPForDAServerUrl]: "https://taskmaster.example.com", + [QuestionNames.MCPForDAAuthType]: "oauth", + }; + + const res = await generatorHelper.generateForMCPForDA(testDestinationPath, inputs); + + assert.isTrue(res.isOk()); + if (res.isOk()) { + const warning = res.value.warnings?.find((w) => w.type === "mcpServerUrlNotAnEndpoint"); + assert.isDefined(warning); + assert.include(warning!.content, "https://taskmaster.example.com"); + assert.include(warning!.content, "404"); + } + }); + + it("DT auth: warns for a non-404 answer that is still not MCP", async () => { + // The advisory warning is broader than the blocking rule applied when the url is first + // entered: a 403 from a WAF is not certain enough to reject the url, but it is worth + // saying out loud once the scaffold exists. + vi.spyOn(featureFlagManager, "getBooleanValue").mockReturnValue(true); + vi.spyOn(fs, "pathExists").mockResolvedValue(true); + vi.spyOn(fs, "readJSON").mockResolvedValue({ + schema_version: "v1", + functions: [], + runtimes: [], + }); + vi.spyOn(fs, "writeJSON").mockResolvedValue(); + vi.spyOn(fs, "readFile").mockResolvedValue( + "provision:\n - uses: teamsApp/create\n writeToEnvironmentFile:\n teamsAppId: TEAMS_APP_ID\n" as any + ); + vi.spyOn(fs, "writeFile").mockResolvedValue(); + + vi.spyOn(mcpToolFetcher, "probeMCPServerAuth").mockResolvedValue({ + requiresAuth: false, + endpointStatus: "notEndpoint", + responseStatus: 403, + }); + + const inputs: Inputs = { + platform: Platform.VSCode, + [QuestionNames.MCPForDAServerUrl]: "https://waf.example.com/api", + [QuestionNames.MCPForDAAuthType]: "oauth", + }; + + const res = await generatorHelper.generateForMCPForDA(testDestinationPath, inputs); + + assert.isTrue(res.isOk()); + if (res.isOk()) { + assert.isDefined(res.value.warnings?.find((w) => w.type === "mcpServerUrlNotAnEndpoint")); + } + }); + + it("DT auth: stays quiet when the probe learned nothing about the url", async () => { + // A timeout or 5xx says the network misbehaved, not that the url is wrong. Warning here + // would train developers to ignore the message. + vi.spyOn(featureFlagManager, "getBooleanValue").mockReturnValue(true); + vi.spyOn(fs, "pathExists").mockResolvedValue(true); + vi.spyOn(fs, "readJSON").mockResolvedValue({ + schema_version: "v1", + functions: [], + runtimes: [], + }); + vi.spyOn(fs, "writeJSON").mockResolvedValue(); + vi.spyOn(fs, "readFile").mockResolvedValue( + "provision:\n - uses: teamsApp/create\n writeToEnvironmentFile:\n teamsAppId: TEAMS_APP_ID\n" as any + ); + vi.spyOn(fs, "writeFile").mockResolvedValue(); + + vi.spyOn(mcpToolFetcher, "probeMCPServerAuth").mockResolvedValue({ + requiresAuth: false, + endpointStatus: "undetermined", + }); + + const inputs: Inputs = { + platform: Platform.VSCode, + [QuestionNames.MCPForDAServerUrl]: "https://down.example.com/mcp", + [QuestionNames.MCPForDAAuthType]: "oauth", + }; + + const res = await generatorHelper.generateForMCPForDA(testDestinationPath, inputs); + + assert.isTrue(res.isOk()); + if (res.isOk()) { + assert.isUndefined(res.value.warnings?.find((w) => w.type === "mcpServerUrlNotAnEndpoint")); + } + }); + it("flag OFF with auth type uses legacy static path (no DT)", async () => { // With TEAMSFX_MCP_FOR_DA_DT off, the dispatcher must keep the legacy // static-tools behavior even when an auth-type answer is present: @@ -2179,6 +2300,93 @@ describe("helper", async () => { ); } }); + + it("DT auth: oauth url placeholders used adds warning", async () => { + vi.spyOn(featureFlagManager, "getBooleanValue").mockReturnValue(true); + vi.spyOn(fs, "pathExists").mockResolvedValue(true); + vi.spyOn(fs, "readJSON").mockResolvedValue({ + schema_version: "v1", + functions: [], + runtimes: [], + }); + vi.spyOn(fs, "writeJSON").mockResolvedValue(); + vi.spyOn(fs, "readFile").mockResolvedValue( + "provision:\n - uses: teamsApp/create\n writeToEnvironmentFile:\n teamsAppId: TEAMS_APP_ID\n" as any + ); + vi.spyOn(fs, "writeFile").mockResolvedValue(); + vi.spyOn(fs, "pathExistsSync").mockReturnValue(true); + + // Discovery produced neither endpoint, so `oauth/register` is written with fill-in + // placeholders and the developer has to be told before provision fails on them. + vi.spyOn( + mcpAuthScaffolderDeps.mcpAuthScaffolderDeps, + "resolveMCPOAuthMetadata" + ).mockResolvedValue({}); + + const inputs: Inputs = { + platform: Platform.VSCode, + [QuestionNames.MCPForDAServerUrl]: "https://secure.example.com/mcp", + [QuestionNames.MCPForDAAuthType]: "oauth", + [QuestionNames.MCPForDAAuthMetadataUrl]: + "https://auth.example.com/.well-known/oauth-authorization-server", + }; + + const res = await generatorHelper.generateForMCPForDA(testDestinationPath, inputs); + + assert.isTrue(res.isOk()); + if (res.isOk()) { + assert.isTrue(res.value.warnings!.some((w) => w.type === "mcpAuthOAuthUrlPlaceholder")); + } + }); + + it("static tools path: oauth url placeholders used adds warning", async () => { + // Same gap on the legacy (DT flag off) path, which injects the action from a different + // call site and would otherwise leave the placeholders unannounced. + vi.spyOn(featureFlagManager, "getBooleanValue").mockReturnValue(false); + vi.spyOn(fs, "pathExists").mockResolvedValue(true); + vi.spyOn(fs, "readJSON").mockResolvedValue({ + schema_version: "v1", + functions: [], + runtimes: [], + }); + vi.spyOn(fs, "writeJSON").mockResolvedValue(); + vi.spyOn(fs, "readFile").mockResolvedValue( + "provision:\n - uses: teamsApp/create\n writeToEnvironmentFile:\n teamsAppId: TEAMS_APP_ID\n" as any + ); + vi.spyOn(fs, "writeFile").mockResolvedValue(); + vi.spyOn(fs, "pathExistsSync").mockReturnValue(true); + + vi.spyOn( + mcpAuthScaffolderDeps.mcpAuthScaffolderDeps, + "resolveMCPOAuthMetadata" + ).mockResolvedValue({}); + + const inputs: Inputs = { + platform: Platform.CLI, + [QuestionNames.MCPForDAServerUrl]: "https://secure.example.com/mcp", + [QuestionNames.MCPForDAServerName]: "testServer", + [QuestionNames.MCPForDAAvailableTools]: [ + { + name: "testServer_secureTool", + description: "Secure tool", + inputSchema: { type: "object" }, + tags: [], + }, + ], + [QuestionNames.MCPForDAPreFetchTools]: ["testServer_secureTool"], + [QuestionNames.MCPForDAAuth]: "OAuthPluginVault", + [QuestionNames.MCPForDAAuthType]: "oauth", + [QuestionNames.MCPForDAAuthMetadataUrl]: + "https://auth.example.com/.well-known/oauth-authorization-server", + }; + + const res = await generatorHelper.generateForMCPForDA(testDestinationPath, inputs); + + assert.isTrue(res.isOk()); + if (res.isOk()) { + assert.isTrue(res.value.warnings!.some((w) => w.type === "mcpAuthOAuthUrlPlaceholder")); + } + }); }); describe("generator.post MCPForDA branch", () => { diff --git a/packages/fx-core/tests/component/generator/openApiSpec/helper.test.ts b/packages/fx-core/tests/component/generator/openApiSpec/helper.test.ts index 4220cebdf57..7881dbc9bf5 100644 --- a/packages/fx-core/tests/component/generator/openApiSpec/helper.test.ts +++ b/packages/fx-core/tests/component/generator/openApiSpec/helper.test.ts @@ -89,6 +89,29 @@ describe("generateScaffoldingSummary", async () => { assert.equal(res.length, 0); }); + it("surfaces MCP scaffolding warnings but not suppressed spec-parser warnings", async () => { + vi.spyOn(fs, "existsSync").mockReturnValue(true); + const res = await generateScaffoldingSummary( + [ + { + type: "mcpAuthOAuthUrlPlaceholder", + content: "fill in the oauth urls", + }, + { + type: WarningType.GenerateJsonDataFailed, + content: "should stay hidden", + }, + ], + teamsManifest, + "path", + undefined, + "" + ); + + assert.isTrue(res.includes("fill in the oauth urls")); + assert.isFalse(res.includes("should stay hidden")); + }); + it("warnings about missing property", async () => { const res = await generateScaffoldingSummary( [], diff --git a/packages/fx-core/tests/component/generator/v4TemplateBridge.test.ts b/packages/fx-core/tests/component/generator/v4TemplateBridge.test.ts index 7b983cbf321..ecc3615c8af 100644 --- a/packages/fx-core/tests/component/generator/v4TemplateBridge.test.ts +++ b/packages/fx-core/tests/component/generator/v4TemplateBridge.test.ts @@ -603,8 +603,11 @@ describe("v4TemplateBridge.scaffoldDeclarativeFromV4Channel", () => { {} ); - assert.equal(warning.mock.calls.length, 1); + assert.equal(warning.mock.calls.length, 2); assert.include(warning.mock.calls[0][0], "metadata unavailable"); + // the action is still injected, with placeholders the developer must replace + assert.include(warning.mock.calls[1][0], "authorizationUrl"); + assert.include(warning.mock.calls[1][0], "tokenUrl"); assert.include( (await fs.readFile(path.join(tmpDir, "m365agents.yml"))).toString(), "oauth/register" diff --git a/packages/fx-core/tests/component/utils/mcpAuthScaffolder.test.ts b/packages/fx-core/tests/component/utils/mcpAuthScaffolder.test.ts index d8295c228af..e555897d7a6 100644 --- a/packages/fx-core/tests/component/utils/mcpAuthScaffolder.test.ts +++ b/packages/fx-core/tests/component/utils/mcpAuthScaffolder.test.ts @@ -9,7 +9,10 @@ import { envUtil } from "../../../src/component/utils/envUtil"; import { deriveMCPManifestOAuth, injectMCPAuthActionToYml, + isMCPScaffoldWarning, MCP_DCR_WELL_KNOWN_URL_PLACEHOLDER, + MCP_OAUTH_AUTHORIZATION_URL_PLACEHOLDER, + MCP_OAUTH_TOKEN_URL_PLACEHOLDER, mcpAuthScaffolderDeps, persistMCPAuthCredentialEnvVars, resolveMCPAuthEndpoints, @@ -89,6 +92,7 @@ describe("mcpAuthScaffolder", () => { const inputs: Inputs = { platform: Platform.VSCode, [QuestionNames.MCPForDAAuthMetadataUrl]: "https://example.com/metadata", + [QuestionNames.MCPForDAServerUrl]: "https://example.com/mcp", }; const result = await resolveMCPAuthEndpoints("oauth", inputs); assert.deepEqual(result, { @@ -97,7 +101,12 @@ describe("mcpAuthScaffolder", () => { refreshUrl: "https://auth/token", wellKnownUrl: "https://auth/.well-known/oauth-authorization-server", }); - expect(stub).toHaveBeenCalledExactlyOnceWith("https://example.com/metadata", undefined); + // the server url is the fallback discovery source when the metadata url leads nowhere + expect(stub).toHaveBeenCalledExactlyOnceWith( + "https://example.com/metadata", + undefined, + "https://example.com/mcp" + ); }); it("resolves endpoints for oauth-dynamic via well-known url", async () => { @@ -116,7 +125,8 @@ describe("mcpAuthScaffolder", () => { assert.equal(result.wellKnownUrl, "https://auth/.well-known/oauth-authorization-server"); expect(stub).toHaveBeenCalledExactlyOnceWith( undefined, - "https://auth/.well-known/oauth-authorization-server" + "https://auth/.well-known/oauth-authorization-server", + undefined ); }); }); @@ -172,6 +182,48 @@ describe("mcpAuthScaffolder", () => { assert.equal(dcrStub.mock.calls[0][4], MCP_DCR_WELL_KNOWN_URL_PLACEHOLDER); }); + it("injects OAuth action with placeholders when the endpoints are missing", async () => { + const oauthStub = vi + .spyOn(ActionInjector, "injectCreateOAuthActionForMCP") + .mockResolvedValue(); + const result = await injectMCPAuthActionToYml({ + ...baseArgs, + authType: "oauth", + endpoints: {}, + }); + assert.deepEqual(result, { oauthUrlPlaceholderUsed: true }); + assert.equal(oauthStub.mock.calls[0][5], MCP_OAUTH_AUTHORIZATION_URL_PLACEHOLDER); + assert.equal(oauthStub.mock.calls[0][6], MCP_OAUTH_TOKEN_URL_PLACEHOLDER); + }); + + it("flags the placeholder when only one of the two OAuth urls resolved", async () => { + const oauthStub = vi + .spyOn(ActionInjector, "injectCreateOAuthActionForMCP") + .mockResolvedValue(); + const result = await injectMCPAuthActionToYml({ + ...baseArgs, + authType: "oauth", + endpoints: { authorizationUrl: "https://auth/authorize" }, + }); + assert.deepEqual(result, { oauthUrlPlaceholderUsed: true }); + assert.equal(oauthStub.mock.calls[0][5], "https://auth/authorize"); + assert.equal(oauthStub.mock.calls[0][6], MCP_OAUTH_TOKEN_URL_PLACEHOLDER); + }); + + it("does not substitute placeholders for entra-sso", async () => { + const oauthStub = vi + .spyOn(ActionInjector, "injectCreateOAuthActionForMCP") + .mockResolvedValue(); + const result = await injectMCPAuthActionToYml({ + ...baseArgs, + authType: "entra-sso", + endpoints: {}, + }); + assert.deepEqual(result, {}); + assert.isUndefined(oauthStub.mock.calls[0][5]); + assert.isUndefined(oauthStub.mock.calls[0][6]); + }); + it("injects OAuth action with credential env refs when persisting (oauth)", async () => { const oauthStub = vi .spyOn(ActionInjector, "injectCreateOAuthActionForMCP") @@ -262,6 +314,19 @@ describe("mcpAuthScaffolder", () => { }); }); + describe("isMCPScaffoldWarning", () => { + it("accepts MCP scaffolding warning types", () => { + assert.isTrue(isMCPScaffoldWarning({ type: "mcpAuthOAuthUrlPlaceholder" })); + assert.isTrue(isMCPScaffoldWarning({ type: "mcpAuthDcrWellKnownUrlPlaceholder" })); + assert.isTrue(isMCPScaffoldWarning({ type: "mcpNoToolsFetched" })); + }); + + it("rejects spec-parser warning types", () => { + assert.isFalse(isMCPScaffoldWarning({ type: "operationid-missing" })); + assert.isFalse(isMCPScaffoldWarning({ type: "generate-card-failed" })); + }); + }); + describe("persistMCPAuthCredentialEnvVars", () => { it("is a no-op for oauth-dynamic", async () => { const listStub = vi.spyOn(envUtil, "listEnv"); diff --git a/packages/fx-core/tests/component/utils/mcpToolFetcher.test.ts b/packages/fx-core/tests/component/utils/mcpToolFetcher.test.ts index d64cde0fa35..b9042be3d6d 100644 --- a/packages/fx-core/tests/component/utils/mcpToolFetcher.test.ts +++ b/packages/fx-core/tests/component/utils/mcpToolFetcher.test.ts @@ -3,8 +3,12 @@ import axios from "axios"; import fs from "fs-extra"; +import { Readable } from "stream"; import { assert, vi } from "vitest"; import { + buildMCPServerWellKnownCandidates, + buildProtectedResourceCandidates, + buildWellKnownCandidates, fetchMCPTools, probeMCPServerAuth, readMCPToolsFromFile, @@ -271,14 +275,56 @@ describe("mcpToolFetcher", () => { }); describe("probeMCPServerAuth", () => { - it("should return requiresAuth=false when server responds 200", async () => { - vi.spyOn(axios, "post").mockResolvedValue({ status: 200 }); + it("should confirm the endpoint when a 200 carries a JSON-RPC envelope", async () => { + vi.spyOn(axios, "post").mockResolvedValue({ + status: 200, + data: { jsonrpc: "2.0", id: 1, result: { protocolVersion: "2025-03-26" } }, + }); const result = await probeMCPServerAuth("https://example.com/mcp"); assert.isFalse(result.requiresAuth); + assert.equal(result.endpointStatus, "confirmed"); assert.isUndefined(result.authMetadataUrl); }); + it("should confirm the endpoint when the JSON-RPC envelope arrives SSE-framed", async () => { + // Streamable-HTTP servers answer `initialize` with `text/event-stream`, which axios hands + // back as raw text. The envelope is inside the `data:` payload, so the body text carries + // the proof even though nothing parsed the frames. + vi.spyOn(axios, "post").mockResolvedValue({ + status: 200, + data: 'event: message\ndata: {"result":{"protocolVersion":"2025-03-26"},"id":1,"jsonrpc":"2.0"}\n\n', + }); + + const result = await probeMCPServerAuth("https://learn.example.com/api/mcp"); + assert.isFalse(result.requiresAuth); + assert.equal(result.endpointStatus, "confirmed"); + }); + + it("should reject a 200 that carries no JSON-RPC envelope", async () => { + // A truncated url on a host that serves a landing page answers 200 with HTML, so the + // status alone proves nothing. + vi.spyOn(axios, "post").mockResolvedValue({ + status: 200, + data: "Welcome", + }); + + const result = await probeMCPServerAuth("https://example.com/"); + assert.isFalse(result.requiresAuth); + assert.equal(result.endpointStatus, "notEndpoint"); + assert.equal(result.responseStatus, 200); + }); + + it("should reject a 200 whose body is neither text nor an object", async () => { + // A 204-style empty body deserializes to undefined; it carries no envelope either. + vi.spyOn(axios, "post").mockResolvedValue({ status: 200, data: undefined }); + + const result = await probeMCPServerAuth("https://example.com/"); + assert.isFalse(result.requiresAuth); + assert.equal(result.endpointStatus, "notEndpoint"); + assert.equal(result.responseStatus, 200); + }); + it("should return requiresAuth=true when server responds 401", async () => { vi.spyOn(axios, "post").mockRejectedValue({ response: { @@ -308,11 +354,119 @@ describe("mcpToolFetcher", () => { assert.equal(result.authMetadataUrl, "https://secure.example.com/.well-known/oauth"); }); - it("should return requiresAuth=false on non-401 errors", async () => { + it("should leave the endpoint undetermined on a transport failure", async () => { + // Nothing was learned about the url, so it must not be reported as wrong. vi.spyOn(axios, "post").mockRejectedValue(new Error("ECONNREFUSED")); const result = await probeMCPServerAuth("https://down.example.com/mcp"); assert.isFalse(result.requiresAuth); + assert.equal(result.endpointStatus, "undetermined"); + assert.isUndefined(result.responseStatus); + }); + + it("should leave the endpoint undetermined on a 5xx", async () => { + vi.spyOn(axios, "post").mockRejectedValue({ response: { status: 503 } }); + + const result = await probeMCPServerAuth("https://down.example.com/mcp"); + assert.equal(result.endpointStatus, "undetermined"); + }); + + it("should leave the endpoint undetermined on a transient 4xx", async () => { + // 429 says the server is throttling, not that the url is wrong. + vi.spyOn(axios, "post").mockRejectedValue({ response: { status: 429 } }); + + const result = await probeMCPServerAuth("https://busy.example.com/mcp"); + assert.equal(result.endpointStatus, "undetermined"); + assert.isUndefined(result.responseStatus); + }); + + for (const status of [403, 404, 405]) { + it(`should flag a ${status} as a url that is not an MCP endpoint`, async () => { + // The mistyped form of an MCP url often still serves a page on GET, so only the + // initialize POST reveals that nothing MCP is routed there. + vi.spyOn(axios, "post").mockRejectedValue({ response: { status } }); + vi.spyOn(axios, "get").mockRejectedValue({ response: { status } }); + + const result = await probeMCPServerAuth("https://taskmaster.example.com"); + assert.isFalse(result.requiresAuth); + assert.equal(result.endpointStatus, "notEndpoint"); + assert.equal(result.responseStatus, status); + }); + } + + it("should confirm a 4xx that turns out to be the deprecated HTTP+SSE transport", async () => { + // A 2024-11-05 server has nothing to POST to, so the transport spec has the client fall + // back to a GET and read the `endpoint` event rather than condemn the url. + vi.spyOn(axios, "post").mockRejectedValue({ response: { status: 405 } }); + vi.spyOn(axios, "get").mockResolvedValue({ + headers: { "content-type": "text/event-stream; charset=utf-8" }, + data: Readable.from(["event: endpoint\ndata: /messages?sessionId=abc\n\n"]), + }); + + const result = await probeMCPServerAuth("https://legacy.example.com/sse"); + assert.isFalse(result.requiresAuth); + assert.equal(result.endpointStatus, "confirmed"); + assert.isUndefined(result.responseStatus); + }); + + it("should keep the notEndpoint verdict when the fallback GET is not an event stream", async () => { + const stream = Readable.from(["not found"]); + const destroySpy = vi.spyOn(stream, "destroy"); + vi.spyOn(axios, "post").mockRejectedValue({ response: { status: 404 } }); + vi.spyOn(axios, "get").mockResolvedValue({ + headers: { "content-type": "text/html" }, + data: stream, + }); + + const result = await probeMCPServerAuth("https://example.com/"); + assert.equal(result.endpointStatus, "notEndpoint"); + assert.equal(result.responseStatus, 404); + assert.isTrue(destroySpy.mock.calls.length > 0); + }); + + it("should keep the notEndpoint verdict when the event stream names no endpoint", async () => { + vi.spyOn(axios, "post").mockRejectedValue({ response: { status: 405 } }); + vi.spyOn(axios, "get").mockResolvedValue({ + headers: { "content-type": "text/event-stream" }, + data: Readable.from(["event: ping\ndata: {}\n\n"]), + }); + + const result = await probeMCPServerAuth("https://noisy.example.com/sse"); + assert.equal(result.endpointStatus, "notEndpoint"); + assert.equal(result.responseStatus, 405); + }); + + it("should keep the notEndpoint verdict when the event stream errors out", async () => { + const stream = new Readable({ read: () => undefined }); + vi.spyOn(axios, "post").mockRejectedValue({ response: { status: 404 } }); + vi.spyOn(axios, "get").mockResolvedValue({ + headers: { "content-type": "text/event-stream" }, + data: stream, + }); + setTimeout(() => stream.emit("error", new Error("ECONNRESET")), 0); + + const result = await probeMCPServerAuth("https://flaky.example.com/sse"); + assert.equal(result.endpointStatus, "notEndpoint"); + assert.equal(result.responseStatus, 404); + }); + + it("should stop reading an event stream that never names an endpoint", async () => { + vi.spyOn(axios, "post").mockRejectedValue({ response: { status: 404 } }); + vi.spyOn(axios, "get").mockResolvedValue({ + headers: { "content-type": "text/event-stream" }, + data: Readable.from(["data: " + "x".repeat(9000) + "\n\n", "event: endpoint\n\n"]), + }); + + const result = await probeMCPServerAuth("https://chatty.example.com/sse"); + assert.equal(result.endpointStatus, "notEndpoint"); + }); + + it("should confirm rather than reject a 401", async () => { + vi.spyOn(axios, "post").mockRejectedValue({ status: 401 }); + + const result = await probeMCPServerAuth("https://taskmaster.example.com/mcp"); + assert.isTrue(result.requiresAuth); + assert.equal(result.endpointStatus, "confirmed"); }); it("should handle 401 via error.status (no response object)", async () => { @@ -321,6 +475,15 @@ describe("mcpToolFetcher", () => { const result = await probeMCPServerAuth("https://secure.example.com/mcp"); assert.isTrue(result.requiresAuth); }); + + it("should flag a 404 reported via error.status (no response object)", async () => { + vi.spyOn(axios, "post").mockRejectedValue({ status: 404 }); + vi.spyOn(axios, "get").mockRejectedValue({ status: 404 }); + + const result = await probeMCPServerAuth("https://example.com/"); + assert.equal(result.endpointStatus, "notEndpoint"); + assert.equal(result.responseStatus, 404); + }); }); describe("resolveMCPOAuthMetadata", () => { @@ -544,5 +707,365 @@ describe("mcpToolFetcher", () => { assert.isNotEmpty(e.message); } }); + + it("should fall back to the appended OIDC discovery form when RFC 8414 insertion 404s", async () => { + const getStub = vi.spyOn(axios, "get"); + getStub.mockImplementation(async (url: string): Promise => { + if (url === "https://example.com/.well-known/oauth-protected-resource") { + return { + status: 200, + data: { authorization_servers: ["https://login.microsoftonline.com/common/v2.0"] }, + }; + } + // Entra serves ONLY the appended OIDC form; every other candidate 404s. + if ( + url === "https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration" + ) { + return { + data: { + authorization_endpoint: + "https://login.microsoftonline.com/common/oauth2/v2.0/authorize", + token_endpoint: "https://login.microsoftonline.com/common/oauth2/v2.0/token", + }, + }; + } + throw new Error("Request failed with status code 404"); + }); + + const result = await resolveMCPOAuthMetadata( + "https://example.com/.well-known/oauth-protected-resource" + ); + + assert.equal( + result.authorizationUrl, + "https://login.microsoftonline.com/common/oauth2/v2.0/authorize" + ); + assert.equal(result.tokenUrl, "https://login.microsoftonline.com/common/oauth2/v2.0/token"); + assert.equal( + result.wellKnownUrl, + "https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration" + ); + }); + + it("should skip a candidate that responds without endpoints and try the next one", async () => { + const getStub = vi.spyOn(axios, "get"); + getStub.mockImplementation(async (url: string): Promise => { + if (url === "https://example.com/.well-known/oauth-protected-resource") { + return { status: 200, data: { authorization_servers: ["https://auth.example.com/t1"] } }; + } + if (url === "https://auth.example.com/.well-known/oauth-authorization-server/t1") { + // Responds 200 but with a protected-resource document — no endpoints. + return { data: { resource: "https://auth.example.com/", authorization_servers: [] } }; + } + if (url === "https://auth.example.com/.well-known/openid-configuration/t1") { + return { + data: { + authorization_endpoint: "https://auth.example.com/authorize", + token_endpoint: "https://auth.example.com/token", + }, + }; + } + throw new Error("Request failed with status code 404"); + }); + + const result = await resolveMCPOAuthMetadata( + "https://example.com/.well-known/oauth-protected-resource" + ); + + assert.equal(result.authorizationUrl, "https://auth.example.com/authorize"); + assert.equal( + result.wellKnownUrl, + "https://auth.example.com/.well-known/openid-configuration/t1" + ); + }); + + it("should report every attempted candidate when all of them fail", async () => { + const getStub = vi.spyOn(axios, "get"); + getStub.mockImplementation(async (url: string): Promise => { + if (url === "https://example.com/.well-known/oauth-protected-resource") { + return { status: 200, data: { authorization_servers: ["https://auth.example.com/t1"] } }; + } + throw new Error("Request failed with status code 404"); + }); + + try { + await resolveMCPOAuthMetadata("https://example.com/.well-known/oauth-protected-resource"); + assert.fail("Should have thrown"); + } catch (e: any) { + assert.include( + e.message, + "https://auth.example.com/.well-known/oauth-authorization-server/t1" + ); + assert.include(e.message, "https://auth.example.com/t1/.well-known/openid-configuration"); + } + }); + + it("should not probe alternative candidates when wellKnownUrl is explicitly configured", async () => { + const getStub = vi.spyOn(axios, "get"); + getStub.mockRejectedValue(new Error("Request failed with status code 404")); + + try { + await resolveMCPOAuthMetadata( + undefined, + "https://auth.example.com/.well-known/oauth-authorization-server" + ); + assert.fail("Should have thrown"); + } catch (e: any) { + assert.isNotEmpty(e.message); + } + assert.strictEqual(getStub.mock.calls.length, 1); + }); + + it("should fall back to the MCP server origin when the 401 carries no resource_metadata", async () => { + // A server on the 2025-03-26 MCP auth spec: it is its own authorization server, publishes + // RFC 8414 metadata at the origin root, and serves no protected-resource document. + const getStub = vi.spyOn(axios, "get"); + getStub.mockImplementation(async (url: string): Promise => { + if (url === "https://mcp.contoso.com/.well-known/oauth-authorization-server") { + return { + status: 200, + data: { + authorization_endpoint: "https://mcp.contoso.com/oauth/authorize", + token_endpoint: "https://mcp.contoso.com/oauth/token", + }, + }; + } + throw new Error("Request failed with status code 404"); + }); + + const metadata = await resolveMCPOAuthMetadata( + undefined, + undefined, + "https://mcp.contoso.com/mcp" + ); + + assert.strictEqual(metadata.authorizationUrl, "https://mcp.contoso.com/oauth/authorize"); + assert.strictEqual(metadata.tokenUrl, "https://mcp.contoso.com/oauth/token"); + assert.strictEqual( + metadata.wellKnownUrl, + "https://mcp.contoso.com/.well-known/oauth-authorization-server" + ); + }); + + it("should fall back to the MCP server origin when the advertised metadata is unreachable", async () => { + const getStub = vi.spyOn(axios, "get"); + getStub.mockImplementation(async (url: string): Promise => { + if (url === "https://mcp.contoso.com/.well-known/oauth-authorization-server") { + return { + status: 200, + data: { + authorization_endpoint: "https://mcp.contoso.com/oauth/authorize", + token_endpoint: "https://mcp.contoso.com/oauth/token", + }, + }; + } + throw new Error("Request failed with status code 404"); + }); + + const metadata = await resolveMCPOAuthMetadata( + "https://mcp.contoso.com/.well-known/oauth-protected-resource", + undefined, + "https://mcp.contoso.com/mcp" + ); + + assert.strictEqual(metadata.tokenUrl, "https://mcp.contoso.com/oauth/token"); + }); + + it("should prefer the advertised authorization server over the MCP server origin", async () => { + const getStub = vi.spyOn(axios, "get"); + getStub.mockImplementation(async (url: string): Promise => { + if (url === "https://mcp.contoso.com/.well-known/oauth-protected-resource") { + return { status: 200, data: { authorization_servers: ["https://auth.contoso.com"] } }; + } + if (url === "https://auth.contoso.com/.well-known/oauth-authorization-server") { + return { + status: 200, + data: { + authorization_endpoint: "https://auth.contoso.com/authorize", + token_endpoint: "https://auth.contoso.com/token", + }, + }; + } + throw new Error("Request failed with status code 404"); + }); + + const metadata = await resolveMCPOAuthMetadata( + "https://mcp.contoso.com/.well-known/oauth-protected-resource", + undefined, + "https://mcp.contoso.com/mcp" + ); + + assert.strictEqual(metadata.authorizationUrl, "https://auth.contoso.com/authorize"); + }); + + it("should derive the protected-resource document when the server never advertised one", async () => { + // Google's MCP servers defer authorization to the tool calls, so an unauthenticated + // initialize returns 200 with no challenge and there is no resource_metadata to follow. + // The document still exists at the RFC 9728 §3.1 insertion location. + const getStub = vi.spyOn(axios, "get"); + getStub.mockImplementation(async (url: string): Promise => { + if (url === "https://drivemcp.googleapis.com/.well-known/oauth-protected-resource/mcp/v1") { + return { + status: 200, + data: { authorization_servers: ["https://accounts.google.com/"] }, + }; + } + if (url === "https://accounts.google.com/.well-known/oauth-authorization-server") { + return { + status: 200, + data: { + authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint: "https://oauth2.googleapis.com/token", + }, + }; + } + throw new Error("Request failed with status code 404"); + }); + + const metadata = await resolveMCPOAuthMetadata( + undefined, + undefined, + "https://drivemcp.googleapis.com/mcp/v1" + ); + + assert.strictEqual(metadata.authorizationUrl, "https://accounts.google.com/o/oauth2/v2/auth"); + assert.strictEqual(metadata.tokenUrl, "https://oauth2.googleapis.com/token"); + assert.strictEqual( + metadata.wellKnownUrl, + "https://accounts.google.com/.well-known/oauth-authorization-server" + ); + // The authoritative document answered, so the server was never guessed to be its own + // authorization server. + assert.isFalse( + getStub.mock.calls.some( + (call) => + call[0] === "https://drivemcp.googleapis.com/.well-known/oauth-authorization-server" + ) + ); + }); + + it("should derive the protected-resource document from the origin root as well", async () => { + const getStub = vi.spyOn(axios, "get"); + getStub.mockImplementation(async (url: string): Promise => { + if (url === "https://mcp.contoso.com/.well-known/oauth-protected-resource") { + return { status: 200, data: { authorization_servers: ["https://auth.contoso.com"] } }; + } + if (url === "https://auth.contoso.com/.well-known/oauth-authorization-server") { + return { + status: 200, + data: { + authorization_endpoint: "https://auth.contoso.com/authorize", + token_endpoint: "https://auth.contoso.com/token", + }, + }; + } + throw new Error("Request failed with status code 404"); + }); + + const metadata = await resolveMCPOAuthMetadata( + undefined, + undefined, + "https://mcp.contoso.com/mcp" + ); + + assert.strictEqual(metadata.tokenUrl, "https://auth.contoso.com/token"); + }); + + it("should still throw when neither the metadata url nor the server url resolves", async () => { + vi.spyOn(axios, "get").mockRejectedValue(new Error("Request failed with status code 404")); + + try { + await resolveMCPOAuthMetadata(undefined, undefined, "https://mcp.contoso.com/mcp"); + assert.fail("Should have thrown"); + } catch (e: any) { + assert.include(e.message, "https://mcp.contoso.com/.well-known/oauth-authorization-server"); + } + }); + + it("should surface the fetch failure when there is no server url to fall back to", async () => { + // Without an MCP server url there is nothing left to try, so the transport error is + // reported as-is rather than being swallowed into a generic "no candidates" message. + vi.spyOn(axios, "get").mockRejectedValue(new Error("getaddrinfo ENOTFOUND mcp.contoso.com")); + + try { + await resolveMCPOAuthMetadata( + "https://mcp.contoso.com/.well-known/oauth-protected-resource", + undefined, + undefined + ); + assert.fail("Should have thrown"); + } catch (e: any) { + assert.include(e.message, "ENOTFOUND"); + } + }); + }); + + describe("buildMCPServerWellKnownCandidates", () => { + it("should probe the path-derived forms before the origin root", () => { + assert.deepEqual(buildMCPServerWellKnownCandidates("https://mcp.contoso.com/mcp"), [ + "https://mcp.contoso.com/.well-known/oauth-authorization-server/mcp", + "https://mcp.contoso.com/.well-known/openid-configuration/mcp", + "https://mcp.contoso.com/mcp/.well-known/oauth-authorization-server", + "https://mcp.contoso.com/mcp/.well-known/openid-configuration", + "https://mcp.contoso.com/.well-known/oauth-authorization-server", + "https://mcp.contoso.com/.well-known/openid-configuration", + ]); + }); + + it("should deduplicate when the server url has no path", () => { + assert.deepEqual(buildMCPServerWellKnownCandidates("https://mcp.contoso.com"), [ + "https://mcp.contoso.com/.well-known/oauth-authorization-server", + "https://mcp.contoso.com/.well-known/openid-configuration", + ]); + }); + }); + + describe("buildProtectedResourceCandidates", () => { + it("should probe the RFC 9728 insertion form before the origin root", () => { + assert.deepEqual(buildProtectedResourceCandidates("https://drivemcp.googleapis.com/mcp/v1"), [ + "https://drivemcp.googleapis.com/.well-known/oauth-protected-resource/mcp/v1", + "https://drivemcp.googleapis.com/.well-known/oauth-protected-resource", + ]); + }); + + it("should deduplicate when the server url has no path", () => { + assert.deepEqual(buildProtectedResourceCandidates("https://mcp.contoso.com"), [ + "https://mcp.contoso.com/.well-known/oauth-protected-resource", + ]); + }); + + it("should strip a trailing slash from the resource path", () => { + assert.deepEqual(buildProtectedResourceCandidates("https://mcp.contoso.com/mcp/"), [ + "https://mcp.contoso.com/.well-known/oauth-protected-resource/mcp", + "https://mcp.contoso.com/.well-known/oauth-protected-resource", + ]); + }); + }); + + describe("buildWellKnownCandidates", () => { + it("should order insertion before append for an issuer with a path", () => { + assert.deepEqual(buildWellKnownCandidates("https://login.microsoftonline.com/common/v2.0"), [ + "https://login.microsoftonline.com/.well-known/oauth-authorization-server/common/v2.0", + "https://login.microsoftonline.com/.well-known/openid-configuration/common/v2.0", + "https://login.microsoftonline.com/common/v2.0/.well-known/oauth-authorization-server", + "https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration", + ]); + }); + + it("should deduplicate candidates for a host-only issuer", () => { + assert.deepEqual(buildWellKnownCandidates("https://mcp.notion.com"), [ + "https://mcp.notion.com/.well-known/oauth-authorization-server", + "https://mcp.notion.com/.well-known/openid-configuration", + ]); + }); + + it("should strip a trailing slash from the issuer path", () => { + assert.deepEqual(buildWellKnownCandidates("https://auth.example.com/tenant/"), [ + "https://auth.example.com/.well-known/oauth-authorization-server/tenant", + "https://auth.example.com/.well-known/openid-configuration/tenant", + "https://auth.example.com/tenant/.well-known/oauth-authorization-server", + "https://auth.example.com/tenant/.well-known/openid-configuration", + ]); + }); }); }); diff --git a/packages/fx-core/tests/core/FxCore.declarativeAgent.test.ts b/packages/fx-core/tests/core/FxCore.declarativeAgent.test.ts index 04a55040632..578fb44b57d 100644 --- a/packages/fx-core/tests/core/FxCore.declarativeAgent.test.ts +++ b/packages/fx-core/tests/core/FxCore.declarativeAgent.test.ts @@ -3305,6 +3305,93 @@ describe("addPlugin", async () => { } }); + it("from MCP (DT flag on): warns when the probed url is not an MCP endpoint", async () => { + const appName = await mockV3Project(); + const projectPath = path.join(os.tmpdir(), appName); + const inputs: Inputs = { + platform: Platform.CLI, + [QuestionNames.Folder]: os.tmpdir(), + [QuestionNames.TeamsAppManifestFilePath]: "manifest.json", + [QuestionNames.ActionType]: ActionStartOptions.mcp().id, + [QuestionNames.MCPForDAServerUrl]: "https://example.com", + [QuestionNames.MCPToolsFilePath]: "", + [QuestionNames.MCPForDAAuthType]: "oauth", + [QuestionNames.MCPForDAClientId]: "client-id", + [QuestionNames.MCPForDAClientSecret]: "client-secret", + [QuestionNames.MCPForDAScopes]: "scope-a", + projectPath, + }; + + const manifest = new TeamsAppManifest(); + manifest.copilotExtensions = { + declarativeCopilots: [{ file: "test1.json", id: "action_1" }], + }; + + vi.spyOn(featureFlagManager, "getBooleanValue").mockImplementation((flag) => { + return flag === FeatureFlags.MCPForDADT; + }); + vi.spyOn(validationUtils, "validateInputs").mockResolvedValue(undefined); + vi.spyOn(manifestUtils, "_readAppManifest").mockResolvedValue(ok(manifest)); + vi.spyOn(copilotGptManifestUtils, "getManifestPath").mockResolvedValue(ok("dcManifest.json")); + vi.spyOn(copilotGptManifestUtils, "readCopilotGptManifestFile").mockResolvedValue( + ok({} as DeclarativeCopilotManifestSchema) + ); + vi.spyOn( + copilotGptManifestUtils, + "getDefaultNextAvailablePluginManifestPath" + ).mockResolvedValue("ai-plugin_1.json"); + vi.spyOn(copilotGptManifestUtils, "addAction").mockResolvedValue( + ok({} as DeclarativeCopilotManifestSchema) + ); + + vi.spyOn(addPluginTools.ui, "showMessage").mockImplementation((level) => { + if (level === "warn") return Promise.resolve(ok("Add")); + return Promise.resolve(ok("")); + }); + + const mcpToolFetcherModule = await import("../../src/component/utils/mcpToolFetcher"); + vi.spyOn(mcpToolFetcherModule, "probeMCPServerAuth").mockResolvedValue({ + requiresAuth: true, + endpointStatus: "notEndpoint", + responseStatus: 403, + }); + + const mcpAuthScaffolderModule = await import("../../src/component/utils/mcpAuthScaffolder"); + vi.spyOn( + mcpAuthScaffolderModule.mcpAuthScaffolderDeps, + "resolveMCPOAuthMetadata" + ).mockResolvedValue({ + authorizationUrl: "https://example.com/oauth/authorize", + tokenUrl: "https://example.com/oauth/token", + refreshUrl: "https://example.com/oauth/token", + wellKnownUrl: "https://example.com/.well-known/oauth-authorization-server", + }); + + const actionInjectorModule = await import("../../src/component/configManager/actionInjector"); + vi.spyOn( + actionInjectorModule.ActionInjector, + "injectCreateOAuthActionForMCP" + ).mockResolvedValue(); + + vi.spyOn(pathUtils, "getYmlFilePath").mockReturnValue("m365agents.yml"); + vi.spyOn(fs, "ensureFile").mockResolvedValue(); + vi.spyOn(fs, "writeJSON").mockResolvedValue(); + const warnStub = vi.spyOn(addPluginTools.logProvider, "warning").mockResolvedValue(); + + const core = new FxCore(addPluginTools); + const result = await core.addPlugin(inputs); + + assert.isTrue(result.isOk()); + // The DT branch never fetches tools, so this warning is the only trace a wrong + // url leaves — the same one the create flow raises. + const warnings = warnStub.mock.calls.map((c) => String(c[0])); + assert.isTrue(warnings.some((w) => w.includes("https://example.com") && w.includes("403"))); + + if (await fs.pathExists(projectPath)) { + await fs.remove(projectPath); + } + }); + it("from MCP (DT flag on): still injects oauth/register when auth metadata cannot be resolved", async () => { const appName = await mockV3Project(); const projectPath = path.join(os.tmpdir(), appName); @@ -3381,6 +3468,87 @@ describe("addPlugin", async () => { } }); + it("from MCP (DT flag on): notifies in VS Code when the yml is left with oauth placeholders", async () => { + const appName = await mockV3Project(); + const projectPath = path.join(os.tmpdir(), appName); + const inputs: Inputs = { + platform: Platform.VSCode, + [QuestionNames.Folder]: os.tmpdir(), + [QuestionNames.TeamsAppManifestFilePath]: "manifest.json", + [QuestionNames.ActionType]: ActionStartOptions.mcp().id, + [QuestionNames.MCPForDAServerUrl]: "https://example.com/mcp", + [QuestionNames.MCPToolsFilePath]: "", + [QuestionNames.MCPForDAAuthType]: "oauth", + [QuestionNames.MCPForDAClientId]: "client-id", + [QuestionNames.MCPForDAClientSecret]: "client-secret", + [QuestionNames.MCPForDAScopes]: "scope-a", + projectPath, + }; + + const manifest = new TeamsAppManifest(); + manifest.copilotExtensions = { + declarativeCopilots: [{ file: "test1.json", id: "action_1" }], + }; + + vi.spyOn(featureFlagManager, "getBooleanValue").mockImplementation((flag) => { + return flag === FeatureFlags.MCPForDADT; + }); + vi.spyOn(validationUtils, "validateInputs").mockResolvedValue(undefined); + vi.spyOn(manifestUtils, "_readAppManifest").mockResolvedValue(ok(manifest)); + vi.spyOn(copilotGptManifestUtils, "getManifestPath").mockResolvedValue(ok("dcManifest.json")); + vi.spyOn(copilotGptManifestUtils, "readCopilotGptManifestFile").mockResolvedValue( + ok({} as DeclarativeCopilotManifestSchema) + ); + vi.spyOn( + copilotGptManifestUtils, + "getDefaultNextAvailablePluginManifestPath" + ).mockResolvedValue("ai-plugin_1.json"); + vi.spyOn(copilotGptManifestUtils, "addAction").mockResolvedValue( + ok({} as DeclarativeCopilotManifestSchema) + ); + + const showMessage = vi + .spyOn(addPluginTools.ui, "showMessage") + .mockImplementation((level, message, modal, ...items) => Promise.resolve(ok(items[0]))); + const openFile = vi.spyOn(addPluginTools.ui, "openFile").mockResolvedValue(ok(undefined)); + + // Metadata resolution fails, so the injector falls back to placeholder URLs. + const mcpAuthScaffolderModule = await import("../../src/component/utils/mcpAuthScaffolder"); + vi.spyOn( + mcpAuthScaffolderModule.mcpAuthScaffolderDeps, + "resolveMCPOAuthMetadata" + ).mockRejectedValue(new Error("no resource_metadata")); + + const actionInjectorModule = await import("../../src/component/configManager/actionInjector"); + vi.spyOn( + actionInjectorModule.ActionInjector, + "injectCreateOAuthActionForMCP" + ).mockResolvedValue(); + + vi.spyOn(pathUtils, "getYmlFilePath").mockReturnValue("m365agents.yml"); + vi.spyOn(fs, "ensureFile").mockResolvedValue(); + vi.spyOn(fs, "writeJSON").mockResolvedValue(); + + const core = new FxCore(addPluginTools); + const result = await core.addPlugin(inputs); + + assert.isTrue(result.isOk()); + // The action reports success right after, so a warning left in the output channel + // reads as "everything worked". Placeholders must be raised the way create raises them. + const warned = showMessage.mock.calls.find((call) => call[0] === "warn"); + assert.isDefined(warned); + assert.include(String(warned![1]), "authorizationUrl"); + assert.include(String(warned![1]), "placeholders"); + + // The notification's only action opens the file that holds the placeholders. + await new Promise((resolve) => setTimeout(resolve, 0)); + assert.isTrue(openFile.mock.calls.some((call) => call[0] === "m365agents.yml")); + + if (await fs.pathExists(projectPath)) { + await fs.remove(projectPath); + } + }); + it("from MCP (DT flag on): still writes the plugin manifest when oauth/register injection fails", async () => { const appName = await mockV3Project(); const projectPath = path.join(os.tmpdir(), appName); diff --git a/packages/fx-core/tests/question/question.test.ts b/packages/fx-core/tests/question/question.test.ts index 2dfc735420c..62ff6c899a8 100644 --- a/packages/fx-core/tests/question/question.test.ts +++ b/packages/fx-core/tests/question/question.test.ts @@ -1313,6 +1313,54 @@ describe("addPluginQuestionNode", async () => { QuestionNames.TeamsAppManifestFilePath, ]); }); + + it("rejects an MCP server url the server answers with 404", async () => { + // The add-action flow must reject the same urls the create flow does. + vi.spyOn(teamsProjectTypeDeps, "probeMCPServerAuth").mockResolvedValue({ + requiresAuth: false, + endpointStatus: "notEndpoint", + responseStatus: 404, + }); + const serverUrlNode = addPluginQuestionNode().children!.find( + (child) => child.data.name === QuestionNames.MCPForDAServerUrl + ); + const validation = (serverUrlNode!.data as TextInputQuestion).additionalValidationOnAccept; + const validFunc = (validation as FuncValidation).validFunc; + + assert.isUndefined(await validFunc("", {} as Inputs)); + const result = await validFunc("https://taskmaster.example.com", {} as Inputs); + assert.isString(result); + assert.include(result as string, "404"); + }); + + it("accepts an MCP server url the server answers with anything other than 404", async () => { + vi.spyOn(teamsProjectTypeDeps, "probeMCPServerAuth").mockResolvedValue({ + requiresAuth: false, + endpointStatus: "notEndpoint", + responseStatus: 403, + }); + const serverUrlNode = addPluginQuestionNode().children!.find( + (child) => child.data.name === QuestionNames.MCPForDAServerUrl + ); + const validation = (serverUrlNode!.data as TextInputQuestion).additionalValidationOnAccept; + const validFunc = (validation as FuncValidation).validFunc; + + assert.isUndefined(await validFunc("https://waf.example.com/mcp", {} as Inputs)); + }); + + it("rejects an MCP server url that is not an absolute http(s) url", async () => { + const probeStub = vi.spyOn(teamsProjectTypeDeps, "probeMCPServerAuth"); + const serverUrlNode = addPluginQuestionNode().children!.find( + (child) => child.data.name === QuestionNames.MCPForDAServerUrl + ); + const validation = (serverUrlNode!.data as TextInputQuestion).additionalValidationOnAccept; + const validFunc = (validation as FuncValidation).validFunc; + + const result = await validFunc("taskmaster.example.com", {} as Inputs); + assert.isString(result); + assert.include(result as string, "https://"); + assert.strictEqual(probeStub.mock.calls.length, 0); + }); }); describe("addKnowledgeQuestionNode", async () => { diff --git a/packages/fx-core/tests/question/scaffold.test.ts b/packages/fx-core/tests/question/scaffold.test.ts index f280c1777da..a7dfc5e93ba 100644 --- a/packages/fx-core/tests/question/scaffold.test.ts +++ b/packages/fx-core/tests/question/scaffold.test.ts @@ -917,6 +917,14 @@ describe("MCPCliPreFetchToolsNode", () => { describe("MCPForDAServerUrlNode", () => { const sandbox = vi; + beforeEach(() => { + // Every case below reaches the endpoint probe, so give it a benign default. Without + // this the validator would issue a real request to the example urls. + vi.spyOn(teamsProjectTypeDeps, "probeMCPServerAuth").mockResolvedValue({ + requiresAuth: false, + endpointStatus: "confirmed", + }); + }); afterEach(() => { vi.restoreAllMocks(); }); @@ -944,6 +952,78 @@ describe("MCPForDAServerUrlNode", () => { const result = await validFunc("https://example.com", inputs); assert.isUndefined(result); assert.isTrue(fetchStub.mock.calls.length === 0); + // The endpoint probe is not CLI-only: VS Code skips the tool fetch but must still + // validate the url. + assert.strictEqual(vi.mocked(teamsProjectTypeDeps.probeMCPServerAuth).mock.calls.length, 1); + }); + it("validFunc rejects a url the server answers with 404", async () => { + vi.mocked(teamsProjectTypeDeps.probeMCPServerAuth).mockResolvedValue({ + requiresAuth: false, + endpointStatus: "notEndpoint", + responseStatus: 404, + }); + const node = MCPForDAServerUrlNode(); + const data = node.data as any; + const validFunc = data.additionalValidationOnAccept.validFunc; + const inputs: Inputs = { platform: Platform.VSCode }; + const result = await validFunc("https://taskmaster.example.com", inputs); + assert.isString(result); + assert.include(result, "404"); + }); + it("validFunc rejects a value that is not an absolute http(s) url", async () => { + // A scheme-less value never reaches the network, so the probe would report no status + // and the url would slip through as `undetermined`. Rule it out syntactically. + const node = MCPForDAServerUrlNode(); + const data = node.data as any; + const validFunc = data.additionalValidationOnAccept.validFunc; + const inputs: Inputs = { platform: Platform.VSCode }; + + for (const value of ["taskmaster.example.com", "example.com/mcp", "ftp://example.com/mcp"]) { + const result = await validFunc(value, inputs); + assert.isString(result, value); + assert.include(result, "https://"); + } + // Not a network problem, so the server is never contacted. + assert.strictEqual(vi.mocked(teamsProjectTypeDeps.probeMCPServerAuth).mock.calls.length, 0); + + // http is allowed: a locally hosted MCP server is a normal thing to point at. + assert.isUndefined(await validFunc("http://localhost:3000/mcp", inputs)); + }); + it("validFunc accepts every outcome other than 404", async () => { + // 404 is the only status a valid endpoint was never measured returning. Anything else, + // including an unreachable server, must not block scaffolding. + const node = MCPForDAServerUrlNode(); + const data = node.data as any; + const validFunc = data.additionalValidationOnAccept.validFunc; + + vi.mocked(teamsProjectTypeDeps.probeMCPServerAuth).mockResolvedValue({ + requiresAuth: true, + endpointStatus: "confirmed", + }); + assert.isUndefined( + await validFunc("https://secure.example.com/mcp", { platform: Platform.VSCode }) + ); + + vi.mocked(teamsProjectTypeDeps.probeMCPServerAuth).mockResolvedValue({ + requiresAuth: false, + endpointStatus: "undetermined", + }); + assert.isUndefined( + await validFunc("https://down.example.com/mcp", { platform: Platform.VSCode }) + ); + + // A WAF in front of a valid endpoint answers 403, so the softer `notEndpoint` shapes + // warn after scaffolding rather than blocking the url outright here. + for (const responseStatus of [403, 405, 200]) { + vi.mocked(teamsProjectTypeDeps.probeMCPServerAuth).mockResolvedValue({ + requiresAuth: false, + endpointStatus: "notEndpoint", + responseStatus, + }); + assert.isUndefined( + await validFunc("https://waf.example.com/mcp", { platform: Platform.VSCode }) + ); + } }); it("validFunc sets auth inputs when requiresAuth=true and no authMetadataUrl", async () => { vi.spyOn(teamsProjectTypeDeps, "fetchMCPTools").mockResolvedValue({ diff --git a/packages/fx-core/tests/v4/mcp/mcpAuthAction.test.ts b/packages/fx-core/tests/v4/mcp/mcpAuthAction.test.ts index cde5220e226..9e3b5b14278 100644 --- a/packages/fx-core/tests/v4/mcp/mcpAuthAction.test.ts +++ b/packages/fx-core/tests/v4/mcp/mcpAuthAction.test.ts @@ -53,6 +53,36 @@ describe("v4 MCP auth YAML action", () => { }); }); + it("injects Custom OAuth with placeholders when the endpoints could not be discovered", () => { + const result = injectMcpAuthActionYaml(BASE_YML, { + ...BASE_ARGS, + authType: "oauth", + endpoints: {}, + }); + + assert.isTrue(result.isOk(), result.isErr() ? result.error.message : "expected ok"); + assert.isTrue(result._unsafeUnwrap().oauthUrlPlaceholderUsed); + assert.deepEqual(provisionActions(result._unsafeUnwrap().yaml)[1].with, { + name: "apigithubc", + appId: "${{TEAMS_APP_ID}}", + flow: "authorizationCode", + authorizationUrl: "", + tokenUrl: "", + identityProvider: "Custom", + baseUrl: "https://api.github.com/mcp", + }); + }); + + it("does not flag Entra as needing OAuth URLs", () => { + const result = injectMcpAuthActionYaml(BASE_YML, { + ...BASE_ARGS, + authType: "entra-sso", + endpoints: {}, + }); + + assert.isFalse(result._unsafeUnwrap().oauthUrlPlaceholderUsed); + }); + it("SCN-CREATE-MCP-16: injects Entra without credential references", () => { const result = injectMcpAuthActionYaml(BASE_YML, { ...BASE_ARGS, @@ -118,6 +148,7 @@ describe("v4 MCP auth YAML action", () => { assert.deepEqual(result._unsafeUnwrap(), { yaml: BASE_YML, wellKnownUrlPlaceholderUsed: false, + oauthUrlPlaceholderUsed: false, }); }); diff --git a/packages/fx-core/tests/v4/runtime/steps/mcpAuth.test.ts b/packages/fx-core/tests/v4/runtime/steps/mcpAuth.test.ts index 02495029f4c..321b39485ab 100644 --- a/packages/fx-core/tests/v4/runtime/steps/mcpAuth.test.ts +++ b/packages/fx-core/tests/v4/runtime/steps/mcpAuth.test.ts @@ -187,8 +187,11 @@ describe("mcp-auth steps (v4)", () => { ); assert.isTrue(res.isOk(), res.isErr() ? res.error.message : "expected ok"); - assert.lengthOf(warnings, 1); + assert.lengthOf(warnings, 2); assert.include(warnings[0], "metadata unavailable"); + // the action is still injected, with placeholders the developer must replace + assert.include(warnings[1], "authorizationUrl"); + assert.include(warnings[1], "tokenUrl"); }); it("warns when oauth-dynamic requires manual replacement of the well-known URL", async () => { @@ -205,7 +208,7 @@ describe("mcp-auth steps (v4)", () => { assert.isTrue(res.isOk(), res.isErr() ? res.error.message : "expected ok"); assert.lengthOf(warnings, 2); assert.include(warnings[0], "metadata unavailable"); - assert.include(warnings[1], SERVER_URL); + assert.include(warnings[1], "wellKnownAuthorizationServer"); }); it("is idempotent — a re-run does not duplicate the registration action", async () => { diff --git a/packages/vscode-extension/package.nls.json b/packages/vscode-extension/package.nls.json index a9d404b0677..29044c3b962 100644 --- a/packages/vscode-extension/package.nls.json +++ b/packages/vscode-extension/package.nls.json @@ -297,6 +297,8 @@ "teamstoolkit.handlers.manualStepRequired": "[%s] is created at [local address](%s). Follow the instructions in README file to preview your app.", "teamstoolkit.handlers.manualStepRequired.fallback": "[%s] is created at %s. Follow the instructions in README file to preview your app.", "teamstoolkit.handlers.manualStepRequiredTitle": "Open README", + "teamstoolkit.handlers.mcpAuthPlaceholder.openYmlTitle": "Open m365agents.yml", + "teamstoolkit.handlers.mcpAuthPlaceholder.recreateTitle": "Create New App", "teamstoolkit.handlers.referLinkForMoreDetails": "Please refer to this link for more details: ", "teamstoolkit.handlers.reportIssue": "Report Issue", "teamstoolkit.handlers.similarIssues": "Similar Issues", diff --git a/packages/vscode-extension/src/handlers/autoOpenProjectHandler.ts b/packages/vscode-extension/src/handlers/autoOpenProjectHandler.ts index ec827bd32fd..c5e992f5f45 100644 --- a/packages/vscode-extension/src/handlers/autoOpenProjectHandler.ts +++ b/packages/vscode-extension/src/handlers/autoOpenProjectHandler.ts @@ -24,8 +24,11 @@ export async function autoOpenProjectHandler(): Promise { const autoInstallDependency = (await teamsfxCore.globalStateGet( GlobalKey.AutoInstallDependency )) as boolean; + // A project scaffolded with unresolved placeholders cannot provision or debug yet, so the + // generic next-step notification gives way to the scaffolding warning notification. + const skipNextStepNotification = autoOpenHelper.hasProvisionBlockingWarning(createWarnings); if (isOpenWalkThrough) { - await autoOpenHelper.showLocalDebugMessage(); + await autoOpenHelper.showLocalDebugMessage(skipNextStepNotification); await teamsfxCore.globalStateUpdate(GlobalKey.OpenWalkThrough, false); if (globalVariables.workspaceUri?.fsPath) { @@ -37,7 +40,7 @@ export async function autoOpenProjectHandler(): Promise { } } if (isOpenReadMe === globalVariables.workspaceUri?.fsPath) { - await autoOpenHelper.showLocalDebugMessage(); + await autoOpenHelper.showLocalDebugMessage(skipNextStepNotification); await readmeHandlers.openReadMeHandler(TelemetryTriggerFrom.Auto); await readmeHandlers.openWorkspaceMCPConfigHandler(TelemetryTriggerFrom.Auto); await projectStatusUtils.updateProjectStatus( diff --git a/packages/vscode-extension/src/telemetry/extTelemetryEvents.ts b/packages/vscode-extension/src/telemetry/extTelemetryEvents.ts index 4be2a61aea8..ca0419a437c 100644 --- a/packages/vscode-extension/src/telemetry/extTelemetryEvents.ts +++ b/packages/vscode-extension/src/telemetry/extTelemetryEvents.ts @@ -302,6 +302,11 @@ export enum TelemetryEvent { ShowScaffoldingWarningSummary = "show-scaffolding-warning-summary", ShowScaffoldingWarningSummaryError = "show-scaffolding-warning-summary-error", + // MCP auth endpoints could not be discovered, so m365agents.yml holds placeholders + ShowMCPAuthPlaceholderNotification = "show-mcp-auth-placeholder-notification", + ClickOpenMCPAuthYml = "click-open-mcp-auth-yml", + ClickRecreateMCPApp = "click-recreate-mcp-app", + FindSimilarIssues = "find-similar-issues", // Teams Github Copilot UI diff --git a/packages/vscode-extension/src/utils/autoOpenHelper.ts b/packages/vscode-extension/src/utils/autoOpenHelper.ts index 44be45706cc..fd42d927157 100644 --- a/packages/vscode-extension/src/utils/autoOpenHelper.ts +++ b/packages/vscode-extension/src/utils/autoOpenHelper.ts @@ -18,6 +18,7 @@ import { JSONSyntaxError, manifestUtils, outputScaffoldingWarningMessage, + pathUtils, pluginManifestUtils, } from "@microsoft/teamsfx-core"; import fs from "fs-extra"; @@ -36,7 +37,7 @@ import { getAppName } from "./appDefinitionUtils"; import { getLocalDebugMessageTemplate } from "./commonUtils"; import { localize } from "./localizeUtils"; -export async function showLocalDebugMessage() { +export async function showLocalDebugMessage(skipNextStepNotification = false) { const shouldShowLocalDebugMessage = (await teamsfxCore.globalStateGet( GlobalKey.ShowLocalDebugMessage, false @@ -71,6 +72,14 @@ export async function showLocalDebugMessage() { showSetSensitivityLabelMessage(); } + // Every call to action below (local debug, preview, provision) starts a lifecycle that is + // guaranteed to fail while the project still holds fill-in placeholders. The scaffolding + // warning notification takes over as the single next step. The flag above is still consumed + // so the stale invitation does not resurface the next time the workspace is opened. + if (skipNextStepNotification) { + return; + } + if (hasKeyGenJsFile || hasKeyGenTsFile) { const openReadMe = { title: localize("teamstoolkit.handlers.manualStepRequiredTitle"), @@ -249,12 +258,92 @@ export async function ShowScaffoldingWarningSummary( manifestRes.error ); } + + // The output channel is easy to miss, and an unreplaced placeholder guarantees a + // provision failure later, so raise this one case to a notification. + showMCPAuthPlaceholderNotification(workspacePath, createWarnings); } catch (e) { const error = assembleError(e); ExtTelemetry.sendTelemetryErrorEvent(TelemetryEvent.ShowScaffoldingWarningSummaryError, error); } } +/** + * `Warning.type` values reported when MCP auth endpoint discovery failed and the scaffolder + * fell back to fill-in placeholders in `m365agents.yml`. The other MCP warnings are advisory + * and stay in the output channel. + */ +const MCP_AUTH_PLACEHOLDER_WARNING_TYPES = [ + "mcpAuthDcrWellKnownUrlPlaceholder", + "mcpAuthOAuthUrlPlaceholder", +]; + +/** + * Whether the scaffolded project needs a manual edit before any lifecycle can succeed, given + * the raw `GlobalKey.CreateWarnings` payload. Callers use it to suppress notifications that + * would invite the developer into a guaranteed failure. + */ +export function hasProvisionBlockingWarning(createWarnings: string): boolean { + if (!createWarnings) { + return false; + } + try { + const warnings = JSON.parse(createWarnings) as Warning[]; + return Array.isArray(warnings) && warnings.some(isProvisionBlockingWarning); + } catch { + // ShowScaffoldingWarningSummary reports the parse failure. Here we only decide whether to + // hide a notification, so an unreadable payload degrades to showing the usual one. + return false; + } +} + +function isProvisionBlockingWarning(warning: Warning): boolean { + return MCP_AUTH_PLACEHOLDER_WARNING_TYPES.includes(warning.type); +} + +export function showMCPAuthPlaceholderNotification( + workspacePath: string, + warnings: Warning[] +): void { + // The warning already carries the wording the other MCP auth flows show, so reuse it + // instead of a second phrasing of the same problem. + const message = warnings + .filter(isProvisionBlockingWarning) + .map((warning) => warning.content) + .join(" "); + if (!message) { + return; + } + + const openYml = { + title: localize("teamstoolkit.handlers.mcpAuthPlaceholder.openYmlTitle"), + run: async (): Promise => { + const ymlPath = pathUtils.getYmlFilePath(workspacePath, undefined, true); + if (ymlPath) { + await vscode.window.showTextDocument(vscode.Uri.file(ymlPath)); + } + }, + }; + const recreate = { + title: localize("teamstoolkit.handlers.mcpAuthPlaceholder.recreateTitle"), + run: async (): Promise => { + await vscode.commands.executeCommand(CommandKey.Create, TelemetryTriggerFrom.Notification); + }, + }; + + ExtTelemetry.sendTelemetryEvent(TelemetryEvent.ShowMCPAuthPlaceholderNotification); + void vscode.window.showWarningMessage(message, openYml, recreate).then((selection) => { + if (selection) { + ExtTelemetry.sendTelemetryEvent( + selection.title === openYml.title + ? TelemetryEvent.ClickOpenMCPAuthYml + : TelemetryEvent.ClickRecreateMCPApp + ); + void selection.run(); + } + }); +} + export async function autoInstallDependencyHandler() { await VS_CODE_UI.runCommand({ cmd: "npm i", diff --git a/packages/vscode-extension/test/utils/autoOpenHelper.test.ts b/packages/vscode-extension/test/utils/autoOpenHelper.test.ts index bd18341e502..230331eff56 100644 --- a/packages/vscode-extension/test/utils/autoOpenHelper.test.ts +++ b/packages/vscode-extension/test/utils/autoOpenHelper.test.ts @@ -1,9 +1,10 @@ -import { ok, TeamsAppManifest } from "@microsoft/teamsfx-api"; +import { ok, TeamsAppManifest, Warning } from "@microsoft/teamsfx-api"; import * as globalState from "@microsoft/teamsfx-core"; import { featureFlagManager, FeatureFlags, manifestUtils, + pathUtils, pluginManifestUtils, } from "@microsoft/teamsfx-core"; import * as apiSpec from "@microsoft/teamsfx-core/build/component/generator/openApiSpec/helper"; @@ -17,7 +18,9 @@ import * as readmeHandlers from "../../src/handlers/readmeHandlers"; import { ExtTelemetry } from "../../src/telemetry/extTelemetry"; import * as appDefinitionUtils from "../../src/utils/appDefinitionUtils"; import { + hasProvisionBlockingWarning, showLocalDebugMessage, + showMCPAuthPlaceholderNotification, ShowScaffoldingWarningSummary, } from "../../src/utils/autoOpenHelper"; import { mockValue } from "../mocks/vitestMockUtils"; @@ -614,4 +617,149 @@ describe("autoOpenHelper", () => { // Call the function await ShowScaffoldingWarningSummary(workspacePath, ""); }); + + it("ShowScaffoldingWarningSummary() - raises unresolved MCP placeholders as a notification", async () => { + // The summary only reaches the output channel, which is easy to miss, so a placeholder that + // will break provision has to escalate to a notification as well. + const workspacePath = "/path/to/workspace"; + + const manifest: TeamsAppManifest = { + manifestVersion: "version", + id: "mock-app-id", + name: { short: "short-name" }, + description: { short: "", full: "" }, + version: "version", + icons: { outline: "outline.png", color: "color.png" }, + accentColor: "#ffffff", + developer: { + privacyUrl: "", + websiteUrl: "", + termsOfUseUrl: "", + name: "", + }, + staticTabs: [ + { + name: "name0", + entityId: "index0", + scopes: ["personal"], + contentUrl: "localhost/content", + websiteUrl: "localhost/website", + }, + ], + }; + vi.spyOn(manifestUtils, "_readAppManifest").mockResolvedValue(ok(manifest)); + vi.spyOn(VscodeLogInstance, "info").mockImplementation(() => {}); + const fakeOutputChannel = { + show: vi.fn().mockResolvedValue(), + }; + mockValue(VscodeLogInstance, "outputChannel", fakeOutputChannel); + vi.spyOn(ExtTelemetry, "sendTelemetryEvent").mockReturnValue(); + const warning = vi.spyOn(vscode.window, "showWarningMessage").mockResolvedValue(undefined); + + await ShowScaffoldingWarningSummary( + workspacePath, + JSON.stringify([{ type: "mcpAuthOAuthUrlPlaceholder", content: "fill in the oauth urls" }]) + ); + + assert.equal(warning.mock.calls.length, 1); + }); + + describe("showMCPAuthPlaceholderNotification", () => { + const placeholderWarning: Warning = { + type: "mcpAuthOAuthUrlPlaceholder", + content: "fill in the oauth urls", + }; + + it("showLocalDebugMessage() - suppressed while the project cannot provision", async () => { + mockValue(process, "platform", "win32"); + vi.spyOn(fs, "pathExists").mockResolvedValue(false); + mockValue(globalVariables, "workspaceUri", vscode.Uri.file("test")); + const showMessageStub = vi.spyOn(vscode.window, "showInformationMessage"); + + await showLocalDebugMessage(true); + + assert.equal(showMessageStub.mock.calls.length, 0); + // the stale invitation must not resurface the next time the workspace is opened + assert.isFalse(await globalState.globalStateGet("ShowLocalDebugMessage", false)); + }); + + it("hasProvisionBlockingWarning() - only for unresolved placeholders", () => { + assert.isTrue(hasProvisionBlockingWarning(JSON.stringify([placeholderWarning]))); + assert.isTrue( + hasProvisionBlockingWarning( + JSON.stringify([{ type: "mcpAuthDcrWellKnownUrlPlaceholder", content: "fill in" }]) + ) + ); + assert.isFalse( + hasProvisionBlockingWarning( + JSON.stringify([{ type: "mcpNoToolsFetched", content: "none" }]) + ) + ); + assert.isFalse(hasProvisionBlockingWarning("")); + // an unreadable payload degrades to the usual notification + assert.isFalse(hasProvisionBlockingWarning("not json")); + }); + + it("stays silent for advisory MCP warnings", () => { + const warning = vi.spyOn(vscode.window, "showWarningMessage"); + + showMCPAuthPlaceholderNotification("/path/to/workspace", [ + { type: "mcpNoToolsFetched", content: "no tools" }, + ]); + + assert.equal(warning.mock.calls.length, 0); + }); + + it("opens the yml file when the developer picks the first action", async () => { + vi.spyOn(ExtTelemetry, "sendTelemetryEvent").mockReturnValue(); + vi.spyOn(pathUtils, "getYmlFilePath").mockReturnValue("/path/to/workspace/m365agents.yml"); + const showTextDocument = vi + .spyOn(vscode.window, "showTextDocument") + .mockResolvedValue({} as any); + const warning = vi + .spyOn(vscode.window, "showWarningMessage") + .mockImplementation( + (_message: string, ...items: any[]) => Promise.resolve(items[0]) as any + ); + + showMCPAuthPlaceholderNotification("/path/to/workspace", [placeholderWarning]); + await warning.mock.results[0].value; + + // the notification repeats the core warning verbatim rather than rewording it + assert.equal(warning.mock.calls[0][0], placeholderWarning.content); + assert.equal(showTextDocument.mock.calls.length, 1); + }); + + it("re-runs create when the developer picks the second action", async () => { + vi.spyOn(ExtTelemetry, "sendTelemetryEvent").mockReturnValue(); + const executeCommand = vi + .spyOn(vscode.commands, "executeCommand") + .mockResolvedValue(undefined); + const warning = vi + .spyOn(vscode.window, "showWarningMessage") + .mockImplementation( + (_message: string, ...items: any[]) => Promise.resolve(items[1]) as any + ); + + showMCPAuthPlaceholderNotification("/path/to/workspace", [ + { type: "mcpAuthDcrWellKnownUrlPlaceholder", content: "fill in the well-known url" }, + ]); + await warning.mock.results[0].value; + + assert.equal(executeCommand.mock.calls[0][0], "fx-extension.create"); + }); + + it("does nothing when the notification is dismissed", async () => { + vi.spyOn(ExtTelemetry, "sendTelemetryEvent").mockReturnValue(); + const showTextDocument = vi.spyOn(vscode.window, "showTextDocument"); + const warning = vi + .spyOn(vscode.window, "showWarningMessage") + .mockResolvedValue(undefined as any); + + showMCPAuthPlaceholderNotification("/path/to/workspace", [placeholderWarning]); + await warning.mock.results[0].value; + + assert.equal(showTextDocument.mock.calls.length, 0); + }); + }); });