docs: state the exemption's canonicalization contract, and fix the smoke recipe's cwd
Copilot review round 2. The exemption compares the WHATWG-canonicalized `URL.hostname`, so it covers alternate spellings of three loopback *addresses* rather than three input strings: `127.1`, `2130706433`, `0x7f.0.0.1`, `[0:0:0:0:0:0:0:1]` and a root-anchored `127.0.0.1.` all pass. The behavior is right — the security argument is about the address the request reaches, and all of these are the loopback interface — but the comment described it as a literal-input match, which it is not. It now states the contract, and six regression cases pin the aliases alongside the existing near-miss cases. This is also still an exact match for the SDK: `assertSecureTokenEndpoint` tests `url.hostname` against the same three values after the same `new URL` canonicalization, so an alias accepted here is one the SDK accepts too. Separately, the smoke guide's fixture recipe left the shell in `clients/web` after the build line, so the repo-root-relative paths on the next line resolved under `clients/web` and the copy-pasted setup failed before the server started. The build now runs in a subshell, and the block notes that the server announces its URL on stderr and walks past a taken port. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfHQ8vMm5NBgzbUJ6UwxmB Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
7b9090c024
commit
a21fd0c161
3 changed files with 49 additions and 10 deletions
|
|
@ -335,7 +335,27 @@ describe("client config", () => {
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("does not widen the exemption beyond the three literals", () => {
|
it("accepts alternate spellings that canonicalize to the same addresses", () => {
|
||||||
|
// The check runs against the WHATWG-canonicalized `URL.hostname`, so the
|
||||||
|
// exemption covers these three *addresses*, not three input strings. Each
|
||||||
|
// of these reaches the loopback interface, which is what the security
|
||||||
|
// argument is about — and the SDK canonicalizes identically, so an alias
|
||||||
|
// accepted here is one it accepts for a token endpoint too.
|
||||||
|
for (const value of [
|
||||||
|
"http://127.1:8090/client-metadata.json",
|
||||||
|
"http://2130706433:8090/client-metadata.json",
|
||||||
|
"http://0x7f.0.0.1:8090/client-metadata.json",
|
||||||
|
"http://[0:0:0:0:0:0:0:1]:8090/client-metadata.json",
|
||||||
|
// WHATWG strips a root-anchored dot from an IP literal but not from a
|
||||||
|
// name, which is why this passes where `localhost.` below does not.
|
||||||
|
"http://127.0.0.1.:8090/client-metadata.json",
|
||||||
|
"http://LOCALHOST:8090/client-metadata.json",
|
||||||
|
]) {
|
||||||
|
expect(getCimdClientMetadataUrlError(value)).toBeUndefined();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not widen the exemption beyond those three addresses", () => {
|
||||||
// `localhost.` and `*.localhost` resolve to loopback on every resolver,
|
// `localhost.` and `*.localhost` resolve to loopback on every resolver,
|
||||||
// but they are outside the SDK's allow-list — so they are outside ours
|
// but they are outside the SDK's allow-list — so they are outside ours
|
||||||
// too, deliberately, rather than the two disagreeing about one URL.
|
// too, deliberately, rather than the two disagreeing about one URL.
|
||||||
|
|
|
||||||
|
|
@ -83,19 +83,34 @@ export const CIMD_METADATA_URL_HTTPS_ERROR =
|
||||||
* validation would not let anyone enter, and CIMD could not be driven against
|
* validation would not let anyone enter, and CIMD could not be driven against
|
||||||
* any fixture in this repo (#2305).
|
* any fixture in this repo (#2305).
|
||||||
*
|
*
|
||||||
* ⚠️ This is the SDK's *three-literal* list, deliberately, and not the broader
|
* ⚠️ This is the SDK's own list, matched member for member — not the broader
|
||||||
* `isLoopbackHost` in `core/node/hostUrl.ts` — which imports `node:net` and so
|
* `isLoopbackHost` in `core/node/hostUrl.ts`, which imports `node:net` and so
|
||||||
* cannot be reached from this browser-safe module. Matching the SDK literal for
|
* cannot be reached from this browser-safe module. The SDK's
|
||||||
* literal also keeps the two allow-lists from disagreeing about the same URL.
|
* `assertSecureTokenEndpoint` tests `url.hostname` against exactly
|
||||||
* `::1` arrives from `URL.hostname` bracketed, which is the form stored here.
|
* `localhost` / `127.0.0.1` / `[::1]` (plus a `::1` arm `URL.hostname` never
|
||||||
|
* produces, since it brackets IPv6 literals). Keeping the two identical is what
|
||||||
|
* stops them disagreeing about one URL.
|
||||||
|
*
|
||||||
|
* ⚠️ **The comparison is against the WHATWG-canonicalized `URL.hostname`, so it
|
||||||
|
* covers alternate spellings of these three *addresses*, not three input
|
||||||
|
* strings.** `http://127.1/…`, `http://2130706433/…`, `http://0x7f.0.0.1/…` and
|
||||||
|
* `http://[0:0:0:0:0:0:0:1]/…` all canonicalize into the set and are accepted;
|
||||||
|
* so is a root-anchored `http://127.0.0.1./…`, which WHATWG strips the dot from
|
||||||
|
* for IP literals but *not* for `localhost.`. That is deliberate on both counts:
|
||||||
|
* the security argument is about the address the request actually reaches, and
|
||||||
|
* every one of these is the loopback interface. It is also exactly what the SDK
|
||||||
|
* does — it canonicalizes through `new URL` the same way — so an alias accepted
|
||||||
|
* here is an alias the SDK accepts too.
|
||||||
*/
|
*/
|
||||||
const CIMD_HTTP_EXEMPT_HOSTS = new Set(["localhost", "127.0.0.1", "[::1]"]);
|
const CIMD_HTTP_EXEMPT_HOSTS = new Set(["localhost", "127.0.0.1", "[::1]"]);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* True when a plain-`http:` CIMD metadata URL on this host is acceptable. Takes
|
* True when a plain-`http:` CIMD metadata URL on this host is acceptable. Takes
|
||||||
* the bare host (`URL.hostname`), never a `host:port`. Not exported: the only
|
* the already-canonicalized `URL.hostname`, never a raw authority or a
|
||||||
* caller is the validator below, and the exemption is exercised through it
|
* `host:port` — passing an unparsed string would compare the wrong thing and
|
||||||
* rather than through a second public surface that could drift from it.
|
* silently reject every alias above. Not exported: the only caller is the
|
||||||
|
* validator below, and the exemption is exercised through it rather than
|
||||||
|
* through a second public surface that could drift from it.
|
||||||
*/
|
*/
|
||||||
function isCimdHttpExemptHost(hostname: string): boolean {
|
function isCimdHttpExemptHost(hostname: string): boolean {
|
||||||
return CIMD_HTTP_EXEMPT_HOSTS.has(hostname);
|
return CIMD_HTTP_EXEMPT_HOSTS.has(hostname);
|
||||||
|
|
|
||||||
|
|
@ -513,10 +513,14 @@ Same MCP URL (`stytch-as-demo.val.run/mcp` or `mcp.stytch.dev/mcp`), leave CIMD
|
||||||
For offline CIMD regression — or a **strict** fail-with-CIMD-off / succeed-with-CIMD-on pair without Stytch’s DCR fallback — use the shipped `oauth-cimd-http.json` fixture, which sets `supportCIMD: true` and **`supportDCR: false`** and hosts the client metadata document itself at `/client-metadata.json`.
|
For offline CIMD regression — or a **strict** fail-with-CIMD-off / succeed-with-CIMD-on pair without Stytch’s DCR fallback — use the shipped `oauth-cimd-http.json` fixture, which sets `supportCIMD: true` and **`supportDCR: false`** and hosts the client metadata document itself at `/client-metadata.json`.
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
cd clients/web && npm run test-servers:build
|
# The build script lives in clients/web; both paths below are repo-root-relative,
|
||||||
|
# so come back up before starting the server.
|
||||||
|
(cd clients/web && npm run test-servers:build)
|
||||||
node test-servers/build/server-composable.js --config test-servers/configs/oauth-cimd-http.json
|
node test-servers/build/server-composable.js --config test-servers/configs/oauth-cimd-http.json
|
||||||
```
|
```
|
||||||
|
|
||||||
|
The server announces its URL on **stderr**, and walks to the next free port on `EADDRINUSE` — read the announced URL rather than assuming 8092.
|
||||||
|
|
||||||
Since [#2305](https://github.com/modelcontextprotocol/inspector/issues/2305) a loopback `http://` metadata URL is a **valid persisted `client.json` setting and a valid entry in the settings form**, not a test-only affordance — `getCimdClientMetadataUrlError` exempts `localhost`, `127.0.0.1` and `[::1]`, the same three literals the SDK exempts for token endpoints. So point `clientMetadataUrl` straight at the URL the fixture announced (`http://127.0.0.1:8092/client-metadata.json` on its configured port) with no HTTPS listener. Before that change the exemption existed only in `ensureCimdClientRegistration`, for an already-stored `client_id`, and driving this by hand needed a self-signed HTTPS listener plus `NODE_TLS_REJECT_UNAUTHORIZED=0`.
|
Since [#2305](https://github.com/modelcontextprotocol/inspector/issues/2305) a loopback `http://` metadata URL is a **valid persisted `client.json` setting and a valid entry in the settings form**, not a test-only affordance — `getCimdClientMetadataUrlError` exempts `localhost`, `127.0.0.1` and `[::1]`, the same three literals the SDK exempts for token endpoints. So point `clientMetadataUrl` straight at the URL the fixture announced (`http://127.0.0.1:8092/client-metadata.json` on its configured port) with no HTTPS listener. Before that change the exemption existed only in `ensureCimdClientRegistration`, for an already-stored `client_id`, and driving this by hand needed a self-signed HTTPS listener plus `NODE_TLS_REJECT_UNAUTHORIZED=0`.
|
||||||
|
|
||||||
With CIMD off in Client Settings, connect + authenticate should **fail**; with CIMD on and that local metadata URL, it should **succeed**. `docs/test-servers.md` has the full walkthrough, including the `redirect_uris` port trap.
|
With CIMD off in Client Settings, connect + authenticate should **fail**; with CIMD on and that local metadata URL, it should **succeed**. `docs/test-servers.md` has the full walkthrough, including the `redirect_uris` port trap.
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue