diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0bc70bfc..32122205 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -100,7 +100,7 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo ## Gates and PR hygiene - `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating. -- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this, so a PR bumping a package that the shared sources import should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency. +- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this — it derives its candidates from what each `tsc` program actually resolves (`tsc --listFilesOnly`, keeping packages that reach one program from two installs), so a package reached only through another package's `.d.ts` counts too (#1965) — so a PR bumping a package the shared sources pull in should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency. - **A test or smoke must not touch real user state.** The web smokes run against a throwaway catalog via the shared `scripts/lib/prod-web-server.mjs` helper, never the developer's `~/.mcp-inspector/mcp.json` (#1977); the cli/tui smokes drive a temp `--catalog`. A new smoke spawning its own server, or teardown that removes a work dir without first awaiting `stopChild` (the #1801 race — `child-cleanup.mjs` exports both halves and both are required), should be flagged. - **Every PR references an issue**, first body line `Closes #`. - **Every PR carries exactly one version label**, `v1` or `v2`, matching its base branch. diff --git a/AGENTS.md b/AGENTS.md index 87d51230..d07e84f4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -38,7 +38,14 @@ v2/main/ │ │ # issuerBinding.ts SEP-2352 callback-leg failure │ │ # classification — separates a recoverable │ │ # "lost authorization state" from a genuine -│ │ # cross-AS issuer mismatch — #1808) +│ │ # cross-AS issuer mismatch — #1808; +│ │ # authorizationParams.ts per-server custom +│ │ # authorization-request parameters — the +│ │ # reserved-key list plus the merge applied in +│ │ # providers.redirectToAuthorization, the only +│ │ # seam that sees the SDK-built authorize URL; +│ │ # authorization request only, never the token +│ │ # request — #2018) │ │ ├── browser/ # Browser-side OAuth (sessionStorage, BrowserNavigation) │ │ ├── node/ # Node-side OAuth (NodeOAuthStorage, OAuthCallbackServer, │ │ │ # runner-interactive-oauth loopback callback flow) @@ -68,7 +75,11 @@ v2/main/ │ ├── mcp/ # InspectorClient runtime + state stores │ │ # (modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, -│ │ # used by the raw-wire tasks/* channel — #1631) +│ │ # used by the raw-wire tasks/* channel — #1631; +│ │ # listSalvage.ts: per-item salvage for list results — +│ │ # keeps the valid entries when one is non-conforming +│ │ # instead of losing the whole list, and owns the +│ │ # shared isClientDecodeRejection predicate — #1909) │ │ ├── import/ # Config import strategies (#1348): client-config parsers │ │ │ # (Claude Desktop/Cursor/Cline/VS Code), registry │ │ │ # server.json parser, strategy registry + well-known @@ -107,8 +118,11 @@ v2/main/ ├── scripts/ # Root build/verify tooling: install-clients.mjs (the │ # postinstall cascade), the smoke-*.mjs runners, │ # verify-build-gate / verify-format-coverage / -│ # verify-typecheck-coverage, pack-and-verify.mjs, -│ # and lib/ shared helpers. Prettier-gated via +│ # verify-typecheck-coverage / verify-dep-lockstep, +│ # pack-and-verify.mjs, and lib/ shared helpers +│ # (tsc-program.mjs is the `tsc --listFilesOnly` +│ # measurement both coverage guards read a program +│ # through — #1965). Prettier-gated via │ # `format:check:scripts`; its own pure parsers are │ # unit-tested by `npm run test:scripts` (node --test). ├── specification/ # Build specification @@ -675,12 +689,12 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - ALWAYS do `npm run format` before committing — the **root** `format` auto-fixes `core/` (`format:core`), the root `scripts/` tooling (`format:scripts`), the root "shared" surface (`format:shared` — `test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`), and every client's scope in one shot. Every **client** format glob uses the uniform extension set `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` (#1792) so a new-extension file can't slip the gate; `core/` stays `{ts,tsx}` and the shared surface `{ts,tsx,mts,cts}` (their surfaces can't hold the other extensions), and `npm run verify:format-coverage` (the first step of `validate`, #1792) is the backstop — it fails if any tracked source file is left uncovered by a `format:check` glob regardless of which glob was expected to catch it. `validate` runs `format:check` (the non-fixing variant, including `format:check:core`, `format:check:scripts`, and `format:check:shared`) and will fail in CI on any unformatted file, so always run the auto-fixer first rather than letting `format:check` catch it. - **`npm run ci` is the mandatory pre-push command** — it mirrors `.github/workflows/main.yml` (minus `npm install`): `validate` → `coverage` → `verify:build-gate` (the #1769 browser-externalized-builtin build gate) → `smoke` → Storybook play-function tests (installs Playwright chromium if needed). It now runs **`npm run coverage`**, the per-file ≥90 gate (lines/statements/functions/branches) that CI enforces — so `npm run ci` is a true superset of GitHub CI, and passing it locally means CI's gates will pass. Expect several minutes. **`npm run validate`** remains the fast inner-loop check during development (unit tests only — no coverage gate, no smoke, no Storybook), but it is **NOT** an acceptable substitute for `npm run ci` before pushing: `validate` runs `test`, not `test:coverage`, so it does **zero** coverage gating. Skipping the gate is how a push passes every fast local check and still fails CI (this exact gap broke PR #1601 on a function-coverage regression). -- ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then **`verify:typecheck-coverage`** (the #1791 guard — asserts every tracked `.ts`/`.tsx`/`.mts`/`.cts` in each gated Node client, plus the non-client first-party TS like `core/` and `test-servers/src`, lands in a tsconfig project), then **`verify:dep-lockstep`** (the #1896 guard — asserts no dependency *directly imported* by the shared sources resolves to two different versions across installs; transitive-only declarations are a known boundary, #1965), then **`test:scripts`** (the guards' own parser unit tests, `node --test`), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). +- ALWAYS do `npm run format` before committing, then **`npm run ci`** before pushing. From the repo root, `validate` runs **`verify:format-coverage` first** (the #1792 guard — asserts every tracked source file is covered by a `format:check` glob), then **`verify:typecheck-coverage`** (the #1791 guard — asserts every tracked `.ts`/`.tsx`/`.mts`/`.cts` in each gated Node client, plus the non-client first-party TS like `core/` and `test-servers/src`, lands in a tsconfig project), then **`verify:dep-lockstep`** (the #1896 guard — asserts no dependency that reaches a single `tsc` program from two installs resolves to two different versions across them), then **`test:scripts`** (the guards' own parser unit tests, `node --test`), then the **`core/` gate** (`validate:core`), then chains the four per-client validations (`validate:web` → `validate:cli` → `validate:tui` → `validate:launcher`); each client delegates to its own `npm run validate` in its own folder (no coverage — fast). Every client is self-validating and the top level just chains them, building each client's bundle along the way (no cross-client build dependencies). - **`validate:core` is the root-owned format + lint gate (#1689, widened in #1778 and #1767).** Each client's `prettier`/`eslint` is scoped to its own dir, so nothing reached `core/`, the root `scripts/`, or the root "shared" surface before — `validate:core` closes that: it runs `format:check:core` (`prettier --check "core/**/*.{ts,tsx}"`) + `format:check:scripts` (`prettier --check "scripts/**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}"`, the root build/verify tooling — #1778) + `format:check:shared` + `lint:core` (`eslint "core/**/*.{ts,tsx}"` via the **root** `eslint.config.js`) + `lint:shared`. Use `npm run format:core` / `npm run format:scripts` / `npm run format:shared` to auto-fix (all folded into the root `format`). The **shared surface** (#1767) is `test-servers/src/**/*.{ts,tsx,mts,cts}`, the root `vitest.shared.mts`, and the root `eslint.config.js` — first-party code no client's `eslint .` / `prettier` reaches; it is both prettier-gated (`format:check:shared`) and eslint-gated (`lint:shared`, via a second `files` block in the root `eslint.config.js` scoped to Node globals). The `scripts/` gate is prettier-only — the root has no eslint config for `.mjs`. The root carries prettier/eslint as devDependencies for this; `core/` is isomorphic (browser + Node globals, no JSX today — the `{ts,tsx}` glob future-proofs against a `core/**/*.tsx`). The root `eslint.config.js` honors an `_`-prefix as the intentionally-unused marker (`argsIgnorePattern`/`varsIgnorePattern`/`caughtErrorsIgnorePattern: '^_'`). **prettier is pinned to an exact version** (not a caret) in all five `package.json`s (#1790) so the gate's verdict can't shift with an in-range patch bump. - **cli and tui now typecheck their `src` (#1689).** Their `build`/`test` run through esbuild (no type check), so each has a `typecheck` script folded into `validate`. Their `tsconfig.json` matches `clients/web/tsconfig.app.json`'s module/lib _resolution_ options — DOM lib, `moduleResolution: bundler`, and **no** `noUncheckedIndexedAccess` (web's app config does not extend `tsconfig.base`, so re-enabling it would surface `core/` issues web never gates) — so the imported `core/` sources are validated the same way web validates them. It does **not** mirror web's extra strictness flags (`noUnusedLocals`, `verbatimModuleSyntax`, ES2023 target, …), so cli/tui's own `src` is checked slightly more loosely than web's. `core/` itself still typechecks through web's `tsc -b`. - **The `__tests__` dirs are typechecked too (#1791).** The src-only `tsconfig.json` excludes `**/*.test.*`, so each of cli, tui, and launcher carries a **`tsconfig.test.json`** — extending the build config, `noEmit`, including `__tests__/**/*` (only the tests root the project; tsc pulls in the `src` they import, and the src-only config already validates all of `src` without the test-only aliases) and adding the test-only path aliases that resolve what vitest resolves via `vitest.shared.mts`. The alias set differs per client: **cli's is the widest** (`@modelcontextprotocol/inspector-test-server` → `test-servers/src`, the `@inspector/core/*` deep paths, express/vitest — cli is the only one importing the test-server package); **tui's** carries only the `@inspector/core/*` + react/vitest redirects; **launcher's** has **no** `paths` at all — it's a plain `rootDir: "."` sibling of the build config (whose `rootDir: ./src` is what rejects the tests). Each client's `typecheck` script runs **both** projects (`tsc -p tsconfig.json && tsc -p tsconfig.test.json`) so running it standalone means the same thing everywhere (launcher's `build` also `tsc`s `src`, but `typecheck` doesn't rely on that). cli additionally carries `@types/express` (devDep) so the transitively-aliased test-server source typechecks, mirroring `clients/web` (cli's `tsconfig.test.json` also names `test-servers/src/server-composable.ts` explicitly — a bin entry the barrel doesn't import, so nothing else gives it a tsc pass). The client **config files** are typechecked too: cli's/tui's (`vitest.config.ts`, `tsup.config.ts`, tui `dev.ts`) are folded into each src `tsconfig.json`'s `include`; launcher's `vitest.config.ts` goes in its `tsconfig.test.json` instead (again the `rootDir: ./src` reason). Note the gate checks mock **implementations and return types** (typing a `vi.fn()` against a real signature keeps its `mockResolvedValue`/impl in sync) but **not** `toHaveBeenCalledWith(...)` arguments — vitest types those to accept anything regardless of the mock's type parameter. **`npm run verify:typecheck-coverage`** (`scripts/verify-typecheck-coverage.mjs`, run as the second step of `validate` right after `verify:format-coverage`) is the durable guard for this invariant: it runs each client's `typecheck` projects with `tsc --listFilesOnly`, unions them, and fails on any tracked `.ts`/`.tsx`/`.mts`/`.cts` that lands in no project — for every gated Node client, which it discovers from disk (each `clients/*` is enrolled through its `typecheck` script's projects, or — for a `tsc -b` client like `clients/web` with no `typecheck` script — through its `tsconfig.json` `references`), so a new client is covered without editing the guard — the typecheck analog of `verify:format-coverage`, since a project only reaches the files its `include` names plus their transitive imports, so a new top-level file (launcher especially, whose build `rootDir: ./src` rejects package-root files) can otherwise fall out silently. Like its sibling it also asserts the gate is _wired_ (each client's typecheck pass is reachable from its `validate` — its `typecheck` script for cli/tui/launcher, or a real `tsc -b` for web — and the root chain runs each client's `validate`), so it can't stay green while measuring a pass nothing invokes. It asserts the same of **`test:scripts`** — its own parser tests — on three axes: reachable from the root `validate`, a **non-empty** tracked `scripts/**/*.{test,spec}.*` set, and **every one of those files matched by a glob harvested across the scripts reachable from `test:scripts`** (so a delegating `test:scripts` still measures correctly). The third axis exists because `node --test` silently _skips_ a file its glob misses and still exits 0 — a rename to `*.spec.mjs` would shrink the suite with a green run. Beyond the clients it also covers, **deny-by-default**, the first-party TS no client owns — everything tracked outside `clients/*` (`test-servers/src/**`, the root `vitest.shared.mts`, **all of `core/`**, and any new top-level TS location) must land in the _global_ union of client projects (cli aliases the test-server source; web's enrolled projects include `core/`). So a `core` `*.tsx` web's `include` doesn't reach, or an unimported `test-servers/src` bin entry, can't ship uncompiled-but-unchecked. The one "listed but unchecked" tier the guard structurally can't see — a per-file `// @ts-nocheck` — is owned by a different gate: `@typescript-eslint/ban-ts-comment` rejects it across every surface (`lint:core`, `lint:shared`, and each client's `eslint .`). The guard's own pure parsers (`scripts/lib/npm-scripts.mjs` + the exported helpers of `verify-typecheck-coverage.mjs`, whose execution is behind a `main()` so importing it for tests doesn't run it) are **unit-tested** — `npm run test:scripts` (node's built-in `node --test`, in `validate`; the root has no vitest harness by design) runs table-driven cases, one per rule the guard's parsers encode, and the guard itself enforces that this stays wired (above). - The one CLI nuance: `clients/cli`'s out-of-process `e2e.test.ts` spawns the built binary, so its `test` **builds first** via `pretest` (`test-servers:build && build`). To avoid building it twice, `clients/cli`'s `validate` folds that in — it is `format:check && lint && typecheck && test` with **no** separate `build` step (the other clients, whose tests don't spawn their bundle, keep an explicit `build`). `validate:web`/`validate:tui`/`validate:launcher` are the uniform `format:check && lint && (typecheck &&) build && test`. (#1778, #1789, #1792) `clients/web`'s `format`/`format:check` covers `src`, `server`, `.storybook`, and its top-level configs (the uniform `*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` glob — `vite.config.ts`, `tsup.runner.config.ts`, `eslint.config.js`, …), not just `src`, so the Node backend, Storybook config, and Vite/build config are prettier-gated too; `clients/launcher`'s covers `src`, `__tests__`, `scripts`, and its top-level configs (the `*.` top-level glob is non-recursive, so each nested dir — `.storybook`, `scripts` — is named explicitly). The `verify:format-coverage` guard (#1792) enforces that this coverage stays complete. - - **One version per install-crossing dependency (#1896).** Because v2 is not a workspace, the root and each `clients/*` carry their own `node_modules` — and a client's `tsconfig.test.json` compiles first-party sources that live *outside* the client (`test-servers/src`, `core/`), which resolve their dependencies from the **root** install while the client's own sources resolve from the client install. So the same package can appear **twice in one `tsc` program**. At the same version that duplication is harmless; on a skew, TypeScript must relate two structurally-distinct declarations of the same type. For a deeply recursive-generic surface that is exponential: zod `4.3.6` (root) against zod `4.4.3` (`clients/web`) made `clients/web`'s `tsc -b` exhaust the 4GB default heap outright via `TS2589 Type instantiation is excessively deep`, because every `@modelcontextprotocol/*` schema is built out of zod generics. **Raising the heap with `--max-old-space-size` hides this class rather than fixing it — align the versions instead.** `npm run verify:dep-lockstep` (`scripts/verify-dep-lockstep.mjs`, in `validate`) is the durable guard: it **derives** the candidate set from the packages the shared first-party TypeScript imports — `core/`, `test-servers/src`, and the individually-named `vitest.shared.mts` (root-owned and imported by every client's vitest config) — so a new shared dependency is covered without editing the guard. It reads the committed lockfiles' **top-level** `node_modules/` entries — a *nested* transitive duplicate inside one install is routine and deliberately ignored — and fails, **deny-by-default**, on any candidate held at two versions across installs. The escape hatch is `TOLERATED_SKEW` in that file, an allowlist of *names* (not version pairs, so an ordinary patch float doesn't churn it), each entry carrying why that package's types can't blow up; `react`, `hono`, `jose`, and `@modelcontextprotocol/ext-apps` are listed today. **Being listed is not a blanket exemption** — it tolerates skew only *within a major version*, since a rationale about patch-level differences says nothing about a React 18-vs-19 split, where the type surface itself changes; a cross-major skew fails even for a listed package. **When bumping a dependency that the shared sources import, bump it in every install that declares it** — that's the root plus whichever clients list it, not all four unconditionally (launcher declares no zod, for instance, and a package absent from an install can't skew, so the guard ignores it there). Don't add a dependency to a client just to satisfy this. Its pure helpers are unit-tested via `test:scripts`, and it vouches — with `verify:format-coverage` and `verify:typecheck-coverage` — that its siblings are still wired into `validate`. + - **One version per install-crossing dependency (#1896).** Because v2 is not a workspace, the root and each `clients/*` carry their own `node_modules` — and a client's `tsconfig.test.json` compiles first-party sources that live *outside* the client (`test-servers/src`, `core/`), which resolve their dependencies from the **root** install while the client's own sources resolve from the client install. So the same package can appear **twice in one `tsc` program**. At the same version that duplication is harmless; on a skew, TypeScript must relate two structurally-distinct declarations of the same type. For a deeply recursive-generic surface that is exponential: zod `4.3.6` (root) against zod `4.4.3` (`clients/web`) made `clients/web`'s `tsc -b` exhaust the 4GB default heap outright via `TS2589 Type instantiation is excessively deep`, because every `@modelcontextprotocol/*` schema is built out of zod generics. **Raising the heap with `--max-old-space-size` hides this class rather than fixing it — align the versions instead.** `npm run verify:dep-lockstep` (`scripts/verify-dep-lockstep.mjs`, in `validate`) is the durable guard: it **derives** the candidate set from **what actually enters each `tsc` program** (#1965) — every client tsconfig project is listed with `tsc --listFilesOnly` through the shared `scripts/lib/tsc-program.mjs` helper (the same machinery `verify:typecheck-coverage` reads a program through, so the two guards can't disagree about what one contains), each resolved `node_modules` file is mapped to its owning install and package, and a package reaching **one** program from **two** installs is a candidate. That is precisely the set that can put two structurally-distinct copies of a type in front of one checker — and it needs no editing when a new dependency arrives. It replaced a derivation that read the packages the shared sources named *directly*, which could not see one whose declarations arrive only through another package's `.d.ts`: `@modelcontextprotocol/sdk` is never written in first-party code (the shared sources import the split `@modelcontextprotocol/client|core|…`) yet 16 of its `.d.ts` files land in `clients/web`'s test program, so a second copy under `clients/web/node_modules` skewed unseen. Two properties are worth knowing: a package present in two installs but reached from only one in a given program is correctly **not** a candidate, and TypeScript's package-identity redirect collapses two copies at the *same* name@version (so an aligned package's own transitive dependencies load once) — the redirect stops applying the moment they skew, which is exactly when the guard needs to see both. Versions still come from the committed lockfiles, but from the entry for the **exact install path the program resolved** (`node_modules/zod`, `node_modules/a/node_modules/zod`) rather than from the install's top-level entry: a *nested* duplicate inside one install is still not a candidate on its own — folding it onto its outermost install is what keeps the set small — but once a program has loaded one, pricing it from a top-level entry that may be absent or differently versioned would let a real pair pass (Copilot). Only the installs that actually **met in one program** are compared, so a third install's copy that no program loads beside another is not evidence of anything. Any co-occurrence whose copies disagree fails, **deny-by-default**; a resolved copy with no lockfile entry fails too, since the tree and the lockfile then disagree about what was loaded. The escape hatch is `TOLERATED_SKEW` in that file, an allowlist of *names* (not version pairs, so an ordinary patch float doesn't churn it), each entry carrying why that package's types can't blow up; it is **empty today** — the four names it used to carry (`react`, `hono`, `jose`, `@modelcontextprotocol/ext-apps`) were admitted under the old derivation and none is a candidate under this one, so each would be a rationale for a skew that cannot occur. **Being listed is not a blanket exemption** — it tolerates skew only *within a major version*, since a rationale about patch-level differences says nothing about a React 18-vs-19 split, where the type surface itself changes; a cross-major skew fails even for a listed package. **When bumping a dependency that the shared sources pull in, bump it in every install that declares it** — that's the root plus whichever clients list it, not all four unconditionally (launcher declares no zod, for instance, and a package absent from an install can't skew, so the guard ignores it there). Don't add a dependency to a client just to satisfy this. Its pure helpers are unit-tested via `test:scripts` (its own, plus `scripts/lib/tsc-program.test.mjs` for the shared derivation), and it vouches — with `verify:format-coverage` and `verify:typecheck-coverage` — that its siblings are still wired into `validate`. It runs the same `tsc --listFilesOnly` pass its sibling does, in its own process, costing ~14s: the listing is deliberately **not** cached to disk between the two, because a fingerprint that missed an input would make a guard measure a program that no longer exists and pass on a real miss. - **`npm run coverage`** is the per-file ≥90 gate and is now part of `npm run ci` — never treat it as optional before a push. It supersedes the old standalone `test:integration` step: web's `test:coverage` runs the `unit` **and** `integration` projects under v8 instrumentation, so `coverage` both enforces the ≥90 gate and exercises the same web integration paths CI covers. - **`smoke` is NOT part of `validate`** — it is included in `npm run ci`. It runs `smoke:launcher` (`--help` dispatch) plus the prod `smoke:cli` / `smoke:tui` / `smoke:web` / `smoke:web:browser` / `smoke:web:app`, and contains **no build commands** — it assumes the cli/tui/launcher bundles already exist (a full `validate` builds them; `smoke:web` builds `clients/web/dist` on demand). CI runs `validate`, then the `coverage` gate (which also covers the web integration project), then `verify:build-gate` (the #1769 build gate — see below), then `smoke` (with Playwright chromium installed just before it, since `smoke:web:browser` needs it). GitHub CI runs this same chain as separate workflow steps, with the Storybook play-function tests last (see below). - `smoke:launcher` (`scripts/smoke-launcher.mjs`) runs the built launcher with `--help`, `--cli --help`, and `--tui --help`, asserting each exits 0 and prints that mode's usage banner (which also proves the launcher resolved and loaded the right client build). It's the cheap dispatch check before the heavier prod smokes below. diff --git a/README.md b/README.md index 467a4fc0..cb57dd35 100644 --- a/README.md +++ b/README.md @@ -294,14 +294,14 @@ Each client self-validates from its own folder; the root scripts chain them. The | Script | What it does | | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency the shared sources directly import skews across installs) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | +| `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency reaching one `tsc` program from two installs skews across them) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | | `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus two headless-Chromium smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | +| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | -| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from what those shared sources import, compares the committed lockfiles' top-level entries, and **fails deny-by-default** on any skew not in the annotated `TOLERATED_SKEW` allowlist — and an allowlisted package is tolerated only *within a major version*. Runs in `validate`. | +| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | | `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index d70b9c27..1efcd408 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -507,6 +507,9 @@ function renderApp(servers: Record) { async function mount(servers: Record) { const r = renderApp(servers); await tick(); + // The mount commit's effect flush can still be queued behind this tick, and + // this write is the one that absorbs the dropped first keypress. + await settleInputHandlers(); r.stdin.write("x"); await tick(); return r; @@ -532,6 +535,11 @@ const ENTER = "\r"; */ async function press(r: RenderResult, keys: string[]) { for (const k of keys) { + // Same hazard `waitUntil` guards against, at the other end: a caller can + // reach here on a turn that still has React's passive-effect flush queued + // (e.g. straight after a plain `tick`, or after an earlier key committed a + // render), so settle before every write rather than only after a poll. + await settleInputHandlers(); r.stdin.write(k); await tick(); await tick(); @@ -552,9 +560,29 @@ async function press(r: RenderResult, keys: string[]) { */ const POLL_TRIES = 100; +/** + * One check-phase turn, queued BEHIND React's already-scheduled passive-effect + * flush. A frame observed by a poll predicate is written during React's + * COMMIT, but ink re-arms its useInput listeners in the passive-effect flush + * React schedules (via setImmediate in Node) during that same commit. Node's + * event loop runs the timers phase before the check phase, so a 25ms poll + * tick can observe the new frame and let the test write the next keypress + * BEFORE that flush has run — the key is then dispatched to the previous + * commit's stale useInput closures (where e.g. pendingStepUp is still null) + * and silently swallowed (#1942). Yielding one setImmediate turn after the + * predicate passes sequences the next stdin write after the flush (FIFO + * within the check queue), so "frame visible" once again implies "input + * handlers armed". + */ +const settleInputHandlers = () => + new Promise((resolve) => setImmediate(resolve)); + async function waitUntil(predicate: () => boolean, tries = POLL_TRIES) { for (let i = 0; i < tries; i++) { - if (predicate()) return; + if (predicate()) { + await settleInputHandlers(); + return; + } await tick(); } } @@ -626,6 +654,23 @@ afterEach(() => { while (mounted.length) mounted.pop()?.unmount(); }); +// Pins the synchronization contract the OAuth step-up assertions depend on +// (#1942). The `setImmediate` sentinel below stands in for React's pending +// passive-effect flush — the turn where ink re-arms `useInput`. If `waitUntil` +// ever returns without yielding a check-phase turn, the sentinel has not run +// and this fails, instead of the regression resurfacing as a differently-named +// flaky OAuth test under coverage instrumentation. +describe("test helpers", () => { + it("waitUntil settles input handlers before resolving", async () => { + let flushed = false; + setImmediate(() => { + flushed = true; + }); + await waitUntil(() => true); + expect(flushed).toBe(true); + }); +}); + describe("App (foundation)", () => { it("renders the server list with the MCP Servers header", async () => { const r = renderApp(stdioServer()); diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 0869d0d5..8ca842ed 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -598,6 +598,7 @@ const DEFAULT_USE_INSPECTOR_CLIENT: ReturnType = { serverInfo: undefined, instructions: undefined, excludedTools: [], + malformedListItems: [], appRendererClient: null, connect: vi.fn().mockResolvedValue(undefined), disconnect: vi.fn().mockResolvedValue(undefined), diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 509fc079..72d8bd1f 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -63,6 +63,7 @@ import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedReq import { ResourceSubscriptionsState } from "@inspector/core/mcp/state/resourceSubscriptionsState.js"; import { cleanRoots, + oauthAuthorizationParamsFromSettings, serializeMcpConfig, } from "@inspector/core/mcp/serverList.js"; import type { ClientConfig } from "@inspector/core/client/types.js"; @@ -163,6 +164,7 @@ import { import { buildExportFilename, downloadJsonFile } from "./lib/downloadFile"; import { INSPECTOR_SERVERS_TAB } from "./utils/inspectorTabs"; import { enrichProtocolEntries } from "./utils/correlateTransportErrors"; +import { visibleMalformedListItems } from "./utils/malformedListReport"; import { parseDeepLink, deepLinkConfigEquals, @@ -990,6 +992,7 @@ function App() { protocolEra, discoverResult, excludedTools, + malformedListItems, lastError, } = useInspectorClient(inspectorClient); const { @@ -1055,6 +1058,14 @@ function App() { setPaginatedListsOverride(null); }, [persistedPaginatedLists, activeServerId]); const paginatedLists = paginatedListsOverride ?? persistedPaginatedLists; + // The malformed-entry report is written by the aggregate walk's salvage. In + // paginated mode the tools/prompts/resources panels render the paged stores + // instead, which never write or clear it — so it would linger above a page it + // does not describe. See `visibleMalformedListItems` (#1909 + #1721). + const shownMalformedListItems = useMemo( + () => visibleMalformedListItems(malformedListItems, paginatedLists), + [malformedListItems, paginatedLists], + ); const connected = connectionStatus === "connected"; const toolsPagination = usePaginatedList({ connected, @@ -2301,11 +2312,15 @@ function App() { .map((m) => [m.key, m.value]), ) : undefined; + const serverAuthorizationParams = savedSettings + ? oauthAuthorizationParamsFromSettings(savedSettings) + : undefined; const oauthFromServer = savedSettings && (savedSettings.oauthClientId || savedSettings.oauthClientSecret || savedSettings.oauthScopes || + serverAuthorizationParams || savedSettings.enterpriseManaged) ? { ...(savedSettings.oauthClientId && { @@ -2317,6 +2332,9 @@ function App() { ...(savedSettings.oauthScopes && { scope: savedSettings.oauthScopes, }), + ...(serverAuthorizationParams && { + authorizationParams: serverAuthorizationParams, + }), ...(savedSettings.enterpriseManaged && { enterpriseManaged: true, }), @@ -4376,6 +4394,7 @@ function App() { latencyMs={latencyMs} tools={tools} excludedTools={excludedTools} + malformedListItems={shownMalformedListItems} prompts={prompts} resources={resources} resourceTemplates={resourceTemplates} diff --git a/clients/web/src/components/elements/CopyButton/CopyButton.test.tsx b/clients/web/src/components/elements/CopyButton/CopyButton.test.tsx index 9a4b462a..0b663cae 100644 --- a/clients/web/src/components/elements/CopyButton/CopyButton.test.tsx +++ b/clients/web/src/components/elements/CopyButton/CopyButton.test.tsx @@ -10,6 +10,17 @@ describe("CopyButton", () => { expect(screen.getByText("⎘")).toBeInTheDocument(); }); + it("names itself 'Copy' with no label, and qualifies the name when given one", () => { + const { unmount } = renderWithMantine(); + expect(screen.getByRole("button", { name: "Copy" })).toBeInTheDocument(); + unmount(); + + renderWithMantine(); + expect( + screen.getByRole("button", { name: "Copy ID Token" }), + ).toBeInTheDocument(); + }); + it("triggers a click without throwing when clipboard is unavailable", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/elements/CopyButton/CopyButton.tsx b/clients/web/src/components/elements/CopyButton/CopyButton.tsx index 7a86ca08..5b87719d 100644 --- a/clients/web/src/components/elements/CopyButton/CopyButton.tsx +++ b/clients/web/src/components/elements/CopyButton/CopyButton.tsx @@ -11,6 +11,15 @@ export interface CopyButtonProps { * rows (e.g. beside a Code block). Icon size is unchanged. */ flush?: boolean; + /** + * Names what is being copied, for the accessible label only ("Copy ID + * Token"). Set it when more than one CopyButton can appear in the same + * region, so button-by-button screen-reader navigation can tell them apart; + * a lone control needs no qualifier. The visible tooltip stays "Copy" either + * way, and the accessible name keeps "Copy"/"Copied" as its first word so it + * still contains the visible text (WCAG 2.5.3). + */ + label?: string; } const CopyActionIcon = ActionIcon.withProps({ @@ -18,7 +27,8 @@ const CopyActionIcon = ActionIcon.withProps({ fz: 24, }); -export function CopyButton({ value, flush = false }: CopyButtonProps) { +export function CopyButton({ value, flush = false, label }: CopyButtonProps) { + const qualifier = label ? ` ${label}` : ""; return ( {({ copied, copy }) => ( @@ -26,7 +36,7 @@ export function CopyButton({ value, flush = false }: CopyButtonProps) { {copied ? "\u2713" : "\u2398"} diff --git a/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx new file mode 100644 index 00000000..756e8667 --- /dev/null +++ b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.stories.tsx @@ -0,0 +1,89 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { MalformedItemsWarning } from "./MalformedItemsWarning"; + +const meta: Meta = { + title: "Elements/MalformedItemsWarning", + component: MalformedItemsWarning, +}; + +export default meta; +type Story = StoryObj; + +/** The failure this was built for: a PHP server whose empty `annotations` + * object reaches the wire as `[]` (#1909). */ +export const OneDroppedTemplate: Story = { + args: { + method: "resources/templates/list", + what: "resource templates", + items: [ + { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: "annotations: Invalid input: expected object, received array", + }, + ], + }, +}; + +export const SeveralDropped: Story = { + args: { + method: "tools/list", + what: "tools", + items: [ + { + method: "tools/list", + index: 0, + label: "get_weather", + reason: "inputSchema: Invalid input: expected object, received string", + }, + { + method: "tools/list", + index: 4, + label: "add", + reason: "name: Invalid input: expected string, received number", + }, + ], + }, +}; + +/** An entry too broken to carry a name is reported by its position. */ +export const UnlabeledEntry: Story = { + args: { + method: "prompts/list", + what: "prompts", + items: [ + { + method: "prompts/list", + index: 2, + reason: "Invalid input: expected object, received null", + }, + ], + }, +}; + +/** Entries belonging to another list method are ignored — the same set is + * passed to every panel, and each selects its own. */ +export const OtherMethodOnly: Story = { + args: { + method: "resources/templates/list", + what: "resource templates", + items: [ + { + method: "tools/list", + index: 0, + label: "get_weather", + reason: "inputSchema: Invalid input: expected object, received string", + }, + ], + }, +}; + +/** The resting state: a conforming server, nothing rendered. */ +export const Conforming: Story = { + args: { + method: "resources/templates/list", + what: "resource templates", + items: [], + }, +}; diff --git a/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx new file mode 100644 index 00000000..ed8c03f0 --- /dev/null +++ b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.test.tsx @@ -0,0 +1,97 @@ +import { describe, it, expect } from "vitest"; +import { screen } from "@testing-library/react"; +import type { MalformedListItem } from "@inspector/core/mcp"; +import { renderWithMantine } from "../../../test/renderWithMantine"; +import { MalformedItemsWarning } from "./MalformedItemsWarning"; + +const TEMPLATE_ENTRY: MalformedListItem = { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: "annotations: Invalid input: expected object, received array", +}; + +const TOOL_ENTRY: MalformedListItem = { + method: "tools/list", + index: 0, + label: "get_weather", + reason: "inputSchema: Invalid input: expected object, received string", +}; + +describe("MalformedItemsWarning", () => { + it("names the dropped entry and why it was dropped", () => { + renderWithMantine( + , + ); + + expect(screen.getByText("1 malformed entry dropped")).toBeInTheDocument(); + expect(screen.getByText("array_annotations")).toBeInTheDocument(); + expect(screen.getByText(TEMPLATE_ENTRY.reason)).toBeInTheDocument(); + }); + + it("renders nothing when the server was conforming", () => { + renderWithMantine( + , + ); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + }); + + it("ignores entries belonging to another list method", () => { + // Every panel receives the whole set and selects its own — a tools failure + // must not surface above the templates list. + renderWithMantine( + , + ); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + expect(screen.queryByText("get_weather")).not.toBeInTheDocument(); + }); + + it("pluralizes the count and lists every entry", () => { + renderWithMantine( + , + ); + + expect(screen.getByText("2 malformed entries dropped")).toBeInTheDocument(); + expect(screen.getByText("get_weather")).toBeInTheDocument(); + expect(screen.getByText("add")).toBeInTheDocument(); + // The template entry belongs to the other panel. + expect(screen.queryByText("array_annotations")).not.toBeInTheDocument(); + }); + + it("falls back to the position when the entry had no usable name", () => { + renderWithMantine( + , + ); + expect(screen.getByText("entry 2")).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx new file mode 100644 index 00000000..26bc5d5c --- /dev/null +++ b/clients/web/src/components/elements/MalformedItemsWarning/MalformedItemsWarning.tsx @@ -0,0 +1,99 @@ +import { Alert, Code, List, ScrollArea, Text } from "@mantine/core"; +import type { MalformedListItem } from "@inspector/core/mcp"; + +export interface MalformedItemsWarningProps { + /** + * Every malformed entry the client is currently reporting, across all list + * methods — pass the hook's set straight through; the component selects its + * own by {@link method}. + */ + items: MalformedListItem[]; + /** The list method this panel renders, e.g. `"resources/templates/list"`. */ + method: string; + /** What the entries are, for the alert title — e.g. "resource templates". */ + what: string; +} + +// Yellow, not red: unlike ListLoadError this is not a failure — the list DID +// load and is rendered below. It reports what the server sent that the +// Inspector could not accept. +const WarningAlert = Alert.withProps({ + color: "yellow", + variant: "light", +}); + +// The entry list stays compact; the reasons are the payload, so they get the +// monospace treatment while the labels stay in body type. +const EntryList = List.withProps({ + size: "xs", + spacing: 4, + withPadding: true, +}); + +const EntryReason = Code.withProps({ + variant: "wrapping", +}); + +// A server can be wrong about many entries at once; cap the height so a badly +// broken list can't push the list itself off the sidebar. +const EntryScroll = ScrollArea.withProps({ + mah: 180, + type: "auto", +}); + +const SummaryText = Text.withProps({ + size: "xs", + mb: 4, +}); + +// The entry's identity, emphasized against its monospaced reason. +const EntryLabel = Text.withProps({ + component: "span", + size: "xs", + fw: 600, +}); + +/** `label` when the entry had one, else its position in the response. */ +function describeEntry(item: MalformedListItem): string { + return item.label ?? `entry ${item.index}`; +} + +/** + * "Some entries were dropped" — the visible half of the #1909 salvage. + * + * The client keeps the valid entries of a list whose result failed schema + * validation, rather than losing the whole list to one bad entry. Dropping them + * silently would trade one lie for another, so what was dropped, and why, is + * reported here beside the list that rendered without them. + */ +export function MalformedItemsWarning({ + items, + method, + what, +}: MalformedItemsWarningProps) { + const mine = items.filter((item) => item.method === method); + if (mine.length === 0) return null; + + return ( + + + The server returned {what} that don't match the MCP schema. The + rest of the list is shown below. + + + + {mine.map((item) => ( + + {describeEntry(item)}{" "} + {item.reason} + + ))} + + + + ); +} diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx index 3b1c919b..c6f1e598 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx @@ -3,7 +3,7 @@ import type { InitializeResult, } from "@modelcontextprotocol/client"; import type { Meta, StoryObj } from "@storybook/react-vite"; -import { expect, within } from "storybook/test"; +import { expect, userEvent, within } from "storybook/test"; import { ConnectionInfoContent, SERVER_INFO_NOT_REPORTED_LABEL, @@ -161,3 +161,37 @@ export const WithOAuth: Story = { }, }, }; + +// An authorization server that also returned an `id_token` (#2019). Shown for +// inspection only — decoding it is display-only, and it is never used as a +// credential. +export const WithOAuthIdToken: Story = { + args: { + initializeResult: { + protocolVersion: "2025-03-26", + serverInfo: { name: "OIDC-fronted Server", version: "3.0.0" }, + capabilities: { tools: { listChanged: true } }, + }, + clientCapabilities: { roots: { listChanged: true } }, + transport: "streamable-http", + oauth: { + protocol: "standard", + authorized: true, + authUrl: "https://auth.example.com/oauth2/authorize", + scopes: ["openid", "email", "read"], + accessToken: "opaque-access-token", + // header {"alg":"none"} / payload {"sub":"user-42","email":"a@b.test"} + idToken: + "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyLTQyIiwiZW1haWwiOiJhQGIudGVzdCJ9.", + }, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect(canvas.getByText("ID Token")).toBeInTheDocument(); + // Only the ID token is a JWT here, so exactly one decode toggle is offered. + await userEvent.click( + canvas.getByRole("button", { name: "Decode JWT for ID Token" }), + ); + expect(canvas.getByText(/"sub": "user-42"/)).toBeInTheDocument(); + }, +}; diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index 9f9a8ea4..f9f3d2fa 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -473,6 +473,95 @@ describe("ConnectionInfoContent", () => { expect(screen.queryByText("Auth URL")).not.toBeInTheDocument(); expect(screen.queryByText("Scopes")).not.toBeInTheDocument(); expect(screen.queryByText("Access Token")).not.toBeInTheDocument(); + expect(screen.queryByText("ID Token")).not.toBeInTheDocument(); + }); + + it("renders the ID Token row when the token set carries one", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Access Token")).toBeInTheDocument(); + expect(screen.getByText("ID Token")).toBeInTheDocument(); + expect(screen.getByText("id-token-456")).toBeInTheDocument(); + }); + + it("gives the two token rows' controls distinct accessible names", () => { + // Both rows carry a copy control, and both tokens here are JWTs so both + // carry a decode toggle. Screen-reader button navigation has only the + // accessible name to go on, so the four must not collide (#2019 review). + const jwt = "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyIn0."; + renderWithMantine( + , + ); + + const names = screen + .getAllByRole("button") + .map((button) => button.getAttribute("aria-label") ?? button.textContent) + .filter((name): name is string => Boolean(name)); + expect(new Set(names).size).toBe(names.length); + + for (const name of [ + "Copy Access Token", + "Copy ID Token", + "Decode JWT for Access Token", + "Decode JWT for ID Token", + ]) { + expect(screen.getByRole("button", { name })).toBeInTheDocument(); + } + }); + + it("renders the ID Token row on its own when there is no access token", () => { + renderWithMantine( + , + ); + expect(screen.queryByText("Access Token")).not.toBeInTheDocument(); + expect(screen.getByText("ID Token")).toBeInTheDocument(); + }); + + it("omits the ID Token row when only an access token is present", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Access Token")).toBeInTheDocument(); + expect(screen.queryByText("ID Token")).not.toBeInTheDocument(); }); it("does not render OAuth section when oauth prop is omitted", () => { diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx index a7cd53f2..dde264b6 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx @@ -24,7 +24,7 @@ import { import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; import { EraBadge } from "../../elements/EraBadge/EraBadge"; import { isModernEra } from "../../elements/EraBadge/eraUtils"; -import { OAuthAccessTokenField } from "./OAuthAccessTokenField"; +import { OAuthTokenField } from "./OAuthTokenField"; export interface OAuthDetails { protocol: "standard" | "ema"; @@ -34,6 +34,13 @@ export interface OAuthDetails { authUrl?: string; scopes?: string[]; accessToken?: string; + /** + * OIDC `id_token` from the stored token set, when the authorization server + * returned one. Shown for inspection only — the MCP authorization spec is + * plain OAuth 2.1 and the Inspector never treats this as a credential + * (#2019). Absent when the token set carries none, so no empty row renders. + */ + idToken?: string; /** EMA only — install-level IdP session for legs 1–2. */ idpSession?: "none" | "logged_in" | "expired"; } @@ -370,20 +377,27 @@ export function ConnectionInfoContent({ {formatScopes(oauth.scopes)} )} - {oauth.accessToken ? ( - - ) : ( - onClearOAuth && ( - - - {CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL} - - - ) + )} + {/* Viewer only — an `id_token` the AS happened to return, decoded + on request. It carries no clear action: clearing OAuth state is + one action for the whole token set, owned by the access-token + row (or the standalone button below when there is none). */} + {oauth.idToken && ( + + )} + {!oauth.accessToken && onClearOAuth && ( + + + {CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL} + + )} diff --git a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.test.tsx deleted file mode 100644 index f1380f12..00000000 --- a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.test.tsx +++ /dev/null @@ -1,90 +0,0 @@ -import { describe, it, expect, vi } from "vitest"; -import userEvent from "@testing-library/user-event"; -import { renderWithMantine, screen } from "../../../test/renderWithMantine"; -import { CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL } from "./ConnectionInfoContent"; -import { OAuthAccessTokenField } from "./OAuthAccessTokenField"; - -const jwt = "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyIn0."; - -describe("OAuthAccessTokenField", () => { - it("renders token with copy control beside the content", () => { - renderWithMantine(); - expect(screen.getByText("Access Token")).toBeInTheDocument(); - expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); - expect(screen.getByText(/eyJzdWIiOiJ1c2VyIn0/)).toBeInTheDocument(); - expect(screen.getByRole("button", { name: "Copy" })).toBeInTheDocument(); - }); - - it("replaces raw token with decoded JSON and restores on toggle", async () => { - const user = userEvent.setup(); - renderWithMantine(); - - expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); - await user.click(screen.getByRole("button", { name: "Decode JWT" })); - expect(screen.getByText(/"sub": "user"/)).toBeInTheDocument(); - expect(screen.queryByText(/eyJhbGciOiJub25lIn0/)).not.toBeInTheDocument(); - - await user.click(screen.getByRole("button", { name: "Show token" })); - expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); - expect(screen.queryByText(/"sub": "user"/)).not.toBeInTheDocument(); - }); - - it("omits decode toggle for opaque tokens", () => { - renderWithMantine( - , - ); - expect( - screen.queryByRole("button", { name: "Decode JWT" }), - ).not.toBeInTheDocument(); - }); - - it("copies decoded JSON while decode view is shown", async () => { - const user = userEvent.setup(); - const writeText = vi.fn().mockResolvedValue(undefined); - Object.defineProperty(navigator, "clipboard", { - value: { writeText }, - configurable: true, - }); - - renderWithMantine(); - await user.click(screen.getByRole("button", { name: "Decode JWT" })); - await user.click(screen.getByRole("button", { name: "Copy" })); - - expect(writeText).toHaveBeenCalledWith( - expect.stringContaining('"sub": "user"'), - ); - expect(writeText).not.toHaveBeenCalledWith(jwt); - }); - - it("copies the raw token while token view is shown", async () => { - const user = userEvent.setup(); - const writeText = vi.fn().mockResolvedValue(undefined); - Object.defineProperty(navigator, "clipboard", { - value: { writeText }, - configurable: true, - }); - - renderWithMantine(); - await user.click(screen.getByRole("button", { name: "Copy" })); - - expect(writeText).toHaveBeenCalledWith(jwt); - }); - - it("renders clear action on the access token header row", async () => { - const user = userEvent.setup(); - const onClear = vi.fn(); - renderWithMantine( - , - ); - await user.click( - screen.getByRole("button", { - name: CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL, - }), - ); - expect(onClear).toHaveBeenCalledTimes(1); - }); -}); diff --git a/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.test.tsx new file mode 100644 index 00000000..a46e6538 --- /dev/null +++ b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.test.tsx @@ -0,0 +1,139 @@ +import { describe, it, expect, vi } from "vitest"; +import userEvent from "@testing-library/user-event"; +import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL } from "./ConnectionInfoContent"; +import { OAuthTokenField } from "./OAuthTokenField"; + +const jwt = "eyJhbGciOiJub25lIn0.eyJzdWIiOiJ1c2VyIn0."; + +describe("OAuthTokenField", () => { + it("renders token with copy control beside the content", () => { + renderWithMantine(); + expect(screen.getByText("Access Token")).toBeInTheDocument(); + expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); + expect(screen.getByText(/eyJzdWIiOiJ1c2VyIn0/)).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Copy Access Token" }), + ).toBeInTheDocument(); + }); + + it("replaces raw token with decoded JSON and restores on toggle", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); + await user.click( + screen.getByRole("button", { name: "Decode JWT for Access Token" }), + ); + expect(screen.getByText(/"sub": "user"/)).toBeInTheDocument(); + expect(screen.queryByText(/eyJhbGciOiJub25lIn0/)).not.toBeInTheDocument(); + + await user.click( + screen.getByRole("button", { name: "Show token for Access Token" }), + ); + expect(screen.getByText(/eyJhbGciOiJub25lIn0/)).toBeInTheDocument(); + expect(screen.queryByText(/"sub": "user"/)).not.toBeInTheDocument(); + }); + + it("omits decode toggle for opaque tokens", () => { + renderWithMantine( + , + ); + expect( + screen.queryByRole("button", { name: "Decode JWT for Access Token" }), + ).not.toBeInTheDocument(); + }); + + it("copies decoded JSON while decode view is shown", async () => { + const user = userEvent.setup(); + const writeText = vi.fn().mockResolvedValue(undefined); + Object.defineProperty(navigator, "clipboard", { + value: { writeText }, + configurable: true, + }); + + renderWithMantine(); + await user.click( + screen.getByRole("button", { name: "Decode JWT for Access Token" }), + ); + await user.click(screen.getByRole("button", { name: "Copy Access Token" })); + + expect(writeText).toHaveBeenCalledWith( + expect.stringContaining('"sub": "user"'), + ); + expect(writeText).not.toHaveBeenCalledWith(jwt); + }); + + it("copies the raw token while token view is shown", async () => { + const user = userEvent.setup(); + const writeText = vi.fn().mockResolvedValue(undefined); + Object.defineProperty(navigator, "clipboard", { + value: { writeText }, + configurable: true, + }); + + renderWithMantine(); + await user.click(screen.getByRole("button", { name: "Copy Access Token" })); + + expect(writeText).toHaveBeenCalledWith(jwt); + }); + + it("renders clear action on the access token header row", async () => { + const user = userEvent.setup(); + const onClear = vi.fn(); + renderWithMantine( + , + ); + await user.click( + screen.getByRole("button", { + name: CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL, + }), + ); + expect(onClear).toHaveBeenCalledTimes(1); + }); + + it("labels the row from the label prop and decodes an ID token", async () => { + const user = userEvent.setup(); + // header {"alg":"none"} / payload {"sub":"user"} — same shape an AS returns + // for an `id_token`; the decode path is identical to the access token's. + renderWithMantine(); + + expect(screen.getByText("ID Token")).toBeInTheDocument(); + expect(screen.queryByText("Access Token")).not.toBeInTheDocument(); + + await user.click( + screen.getByRole("button", { name: "Decode JWT for ID Token" }), + ); + expect(screen.getByText(/"sub": "user"/)).toBeInTheDocument(); + }); + + it("qualifies the decode and copy accessible names with the row label", () => { + // Two rows can render simultaneously (Access Token + ID Token), so the + // controls must not share accessible names — see #2019 review. + renderWithMantine(); + + expect( + screen.getByRole("button", { name: "Decode JWT for ID Token" }), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Copy ID Token" }), + ).toBeInTheDocument(); + }); + + it("omits the clear action when no handler is given", () => { + renderWithMantine(); + expect( + screen.queryByRole("button", { + name: CLEAR_OAUTH_STATE_AND_DISCONNECT_LABEL, + }), + ).not.toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.tsx b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx similarity index 70% rename from clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.tsx rename to clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx index 825a8507..abd5c4a9 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/OAuthAccessTokenField.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/OAuthTokenField.tsx @@ -3,8 +3,18 @@ import { Button, Code, Flex, Stack, Text } from "@mantine/core"; import { decodeJwtPayload, isJwtFormat } from "@inspector/core/auth/ema/jwt.js"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; -export interface OAuthAccessTokenFieldProps { - accessToken: string; +/** + * One labelled OAuth token row: the raw value, a copy control, and — when the + * value has the three-segment JWT shape — an in-place "Decode JWT" toggle. + * + * Shared by the Access Token and ID Token rows (#2019). The decode is + * display-only (`decodeJwtPayload` does no signature verification), so this is + * a viewer: neither token is treated as a credential here. + */ +export interface OAuthTokenFieldProps { + /** Row caption, e.g. "Access Token" / "ID Token". */ + label: string; + token: string; onClear?: () => void; clearLabel?: string; } @@ -64,16 +74,17 @@ function JwtTokenText({ token }: { token: string }) { ); } -export function OAuthAccessTokenField({ - accessToken, +export function OAuthTokenField({ + label, + token, onClear, clearLabel = "Clear", -}: OAuthAccessTokenFieldProps) { +}: OAuthTokenFieldProps) { const [showDecoded, setShowDecoded] = useState(false); - const isJwt = isJwtFormat(accessToken); + const isJwt = isJwtFormat(token); const jwtDecoded = useMemo( - () => (isJwt ? decodeJwtPayload(accessToken) : undefined), - [accessToken, isJwt], + () => (isJwt ? decodeJwtPayload(token) : undefined), + [token, isJwt], ); const decodedText = useMemo(() => { @@ -85,17 +96,18 @@ export function OAuthAccessTokenField({ ); }, [jwtDecoded]); - const copyValue = showDecoded && decodedText ? decodedText : accessToken; + const copyValue = showDecoded && decodedText ? decodedText : token; return ( - Access Token + {label} {jwtDecoded && ( setShowDecoded((open) => !open)} aria-pressed={showDecoded} + aria-label={`${showDecoded ? "Show token" : "Decode JWT"} for ${label}`} > {showDecoded ? "Show token" : "Decode JWT"} @@ -113,13 +125,13 @@ export function OAuthAccessTokenField({ {showDecoded && decodedText ? ( decodedText ) : isJwt ? ( - + ) : ( - accessToken + token )} - + ); diff --git a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts index d6a89da6..d047ab23 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts +++ b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.test.ts @@ -34,6 +34,39 @@ describe("oauthDetailsFromConnectionState", () => { }); }); + it("surfaces an id_token when the token set carries one", () => { + const state: OAuthConnectionState = { + authorized: true, + protocol: "standard", + serverUrl: "https://mcp.example.com/mcp", + tokens: { + access_token: "tok", + token_type: "Bearer", + id_token: "header.payload.signature", + }, + }; + + expect(oauthDetailsFromConnectionState(state)).toEqual({ + protocol: "standard", + authorized: true, + accessToken: "tok", + idToken: "header.payload.signature", + }); + }); + + it("omits idToken when the token set carries none", () => { + const state: OAuthConnectionState = { + authorized: true, + protocol: "standard", + serverUrl: "https://mcp.example.com/mcp", + tokens: { access_token: "tok", token_type: "Bearer" }, + }; + + expect(oauthDetailsFromConnectionState(state)).not.toHaveProperty( + "idToken", + ); + }); + it("includes EMA idp session when present", () => { const state: OAuthConnectionState = { authorized: false, diff --git a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts index 14c8847e..2e514633 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts +++ b/clients/web/src/components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState.ts @@ -22,6 +22,11 @@ export function oauthDetailsFromConnectionState( ...(state.tokens?.access_token && { accessToken: state.tokens.access_token, }), + // Surfaced for inspection when the AS returned one; omitted entirely + // otherwise so Connection Info renders no empty row (#2019). + ...(state.tokens?.id_token && { + idToken: state.tokens.id_token, + }), ...(state.protocol === "ema" && state.ema?.idpSession && { idpSession: state.ema.idpSession }), }; diff --git a/clients/web/src/components/groups/PromptControls/PromptControls.tsx b/clients/web/src/components/groups/PromptControls/PromptControls.tsx index 2a2d5364..2fbb5633 100644 --- a/clients/web/src/components/groups/PromptControls/PromptControls.tsx +++ b/clients/web/src/components/groups/PromptControls/PromptControls.tsx @@ -3,6 +3,8 @@ import { ClearButton } from "../../elements/ClearButton/ClearButton"; import type { Prompt } from "@modelcontextprotocol/client"; import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListChangedIndicator"; import { ListLoadError } from "../../elements/ListLoadError/ListLoadError"; +import { MalformedItemsWarning } from "../../elements/MalformedItemsWarning/MalformedItemsWarning"; +import type { MalformedListItem } from "@inspector/core/mcp"; import { ListPaginationControls, type ListPaginationControlsProps, @@ -41,6 +43,12 @@ export interface PromptControlsProps { * A failed list load, surfaced above the list instead of leaving the panel * empty (which reads as "this server has none") (#1953). */ + /** + * Entries the client dropped from this list because they failed the MCP + * schema. Rendered as a warning above the list, which still shows the rest + * (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls (#1721). */ pagination: ListPaginationControlsProps; @@ -54,6 +62,7 @@ export function PromptControls({ searchText = "", listChanged, onRefreshList, + malformedListItems = [], loadError, pagination, onSearchChange, @@ -83,6 +92,11 @@ export function PromptControls({ /> + {filteredPrompts.map((prompt) => ( diff --git a/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx b/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx index adf5aa1a..6a319fb1 100644 --- a/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx +++ b/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx @@ -14,6 +14,8 @@ import { isModernEra } from "../../elements/EraBadge/eraUtils"; import { SubscriptionStreamBadge } from "../../elements/SubscriptionStreamBadge/SubscriptionStreamBadge"; import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListChangedIndicator"; import { ListLoadError } from "../../elements/ListLoadError/ListLoadError"; +import { MalformedItemsWarning } from "../../elements/MalformedItemsWarning/MalformedItemsWarning"; +import type { MalformedListItem } from "@inspector/core/mcp"; import { ListPaginationControls, type ListPaginationControlsProps, @@ -73,6 +75,12 @@ export interface ResourceControlsProps { * A failed list load, surfaced above the list instead of leaving the panel * empty (which reads as "this server has none") (#1953). */ + /** + * Entries the client dropped from `resources/list` or + * `resources/templates/list` because they failed the MCP schema. Rendered as + * a warning above the list, which still shows the rest (#1909). + */ + malformedListItems?: MalformedListItem[]; loadError?: Error | null; /** Pagination controls for the Resources list (#1721). */ pagination: ListPaginationControlsProps; @@ -117,6 +125,7 @@ export function ResourceControls({ openSections: controlledOpenSections, listChanged, onRefreshList, + malformedListItems = [], loadError, pagination, onSearchChange, @@ -248,6 +257,16 @@ export function ResourceControls({ what="resources" onRetry={onRefreshList} /> + + {/* Stays inline: Accordion is a compound, `multiple`-discriminated generic, so `.withProps({ multiple: true, ... })` loses its JSX call signature (same tooling limit as Box). */} diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 6c7dd77b..b1d3182f 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -1477,3 +1477,316 @@ describe("SchemaForm draft validity (#2020)", () => { expect(screen.getByText(/Not valid JSON/)).toBeInTheDocument(); }); }); + +// Three separate reports of one defect: the JSON editor re-escaping the text +// being typed. #1853 saw it while typing an array, #1856 on the Backspace that +// first makes a valid array invalid, and #1885 on a nullable parameter whose +// default `null` had to be edited in place. All three come from the same +// mechanism — unparseable draft text stored back as the field's *value*, which +// the next controlled render re-`JSON.stringify`d, adding a layer of quotes and +// backslashes per keystroke. +// +// The mechanism was removed by the draft/value split in `SchemaJsonField` +// (#1928/#2007) and the nullable-union collapse that keeps a `T | null` field +// off this editor entirely (#1928). These lock each report's own reproduction +// to it, since the fixes were made for differently-framed issues and nothing +// otherwise pins the reported flows. +describe("JSON editor escaping (#1853, #1856, #1885)", () => { + function EscapingHarness({ + schema, + initial = {}, + }: { + schema: InspectorFormSchema; + initial?: Record; + }) { + const [values, setValues] = useState>(initial); + return ; + } + + // #1853: `batch_process_items`, typed character by character. + it("types a string array through without escaping it (#1853)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + itemIds: { + type: "array", + title: "Item Ids", + items: { type: "string" }, + }, + }, + required: ["itemIds"], + }; + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Item Ids/) as HTMLTextAreaElement; + // `[` opens a userEvent keyboard descriptor, so it is escaped as `[[`. + await user.type(jsonInput, '[["item-1","item-2"]'); + + expect(jsonInput.value).toBe('["item-1","item-2"]'); + expect(jsonInput.value).not.toContain("\\"); + }); + + // #1853 again, from the comment thread: the caret sitting *outside* the + // quotes of a `""` value. One keystroke there made the draft invalid, which + // is all it took — `""` + `a` rendered as `"\"\"a"`. + it("keeps a keystroke typed outside a string's quotes literal (#1853)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + // No `type`, so the field lands on the JSON editor holding a string. + properties: { note: { title: "Note" } }, + }; + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Note/) as HTMLTextAreaElement; + await user.click(jsonInput); + await user.keyboard("{End}a"); + + expect(jsonInput.value).toBe('""a'); + }); + + // #1856: `sum_numbers`, Backspace with the caret after the closing `]`. + it("keeps the draft raw when Backspace invalidates an array (#1856)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + numbers: { + type: "array", + title: "Numbers", + items: { type: "number" }, + }, + }, + required: ["numbers"], + }; + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Numbers/) as HTMLTextAreaElement; + await user.click(jsonInput); + await user.keyboard("{End}{Backspace}"); + + // Exactly the seeded text minus its last character — the draft the reporter + // expected, where the field instead showed a quoted, escaped string. + expect(jsonInput.value).toBe("[\n 1,\n 2\n"); + + // Each further edit compounded the escaping, so keep going. + await user.keyboard("{Backspace}{Backspace}"); + expect(jsonInput.value).not.toContain("\\"); + expect(jsonInput.value.startsWith('"')).toBe(false); + + // And the draft is still live: closing it back up produces a real array. + await user.keyboard("2]"); + expect(jsonInput.value).toBe("[\n 1,\n 2]"); + }); + + // #1885: FastMCP's `b: int | None = None`, which compiles to an `anyOf` with + // a null branch and `default: null`. The reporter was editing the literal + // `null` token in a raw JSON box; collapsing the union routes the field to a + // real number input, so there is no JSON token to edit in the first place. + it("edits a nullable integer as a number input, not a null token (#1885)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + a: { type: "integer", title: "A" }, + b: { + title: "B", + anyOf: [{ type: "integer" }, { type: "null" }], + default: null, + }, + }, + required: ["a"], + }; + // No seeded value: the `null` has to come from the schema's `default`, + // which is where the reporter's did. + renderWithMantine(); + + const b = screen.getByLabelText(/^B$/) as HTMLInputElement; + expect(b.tagName).toBe("INPUT"); + // `null` is rendered as "no value", not as four editable characters. + expect(b.value).toBe(""); + + await user.click(b); + await user.keyboard("42{Backspace}"); + expect(b.value).toBe("4"); + expect(b.value).not.toContain("\\"); + }); + + // The same nullable-with-`default: null` shape on a field the collapse still + // sends to the JSON editor (an array union). Backspacing the `null` token + // there is the exact keystroke sequence from #1885's recording. + it("backspaces a null default in the JSON editor without escaping (#1885)", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + cfg: { + title: "Cfg", + anyOf: [{ type: "array" }, { type: "null" }], + default: null, + }, + }, + }; + // Again unseeded, so the editor's opening `null` is the schema default + // being resolved — the state the reporter's recording starts from. + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Cfg/) as HTMLTextAreaElement; + expect(jsonInput.value).toBe("null"); + + await user.click(jsonInput); + await user.keyboard("{End}{Backspace}"); + expect(jsonInput.value).toBe("nul"); + + await user.keyboard("{Backspace}{Backspace}{Backspace}"); + expect(jsonInput.value).toBe(""); + expect(jsonInput.value).not.toContain("\\"); + }); +}); + +// A schema `default` is what a field *opens* with, not a value re-imposed on +// every unsendable draft. Unsendable text reports `undefined` by design, and +// `resolveValue` used to turn that straight back into the default — a change +// from the previous value, so the draft/value re-sync rewrote the box and +// reverted the keystroke. +// +// The two halves differed in how reachable they were, which the tests below +// mirror deliberately: +// +// - **The number field reverted in the app.** Numbers compare by value, so +// clearing a defaulted box (value `3` → resolved default `30`) always fired +// the re-sync and refilled itself. +// - **The JSON field was latent.** `collectSchemaDefaults` assigns +// `fieldSchema.default` itself, so a field seeded from the schema holds the +// *same reference* the substitution returns and nothing fires. It needs a +// structurally-equal but distinct value object to become observable — which +// is what parsed wire/deep-link values and rebuilt nested-object defaults +// produce, and what these tests construct. +describe("SchemaForm defaulted fields (#2026)", () => { + function DefaultHarness({ + schema, + initial = {}, + }: { + schema: InspectorFormSchema; + initial?: Record; + }) { + const [values, setValues] = useState>(initial); + return ; + } + + const arrayWithDefault: InspectorFormSchema = { + type: "object", + properties: { tags: { type: "array", title: "Tags", default: ["a"] } }, + }; + + it("opens the JSON editor on the schema default", () => { + renderWithMantine(); + expect((screen.getByLabelText(/Tags/) as HTMLTextAreaElement).value).toBe( + '[\n "a"\n]', + ); + }); + + it("keeps a keystroke typed into a defaulted JSON field", async () => { + const user = userEvent.setup(); + // Seeded with a value that is equal to the schema default but is not the + // same object — how values parsed off the wire or out of a deep link + // arrive. Sharing the reference (what `collectSchemaDefaults` produces) is + // what keeps this latent rather than what makes it safe. + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Tags/) as HTMLTextAreaElement; + await user.click(jsonInput); + await user.keyboard("{End}x"); + + // The invalid draft is the user's, not the default reasserting itself. + expect(jsonInput.value).toBe('[\n "a"\n]x'); + }); + + it("lets a defaulted JSON field be edited to a new value", async () => { + const user = userEvent.setup(); + // Distinct-object seed again, for the reason above. + renderWithMantine( + , + ); + + const jsonInput = screen.getByLabelText(/Tags/) as HTMLTextAreaElement; + await user.clear(jsonInput); + await user.type(jsonInput, '[["b"]'); + + expect(jsonInput.value).toBe('["b"]'); + expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument(); + }); + + it("lets a defaulted number field be emptied", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { n: { type: "integer", title: "N", default: 30 } }, + }; + renderWithMantine(); + + const n = screen.getByLabelText(/N/) as HTMLInputElement; + expect(n.value).toBe("30"); + + await user.click(n); + await user.keyboard("{End}{Backspace}{Backspace}"); + + // The box stays empty instead of refilling itself with the default. + expect(n.value).toBe(""); + }); + + // An explicit `null` is a value, not an absent one, so a non-null default + // must not be substituted for it. Note the field itself never emits `null` — + // clearing it reports `undefined` (pinned by "passes undefined to onChange + // when a number field is cleared") — so this arrives from parent state: a + // value received from the server, restored from a deep link, or written by a + // caller for a nullable schema. + it("shows an explicit null as empty, not as the default", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + n: { + title: "N", + anyOf: [{ type: "integer" }, { type: "null" }], + default: 30, + }, + }, + }; + renderWithMantine(); + + expect((screen.getByLabelText(/^N$/) as HTMLInputElement).value).toBe(""); + }); + + it("still re-syncs a defaulted field when the value changes externally", async () => { + function ExternalHarness() { + const [values, setValues] = useState>({ + tags: ["a"], + }); + return ( + <> + + + + ); + } + const user = userEvent.setup(); + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Tags/) as HTMLTextAreaElement; + await user.click(screen.getByRole("button", { name: "load example" })); + expect(jsonInput.value).toBe('[\n "z"\n]'); + }); +}); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 54e50b6e..152fe40f 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -207,7 +207,14 @@ interface SchemaJsonFieldProps { description?: string; withAsterisk: boolean; disabled: boolean; + /** + * The field's own entry in the form's values — **not** run through + * `resolveValue`. A draft-holding field is handed the raw value and the + * schema default separately; see `defaultValue` for why. + */ value: unknown; + /** The schema `default`, used to seed the draft only. See `defaultValue`. */ + defaultValue: unknown; onChange: (value: unknown) => void; onValidityChange: DraftValidityReporter; } @@ -244,18 +251,39 @@ interface SchemaJsonFieldProps { function SchemaJsonField({ fieldName, value, + defaultValue, onChange, onValidityChange, ...inputProps }: SchemaJsonFieldProps) { - const [draft, setDraft] = useState(() => - value === undefined ? "" : serializeJson(value), - ); + const [draft, setDraft] = useState(() => { + const initial = value === undefined ? defaultValue : value; + return initial === undefined ? "" : serializeJson(initial); + }); // Re-sync only when the parent's value genuinely diverges from what the draft // parses to, which leaves an external reset (a cleared form, a loaded // example) working while an in-progress `[` — whose parse is `undefined`, // matching the `undefined` we just emitted — is left alone. + // + // The value compared here is the field's *raw* entry, never `resolveValue`'s + // default substitution (#2026). Unsendable text reports `undefined`, and a + // defaulted field resolves that straight back to its default — a change from + // the previous value, so comparing against the resolved one would rewrite the + // draft to the default and revert the keystroke. Almost every edit passes + // through an unsendable state, so a defaulted array or object argument would + // be uneditable. + // + // Would be, not was: this is latent under today's seeding, where + // `collectSchemaDefaults` assigns `fieldSchema.default` itself, so the + // substitution returns the *same reference* and `useValueChange` never fires. + // It goes live as soon as the value is a structurally-equal but distinct + // object — parsed from the wire or a deep link, or a nested-object default, + // which `collectSchemaDefaults` rebuilds per call. `SchemaNumberInput` has no + // such accidental protection, since a number compares by value. + // + // Either way the default belongs to what the field *opens* with, seeded + // above, not to what is re-imposed while it is being typed into. useValueChange(value, (next) => { if (!isSameJson(parseJsonDraft(draft), next)) { setDraft(next === undefined ? "" : serializeJson(next)); @@ -309,7 +337,17 @@ interface SchemaNumberInputProps { description?: string; withAsterisk: boolean; disabled: boolean; - value: number | undefined; + /** + * Raw, not `resolveValue`-substituted — see `SchemaJsonFieldProps.value`. + * + * `null` is admitted alongside `undefined` because a nullable number schema + * produces it — by way of parent state, never by clearing the box — and the + * two are not interchangeable here: `undefined` means "no value supplied" + * and takes `defaultValue`, while `null` is a value and displays empty. + */ + value: number | null | undefined; + /** The schema `default`, used to seed the draft only (#2026). May be `null`. */ + defaultValue: number | null | undefined; min?: number; max?: number; allowDecimal: boolean; @@ -343,12 +381,29 @@ interface SchemaNumberInputProps { function SchemaNumberInput({ fieldName, value, + defaultValue, onChange, onValidityChange, ...inputProps }: SchemaNumberInputProps) { - const [draft, setDraft] = useState(value ?? ""); + // `undefined` means "no value supplied", which is what the default is for. + // An explicit `null` is a value, so it must not be overwritten by a non-null + // default; it displays as the empty box `resolveValue` produced before. + // Hence the `undefined` test rather than `??`, matching `SchemaJsonField`. + // + // Clearing the box does *not* produce that `null` — `toNumericValue("")` + // reports `undefined`, which is the behavior the "passes undefined to + // onChange when a number field is cleared" test pins. A `null` reaches this + // field only from parent state: a value received from the server, restored + // from a deep link, or written by a caller for a nullable schema. + const [draft, setDraft] = useState( + (value === undefined ? defaultValue : value) ?? "", + ); + // Compared against the raw value, never the default-substituted one, for the + // reason spelled out on `SchemaJsonField` (#2026): clearing this box reports + // no value, which a defaulted field resolved straight back to its default — + // so the box refilled itself and the argument could not be emptied. useValueChange(value, (next) => { if (!Object.is(toNumericValue(draft), next)) { setDraft(next ?? ""); @@ -584,7 +639,12 @@ export function SchemaForm({ description={description} withAsterisk={isRequired} disabled={disabled} - value={rawValue as number | undefined} + // Raw and default kept apart, not pre-resolved (#2026). Both admit + // `null`, which a nullable number schema really produces. + value={values[fieldName] as number | null | undefined} + defaultValue={ + getDefaultValue(fieldSchema) as number | null | undefined + } min={fieldSchema.minimum} max={fieldSchema.maximum} // An `integer` field rejects the decimal point outright rather than @@ -692,7 +752,9 @@ export function SchemaForm({ description={description} withAsterisk={isRequired} disabled={disabled} - value={rawValue} + // Raw and default kept apart, not pre-resolved (#2026). + value={values[fieldName]} + defaultValue={getDefaultValue(fieldSchema)} onChange={(val) => handleFieldChange(fieldName, val)} onValidityChange={reportFieldValidity} /> diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index 43b400bf..d238228a 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -700,10 +700,272 @@ describe("ServerSettingsForm", () => { clientId: "a", clientSecret: "", scopes: "", + authorizationParams: [], enterpriseManaged: false, }); }); + // #2018 — custom authorization-request parameters. + it("shows the empty hint for authorization parameters when none are set", () => { + renderWithMantine( + , + ); + expect( + screen.getByText("Additional authorization parameters"), + ).toBeInTheDocument(); + expect( + screen.getByText("No additional parameters configured"), + ).toBeInTheDocument(); + }); + + it("adds an authorization-parameter row through onOAuthChange", async () => { + const user = userEvent.setup(); + const onOAuthChange = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: "+ Add Parameter" })); + expect(onOAuthChange).toHaveBeenCalledWith( + expect.objectContaining({ + authorizationParams: [{ key: "", value: "" }], + }), + ); + }); + + it("edits and removes an authorization-parameter row", async () => { + const user = userEvent.setup(); + const onOAuthChange = vi.fn(); + renderWithMantine( + , + ); + const valueInput = screen.getByDisplayValue("corp"); + await user.type(valueInput, "x"); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + authorizationParams: [{ key: "kc_idp_hint", value: "corpx" }], + }), + ); + + onOAuthChange.mockClear(); + const removeButton = screen + .getAllByRole("button") + .find((b) => b.textContent === "X"); + await user.click(removeButton as HTMLElement); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ authorizationParams: [] }), + ); + }); + + it("clears an authorization-parameter key via its Clear button", async () => { + const user = userEvent.setup(); + const onOAuthChange = vi.fn(); + renderWithMantine( + , + ); + await user.click(clearButtonFor(screen.getByDisplayValue("kc_idp_hint"))); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + authorizationParams: [{ key: "", value: "corp" }], + }), + ); + + onOAuthChange.mockClear(); + await user.click(clearButtonFor(screen.getByDisplayValue("corp"))); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ + authorizationParams: [{ key: "kc_idp_hint", value: "" }], + }), + ); + }); + + it("rejects a reserved authorization-parameter key inline and with a warning", () => { + renderWithMantine( + , + ); + expect( + screen.getByText( + '"state" is set by the authorization flow and cannot be overridden.', + ), + ).toBeInTheDocument(); + expect(screen.getByText("Reserved parameters ignored")).toBeInTheDocument(); + expect(screen.getByDisplayValue("state")).toHaveAttribute( + "aria-invalid", + "true", + ); + }); + + // #2018 — the section heading and the placeholders are not programmatically + // associated with these inputs, so each control carries its own aria-label and + // the reserved-key reason is the input's `error` string (which Mantine wires + // to the input via aria-describedby) rather than free-standing text. + it("gives each authorization-parameter control an accessible name", () => { + renderWithMantine( + , + ); + expect( + screen.getByRole("textbox", { + name: "Authorization parameter name, kc_idp_hint", + }), + ).toHaveValue("kc_idp_hint"); + expect( + screen.getByRole("textbox", { + name: "Authorization parameter value, kc_idp_hint", + }), + ).toHaveValue("corp"); + expect( + screen.getByRole("button", { + name: "Remove authorization parameter kc_idp_hint", + }), + ).toBeInTheDocument(); + }); + + it("falls back to a row number in the accessible name when the key is blank", () => { + renderWithMantine( + , + ); + expect( + screen.getByRole("textbox", { + name: "Authorization parameter name, row 1", + }), + ).toBeInTheDocument(); + }); + + // The reason must reach assistive tech through the input, not just as text on + // the page: Mantine emits the `error` string with an id the input references. + it("associates the reserved-key reason with the key input", () => { + renderWithMantine( + , + ); + const keyInput = screen.getByRole("textbox", { + name: "Authorization parameter name, state", + }); + const describedBy = keyInput.getAttribute("aria-describedby"); + expect(describedBy).toBeTruthy(); + const reason = describedBy + ?.split(" ") + .map((id) => document.getElementById(id)?.textContent ?? "") + .join(" "); + expect(reason).toContain( + '"state" is set by the authorization flow and cannot be overridden.', + ); + }); + + // #2018 — the EMA leg authorizes against the enterprise IdP, a different + // authorization server, and deliberately sends none of these parameters. The + // description has to say so, or the form shows a configured value as active + // when it will not be sent. + it("warns that authorization parameters are unused under enterprise-managed auth", () => { + const { rerender } = renderWithMantine( + , + ); + expect( + screen.getByText(/Not sent while Enterprise-managed authorization is on/), + ).toBeInTheDocument(); + + rerender( + , + ); + expect( + screen.queryByText( + /Not sent while Enterprise-managed authorization is on/, + ), + ).not.toBeInTheDocument(); + }); + + it("does not flag a blank authorization-parameter row as reserved", () => { + renderWithMantine( + , + ); + expect( + screen.queryByText("Reserved parameters ignored"), + ).not.toBeInTheDocument(); + }); + + it("pluralizes the reserved-parameter warning for multiple keys", () => { + renderWithMantine( + , + ); + expect(screen.getByText(/state, scope are set/)).toBeInTheDocument(); + }); + it("invokes onOAuthChange with the chosen insufficient-scope policy (SEP-2350)", async () => { const user = userEvent.setup(); const onOAuthChange = vi.fn(); @@ -744,6 +1006,7 @@ describe("ServerSettingsForm", () => { clientId: "", clientSecret: "", scopes: "", + authorizationParams: [], enterpriseManaged: true, }); }); @@ -848,6 +1111,7 @@ describe("ServerSettingsForm", () => { clientId: "", clientSecret: "z", scopes: "", + authorizationParams: [], enterpriseManaged: false, }); }); diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index e20ddd44..276c9cf9 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -1,6 +1,7 @@ import { Accordion, ActionIcon, + Alert, Button, Checkbox, Flex, @@ -26,6 +27,11 @@ import { MODERN_LOG_LEVELS, } from "@inspector/core/mcp/types.js"; import { isOAuthCapableServerType } from "@inspector/core/mcp/config.js"; +import { + RESERVED_AUTHORIZATION_PARAMS, + authorizationParamKeyError, + isReservedAuthorizationParam, +} from "@inspector/core/auth/authorizationParams.js"; import { ADVERTISABLE_EXTENSIONS } from "@inspector/core/mcp/extensions.js"; import type { Root } from "@modelcontextprotocol/client"; @@ -184,6 +190,25 @@ const EmptyHint = Text.withProps({ fs: "italic", }); +// Matches the label/description pair Mantine renders for a single input, so the +// authorization-parameter row list reads as one labelled field next to Scopes. +const FieldLabel = Text.withProps({ + size: "sm", + fw: 500, +}); + +const FieldDescription = Text.withProps({ + size: "xs", + c: "dimmed", +}); + +// `align="flex-start"` keeps the key/value/remove controls top-aligned when the +// key input grows an error message, so the row doesn't jump as it appears. +const AuthorizationParamRow = Group.withProps({ + grow: true, + align: "flex-start", +}); + const ClearStoredOAuthButton = Button.withProps({ variant: "light", color: "red", @@ -245,6 +270,74 @@ function KeyValueRows({ ); } +// Reserved-key rejection is inline per row (#2018): the reason is passed as the +// key input's `error` **string**, so Mantine renders it in the input's own error +// slot and wires the `aria-describedby` association — a bare boolean would mark +// the field invalid without telling a screen reader why. The section-level Alert +// below repeats it so the rejection stays visible when the offending row is +// scrolled out of view. +// +// Each control carries a per-row `aria-label`: the section heading and the +// placeholders are not programmatically associated with the inputs, so without +// one an assistive technology cannot tell the key box from the value box, and +// the remove button announces only "X". +function AuthorizationParamRows({ + params, + onChange, + onRemove, +}: { + params: { key: string; value: string }[]; + onChange: (index: number, key: string, value: string) => void; + onRemove: (index: number) => void; +}) { + return ( + <> + {params.map((param, index) => { + const rowLabel = param.key.trim() || `row ${index + 1}`; + return ( + + + onChange(index, e.currentTarget.value, param.value) + } + rightSection={ + param.key ? ( + onChange(index, "", param.value)} + /> + ) : null + } + /> + + onChange(index, param.key, e.currentTarget.value) + } + rightSection={ + param.value ? ( + onChange(index, param.key, "")} /> + ) : null + } + /> + onRemove(index)} + > + X + + + ); + })} + + ); +} + function RootRows({ roots, onChange, @@ -374,16 +467,52 @@ export function ServerSettingsForm({ onTimeoutChange(field, numValue); }; + // #2018 — custom authorization-request parameters. The rows ride the single + // `onOAuthChange` callback with the rest of the Authorization section rather + // than growing three more props on this form. + const authorizationParams = settings.oauthAuthorizationParams ?? []; + function currentOAuth(): OAuthSettings { return { clientId: settings.oauthClientId ?? "", clientSecret: settings.oauthClientSecret ?? "", scopes: settings.oauthScopes ?? "", + authorizationParams, enterpriseManaged: settings.enterpriseManaged ?? false, onInsufficientScope: settings.oauthOnInsufficientScope, }; } + const rejectedParamKeys = authorizationParams + .map((p) => p.key.trim()) + .filter((key) => isReservedAuthorizationParam(key)); + + function changeAuthorizationParams( + params: { key: string; value: string }[], + ): void { + onOAuthChange({ ...currentOAuth(), authorizationParams: params }); + } + + function handleAddAuthorizationParam(): void { + changeAuthorizationParams([...authorizationParams, { key: "", value: "" }]); + } + + function handleRemoveAuthorizationParam(index: number): void { + changeAuthorizationParams( + authorizationParams.filter((_, i) => i !== index), + ); + } + + function handleAuthorizationParamChange( + index: number, + key: string, + value: string, + ): void { + changeAuthorizationParams( + authorizationParams.map((p, i) => (i === index ? { key, value } : p)), + ); + } + return ( // Stays inline: Accordion is a compound, `multiple`-discriminated generic, // so `.withProps({ multiple: true, ... })` loses its JSX call signature @@ -699,6 +828,41 @@ export function ServerSettingsForm({ ) : null } /> + + Additional authorization parameters + + Extra query parameters appended to the authorization request + only — never the token request. Use them for provider-specific + hints such as kc_idp_hint, login_hint, prompt, acr_values, or + audience. + {settings.enterpriseManaged + ? " Not sent while Enterprise-managed authorization is on: that flow authorizes against the enterprise IdP, a different authorization server." + : ""} + + {authorizationParams.length === 0 ? ( + No additional parameters configured + ) : ( + + )} + {rejectedParamKeys.length > 0 ? ( + + {`${rejectedParamKeys.join(", ")} ${ + rejectedParamKeys.length === 1 ? "is" : "are" + } set by the authorization flow and will not be sent. Overriding ${ + rejectedParamKeys.length === 1 ? "it" : "them" + } would break PKCE, the CSRF state binding, or the resource indicator. Reserved: ${RESERVED_AUTHORIZATION_PARAMS.join( + ", ", + )}.`} + + ) : null} + + + Add Parameter + +