feat(test-servers): a CIMD showcase fixture that serves its own client metadata document - #2307
Open
cliffhall wants to merge 1 commit into
Open
feat(test-servers): a CIMD showcase fixture that serves its own client metadata document#2307cliffhall wants to merge 1 commit into
cliffhall wants to merge 1 commit into
Conversation
…t metadata document In CIMD (SEP-991) the `client_id` *is* a URL the authorization server dereferences to learn the client's metadata. Nothing in this repo served such a document, so a fixture could advertise `client_id_metadata_document_supported` and still be unusable — exercising CIMD meant standing up a second host by hand. That is why #2242 shipped verified by its tests alone, and why the v2.6.0 release smoke recorded it as the one contribution with an observable UI surface that could not be reached. The composable server now hosts the document itself when `oauth.clientMetadata` is set, gated on `supportCIMD` — advertising a client the server would then refuse to honour is a worse fixture than serving nothing. The document's `client_id` is derived from the **request** rather than from a configured issuer, so it stays correct when the harness picks the port, as the integration test does. `supportDCR: false` in the fixture is load-bearing rather than incidental. With DCR available a CIMD failure silently succeeds via dynamic registration and the reproduction proves nothing — during development a misconfigured run connected happily and reported `Dynamic (DCR)` with a `test_client_…` id, which reads as success until you check the client id. With DCR off, CIMD is the only path that can complete, so reaching a connected state is itself the assertion; the test pins the absent `registration_endpoint` so it stays that way. Verified end to end against the fixture: Connection Info read `Client registration — Client ID Metadata (CIMD)` with the client id equal to the metadata URL. That drive needed a self-signed HTTPS listener, because the Inspector requires the CIMD metadata URL to be HTTPS with no loopback exemption — filed separately as #2305, since it is a validation gap rather than a fixture one. Closes #2306 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟡 Changes recommended
The configurable path validation and exact query-bearing client_id handling must be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a CIMD showcase fixture that serves its own client metadata document.
Changes:
- Adds configurable CIMD metadata serving.
- Adds a DCR-disabled showcase fixture.
- Adds integration coverage and documentation.
File summaries
| File | Review |
|---|---|
test-servers/src/test-server-oauth.ts |
Serves CIMD metadata. Requires path validation and preserving query parameters in the returned client_id (moderate). |
test-servers/src/load-config.ts |
Adds CIMD configuration fields; clientMetadataPath validation is required (moderate). |
test-servers/src/composable-test-server.ts |
Exposes CIMD options; programmatic path validation is required (moderate). |
test-servers/configs/oauth-cimd-http.json |
Defines the CIMD showcase fixture. |
docs/test-servers.md |
Documents CIMD testing; contains an inaccurate fixed-port warning (nit). |
clients/web/src/test/integration/mcp/oauth-cimd-fixture.test.ts |
Tests fixture metadata and DCR exclusion. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| // nothing. | ||
| if (config.supportCIMD && config.clientMetadata) { | ||
| const doc = config.clientMetadata; | ||
| const metadataPath = config.clientMetadataPath ?? "/client-metadata.json"; |
| // walked to another port on EADDRINUSE. | ||
| const requestBaseUrl = `${req.protocol}://${req.get("host")}`; | ||
| res.json({ | ||
| client_id: new URL(metadataPath, requestBaseUrl).href, |
Comment on lines
+587
to
+590
| ⚠️ **The same fixed-`issuerUrl` hazard as the fixture above applies**, for the same reason: the | ||
| `client_id` this server publishes is derived from its issuer URL, so a server that walked to another | ||
| port on `EADDRINUSE` publishes a `client_id` pointing at whatever process holds 8092. Check with | ||
| `lsof -nP -iTCP:8092 -sTCP:LISTEN` before believing a failure. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2306
Why
In CIMD (SEP-991) the
client_idis a URL that the authorization server dereferences to learn the client's metadata. Nothing in this repo served such a document, so a fixture could advertiseclient_id_metadata_document_supportedand still be unusable — exercising CIMD meant standing up a second host by hand.That is why #2242 shipped verified by its tests alone, and why the v2.6.0 release smoke recorded it as the one contribution with an observable UI surface that could not be reached. This PR is that missing piece, and #2242 has now been driven by hand against it.
What's here
oauth.clientMetadata/oauth.clientMetadataPathand a route serving the document whensupportCIMDis on. Gated onsupportCIMDas well as on the document's presence: advertising a client the server would then refuse to honour is a worse fixture than serving nothing.client_idis derived from the request, not from a configured issuer, so it stays correct when the harness picks the port — which the integration test does.test-servers/configs/oauth-cimd-http.json.src/test/integration/mcp/shape.docs/test-servers.mdsection.supportDCR: falseis load-bearing, not tidinessWith DCR available, a CIMD failure silently succeeds via dynamic registration and the reproduction proves nothing.
This is not hypothetical — it happened while building this. A misconfigured run connected happily and Connection Info read
Dynamic (DCR)with atest_client_…id. It reads as success until you check the client id, which is precisely the confusion #2242 is about. With DCR off, CIMD is the only path that can complete, so reaching a connected state is itself the assertion.The test pins it so it stays that way:
Verification
npm run local:gate— pass, in a dedicated worktree with its own fullnpm install. Web tests went 418 files / 8001 tests → 419 / 8005, i.e. exactly the four added here; 11 smokes OK.Driven end to end against the fixture (this is what cleared the v2.6.0 ledger finding):
The authorize request carried
client_id=https%3A%2F%2F127.0.0.1%3A8443%2Fclient-metadata.jsonagainst a server advertising noregistration_endpoint, so nothing about it could have been DCR.What this does not fix — #2305
That drive needed a self-signed HTTPS listener holding nothing but the JSON document, plus
NODE_TLS_REJECT_UNAUTHORIZED=0in the test server's environment so its own fetch of the document would succeed.The reason is our own validation, not the SDK's:
getCimdClientMetadataUrlErrorrejects any CIMD metadata URL that is not HTTPS, with no loopback exemption, and applies that toclient.jsonon disk as well as to the settings form. So this server'shttp://document is not a legalclientMetadataUrl. It exists for the authorization-server side of the flow and for the assertions in this PR's test.Filed as #2305 — the same over-narrow allow-list shape as #2280 / #1944, from the other side. This PR delivers the fixture; #2305 is what would make it pleasant to use.
Two things worth knowing, now in the docs
client.json→cimd.{enabled,clientMetadataUrl}, reachable from Client settings), not per-server. AclientMetadataUrlwritten into a catalog entry'soauthblock is silently ignored.No UI or TUI surface changes, so no screenshots apply.
🤖 Generated with Claude Code
https://claude.ai/code/session_01RsmR1iQstcrEzJFmgZXGLi