The description had grown into a paragraph that dominated the
Authorization section, and it named a specific vendor's error code
(AADSTS90094) in a control that is not vendor-specific.
Three lines now: what the grant does, when to turn it off, when it
takes effect. The Entra specifics, the automatic-scope nuance and the
does-not-revoke caveats stay in docs/mcp-server-configuration.md, which
is where someone goes for the detail. The inline offline_access warning
and the EMA note are unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 7 on #2114 (suppressed comment). AGENTS.md
forbids inline handler logic; the new checkbox had it.
The eight OAuth controls beside it are inline in the same shape, so
consistency was an argument for leaving it. Complying anyway: this repo
holds new code to its gates even where neighbors predate them, the same
way the coverage floor works. Extracting the siblings would be an
unrelated refactor in a bug-fix PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 6 on #2114.
The early return handed back the persisted scope unchanged, so an
`offline_access` the user had just typed into Scopes never reached the
request when storage predated the change — and storage routinely does,
since `createOAuthProvider` preserves a stored scope rather than
reseeding from current settings. The docs promised configured scopes
are honored and the form warned that `offline_access` "is still
requested"; both were false in exactly that state.
Honoring it means adding it, not merely declining to strip it.
Only the token itself is added, not the rest of the configured scope:
unioning unrelated configured tokens into the request is outside what
this setting governs and could push an otherwise-satisfied connection
into a step-up authorization.
Three tests, all verified to fail against the previous behavior.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5 on #2114 — a hole in last round's fix.
When the persisted scope was `offline_access` alone,
`scopeForDeclinedRefreshGrant` returned undefined.
`OAuthManager.createOAuthProvider` reads that as "nothing stored" and
seeds storage with the configured scope, so the request-only filter
silently overwrote the value it promised to leave alone — and
re-enabling the grant could no longer restore it.
It now falls back to the configured scope, which is both the right
thing to request and inert to that branch. The previous "storage is
never rewritten" test could not see this: it ran at the provider level,
while the seeding branch lives in the manager. The new regression runs
there, and fails when the fallback is removed.
Also corrects the EMA rationale, which was wrong even though its
conclusion held. `createOAuthProviderForTransport` does build a
`BaseOAuthClientProvider` and wrap it, and `EmaTransportOAuthProvider`
forwards `clientMetadata` — so the setting is not bypassed outright.
What it cannot change is the IdP authorization leg's fixed
`openid offline_access`, which is the leg the user signs in through and
therefore the reason the control cannot affect EMA's consent behavior.
Fixed in the docs, the code comment, the UI string and the test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4 on #2114 — a real gap, not a wording one.
Declining the grant stops the SDK *adding* `offline_access`, but the
Inspector's own storage puts it back. A successful default-on
authorization persists the scope the AS granted
(`resolvePersistedScopeAfterGrant`), `offline_access` included;
`prepareForAuth` reloads it and `authenticate` hands it to the SDK as
`scope`, which sees the token in scope and appends `prompt=consent`
again. So any server that had authorized once could uncheck the box and
still hit AADSTS90094 — the users most likely to reach for the setting
were the ones it did not help.
`scopeForDeclinedRefreshGrant` filters it out of the *request* only,
applied in the provider's `scope` getter — the one seam both
`clientMetadata.scope` and `authenticate`'s `scope:` argument read
through. An `offline_access` the user configured is honored; storage is
never rewritten, so re-checking the box restores the old behavior
instead of finding the value destroyed, and scope-satisfaction checks
still see the real grant. Verified by reverting the filter: the new e2e
regression fails.
Also from that round, both previously suppressed:
The checkbox looked effective under enterprise-managed authorization,
which never builds this provider at all — it authorizes against the
enterprise IdP with a fixed `openid offline_access`
(`ema/constants.ts:16`). It now annotates itself the way the endpoint
overrides do. The existing count assertion covered two such notices and
was named for the overrides alone; it is renamed and expects three, with
a dedicated case pinning the notice to this control so the count cannot
be met by three copies elsewhere.
And "obtains no new refresh token" was another overstatement — the AS
decides issuance, and a preconfigured client is reused with whatever its
registration already permits. Replaced with what the setting actually
controls.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 4 raised that the callback catch leaves `activeServerId` set: the
disconnect there emits only on a status change, and a rejected token exchange
throws inside `completeOAuthFlow` before the reconnect, so the client is still
at its initial "disconnected" and the listener that would clear it never fires.
That is accurate, and clearing it is nonetheless wrong. "Authorize again" on
the re-auth banner hands `clearServerOAuthState` the live client only when the
banner's server is the active one; releasing it here passes `null` instead and
the stale tokens are never cleared from the client holding them, which is the
one thing that recovery exists to do. Confirmed by trying it — the existing
lost-authorization-state test fails on `clearOAuthTokens` never being called.
So the retention is now asserted rather than incidental, with the reason at
both the code and the test, and the InspectorView double surfaces `activeServer`
so a later cleanup of the apparent leak fails loudly instead of quietly
breaking the recovery.
Signed-off-by: cliffhall <cliff@futurescale.com>
- The SSE branch selects a caller-supplied `eventSourceInit.fetch` as-is, so
it bypassed the challenge observer and a first-time legacy SSE
authorization lost the advertised URL the same way. Observing is passive,
so it is applied to that fetch too while the deliberate no-proxy-wrap rule
stays. Covered behaviorally against a 401-serving server.
- Dropped an `as unknown as typeof fetch` from the new integration test
(banned by AGENTS.md, and it erased the signature); the spy is typed as
`typeof fetch` directly. The `as MCPServerConfig` beside it was
unnecessary too.
- The integration test reused a per-pid OAuth storage file. SDK `auth()`
persists the advertised URL in its discovery state, so a rerun could
satisfy discovery from that cache and pass with the plumbing removed —
which would have quietly falsified the regression check. Fresh mkdtemp
per run, removed in teardown.
Adds observer unit tests (challenge reported, body untouched, 403 included,
silent on success).
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 3.
Both catches report a failure without ending the attempt, and in each the
preceding rejection is an auth-recovery error — which deliberately holds the
status at "connecting" rather than moving it to "error". So the toggle spins
and the active-server lock is never released:
- the auth-recovery arm, when `checkAuthChallengeSatisfied` is what rejected
(the retrying `connect()` does reach "error", so only this half was stuck);
- the callback leg, whose `resumeAfterOAuth` carries a reconnect that can
reject the same way. Disconnect ahead of the EMA guard there, since a stuck
session is worth clearing whichever way the error classifies.
The fetch log survives a disconnect, so the Network diagnostics this issue is
about are unaffected. Both teardowns are asserted by the tests that already
cover those arms.
Also fixes a stale comment left by round 2: the arming-hooks type carries four
hooks, not three.
Signed-off-by: cliffhall <cliff@futurescale.com>
The challenge-driven plumbing did not reach the default CLI/TUI flow. With
no stored token in the legacy era, InspectorClient deliberately builds the
transport with no authProvider (so the SDK cannot open a browser before the
callback server is listening), which also leaves challenge interception off;
the 401 then surfaces as the SDK's headerless UnauthorizedError and the
runner calls authenticate() with no challenge in hand. So a server serving
metadata only at the advertised path still failed there — including for the
legacy showcase the README instructs.
Add a passive counterpart to the challenge interceptor: an observer fetch
that reports every 401/403 without touching control flow or the body. The
OAuthManager records the advertised URL from it and uses it in
authenticate() and its CIMD pre-registration. Latest-wins, including
clearing when a later challenge advertises none.
Also: the fixture helper dropped an explicitly supplied empty
resourceMetadataPath by truthiness, bypassing validation and silently
reverting to the well-known route.
Covered by a direct-client integration test that drives connect ->
authenticate() against the real server and asserts the advertised path was
requested and the well-known one never was; verified failing without the
fix.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3 on #2114.
The unit tests asserted our own `grant_types` array and stopped there.
What actually unblocks an Entra tenant is what the SDK puts in the
authorize URL, and nothing covered that — so an SDK change that stopped
keying its augmentation off `grant_types` would leave AADSTS90094
intact with every test green.
Adds the complementary case to the existing e2e suite, against a real
AS advertising `offline_access`: with the grant declined, the authorize
URL carries neither `offline_access` nor `prompt=consent`, while the
configured scope still goes out. Verified non-vacuous by reverting the
provider conditional — it fails. The positive test alongside it now
also asserts `prompt=consent` is present, which was unasserted despite
being the literal failure in the issue.
Drops an unjustified `as unknown as ServerEntry` I added in the App
test — AGENTS.md forbids it, and it hid a real mismatch (a partial
settings object against a complete one). Typed `ServerEntry` /
`InspectorServerSettings` fixtures instead, so a field added to that
interface later is a compile error rather than a missing value at
runtime. The `constructedOAuth` helper reads through the constructor's
own parameter type and needs no cast at all now.
Scopes the two doc comments that still claimed the flag removes every
`offline_access` — `StoredMCPServer.oauth` and `OAuthProviderConfig`,
both contradicting the caveat already stated elsewhere.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 2.
The flag in the resume catch sat above the EMA guard, so an unconfigured
enterprise client was marked a failed connect — which contradicts the round-1
argument for leaving EMA unflagged in the two connect-path arms. Moved below
the guard so all three agree, and still above the classification fan-out so
every arm that is a real failure carries it.
Adds the two tests the round flagged as missing: an OAuth storage `load()`
that rejects (spied on `RemoteOAuthStorage.prototype`, so every other test
keeps the real storage), and a *rejecting* challenge check — the distinct
control-flow decision introduced last round, where a broken check is surfaced
rather than treated as an unsatisfied challenge and used as grounds to navigate
the page away. That one asserts no resume snapshot was written, which is what
`prepareOAuthRedirect` would have done first.
Signed-off-by: cliffhall <cliff@futurescale.com>
`isOriginRelativePath` compared the resolved href against the literal, which
`URL` preserves a query and fragment in — so `/doc?version=1` and `/doc#section`
passed. Express matches on the path alone (and treats `?` as a pattern
character), and a fragment never reaches the server, so the advertised URL
could not resolve to the registered route. Reject both, with cases added to
the table-driven test.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2 on #2114. The setting was described as
guaranteeing "no offline_access, no forced prompt", which is not what
the SDK does.
`startAuthorization` appends `prompt=consent` for any `offline_access`
in the effective scope and never consults `grant_types`, and
`determineScope` skips its augmentation when the scope already carries
it. So the prompt still goes out when the user configured
`offline_access` in Scopes, or left Scopes blank and the resource
advertises it in `scopes_supported`. The flag removes the SDK's
*automatic* augmentation and nothing more; the docs, the help text and
the type say that now.
The form also flags the case a user can act on: opted out while
`offline_access` is still listed in Scopes. Whole-token match, so
`offline_access_extra` does not trip it.
Corrects the Clear stored OAuth state claim too. It clears the local
tokens and client information — it is not a revocation and cannot
reach the AS. A dynamically registered client re-registers on the next
connect (the new registration declaring only `authorization_code`); a
preconfigured `oauth.clientId` is reused unchanged.
Adds the two missing seam tests, both confirmed to fail without the
code they cover: App's connect wiring (deleting the spread fails two of
the three) and an /api/servers POST followed by a read-back, since a
200 alone never proved the opt-out persisted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
AGENTS.md requires review-relevant rules to be mirrored into
.github/copilot-instructions.md in the same PR, and that file already
carries a Dependency placement section. Add the distilled form of the
new overrides guidance: use an `overrides` entry rather than
`npm audit fix` when the advisory range has no upward escape inside the
parent's declared range, gate such a pin on the build rather than the
audit, expect the lockfile diff to touch only that package, and say when
the pin can be dropped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
`npm audit` (dev included) reported a low-severity advisory against
`esbuild` in three of the four client installs. It is not in the
production tree — `npm audit --omit=dev` has been clean everywhere since
2058 — but a permanently non-clean audit trains people to skim the
channel, which is the same argument 2058 made.
`tsup@8.5.1` declares `esbuild: ^0.27.0`, and the advisory covers
0.27.3 - 0.28.0 with 0.27.7 the last 0.27.x, so there is no upward
escape inside tsup's declared range. `npm audit fix` "resolves" it by
silently downgrading to 0.27.2 across three installs — the trap hit and
reverted during 2058. An `overrides` entry forces the copy above the
range instead.
- Add `"overrides": { "esbuild": "^0.28.2" }` to clients/{web,cli,tui}.
That also covers the tui's `tsx` copy (0.28.0), so no separate update
is needed.
- Re-resolve each lockfile. Every changed entry is an esbuild one: the
nested `tsup/node_modules/esbuild` and `tsx/node_modules/esbuild`
copies collapse into the single deduped 0.28.2, and no other package
moves.
- Document the pin in AGENTS.md, including when to drop it.
The real gate here is the build, not the audit: this puts tsup on an
esbuild major it does not declare. `npm run build` succeeds for all
three clients, and `npm run ci` passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Review round 1.
The satisfied-challenge arm re-connects from inside the outer `catch` with two
unguarded awaits, so a rejection there escaped `onToggleConnection` entirely —
an unhandled rejection with no toast, no red border and no sidebar, which is
this issue's failure mode in its most invisible form. Surface it as the failed
connect attempt it is. A throw from `checkAuthChallengeSatisfied` lands there
too rather than falling through to the redirect: it is not the same as the
challenge being unsatisfied, and navigating the page away on the strength of an
error would bury it.
The provider-error and OAuth-storage-load arms return before a client is
rebuilt, so they raise the red border and deliberately not the sidebar — the
diagnostic is the provider's `error` param (already in the banner) or a local
storage failure, neither of which is in the network log. Said so at both sites
and on the test, which asserts the flag and nothing more on purpose.
Also drops the three `as unknown as` casts on the test-only arming hooks for a
single intersection cast that keeps the module's real shape checked, and turns
the armed connect rejection into a FIFO queue so a test can arm the two
consecutive connects the recovery retry makes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings from review:
- `ensureCimdClientRegistration` ran protected-resource discovery on the
global fetch while its AS-metadata leg used the configured one. On web
that is `createRemoteFetch`, which proxies through the backend to sidestep
CORS — so the discovery this PR routes to a challenge-advertised URL would
fail in the browser, be swallowed by the catch, and leave CIMD probing the
wrong authorization server. Forward `params.fetchFn`, and assert in the
test that nothing reaches the global.
- `oauth.resourceMetadataPath` accepted `//other-host/doc`. Resolved against
the request base that re-points the origin in the advertised
`resource_metadata` while Express still registers the route locally, so the
server would advertise a document it does not serve. `/\\host` does the
same, since the URL parser folds a backslash into a slash for special
schemes — a leading-slash check misses both. Validate origin-relativity
with a shared predicate, used by the JSON-config validator and again at
server setup for a programmatically built ServerConfig.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review on #2114. The setting was described as taking effect
"immediately", which is wrong twice over.
`applyLiveServerSettings` treats OAuth as connection-time-only, so a
connected client keeps the metadata it was built with — deliberately,
and identically to every other field in that section (client id,
secret, scopes, authorizationParams, endpoint overrides). Not wiring
this one field live; saying "next connect" instead, in the docs, the
checkbox description, and the type.
And two pieces of state outlive the setting: the registration already
held at the authorization server still lists the grant, and a refresh
token issued earlier stays usable, because the SDK's
`refreshAuthorization()` posts the stored token without consulting
`grant_types`. So the stated cost is not guaranteed until stored OAuth
state is cleared. Documented, rather than revoking credentials as a
side effect of a checkbox — Clear stored OAuth state already does that
explicitly.
Also adds the manager->provider seam test the other bridges have:
without it, deleting `requestRefreshToken:` from `createOAuthProvider`
would leave the feature dead with every other test green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
The failure column was gated on the `"error"` connection status, which an
OAuth failure never reaches: the parent drives the auth legs itself and tears
the client down on failure, so the session settles at `"disconnected"`. The
sidebar therefore stayed shut on exactly the failure the Network tab exists to
explain — the case #2048 was closed on.
Gate it on `erroredServerId` instead, the parent's canonical "this server's
last connect attempt failed" flag, and set that flag on every OAuth
connect-attempt failure: the pre-redirect discovery/DCR leg, the callback
leg's token exchange, an OAuth-storage load failure, and a provider that
redirects back with an error instead of a code.
Nothing else was needed to make the requests reachable. `auth` fetches are
already tracked into the Network store, which does not clear on disconnect and
is restored from the session on the callback leg, so discovery, DCR and the
token exchange are all present. Tab selection stays content-gated rather than
hard-coded, so Network leads because it is the one that captured something.
Signed-off-by: cliffhall <cliff@futurescale.com>
The Inspector always declared `refresh_token` in its OAuth client
metadata. The SDK's `determineScope()` then appends `offline_access`
whenever the authorization server advertises it and the client declares
that grant, and `startAuthorization()` appends `prompt=consent` whenever
`offline_access` is in scope. Against Microsoft Entra ID that forced
consent prompt routes a non-admin user into the admin-consent workflow
and fails with AADSTS90094 — even after a tenant admin has consented.
Adds a per-server "Request refresh token" setting (default on) that
drops the grant from the client metadata, breaking that chain at its
source. Persisted as `oauth.requestRefreshToken` and written only when
off, so entries that never touch it keep a byte-stable round-trip. Web
edits it in Server Settings → Authorization; the CLI and TUI pick it up
from the same per-server settings via `buildRunnerClientAuthOptions`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
An RFC 9728 `WWW-Authenticate: Bearer resource_metadata="…"` challenge was
parsed and then dropped by the normalized `AuthChallenge`, so every
challenge-driven `mcpAuth()` call ran discovery without it and probed
locations derived from the MCP server URL instead. OAuth failed outright
against any server whose metadata document lives off the well-known path.
The SDK handles this correctly inside its own transports, but the Inspector
never reaches that path — it intercepts the 401 and drives `auth()` itself
to capture the authorization URL and run the step-up scope union.
Carry the advertised URL on `AuthChallenge` as a string (it crosses the web
client's remote-backend boundary as JSON), convert it at the OAuth boundary,
and pass it to the standard challenge leg, the forced step-up leg, and CIMD
pre-registration — which runs before `auth()` and did its own discovery. A
malformed value is ignored rather than surfaced, matching the SDK's parser.
The callback leg needs nothing: SDK `auth()` persists the URL in its
discovery state.
Adds an `oauth.resourceMetadataPath` test-server mode that moves the
document off the well-known path entirely, so the integration test proves
discovery genuinely fails without the fix.
Closes#2071
Signed-off-by: cliffhall <cliff@futurescale.com>
The web client and the TUI both read `savedSettings.metadata` and pass it
as `InspectorClientOptions.defaultMetadata`. The CLI never did, and
`InspectorClient` only reads the option rather than falling back to
`serverSettings.metadata` — so a per-server `metadata` entry was silently
inert for `--cli` runs.
Pass it beside the existing `roots:`, guarded on a non-empty map the same
way web and the TUI guard theirs, so `{}` still means "no defaults".
Precedence is unchanged: call-time keys override defaults in `mergeMeta`,
so `--metadata` stays per-invocation and wins on a key collision.
Covered by three catalog-driven tests asserting against the test server's
recorded wire requests. `createTestConfig` is retyped `StoredMCPServer` —
the accurate on-disk shape — since a fixture that cannot express the
per-server keys cannot cover the settings that reach the client.
Closes#2093
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4 on #2109. A `tools/list` may repeat a name (#1957) and
ToolsScreen already disambiguates those rows with a key of its own, but the
detail panel handed SchemaForm only the name — so switching between two
same-named tools looked like the same entity and left the first one's enlarged
fields (and its in-progress number/JSON text, which predates this PR) in place.
ToolDetailPanel now takes an optional `resetKey`, defaulting to the name for
callers whose list cannot repeat one, and ToolsScreen passes the row key.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3 on #2109.
- The focus hand-off ran in a passive effect, so the browser presented at least
one frame with focus on the document body. useLayoutEffect instead.
- The right-section widths were pixel totals. They are now built from the
measured button widths plus the gap and inset, and emitted through `rem` so
they track a user's root font size rather than pinning to CSS pixels.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2 on #2109. A disabled form (a tool call in flight) left
the enlarge button live, and activating it swapped in a text area that mounts
disabled — so the focus hand-off targeted an unfocusable element and keyboard
focus was dropped to the document.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1 on #2109.
- Activating Enlarge from the keyboard unmounted the focused button and left
focus on the document body, so the next Tab restarted at the top of the page.
The text area now takes focus on mount, caret after the text already typed.
- Every field's enlarge button carried the same "Enlarge" name, so a form with
several string arguments exposed indistinguishable controls. Each now reads
"Enlarge <field label>". Queries that matched a field by its label alone are
scoped to the textbox role, since the button's name now contains it too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
One conflict, scripts/smoke-web-app.mjs: #2056 added a second phase to the
smoke (the same app declaring _meta.ui.domain, asserting it renders from the
dedicated app origin rather than an opaque one) while this branch extracted
the smoke's flow into scripts/lib/mcp-app-flow.mjs.
Resolved by keeping both: phase 2 now runs through the shared helper too,
which grows three knobs it needs and pack:verify does not —
- expectDeepLink: phase 2 re-navigates the SAME page, so the token gate is
already proven and a rejected re-navigate would fail at the connect wait
regardless;
- what: so a phase-2 ready timeout doesn't read as a phase-1 one;
- extraDiagnostics: phase 2 appends the frame list, which is what separates
"never rendered" from "rendered at the wrong origin".
Each is covered by a new unit test, since neither consumer exercises a
failure branch.
pack:verify deliberately drives phase 1 only. The dedicated origin is served
by the same runner the packaging assertions already cover and reads nothing
new from disk (app-origin-controller.ts has no file reads), so a second
browser launch there would buy no packaging signal.
Verified on the merged tree: smoke:web:app passes both phases (dedicated
origin http://127.0.0.1:6278/app-document/<id>), and pack:verify passes end
to end against the real tarball.
Signed-off-by: cliffhall <cliff@futurescale.com>
A string argument renders as a single-line `<input>`, which swallows Enter —
so a value containing newlines could not be typed at all.
Each string field now carries an enlarge button (ImEnlarge2) to the left of its
clear button. Clicking it swaps the input for an autosizing text area that keeps
the field's value, label, and length constraints. It is deliberately one-way:
shrinking back would have to either discard newlines the user typed or keep a
value the field can no longer show, and both are worse than staying enlarged.
Enlarging is per-field and resets with `resetKey`, so it cannot carry across to
another tool's same-named field.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5 on #2100.
- The store bounded entry COUNT only, and the route admits 8Mi code units per
document with a one-hour TTL — ~256Mi retainable on documents a server
under test chooses the size of. Added a 64Mi retained-size budget evicted
oldest-first alongside the count bound, with a running total kept in step
with the map through a single `drop` seam so it cannot drift.
- The isolation claim was wrong about cookies. A separate port makes a
separate ORIGIN, so DOM access, localStorage, sessionStorage and IndexedDB
are isolated — but cookies are keyed by host and path, ignoring port, so an
app served here shares the 127.0.0.1 jar with the Inspector and every other
loopback service. Not a hole in the Inspector's own auth (the API token is
a header plus a window global and sessionStorage, none of them cookies) and
not closable without a distinct host this listener cannot mint, so it is
stated plainly in both the module header and clients/web/README.md rather
than left as a claim that is false for one of the three surfaces it names.
- Removed a shell-escaping artifact I left in the migration guide.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4 on #2100. All three findings are the same error:
clients/web/README.md documents the limitation correctly, and every
port-forwarding blurb then said the opposite.
An unpublished/unforwarded port is NOT the opaque-origin fallback case. The
backend's listener binds fine inside the container, so publication succeeds
and the browser is handed a URL it cannot reach — the frame stays blank,
with no console warning, because a cross-origin navigation failure is not
observable from the page. The fallback and its warning cover the failures
the BACKEND can see: no listener, a port that never bound, an older backend,
a network error.
Corrected in the root README's Docker blurb and the migration guide's
third-port recipe. The migration guide's troubleshooting entry had the wrong
symptom for the same reason — a missing forward produces a blank frame, not
an app whose requests are rejected — so it now leads with that and keeps the
rejected-requests case beside it as the neighbouring symptom, since the two
have opposite causes and opposite console output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3 on #2100.
- MCP_APP_ORIGIN_PORT could equal CLIENT_PORT or MCP_SANDBOX_PORT. The
app-origin listener starts BEFORE the web server binds, so that is not a
race it loses — it wins the port and the Inspector dies with EADDRINUSE.
`CLIENT_PORT=6278` was a valid config before this feature existed. It now
resolves to a dynamic port with a warning: of the three listeners this is
the only one that can move, and refusing to boot would cost the user the
whole Inspector over a feature they may not be using.
- The proxy's same-origin refusal covered only its own origin. An app served
from the INSPECTOR's origin is no safer: same-origin with the top document
means direct access to window.top, its DOM and its storage, and the
cross-origin frame in between buys nothing. Both trusted origins are now
refused before allow-same-origin is granted.
- Replaced the `as unknown as number` on `server.address()` with a real
narrowing of the string/null members of its return type.
- MCP_APP_ORIGIN_PORT added to the v1→v2 migration guide's env table, its
Docker recipe, and a troubleshooting entry for the symptom this actually
produces (the app renders, but its own backend rejects `Origin: null`).
- clients/web/README.md now states which failure is outside the fallback
guarantee — a published document the browser cannot reach — and why there
is no honest signal to fall back on. Docs and code no longer disagree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Pre-existing flake, not introduced here — the file is untouched by this
branch and the test already carries a comment about failing this way once.
The default 1000ms was still too tight: it failed twice consecutively under
`npm run ci` (~1021ms, 0 calls) while passing every time the file runs
alone, which is a budget signature rather than a broken observer. A
generous `waitFor` costs nothing on the passing path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2 on #2100, plus a collision the review did not find.
The default app-origin port was 6276 — which is already
RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, the CLI/TUI loopback OAuth callback.
That port is fixed precisely so an OAuth app can pre-register
http://127.0.0.1:6276/oauth/callback, which makes it the one listener here
that cannot move when something takes it first. A running --web would hold
it and break a later --cli/--tui OAuth flow at its registered redirect URI,
and docs/mcp-app-review.md has those two running side by side. This
listener has a documented dynamic fallback, so this listener yields: the
default is now 6278 (6277 skipped — v1's retired proxy port, which the
migration guide tells people to stop forwarding). A test asserts the two
defaults stay distinct.
Review comments:
- POST /api/app-document now applies hono's bodyLimit BEFORE parsing. The
8MiB check ran after c.req.json() had buffered and parsed the whole
request, and bounded only `html` — so bulk in `csp`, or in keys the route
never reads, was fully materialized before the 413.
- `csp` is validated as an HTTP header value and length-bounded. The
controller hands it to res.writeHead() verbatim, where Node throws
ERR_INVALID_CHAR synchronously — inside the handler for a later,
unrelated request, outside this route's error handling.
- The app-origin port now propagates everywhere the other two do: the CLI
--print-handoff port-forward command (and its test), the Dockerfile's ENV
and EXPOSE, and the remote-review SSH recipe in docs/mcp-app-review.md.
Without it an app declaring _meta.ui.domain is unreachable through every
documented remote workflow.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1 on #2100.
- POST /api/app-document: `null` and `[1,2]` are valid JSON, so the
destructure that followed threw OUTSIDE the parse try/catch and turned a
malformed body into a 500 instead of the documented 400.
- app-origin controller: the error handler cleared `server` but not
`origin`, and `publish` gates on `origin` — so an error after a successful
listen kept minting URLs on a dead port and the browser never took the
srcdoc fallback. Clear both, and drop the now-unfetchable documents.
- sandbox proxy: drop a stale `srcdoc` before navigating to a dedicated
origin (srcdoc wins over src), and restore the opaque-origin posture —
strip `allow-same-origin`, reset `innerOrigin` — before assigning `srcdoc`
on the fallback path. Not reachable today (each render remounts the
iframe), but the cost of being wrong about that later is an isolation
boundary.
- publishAppDocument tests: replace two `as unknown as Response` casts with
real `Response`s.
- docs: `frame-ancestors` admits the proxy AND the Inspector page (every
ancestor is checked); the shared helper falls back to loopback, not
`'none'`; the showcase origin is `127.0.0.1`, the default bind address.
- tests: null/array body, post-listen error, and both factories receiving a
working `publishAppDocument`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
`frame-ancestors` is checked against every ancestor, not just the parent.
A published app document is framed by the sandbox proxy, which is framed
by the Inspector page — so admitting only the proxy's origin blocked the
frame outright (it rendered as a chrome-error frame that never reached
the bridge). `appDocumentEmbedders` now admits both.
That failure was invisible to every unit test, so `smoke:web:app` grows a
second phase: the same demo app declaring `_meta.ui.domain`, driven
through the same connect → open → ready chain, then asserting the two
facts that only hold on the dedicated path — the inner frame was
navigated to a published document, and its own `location.origin` is a
real http origin rather than "null". Both phases end at
`data-app-status="ready"`, so nothing short of reading the frame's origin
can tell them apart.
Adds `test-servers/configs/mcp-app-domain-http.json` (and a `domain`
param on the `mcp_app_demo_widget` preset) as the showcase server for
both the smoke and hand testing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
An MCP App is rendered by handing its HTML to the sandbox proxy as
`srcdoc`, in a frame sandboxed without `allow-same-origin` — the #1565
isolation model. That gives the app document an opaque origin, so every
request it makes carries `Origin: null` and no CORS / OAuth-callback /
API-key allowlist can admit it. `_meta.ui.domain` is the spec field a
server uses to ask its host for a stable, dedicated origin, and it was
being dropped on the floor.
The field is explicitly host-dependent and the Inspector owns no domain
infrastructure, so it treats any non-empty value as a request rather than
an address, and answers with a real loopback origin on its own listener
(MCP_APP_ORIGIN_PORT, default 6276). One shared origin, path-keyed per
document: that delivers what the field is for without minting a port or
DNS name per app, and is therefore not a per-app isolation boundary.
The inner frame is granted `allow-same-origin` on this path only, which
is what makes the origin real. It is not a #1565 regression: the listener
is on its own port, so the app stays cross-origin to both the proxy and
the Inspector. The proxy refuses the grant if the URL's origin equals its
own, and the grant is never reachable from the server-supplied `sandbox`
string, which is still stripped. The per-app CSP is delivered as a real
response header there rather than only as a <meta>.
Every failure path falls back to the default srcdoc render with a console
warning naming the field, rather than blanking the app.
Also fixes a pre-existing `tsc -b` break on v2/main (useServers.test.tsx
passed `metadata: []` where `RequestMetadata` is expected), which has had
CI red since #2087 and blocks the pre-push gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
