McpUiResourceMeta describes the value of a `_meta` bag's `ui` key, not
`_meta` itself, so casting `content._meta` to it left `csp` and
`permissions` undefined for every spec-conforming app. The generated
sandbox CSP then fell back to `connect-src 'none'` and the app's declared
connectDomains never took effect.
Unwrap `_meta.ui` (with a result-level fallback, which the spec allows)
through a small type-guarded helper, mirroring the tool side of this file
where `getToolUiResourceUri` already reads `_meta.ui.resourceUri`. The
test fixture now builds the real wire shape, and cases cover the
result-level fallback plus the unnested bag being ignored.
Closes#2055
Signed-off-by: cliffhall <cliff@futurescale.com>
Only the top modal owned the focus trap, but every lower `Modal.Content`
remained an exposed `aria-modal` dialog with reachable controls and a live
iframe — so browse navigation could land in a dialog that is visually covered
and that itself claims the rest of the page is inert.
Non-top contents are now `inert`: out of the accessibility tree and out of
focus order, without unmounting, so each app keeps its bridge and its handshake
and is live the moment it becomes top.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two of the three findings; the third is answered in the PR thread.
**A replaced client could still enqueue.** The one-shot sweep only rejected what
was already queued, but the outgoing `InspectorClient` disconnects
asynchronously and can enqueue during its own teardown — and that late entry
would be rendered by a factory bound to the REPLACEMENT client, reading its
resource and answering through a different server. Requests now belong to a
session: each constructed client gets one, closing it rejects that session's
entries AND refuses anything it queues afterwards, and another session's
entries are untouched.
**The advertisement no longer has to guess.** Whether a sandbox exists is only
known once `/api/config` resolves, and the answer is baked into the client at
construction — guessing "available" over-claims, guessing "unavailable" strands
the whole session on the native form despite having a sandbox. The connect path
now awaits that fetch (already in flight since mount, so no human ever waits;
it only orders a deep-link auto-connect racing the same page load), after which
an absent `sandboxUrl` means confirmed-absent.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three of round 21's four findings say the same thing: **the UI half of round 20
was never applied, and I reported it as done.**
The edit script aborted partway on a regex error, after writing one file and
before writing the other two. So `ServerConfigModal` gained the footer prop
but `App.tsx` never passed it — the footer was `undefined` in production and
rendered nothing, visible only to the unit test that passes a descriptor
directly — the extra refresh call sites never landed, and the footer's doc
comment still claimed two secret-entry dialogs while the code had three.
`npm run ci` passed throughout, because every test I added exercises the piece
that *did* land. This is the same failure as round 17's uncommitted files with
a different cause: I verified intent (green gate, clean typecheck) instead of
the artifact. Each edit now asserts it changed the file, and the result is
grepped afterwards — the check costs a second and is the only one that
describes what is actually on disk.
Landed now, and verified in the files rather than inferred:
- `App.tsx` passes `secretStorage` to `ServerConfigModal`, so the disclosure
the previous commit claimed to add actually renders.
- Every secret-bearing write refreshes the descriptor — config submit and both
import flows, not just the two settings saves. Any of them can perform the
pending plaintext-to-encrypted upgrade, after which the footer was reporting
"Plaintext file" until reload.
- The footer's doc names all three dialogs. The wrong count there was not a
description of the bug, it *was* the bug: the omitted dialog was the one
taking secrets with no disclosure.
Also, the fourth finding: **the staging name was reusable across restarts.**
`recoverOrphanedSnapshots` deliberately leaves an orphan when a live
`secrets.json` also exists, and a pid-only name repeats (pid 1 on every
container start) — so the next claim would `rename` straight over that orphan
and permanently discard secrets it may uniquely hold. A per-attempt nonce
makes a staging path unrepeatable.
Full `npm run ci` green, staged tree verified first.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 20, and almost all of it is fallout from round 19's two changes. Both
were right in direction and wrong in the detail, in the same way: each solved
its case by treating a broad thing as a proxy for a narrow one.
**The durability guard wrote newly entered secrets to disk in plaintext.**
Preserving legacy plaintext for a session-scoped store is correct — stripping
it moves the only durable copy into RAM. Using *the whole submitted entry* as
the proxy for "legacy" was not: it turned
`MCP_INSPECTOR_SECRET_STORE=memory` into "write every new secret to
`mcp.json` in the clear", while the footer for that very store says secrets
are written nowhere. A security regression introduced by a data-loss fix, and
the same overshoot in `client.json`.
Provenance decides now. A secret field is kept on disk only when it was
already there, unchanged, in the file being overwritten; a newly entered or
changed value is stripped and lives in the session store, which is exactly
what the footer promises. Where provenance cannot be established — an
unparseable prior file — the value is treated as new, because writing a
secret to disk on a guess is the failure that matters.
**The atomic claim was not as safe as its comments said.** Four ways:
- A crash between the claim and the copy left only
`secrets.json.migrating-<pid>`. The next run checked the canonical path,
found nothing, selected the keychain, and every stored credential vanished
with no warning. Orphans are now adopted at startup.
- Restoring used `rename`, which on POSIX **replaces** its destination — so
the restore path could overwrite a `secrets.json` a concurrent writer had
recreated, destroying the later successful write that claiming exists to
protect. The comment beside it asserted the opposite. `link` + `unlink` is
the no-clobber primitive this needed.
- Every claim failure was read as a lost race. `EACCES`/`EROFS` leave the
file exactly where it is, and returning quietly selected the keychain while
file-backed secrets sat there invisible. Only `ENOENT` is benign now.
- A failed cleanup left a plaintext-capable snapshot on disk with a silent
catch, while the app reported the keychain as the only store. It names the
path now.
**A third secret-entry surface had no disclosure at all.** `ServerConfigModal`
takes stdio `env` values, which are extracted into the store exactly as
Server Settings' are — and the footer's own doc comment claimed there were
only two such dialogs. It has the footer now, and every server write that can
carry a secret (config submit, both import flows) refreshes the descriptor,
not just the settings save.
Full `npm run ci` green, staged tree verified first.
Signed-off-by: cliffhall <cliff@futurescale.com>
**The task-augmented inbound branch bypassed the routing.** A server→client
`elicitation/create` carrying `params.task` answers immediately with a
`CreateTaskResult` and settles the TASK when the user answers, so it cannot
ride `enqueuePendingElicitation` — and it was therefore never offered to an
app, even with a valid `_meta.ui.resourceUri`. It now makes the same attempt,
completing the task with the app's result and falling back to the native queue
otherwise; an abort fails the task rather than reopening it natively. The two
task-settling closures are extracted so both answer routes settle it
identically.
**The web client advertised the capability without a sandbox.**
`useInitialConfig` leaves `sandboxUrl` undefined while `/api/config` is in
flight and when the sandbox controller could not start, but the renderer was
supplied unconditionally — so the client claimed it could host an app in
exactly the window where it could not. The option is now gated on a resolved
sandbox URL: such a connection behaves like the CLI/TUI, native queue and no
claim made.
**Concurrent modals each owned the keyboard.** Every open `Modal.Root`
installed its own focus trap, Escape handler and overlay, so the traps fought
and one Escape could dismiss more than one pending request. Only the topmost
owns them now; the rest stay mounted, keeping their bridges and handshakes,
but inert to the keyboard.
Signed-off-by: cliffhall <cliff@futurescale.com>
The hand-rolled accept check was shallower than the bridge's, so frames it
rejects could still set `elicitation` here — `appInfo: {}` passed, though
`McpUiInitializeRequestSchema` requires an implementation name and version.
Any approximation drifts; the schema itself does not, and ext-apps exports it.
Capabilities are still read from the ORIGINAL frame rather than the parse
output, since that schema is precisely what strips `elicitation` — validating
with it and reading through it would defeat the module. The one check the
schema cannot make is that the frame is a request at all (a notification
carries the same method and params), so the id check stays.
Also stops treating a truthy non-object as an advertisement: the draft declares
`elicitation` as an object, so `elicitation: true` is a value it does not
define and the bridge's schema will reject it the moment it carries the key.
Signed-off-by: cliffhall <cliff@futurescale.com>
The previous round froze the recorded capabilities at the first handshake, on
the belief that the bridge ignores re-initialization. It does not: ext-apps
1.7.5's `_oninitialize` warns about the double-mount and then assigns
`_appCapabilities` and `_appInfo` from the new frame — "the latest appInfo/
appCapabilities replace the previous values", in its own words. Freezing left
this gate reporting capabilities the bridge no longer held, in both directions:
a handshake advertising `elicitation` followed by one without it still read as
advertised.
Every frame that passes the accept gate now replaces the recorded value, and
`appCapabilities` joins `protocolVersion` and `appInfo` in that gate since the
bridge's own schema requires all three. A frame the bridge would reject still
records nothing AND leaves the previous value alone — it is a route to changing
the gate in neither direction.
Signed-off-by: cliffhall <cliff@futurescale.com>
The capability observer recorded `params.appCapabilities` from ANY frame whose
method was `ui/initialize`, before the bridge validated it. A view could
therefore send a second, malformed initialize — one the bridge rejects, keeping
the capabilities it had already accepted — and flip `elicitation` on in this
gate, after which the host would forward an elicitation the bridge never
negotiated.
Now only a well-formed initialize REQUEST is recorded (a JSON-RPC id, plus the
handshake's required `protocolVersion` and `appInfo`), and only the first one:
the bridge keeps what it accepted and ignores re-initialization, so this gate
agrees rather than offering a second, laxer path to the same flag. A malformed
first frame records nothing.
This is a gate, not a second copy of the bridge's schema — the bridge stays the
authority on the rest of the frame.
Signed-off-by: cliffhall <cliff@futurescale.com>
Eight findings from the Copilot review, all accepted.
Ownership across connections was the substantive one, in three parts. A
mid-session transport close reaches teardown only through
`clearAndAnnouncePendingPeerRequests`, whose emptiness check looked at the
native queues alone — so a lone app elicitation survived a dropped connection
with its modal still open; the check now counts the active set too. The web
bridge factory resolves its client at call time, so an entry queued by a
replaced `InspectorClient` could rebuild against the *next* one and answer
through a different server; the host now drops every entry synchronously on
the swap (`AppElicitationController.failAll`) rather than racing core's
awaited teardown. And the request id was only monotonic within one client, so
each replacement client's first request was `app-elicitation-1` — settling
that id could resolve the wrong server's request. It now carries a
per-instance prefix.
Result validation now parses the whole value with `ElicitResultSchema` before
the requested-schema check, instead of inspecting `action` by hand: a
`{ action: "decline", content: { x: {} } }` used to pass, though the standard
result permits only primitives and string arrays in `content`.
The dialog's accessible name now includes the request id and the prompt — two
concurrent elicitations can be for the same app URI, and the previous name
made those indistinguishable, which is what the comment beside it claimed to
prevent.
`observeAppCapabilities` takes a minimal structural `onmessage` contract
(generic over the message type, since a handler typed for a narrower message
is not assignable to one typed for `unknown`), removing the double cast at the
call site.
`smoke:web:elicit` now fails on an uncaught page error after a successful
drive, and captures the async half of that class off the console channel, as
its two sibling smokes do — it could otherwise print OK over a broken bundle.
Adds the tests each fix needs, including the two the review asked for
directly: that `advertiseElicitation` reaches the `AppBridge` constructor, and
that it is absent by default.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three inline findings and one of the two suppressed. The other suppressed
one — that the hand-off's delete is still a TOCTOU — is the cross-process
guarantee, decided against for this PR and tracked as #2082.
**My convergence tests did not converge anything.** The reviewer caught that
the "two stores racing" tests cannot race: `serialize` keys its queue on the
resolved path *process-wide*, which I made it do in the previous commit
precisely so two instances would take turns. So the tests passed by
serialization while their names and comments claimed they were exercising
the optimistic retry, and the retry's successful path had no coverage at all.
A fix I was pleased with silently disabled the tests written to prove it
worked.
Split in two now, both honest about what they drive:
- `in-process mutations are serialized per path` keeps the multi-instance
cases, which pin a real property — it is what makes the in-process case
correct *without* relying on the retry.
- `cross-process convergence` injects one external write between the store's
write and its verifying read, via raw `fs` so it bypasses the queue the way
another process would. The surviving entry can only be explained by the
re-apply, so these pin the mechanism rather than the outcome. Covers a
set, a delete, and the encrypted path where every attempt re-derives.
**A well-formed envelope is not an openable one.** `readOnDiskEncryption`
classified by structure alone, so a wrong or unset `MCP_INSPECTOR_SECRET_KEY`
produced the quiet, healthy "Encrypted file" footer for a file whose every
`get` returns `null` and every `set` is refused — the exact state the
unreadable footer promises to warn about. It now authenticates: decrypt and
discard, costing one scrypt derivation (~23ms) per descriptor build, once per
`/api/config`. A test asserted the old behavior and has been rewritten; the
comment on it explains why "describable" has to mean *accurately* describable.
**The copy button's `aria-label` was eating the warning.** `aria-label`
replaces descendant text, so a screen-reader user heard "Copy secrets file
path …" and never heard "Plaintext file" — the one sentence that should
change their mind about typing a secret. The status now leads the accessible
name and the path follows, which also keeps the name a superset of the
visible text (WCAG 2.5.3).
**The spec still described the pre-#1950 invariant** in its migration
bullets: every plaintext secret lifted into the keychain and removed from
`mcp.json`, with only `KeychainUnavailableError` named. Both bullets now
describe the selected store, the durability gate that withholds the disk
delete for a session-scoped store, the strict read behind the store-wins
lookup, and `SecretStoreUnavailableError` as the family the handler catches.
Full `npm run ci` green, including the per-file ≥90 gate — the new validator
and retry branches needed six more cases to clear it.
Signed-off-by: cliffhall <cliff@futurescale.com>
Let a server hand a form `elicitation/create` to an MCP App and return the
app's ordinary `ElicitResult`, with the native elicitation form as the
fallback on every failure.
No second extension, no custom method, and no custom result shape: the only
new wire surface is a nested `elicitation` flag on the existing
`io.modelcontextprotocol/ui` extension on each side, plus
`_meta.ui.resourceUri` on the request. App rendering is selected only when
all four gates hold — client `elicitation.form`, client MCP Apps MIME type,
the nested `elicitation` setting on BOTH peers, and a valid absolute `ui://`
URI on the request.
Only the web client advertises the nested client-side setting, and only
because it has a sandbox renderer: supplying
`InspectorClientOptions.appElicitation` is what opts a client in, so CLI and
TUI keep advertising the MIME type without ever claiming they can resolve an
elicitation through an app.
Routing lives in the one funnel both entry points already use
(`enqueuePendingElicitation`), so the inbound handler and the MRTR driver
cannot diverge. Ownership is request-scoped — a per-request id keys the
renderer, iframe and bridge — so two concurrent elicitations can never
resolve through each other's bridges. An explicit `decline`/`cancel` is a
completed elicitation and goes back to the server; everything else (absent or
malformed metadata, resource/sandbox/bridge failure, an app with no
elicitation capability, a timeout, an invalid result or one that fails the
requested schema) falls back.
The Inspector speaks the ext-apps#733 / SEP-3118 wire protocol but cannot yet
consume its helpers — the released `@modelcontextprotocol/ext-apps` (1.7.5)
predates that PR. `core/mcp/appElicitation.ts` and
`AppRenderer/requestAppElicitation.ts` mirror it exactly and are marked for
deletion once a release containing it ships. One consequence needed its own
seam: ext-apps 1.7.5 parses the view's `ui/initialize` through a schema that
strips `elicitation`, so an app that correctly advertises it would look like
one that did not, silently turning every negotiated elicitation into a
fallback. `AppRenderer/appCapabilities.ts` records the raw frame instead, and
prefers the bridge's own value once it carries the key.
`AppRenderer` now takes an `AppRenderSource` union rather than a `Tool`, so
the same renderer and bridge factory serve an App tool and an elicitation
without either faking the other's shape.
Adds the public fixture (`app_choose_option` + a self-contained
`ui://demo/choose-option.html` app covering accept/decline/cancel), its two
showcase configs, and `smoke:web:elicit`, which drives the negotiated path
end to end in headless Chromium and then the same tool against a server that
never advertised the capability, asserting the native form takes it.
Closes#1854
Signed-off-by: cliffhall <cliff@futurescale.com>
The headline is a correction to my own claim rather than a new defect.
**The optimistic verify is weaker than the comment said it was.** It
described the residual as a crash between the write and the verifying read.
The reviewer pointed out that no crash is needed: order two writers as
write-A, verify-A, write-B, verify-B and both report success while A's entry
is gone — A's verify simply ran before there was anything to see, and B did
nothing wrong. A crash is one instance of that shape, not the whole of it.
The trace in the old comment quietly assumed A verifies *after* B writes,
which nothing guarantees.
Corrected in the doc comment, the README and the spec, and the honest
summary is now stated rather than implied: this converges in every
interleaving where the clobber lands before the verify, it is not mutual
exclusion, and a deployment that needs the guarantee wants an OS-backed lock
from a dedicated library — not another hand-rolled election. It remains
better than what it replaced, which lost updates across a wider set of
interleavings and could not be closed at all with what Node exposes.
The other findings:
- **An envelope naming the cipher still is not an openable one.** Round 9
added a `kdf`/`data` presence check; three dot-separated strings satisfied
it while an empty tag, a 4-byte IV or `N: 3` still made every decrypt
throw — so the quiet "Encrypted file" footer went to a file whose next
save was guaranteed to fail. Now validates part sizes, a non-empty
ciphertext, and the scrypt cost parameters (node rejects a non-power-of-two
`N` synchronously, out of the derivation rather than the decrypt). A
positive control asserts a file we actually wrote still reads as encrypted.
- **`encryptionUnknown` + `looseMode` claimed an exposure it had not
established.** With both set, `plaintext` is absent, so a `!== false` test
landed on "anyone who can read it can read the secrets in it" — about a
file that may well hold ciphertext we simply could not classify. Three-way
now, in `secretStorageCaveat` and mirrored in the footer's tooltip, with a
test on each.
- **The README described a lock that no longer exists**, which is the worst
kind of stale doc: it stated a durability guarantee the code had stopped
providing. Rewritten to describe the actual design, including the residual.
- **The private-method access in the concurrency test** now carries the
justification the repo's TypeScript rules require: the asserted shape is
`writeMap`'s real signature, so a rename breaks the line instead of
silently detaching the stub, and a production injection seam would let the
real write path drift from the tested one.
Also updates `specification/v2_servers_file.md`, which still described #1356's
world: one keychain-only store, and unavailability as a hard error. It now
covers store selection and the probe, the file format and lazy encryption
upgrade, the concurrency design and its residual, the `getStrict`/`getMany`
seams and the two traps `getStrict` hit, the durability gate on migration,
the keychain hand-off and its conditional delete, and how the active store is
surfaced.
Full `npm run ci` green. (One unrelated `AppRenderer` theme-flip test failed
once under full-suite parallelism and passes in isolation with and without
these changes — a flake, not a regression.)
Signed-off-by: cliffhall <cliff@futurescale.com>
Five taken; one rejected with evidence, below.
- **The keychain hand-off ran outside the file lock.** Read → copy → delete
has to be one transaction: another Inspector completing a `set` between
the read and the delete had its brand-new secret removed without ever
being copied — a write that reported success and then vanished, which is
the loss the lock exists to prevent, reached through the migration meant
to preserve things. The whole sequence now runs under the same
cross-process lock every mutation takes.
- **An unmigratable entry was skipped and the file deleted anyway.** A key
that is not `serverId:field` cannot be addressed through the store API,
so it cannot be copied — which makes the hand-off incomplete by
definition, and the complete-success rule then forbids removing the
source. It was discarding that value permanently in order to tidy up a
migration that had failed. The copyable entries still go across; the file
stays, and says why.
- **`readOnDiskEncryption` accepted any format version.** A version-2 file
was reported as a usable plaintext or encrypted store even though
`readMap` refuses it and every save fails. Checked before the mode now,
matching `readMap`.
- **The loose-mode warning at selection was encryption-blind**, claiming a
reader could read the secrets of a file that is ciphertext, and it
duplicated the caveat printed moments later by `warnAboutSecretStorage`.
Selection now only repairs; the descriptor path does the reporting,
because it is the one that knows whether the file is encrypted.
- **The footer's tooltip offered to copy the path without showing it.** The
band stopped printing it and the accessible name carries it for
screen-reader users, which left a sighted user unable to see where a
custom destination points without pasting it somewhere else.
**Not taken: "`writeStoreFile` is not atomic."** It is. The finding reads
`store-io.ts` line 84 as a direct write, but `writeFile` there is imported
from the `atomically` package, not `node:fs`. Verified rather than argued:
writing over an existing file changes its inode (so a rename happened) and
leaves mode `0600`. I had started implementing the suggested temp+rename
before checking the import, which would have replaced a tested library with
a hand-rolled duplicate.
That check corrected something of mine, though: `tightenSecretFilePermissions`
justified itself with "a rename over a 0644 file keeps the loose mode
forever", which is false — the temp carries `mode: 0o600` and the rename
replaces the inode, so every *write* re-establishes it. The function is
still needed for the case it actually addresses: a file that exists and is
never written. Comment corrected to say so.
Full `npm run ci` green — 5,753 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings, all suppressed, all real. Three are the same mistake in
different places: a state that was never established being reported as a
confident answer.
- **`readOnDiskEncryption` conflated "no file" with "unreadable file".**
Both answered `null`, and the descriptor falls back to the configured
write policy on `null` — so a passphrase-configured install reported
"File (encrypted)" about a file whose envelope had never parsed, and
which `set` was in fact about to refuse rather than overwrite. It now
returns a four-state result, and only `absent` falls back to the policy
(honestly: the first write really will create it that way). `unreadable`
produces `encryptionUnknown` on the descriptor, `plaintext` is omitted
entirely so nothing can read a default out of it, and the label, caveat,
tone and footer all carry "unreadable" rather than guessing a mode.
- **The loose-mode warning overstated exposure for an encrypted file.**
"Anyone who can read it can read the secrets in it" is false when the
file is ciphertext — a reader gets the bytes and still needs the
passphrase. Both the caveat and the footer tooltip now branch on
encryption: the passphrase becomes the only protection, which is the
real consequence. Overstating it is not harmless, because a warning that
cries wolf is discounted on the occasion it is literally true.
- **The keychain hand-off could invert its own keychain-wins rule.**
`SecretStore.get` answers `null` both for a missing entry and for a
keychain it could not read, and the migration *writes* on `null` — so a
transient read failure would overwrite a newer keychain value with the
older file copy. Added an optional `getStrict` (throws instead of
swallowing) implemented by `KeyringSecretStore`, with
`secretStoreGetStrict` falling back to `get` for stores whose reads
cannot fail. A throw now aborts the hand-off with the file left in
place, so the next run retries.
Writing the tests turned up one more, in code the review had passed:
`parseOwner` recorded a garbled pid as `-1`, and `process.kill(-1, 0)` is
a *wildcard* asking about every process this user can signal — it answers
"alive" almost always, which would have made a lock with a corrupt stamp
permanently unstealable. Only a positive pid is usable now; anything else
is treated as no pid at all and the mtime decides.
Tests: the four envelope states kept distinct; the descriptor refusing to
guess, with the genuine no-file case as the control; strict-read abort
without a write and with the file preserved, plus a store whose `get` and
`getStrict` disagree so the test proves which one the migration consults;
encryption-aware wording asserted in both directions in the caveat and the
band; three unusable owner stamps and a wildcard pid; and the descriptor
shapes that must now be rejected — neither half of the encryption question
answered, or both. Full `npm run ci` green — 5,750 tests under the
coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
No behavior change.
Adding `filePermissionsSentence` in decc673a inserted it directly under
`footerMessage`'s doc block, which left two block comments stacked and each
attached to the wrong function: `footerMessage` was undocumented, and its
"why not `secretStorageLabel`" rationale read as though it were about the
permissions helper, which it is not.
Moves that block back above `footerMessage`, and drops the inline comment
inside it that now duplicated the new helper's doc — and had gone stale
besides, since it still described two permission states rather than three.
Caught by @mcp-inspector-v2-f1 on a read of the pushed commit.
Signed-off-by: cliffhall <cliff@futurescale.com>
Repairs origin as well as answering the review. 2d2c787e committed the
`absorbFileSecretsIntoKeyring` tests without the implementation they
import, so the branch has been failing `tsc` since; this commit brings the
source across.
Round 5's four findings (two inline, two suppressed):
- **The stale-lock takeover was still racy.** Round 4 made *stealing*
liveness-aware but left the takeover as remove-then-recreate, which is
not an election: two waiters can both judge the same lock stale, the
first removes it and takes a fresh one, and the second then removes
*that* — both inside the section, which is the lost update the takeover
exists to prevent. Claiming is now an atomic `rename` to a
token-unique name, so exactly one waiter can take a given stale lock and
the losers get ENOENT. A failed claim also falls through to the deadline
instead of `continue`ing, so a takeover that can never succeed times out
rather than spinning forever on a swallowed error.
- **Every `stat` failure was read as "no file yet".** An existing file we
cannot inspect (EACCES on the directory, a mode-less filesystem) then
produced no warning at all, and the footer went on stating owner-only
permissions having verified nothing. There is now a third state,
`unknown`, carried end to end: `permissionsUnknown` on the descriptor, a
caveat that says the permissions could not be checked, a `warn` tone, and
its own branch in the footer sentence — which otherwise would have
claimed "Owner-only permissions." by omission, the same bug one layer up.
- **mountinfo decoded only `\040`.** The kernel escapes tab, newline and
backslash too (`\011`, `\012`, `\134`), so a mounted path containing any
of them was compared in its escaped form, matched nothing, and read as
the container's writable layer — `memory` selected and persistence
silently lost on a box that had arranged for it. Decoded as octal in
general now, since the rule is octal-in-general and a future addition
would otherwise reintroduce exactly this.
- **File-backed secrets vanished once a keychain appeared.** Save secrets
without libsecret, install libsecret, and the next run selects the
keychain and stops seeing them: still on disk, read by nothing, and
nothing looks broken. `absorbFileSecretsIntoKeyring` hands them over on
the run that finds a keychain, under the rules the two existing plaintext
migrations already use — keychain wins on conflict, the file is removed
only on complete success, and a file that cannot be read is never
deleted, because that is precisely the case where the values would be
lost.
Tests: a two-waiter race over one stale lock asserting a single holder
throughout; `unknown` from `readSecretFilePermissions` through the
descriptor to the footer sentence and tone; the hand-off's happy path,
keychain-wins, write-failure, undecryptable-file, empty-file,
malformed-key, and explicitly-configured-keyring cases; and six more
rejected descriptor shapes, including each file-only field on a non-file
kind. Full `npm run ci` green — 5,732 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
The footer's file states now read "Secrets: Plaintext file. Owner-only
permissions." and "Secrets: Encrypted file. Owner-only permissions.",
and the band itself became the copy affordance for the path.
Three things worth reviewing:
- The path is no longer printed on the band. It goes to the clipboard
instead, which means the only place it stays *readable* is the
button's accessible name — so that carries it in full
("Copy secrets file path: /home/node/.mcp-inspector/secrets.json")
rather than a generic "Copy". A clipboard is no use to a screen-reader
user who cannot inspect it.
- "Owner-only permissions." is a claim of fact, so it is made only when
the mode was verified as 0600. `looseMode` is set exactly when it is
something else and could not be tightened, and the message says
"Mode 0644 — not owner-only." there. A footer that asserted the
opposite of the truth about a secrets file is the one failure this
band exists to prevent, so it has its own test.
- The footer phrases its own copy rather than reusing
`secretStorageLabel` for the file case: that label is parenthetical
("File (unencrypted)") because the startup banner appends
" at <path>" to it. The tone and the underlying facts still come from
the shared helpers, so the two surfaces cannot disagree about
anything that matters. Memory's line is still derived from the shared
caveat.
The click target is an `UnstyledButton` filling the band edge to edge —
a real control, reachable by keyboard and announced as a button, rather
than a click handler on a div. Padding moved from the band to the inner
row so the button has no dead margins.
Signed-off-by: cliffhall <cliff@futurescale.com>
Reads "Memory (this session only): Secrets are not written anywhere and
are lost on exit." — shorter tail than "lost when the Inspector exits",
and a colon separating the store from what follows it.
The colon is added at the footer's composition, not inside
`secretStorageLabel`. The label is also consumed alone by
`secretStorageSummary`, which appends " at <path>" for a file-backed
store, so a colon baked into the label would print
"File (encrypted): at /path" in the startup banner. It renders only when
a detail actually follows.
The caveat string is shared with the banner's warning line, so the
terminal picks up the shorter wording too.
Signed-off-by: cliffhall <cliff@futurescale.com>
Weights the label so the band reads as a labelled fact rather than one
run of prose — the eye lands on what the line is about before reading
which store it names.
Rendered as a `span` inside the existing `Text` rather than as a sibling
in the `Group`: `Text` renders a `<p>`, so a nested one would be a `<p>`
inside a `<p>` (invalid, and the browser closes the outer paragraph
early), and a sibling would take the Group's `gap` and sit too far from
the value it labels. `inherit` keeps it at the label's own size.
Splitting the text is why the assertions move off `getByText`: it matches
a string against an element's *direct* text nodes, so "Secrets: OS
keychain" now reads as two fragments even though it renders as one line.
The unit tests, both modal tests, and the four stories assert
`toHaveTextContent` on the band instead, which reads the full subtree.
Verified in the running dev server: prefix 700, store name 500, both at
the same 12px.
Signed-off-by: cliffhall <cliff@futurescale.com>
Seven findings, all taken.
- **The descriptor went stale after the upgrading write.** Round 1 made it
read the file instead of the policy, but it was still computed once and
cached beside the selection — so the first `set` under a newly-set
passphrase encrypted the file while `/api/config` kept serving "still
unencrypted" for the rest of the session, describing bytes this very
process had just changed. The *selection* stays cached (the banner, the
payload, and the store doing the writing must agree on which store it
is); the file descriptor is re-derived per call, which costs one read of
a small file per config fetch.
- **The mount check asked the wrong question.** Comparing a directory's
`st_dev` against its parent's detects a mount *at* that path, but a
volume mounted at `/home/node` leaves `/home/node/.mcp-inspector` an
ordinary subdirectory sharing its parent's device — so a durable
container setup was judged unmounted and demoted to session-only
`memory`. It now reads `/proc/self/mountinfo` and asks whether the
directory is *under* any mount other than `/`, which also catches the
same-device bind mount no `st_dev` test can see. Where mountinfo is
unreadable (macOS, Windows, a `/proc`-less container) it falls back to
the device comparison, now against `/` rather than the parent.
- **A decrypted payload was cast, not checked.** GCM proves the bytes are
authentic, not that they parse to the right shape. An array passes
`typeof === "object"`, takes the named assignment, and then
`JSON.stringify` drops every named property of an array — so `set` would
resolve having written a file without the secret it was handed. Silent
loss on the one operation whose entire contract is that it does not lose
things. Both branches now validate a non-array object of string values,
and a shape refusal is deliberately not rethrown as a key mismatch,
which would send the user to fix a working passphrase.
- **The read-modify-write cycle was only serialized in-process.** A
durable file is shared state and a second Inspector on the same box (a
CLI run beside a web session) could read the same map and overwrite the
first one's save, both reporting success. Mutations now take a
`mkdir`-based lock. A lock left by a killed process is stolen after 10s
— one `docker kill` must not brick secret writes permanently, which
would be a worse failure than the race. If the lock cannot be created at
all, the write proceeds unlocked rather than denying a real user a real
secret to prevent a race with a process that may not exist. Reads do not
lock: `writeStoreFile` is atomic, so a reader sees the old file or the
new one, never a torn one.
- **The permission repair was fire-and-forget while the docs promised
0600.** A file owned by another user, or on a read-only mount, stayed
exposed while the footer described it as protected. It now verifies the
resulting mode and warns, naming the actual mode, when it could not be
tightened.
- **`Group`'s theme carried a domain-named `secretStorageFooter`
variant**, which AGENTS.md reserves for app-wide primitive styling.
Renamed to the structural `stickyModalFooter`; the secret-storage
meaning stays in the element that owns it.
- The PR description still claimed the salt made a short passphrase safe,
contradicting the corrected comment and README. Updated.
Tests: both mount layouts that motivated the change (mount-at and
mount-above) plus the sibling-prefix, space-escaped, first-run, and
no-mountinfo cases; the array and non-string refusals on both the
plaintext and the authentic-ciphertext paths (the latter through a
hand-built fixture, since the store's own writer cannot produce that
state); lock exclusion, stale-lock stealing, the un-lockable fallback, and
two independent store instances racing; the permission report in all three
states; and the descriptor no longer going stale within one process. Full
`npm run ci` green — 5,688 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Six findings, all taken.
- The descriptor reported the *write policy*, not the file. Adding
`MCP_INSPECTOR_SECRET_KEY` to an install that already has a plaintext
file flipped `store.encrypted` immediately, so the banner and footer
claimed "File (encrypted)" while the existing secrets sat readable
until the next `set`. `FileSecretStore.readOnDiskEncryption()` now
reads the envelope (never the payload, so it needs no passphrase) and
the descriptor is built from that. The transitional state gets its own
`pendingEncryption` flag, which changes the advice without softening
the verdict — "set a passphrase" is the wrong thing to tell someone who
already has, so the caveat names the pending write instead.
- The secrets file ignored the configured storage directory. A container
mounting only that directory was judged unmounted, fell back to
`memory`, and lost secrets the user had arranged to keep.
`defaultSecretFilePath()` now follows `MCP_STORAGE_DIR` — the variable
that already relocates OAuth tokens and `client.json` — between the
explicit `MCP_INSPECTOR_SECRET_FILE` and the `~/.mcp-inspector`
default. (The issue comment called it `MCP_INSPECTOR_STORAGE_DIR`; no
such variable exists.)
- `version` was never checked before the plaintext branch, so a
`{ version: 2, encryption: "none" }` file was read and silently
rewritten as version 1, discarding whatever the newer format added —
the same destroy-to-satisfy-an-additive-request case the encrypted
branch already refuses. Checked first now, so the answer no longer
depends on which encryption mode the newer writer chose.
- The doc comment claimed a per-file salt made "a short, memorable value
safe to use". It does not: the salt defeats precomputation, not
guessing, and the scrypt cost is deliberately a few milliseconds
because it runs on every read and write. Both the comment and the
README now say to use a high-entropy value, generated rather than
chosen.
- `usableSecretStorage` validated only `kind`, so a partial
`{ kind: "file" }` was cast through and — because a missing `plaintext`
is falsy — rendered as the quiet, neutral *encrypted* file. That is the
confident half-answer the guard exists to reject, produced by the
absence of information rather than by any claim the backend made.
`reason`, `durable`, and each kind's own required fields are checked.
- The footer's inline `Tooltip` carried four static props; extracted to a
`CaveatTooltip` constant per the `.withProps()` rule.
Tests cover each: the policy/file disagreement in both directions and the
no-file-yet fallback, `MCP_STORAGE_DIR` and its precedence against
`MCP_INSPECTOR_SECRET_FILE`, the version refusal (asserting the bytes
survive it) split from the unrecognized-cipher refusal it used to be
conflated with, `readOnDiskEncryption` including the keyless and
unparseable cases, five new rejected descriptor shapes, and the changed
caveat. Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#1950
Before this, a box with no reachable OS keychain — the published
container (no D-Bus session, #1848), Android/Termux (no platform binary,
#1905), a minimal Linux install without libsecret — could not persist an
OAuth client secret or a stdio `env:` value at all. `KeyringSecretStore`
degraded into a store that silently holds nothing: `get` returned null,
`delete` no-opped, and `set` hard-failed with a 503. That is the correct
failure behavior, but it left the documented, supported way to run the
Inspector with no secret persistence whatsoever.
Adds the two missing implementations and, more importantly, the policy
that picks one and the surfaces that say which was picked.
- `FileSecretStore` — one `0600` JSON document at
`~/.mcp-inspector/secrets.json`, encrypted with AES-256-GCM whenever
`MCP_INSPECTOR_SECRET_KEY` is set (scrypt, per-file random salt). The
whole map is encrypted as a unit rather than value-by-value so the
account names — `${serverId}:${field}` — are hidden too; a per-value
scheme would leave a readable index of which servers you hold a client
secret for. A file that can no longer be decrypted reads as empty
(matching the interface's read tolerance) but *refuses to be written*,
because replacing a file of still-valid secrets to satisfy an additive
request destroys data.
- `secret-store-selection.ts` — the policy.
`MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright;
otherwise probe the keychain and, if unreachable, fall back loudly. The
fallback is `memory` in a container whose secrets directory is not on a
mount, and `file` otherwise — so mounting the volume the README already
recommends for the catalog flips the same run to durable storage with
no configuration, and an unmounted container gets the honest answer
rather than a file `--rm` will discard. Cached per process so the
banner, `/api/config`, and the store doing the writing cannot disagree.
- Automatic, but never silent. The downgrade prints at startup, rides
`GET /api/config`, and is named in a permanent 32px footer at the
bottom of both dialogs that accept a secret. A startup banner is seen
once by whoever started the process; a toast is seen once; a
dismissible banner is the thing a user dismisses before doing the work
it describes. The plaintext and memory cases render their caveat inline
in a warning tone — a warning you must hover to discover is not one.
- `SecretStoreUnavailableError` is now the base every store's `set`
throws, so the routes translate one condition into a 503 instead of
matching `KeychainUnavailableError` by name.
Docs: the container section of the root README gains a table of which
store you get and how to make secrets survive, plus the
`MCP_INSPECTOR_SECRET_KEY` upgrade/mismatch rules.
Tests: the existing interface contract block is extracted to
`secretStoreContract.ts` and run against all three implementations, so
none can drift on the availability contract. `FileSecretStore` is tested
against a real temp filesystem — mode, cross-instance round-trip, that
the ciphertext does not contain the plaintext, the read-modify-write
race, and every refusal path. Full `npm run ci` passes, per-file gate
included.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-17 review on #2035. A real crash, and a class the
earlier prototype work missed rather than a narrower version of it.
`toString`, `constructor` and `__proto__` are valid RFC 6570 variable names,
and `completions` starts as `{}`. So for such a name:
completions["toString"] ?? [] => the prototype's FUNCTION
`??` catches only null and undefined, so that function went to Mantine's
Autocomplete as its `data`, and the field crashed on first render with
`TypeError: data.map is not a function` -- reproduced by reverting the fix
under the new test.
The earlier rounds fixed this for the *values* map, which is seeded with
every declared variable and so always has the key as an own property. The
completions map is the one name-keyed map this component reads WITHOUT
having seeded it, which is what made it the surviving instance. Both reads
now go through `Object.hasOwn` -- the render, and the stale-dropdown check
that clears a variable's options on the next keystroke.
Only the autocomplete branch could reach it, so the regression drives that
branch: completions supported, a `{toString}` template, type and submit.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-13 review on #2035. Two suppressed comments, both
real, both in the required-group work from round 7 rather than in the recent
preview rework.
1. `{a,a}` is ONE requirement named twice, but `requiredGroups` stored it as
a two-name group, so everything downstream read it as "either of these
will do". The TUI was left with a form it could not submit: its
`length === 1` test marked `a` optional while ResourceTestModal's guard
still refused a blank. The web panel offered the useless hint
"Any one of: a, a". Names are now deduplicated within the expression --
expansion still emits both occurrences (`x://{a,a}` -> `x://1,1`), which
RFC 6570 requires.
2. The per-field hint used `find`, so a variable in several shared groups
advertised only the first. With `{a,b}{b,c}{a,c}`, filling `b` satisfies
the first two and every visible hint then looks met while Read Resource
stays disabled on the unmet `{a,c}` -- a requirement with nothing on
screen pointing at it.
Fields now hint every shared group they sit in. That alone is not enough,
because no per-field hint can say WHICH group is still outstanding, so
the form states it directly: "Still needed: a or c", built from the same
`unmetRequiredGroups` the submit gate uses, so the message and the button
cannot disagree. `hasRequiredValues` is no longer called here -- the
panel derives both from the one list.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
#1961 keyed the sidebar's React children by source position, but everything
downstream still identified a tool by `tool.name`. With a `tools/list` that
repeats a name, both copies highlighted at once, the click guard made the
other copy a no-op, and `tools.find((t) => t.name === …)` always resolved to
the first — so a later copy rendered but could never be opened.
Thread the row key through selection instead:
- `utils/toolUtils.ts` now owns `toolRowKey` (moved out of ToolControls) and
adds `findToolByRowKey`, which compares computed keys rather than parsing
one so the two can't drift.
- `ToolControls`: `selectedName` → `selectedKey`; the highlight and the click
guard compare keys, and `onSelectTool` emits the row key.
- `ToolsScreen`: `ToolsUiState.selectedToolName` → `selectedToolKey`, with
both lookups going through `findToolByRowKey`. Renamed through
`screenUiState.ts` and `App.tsx`'s `onToolsUiChange`.
`onCallTool` still sends `selectedTool.name` — the duplicated name is the
genuine wire identity; only the UI identity needed to be unique.
Also prefixes the excluded (SEP-2243) rows' keys: they render into the same
`Stack` as the list rows but index within their own list, so an excluded tool
sharing a name and index with a listed one collided in the key space.
Closes#2001
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-8 review on #2035 (one inline comment, one
suppressed). Both real.
1. RFC 6570 distinguishes an UNDEFINED variable, which omits its whole
expression, from one defined as the empty string, which expands. The
expander was calling `definedValues` on the way in, collapsing the two --
so `x{?q}` with `{ q: "" }` gave `x` instead of `x?q=`, and there was no
way for any caller, `readResourceFromTemplate` included, to request
either that or `x;q`. Key presence is now honored, including §3.2.7's
rule that the matrix operator drops the `=` for an empty value (`x{;q}`
-> `x;q`, while `?`/`&` keep it).
Dropping blanks is a FORM concern, so it moved to the two form
boundaries: the web panel seeds every declared variable with "" and the
TUI's ink-form returns "" for every untouched field, so neither can tell
"never touched" from "deliberately empty" -- but that is a fact about a
text input, not about the template, and it has no business deciding what
a template means for every other caller.
Six unit tests and one integration test encoded the old collapse and now
assert the corrected meaning. The prototype-collision cases get stronger
rather than weaker: a blank `{?toString}` expands to `?toString=` -- its
OWN empty value -- where the original bug expanded the inherited
function's body.
2. The expansion error is now `role="alert"`. It can appear while typing (a
pasted value that cannot be encoded), and plain text is not announced, so
a screen-reader user met a disabled Read Resource with no stated reason.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Server Settings already has a Custom Headers editor, so adding one to the
manual add form created a second place to edit the same field. The reporter
had also conditionally withdrawn #1915 — "editing that information from the
server settings dialog is sufficient" once #1848 closed, which it has.
Reverts the add-form editor, the shared KeyValueRows element, the optional
`settings` argument on addServer / updateServer, and the App-level merge
helper. What remains are two defects found while working in
ServerSettingsForm, which this PR now closes instead (#2040): the Custom
Headers hint misstating OAuth precedence, and the key/value rows' missing
accessible names.
Signed-off-by: cliffhall <cliff@futurescale.com>
Ports the two behaviors PR #2033 (the parallel attempt at #1919, now closed
as a duplicate) got right and this branch did not.
1. An expression declaring no variable is a malformed template, not one
with a member to skip. RFC 6570 requires at least one varspec per
expression and admits no empty member, so `{}`, `{,}`, `{a,}`, `{?}`
and `{*}` (an explode modifier is not a name) now make the template
invalid. Skipping them was the more dangerous reading: `x://{}`
expanded to `x://` while the form rendered no inputs, so its
"everything required is filled" check was vacuously true and it
submitted a URI that is not the template the server published.
2. The read is withheld when expansion fails. `expandUriTemplate`'s
raw-template fallback exists for the preview, which runs during render
-- but the submit path used it too, so an invalid template was read
with its braces intact and the server answered with a confusing "not
found" for a defect that is not the user's. `tryExpandUriTemplate`
returns the URI or the reason as a value the caller cannot mistake for
one; the panel gates Read Resource on it and prints the reason.
This also covers a value that cannot be encoded -- an unpaired
surrogate has no UTF-8 encoding, so `encodeURIComponent` throws
`URIError` on it, and a text input can hold one via paste.
`requiredGroups` now skips an expression naming no variable: an empty
group can never be satisfied, so it would gate the form a second time on
a condition nothing can meet, and the "any one of" hint built from it
would name no fields. The accurate reason is the malformed template,
which the expansion gate reports.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round-4 review follow-ups on #2038.
The Custom Headers hint claimed a custom `Authorization` value is "ignored"
once OAuth is configured. Verified against the installed
@modelcontextprotocol/client: both SSE and streamable-HTTP `_commonHeaders()`
set the bearer first and then spread `requestInit.headers`, so a custom
value *overrides* the OAuth token. The hint told users the opposite of what
would happen, and could have them silently break their own auth. Corrected in
both places — the modal's new hint and the pre-existing one in
ServerSettingsForm, which carried the same wrong claim.
`utils/serverSettingsPatch.ts` imported both its shared types from
components, reversing the one-way `components -> utils` direction AGENTS.md
sets. `KeyValuePair` moves to `utils/keyValuePairs.ts` (KeyValueRows
re-exports it), and the mode is spelled as a literal union — still checked,
since `ServerConfigModalMode` must stay assignable to it at the call site.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round-3 review follow-ups on #2038.
`buildHeaderSettingsPatch` treated the mere presence of stored headers as a
change, so saving an id or URL edit re-sent the modal's settings snapshot —
taken when it opened — and could overwrite a metadata or OAuth change made
in the settings form since. It now compares the submitted headers against
the stored ones and omits `settings` when they match, which is what makes
the PUT route preserve the node. Order counts as a difference: it is what
the form round-trips and what the user sees.
Row labels carried the key alone, so two rows sharing a key — mid-edit, or a
duplicate a server persisted — gave both rows' controls identical accessible
names, the thing the labelling exists to prevent. The row number is now
always part of the label rather than a blank-key fallback.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two review findings on the fetch wrapper:
The content-type test was a substring match for "json", which also
matches `application/x-ndjson` — how the streamable-HTTP transport can
serve its long-lived server-push channel (`isLongLivedStreamResponse`).
Awaiting `.json()` on a clone of an unbounded stream never resolves, so
the MCP connection would hang for as long as an override was configured.
It now matches the media type exactly (`application/json` or a `+json`
suffix), with a regression test driven by a stream that never closes.
The overrides also leave `issuer` as discovery returned it, so pointing
one at an authorization server that advertises a different issuer is
rejected on callback by the RFC 9207 / SEP-2352 mix-up defense. That
check is deliberately not weakened; the limit is now stated on the
module, in the field description, and in the configuration guide.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round-2 review follow-ups on #2038, all three from the suppressed set.
The header rows stayed editable while a save was in flight, unlike the rest
of the modal — a slow request let a user edit a row whose value had already
been captured, and the modal then closed having persisted the earlier one.
`KeyValueRows` takes a `disabled` prop and the modal passes `submitting`.
Each row's clear buttons were still named the bare "Clear", so a screen
reader saw six indistinguishable buttons across three rows. They now name
the field they empty ("Clear header name, Cookie").
The App-level headers-into-settings merge — the seam that distinguishes an
edit from a clone, and where the credential copy lived — moves into
`utils/serverSettingsPatch.ts` with its own tests. App.tsx is outside the
coverage gate, so inline it could not be covered at all.
Signed-off-by: cliffhall <cliff@futurescale.com>
The overrides are per-server configuration for the *resource* server's
authorization server, but the fetch wrapper that applies them is shared
with the enterprise-managed (EMA) flow, whose IdP OIDC discovery runs
through the same fetch and satisfies the metadata predicate. An
authorization override could therefore redirect the IdP login to the
resource AS, and a token override could send the IdP code or refresh
token there.
OAuthManager.getEndpointOverrides() now returns nothing when the server
is enterprise-managed — the same boundary redirectToExternalAuthorization
draws for the custom authorization parameters (#2018) — with a regression
test, the note surfaced on both form fields, and the rule documented.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-4 review on #2035 — again reported as "no new
comments" while carrying three suppressed ones. All three reproduced.
1. The TUI form still named its fields from the SDK. `uriTemplateToForm`
read `UriTemplate.variableNames`, which mangles a name: `{;id}` yields
";id" and `{id:3}` yields "id:3". The form therefore submitted
`{ ";id": "7" }` while the shared expander looked up `id`, found
nothing, and dropped the expression — the value vanished silently.
This is the finding that mattered most, because it falsified the claim
this change is built on. Moving the expander into core only makes the
clients agree if each client's FORM derives its names from the same
parser: a form submits under the names it rendered. The TUI now reads
`templateVariables`, and the SDK template is constructed only to
validate, preserving the existing malformed-template diagnostic.
Its `required` flag comes from `requiredGroups`, and only a variable
that is the sole member of a non-omittable expression is marked: ink-
form cannot express "any one of these", so marking every member of
`{a,b}` required would refuse input the expander accepts.
2. `encodeURIComponent` is not RFC 3986's unreserved set — it leaves the
sub-delims !'()* bare, which RFC 6570 requires encoded for every
operator except + and #.
Fixing that settled a design question left open in the previous round.
With two encoders, the SAME value encoded differently depending on
whether its expression happened to carry a modifier, since only the
modifier pushed it onto our path. So delegation is gone entirely: one
expander, one set of rules. The SDK's `UriTemplate` is still
constructed, but only to validate a template.
3. The "Any one of: a, b" hint could contradict the disabled submit
button. A name can sit in a singleton required group AND a shared one
(`x://{a}/{a,b}`), where the singleton demands that exact field. The
hint is suppressed in that case.
Signed-off-by: cliffhall <cliff@futurescale.com>
A floated promise is invisible at review time — the call reads like an
awaited one minus four characters — and the unhandled rejection it
produces surfaces in a different test, in a different file, as a stack
pointing at SDK internals. In #1947 two un-held `callTool` calls made
`npm run ci` unpassable and took a full investigation to attribute; the
fix was two lines. `@typescript-eslint/no-floating-promises` catches the
class outright, so enable it at `error` everywhere.
The rule is type-aware and `tseslint.configs.recommended` is not, so each
scope's config gains a parser project. The four clients name their own
tsconfig projects (both of them — `src` and `__tests__`/`server` live in
different ones). The root scope has no tsconfig at all, so add
`tsconfig.lint.json`: a lint-only project covering `core/**`,
`test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and
gating nothing. It sets `moduleResolution: bundler` deliberately — under
NodeNext `core/`'s extensionless imports fail to resolve, every import
degrades to `any`, and the rule silently stops seeing promises at all.
Measured surface, and the fixes:
- core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono
invokes subscribers with a bare `subscriber()`, so a promise returned
from the listener is floated one layer up; `closeAbortedStream` owns
the rejection where it can actually be owned.
- tui (5) — `handleDisconnect` had no catch of its own and its only
caller is a key handler that cannot await, so a failing disconnect was
a live unhandled rejection; it now surfaces the message the way
`handleConnect` does. The rest are `void` at callers that genuinely
cannot await, over callees that already own their failures.
- web (92) — 83 are Storybook `expect(...)`, which is instrumented and
returns a promise, now awaited (`scrollAreaStoryAssertions` is async
for the same reason). Two tests were asserting against a provider call
they never awaited. `open(url)` in both web servers now warns instead
of escaping a synchronous callback.
- cli, launcher — already clean.
Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s
to ~19s, inside `validate`. Narrowing the loaded projects is the lever if
that needs reducing later; dropping the rule is not.
Closes#1959
Signed-off-by: cliffhall <cliff@futurescale.com>
Adding a server by hand offered no way to set custom HTTP headers, so the
only route to a cookie or a routing header was importing a config file.
The manual "Add server" form now carries a Custom Headers key/value editor
for the sse / streamable-http transports, pre-populated when editing or
cloning and submitted alongside the config.
Headers are not part of `MCPServerConfig` — they live on the entry's
`settings` — so they travel as `onSubmit`'s third argument rather than
folded into the config, and `addServer` / `updateServer` gained an optional
`settings` parameter to carry them to the backend (`POST /api/servers`
already accepted one). `updateServer`'s omission semantics are unchanged:
with no settings passed, the route still preserves the node on disk.
`KeyValueRows` moves out of ServerSettingsForm into a shared element so the
two editors cannot drift, and each row's controls gain a row-scoped
`aria-label` ("header value, Cookie", "Remove header, Cookie") — previously
every remove button announced only "X", indistinguishable across rows.
Signed-off-by: cliffhall <cliff@futurescale.com>
Adds two optional per-server fields — `oauth.authorizationUrl` and
`oauth.tokenUrl` — that override the endpoints authorization-server
metadata discovery resolved, so a server can be pointed at a development
or staging authorization server without changing what it advertises.
SDK v2 routes both endpoints through the discovered metadata document and
neither through OAuthClientProvider, so the override is applied by
wrapping the client's base fetch and patching the metadata document in
flight (core/auth/endpointOverrides.ts). Wrapping the base fetch — rather
than only the auth fetch — also covers the discovery the SDK runs from
inside the transport on the 401/refresh path.
The fields round-trip through mcp.json, the /api/servers routes, the
runner (CLI/TUI), and the Server Settings → Authorization panel, where a
value that is not an absolute http(s) URL is flagged inline; at runtime
such a value is dropped with a single warning rather than failing the
connection.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-3 review on #2035. It again reported "no new
comments" while carrying two suppressed ones; both reproduced.
1. Requiredness could not live on a variable at all. Deduplication kept the
first occurrence's group, so in `x{?a}{?b}{a,b}` both names ended up
required with singleton groups and the form demanded both -- while the
SDK expands that template with only `a` to "x?a=11". Widening the stored
group would not have been enough either: in `{a,b}{a,c}`, filling `b` and
`c` satisfies both expressions, which no per-variable flag can express.
So requiredness is now returned per expression by `requiredGroups`, and
`hasRequiredValues` asks that each group be satisfied by any one of its
names. `TemplateVariable.groupNames` is gone rather than left as a field
that quietly means something narrower than it reads; `required` remains,
documented as driving the "Optional" marker and nothing else.
2. `encodeURI` is not the allow-reserved encoder `+` and `#` call for. It
escapes `[` and `]`, which are reserved and must survive, and it escapes
`%`, so an already-encoded value is double-encoded. Measured:
"[::1]" -> "%5B::1%5D" and "%2F" -> "%252F". Both corrupt the URI rather
than merely over-escaping it -- an IPv6 literal or a pre-encoded path
reaches the server altered, which is the same class of defect #1919 is
about.
Added an RFC 6570 3.2.1 encoder that preserves reserved characters and
existing pct-triplets, splitting on `%XX` so a lone `%` is still encoded
to `%25`, and matching with the `u` flag so an astral character is
encoded whole. `+` and `#` expressions are now taken over even for a
single name, so both expansion paths agree on what those operators mean.
Signed-off-by: cliffhall <cliff@futurescale.com>
