test: pin the CIMD form copy; narrow the "no exemption" claim in the docs
Copilot review round 5. Two of three findings taken; the third rests on a false premise about the SDK and is declined with evidence in the PR thread. The hint and the input description are part of this fix's user-facing contract — an HTTPS-only instruction there puts the local-fixture workflow back out of reach even while the validator stays permissive — and nothing asserted them. Two cases now do, plus one asserting a loopback http URL raises no error in the form. Each mutation (restoring either piece of old copy) fails exactly one of them. The docs said there was "no loopback exemption at all" before #2305, which contradicts the sentence above it: the runtime already tolerated an `http://` already-stored `client_id`, and that asymmetry is what the issue is about. The gap was in the config/form validator, and the wording now says so. Also added: a note to use the canonical spelling of the host. `127.1` and `2130706433` are accepted here and by the SDK, since both canonicalize to `127.0.0.1` — but a CIMD `client_id` is string-compared by the authorization server against the URL it dereferenced, so an exotic spelling is a real footgun independent of what either validator allows. 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
d7f90144c6
commit
8baa1216fd
2 changed files with 68 additions and 5 deletions
|
|
@ -312,6 +312,60 @@ describe("ClientSettingsForm EMA IdP session", () => {
|
|||
expect(screen.getByText(CIMD_METADATA_URL_HTTPS_ERROR)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("tells the reader the loopback exemption exists, in both pieces of CIMD copy", () => {
|
||||
renderWithMantine(
|
||||
<ClientSettingsForm
|
||||
settings={{
|
||||
...EMPTY_CLIENT_SETTINGS,
|
||||
cimdEnabled: true,
|
||||
clientMetadataUrl: "",
|
||||
}}
|
||||
expandedSections={["cimd"]}
|
||||
onExpandedSectionsChange={vi.fn()}
|
||||
onSettingsChange={vi.fn()}
|
||||
emaIdpLoginState="none"
|
||||
/>,
|
||||
);
|
||||
|
||||
// The copy is part of this feature's contract, not decoration: the whole
|
||||
// point of #2305 is that a local fixture URL is enterable, and an
|
||||
// HTTPS-only instruction here puts that back out of reach for a reader
|
||||
// even while the validator stays permissive. Both places that state the
|
||||
// requirement are asserted, since either one alone would still mislead.
|
||||
const hint = screen.getByText(/must be served over HTTPS/);
|
||||
expect(hint).toHaveTextContent("localhost, 127.0.0.1 or [::1]");
|
||||
|
||||
expect(
|
||||
screen.getByText(/HTTPS URL of your OAuth client metadata JSON document/),
|
||||
).toHaveTextContent(
|
||||
"http:// is accepted only on localhost, 127.0.0.1 or [::1]",
|
||||
);
|
||||
});
|
||||
|
||||
it("shows no CIMD URL error for a loopback http URL", () => {
|
||||
renderWithMantine(
|
||||
<ClientSettingsForm
|
||||
settings={{
|
||||
...EMPTY_CLIENT_SETTINGS,
|
||||
cimdEnabled: true,
|
||||
clientMetadataUrl: "http://localhost:8092/client-metadata.json",
|
||||
}}
|
||||
expandedSections={["cimd"]}
|
||||
onExpandedSectionsChange={vi.fn()}
|
||||
onSettingsChange={vi.fn()}
|
||||
emaIdpLoginState="none"
|
||||
revealErrors
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(
|
||||
screen.queryByText(CIMD_METADATA_URL_HTTPS_ERROR),
|
||||
).not.toBeInTheDocument();
|
||||
expect(
|
||||
screen.queryByText(CIMD_METADATA_URL_INVALID_ERROR),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("shows no CIMD URL error for a valid URL", () => {
|
||||
renderWithMantine(
|
||||
<ClientSettingsForm
|
||||
|
|
|
|||
|
|
@ -572,11 +572,20 @@ actually listening on; a host outside those three — `localhost.` and `tenant.a
|
|||
even though both resolve to loopback — is still rejected, deliberately, so this allow-list and the
|
||||
SDK's cannot disagree about one URL.
|
||||
|
||||
⚠️ **Before [#2305](https://github.com/modelcontextprotocol/inspector/issues/2305) there was no
|
||||
loopback exemption at all**, so driving this flow meant standing up a throwaway self-signed HTTPS
|
||||
listener to hold the document and setting `NODE_TLS_REJECT_UNAUTHORIZED=0` in the *test server's*
|
||||
environment so its own fetch of that document would succeed. That workaround is no longer needed — if
|
||||
you find it in a script or an older note, delete it.
|
||||
⚠️ **Before [#2305](https://github.com/modelcontextprotocol/inspector/issues/2305) the config and
|
||||
form validator had no loopback exemption** — the *runtime* already tolerated an `http://` URL as an
|
||||
already-stored `client_id`, which is precisely the asymmetry that issue is about; there was simply no
|
||||
way to get such a value past validation and into `client.json`. So driving this flow meant standing
|
||||
up a throwaway self-signed HTTPS listener to hold the document and setting
|
||||
`NODE_TLS_REJECT_UNAUTHORIZED=0` in the *test server's* environment so its own fetch of that document
|
||||
would succeed. That workaround is no longer needed — if you find it in a script or an older note,
|
||||
delete it.
|
||||
|
||||
⚠️ **Use the canonical spelling of the host.** `http://127.1/…` and `http://2130706433/…` are accepted
|
||||
by the validator (and by the SDK) because both canonicalize to `127.0.0.1` — but a CIMD `client_id`
|
||||
is compared as a **string** by the authorization server against the URL it dereferenced, so an
|
||||
exotic spelling can fail that comparison on a server that normalizes differently than the one here.
|
||||
Paste the origin the fixture announced rather than an equivalent you typed yourself.
|
||||
|
||||
⚠️ **Clear OAuth state before the run if you have connected to this fixture before.** Tokens and the
|
||||
registered client persist in `~/.mcp-inspector/storage/oauth.json` independently of the install-wide
|
||||
|
|
|
|||
Loading…
Reference in a new issue