Three findings. The first is the fourth consecutive one in the refresh path,
so it is answered by making that path testable rather than by patching it
again.
**The refresh-after-persist wiring was covered by nothing.** It lived as two
inline `await …; refresh()` pairs in `App.tsx`, which is deliberately outside
the coverage `include` — so the hook's tests and the modals' tests could both
pass while the callback joining them was broken, leaving the security footer
stale after exactly the write that changed it. Three of the last four rounds
found defects in this path; none of them could have been caught by a test,
because there was nowhere for one to live.
Extracted to `utils/refreshingPersist`, which is inside the gate and has its
own tests: refresh runs after the persist resolves (not before — refreshing
first re-reads the descriptor the pending write is about to invalidate),
arguments pass through unchanged (the server-settings caller is
`(id, settings)`, and a transparent wrapper is the difference between saving
the entry and dropping it), and a failed persist refreshes nothing while
propagating the error.
**A tmpfs-style false promise, in the path fallback.** With neither `HOME`
nor `USERPROFILE` set — an ordinary service environment —
`defaultSecretFilePath` returned `.mcp-inspector/secrets.json`, a *relative*
path, while `FileSecretStoreOptions.filePath` and the `SecretStorageInfo.path`
the footer offers to copy both promise an absolute one. Resolved like the two
override branches above it.
**A warning that was usually false.** The session-store notice fired on every
`/api/servers` read whenever the store was in-memory — including for the
default empty catalog, where "plaintext values are left on disk" is not true
of anything, repeated on every list refresh. It is now emitted lazily, at most
once per server instance, and only when the loop actually preserves a
non-empty secrets set. A log line that is usually untrue is one people learn
to skip, which costs it the occasion it matters.
The remaining suppressed comment is the hand-off's check-to-delete race,
tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
Three findings. The first is the third consecutive round in which my previous
fix introduced the next defect, all in the same ~15 lines of `useInitialConfig`.
**The request token did not subsume the unmount guard, and I said it did.**
Bumping the generation on effect teardown invalidates loads already *in
flight*; a `refresh()` called after unmount claims the next token, is
therefore current, and commits into a dead hook. That is a live path — the
callers fire `refresh()` from an async settings persist that can outlive
`App`. Mounted state is tracked separately again and `stale()` checks both.
The test for this was also wrong, in the way that matters: it called
`refresh()` after unmount but never settled the fetch, so it could not observe
the commit it existed to prevent. It settles the promise now.
**A tmpfs mount was reported as durable.** `docker run --tmpfs
/home/node/.mcp-inspector` puts a real entry in mountinfo, so a mount-table
lookup answered "durable", selected the file store, and published
`durable: true` for a file that disappears when the container stops — the
exact false promise the in-memory fallback exists to avoid, reached by
trusting the mount table over the filesystem type. `tmpfs`/`ramfs` are
excluded now, read from the field after the ` - ` separator rather than by
position, since the optional fields before it are variable in number.
**A malformed envelope was diagnosed as a changed passphrase.** A truncated
payload, a wrong-length IV or tag, or an out-of-range KDF parameter all threw
`SecretFileKeyMismatchError`, whose 503 tells the user to restore a passphrase
that cannot repair a structurally broken file. The read path now runs the same
`encryptedEnvelopeProblem` check the descriptor path already ran, and reserves
the key-mismatch error for an envelope that is well-formed and fails
authentication — which is the case where that advice is right. Two existing
tests asserted the old behavior and encoded the bug; both updated, and a new
one pins that a well-formed envelope with the wrong key still gets the
passphrase advice.
The remaining suppressed comment is the hand-off's check-to-delete race,
tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
Round 15, one finding — in the refresh path added one commit ago.
`refresh()` fires after each secret-bearing save, and nothing ordered the
responses. Two debounced settings saves landing close together issue two
refreshes; if the earlier request resolves last, it commits the descriptor
from *before* the write and the footer reverts to "Plaintext file" for a file
that is now encrypted. Reverting a security statement to a stale value is the
worst direction for this particular field to be wrong in, and it needed no
adverse network conditions — just two saves in quick succession.
Loads now claim a monotonic token and may commit only while they still hold
the current one. That subsumes the liveness ref the previous commit added:
bumping the token on effect teardown covers unmount *and* a `baseUrl` /
`authToken` change, and bumping it on every load covers the overlap.
The test drives it by holding the `fetch` promises and resolving them out of
order — newer first, then older — which fails against the previous
implementation and passes here.
The remaining suppressed comment is the hand-off's check-to-delete race,
which is the cross-process guarantee tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
One inline and three of the four suppressed. Two of the four are cases where
an earlier fix of mine was applied to one branch and not its twin.
**The footer went stale after the write that changed it.** `useInitialConfig`
fetched once, and `secretStorage` is the one field it carries that describes
state *this app mutates*: the first save under a newly-set passphrase
re-encrypts a pre-existing plaintext file. The backend re-derives the
descriptor per request — deliberately, and there is a test for that — but the
client had no way to ask again, so the footer said "Plaintext file" for the
rest of the session about a file that no longer was. Exactly the misreport
this band exists to prevent, arrived at from the client side.
The hook now exposes `refresh()`, and both persist paths that can carry a
secret call it: server settings (OAuth client secret, stdio `env:` values)
and client settings (the enterprise IdP client secret). The refresh tracks
its own liveness through a ref, since unlike the mount fetch it has no effect
teardown to cancel against.
**The plaintext branch skipped the payload validation the encrypted branch
does.** Round 12 tightened the encrypted envelope check and left this one
behind, so `{ "encryption": "none", "secrets": [] }` — trivial to hand-write —
described a healthy plaintext store whose next save `readMap` refuses. Both
branches validate now.
**`secretStoreGetStrict`'s doc contradicted the code it was written for.** It
said falling back to `get` is right "for the in-memory and file stores —
neither can fail in a way that masquerades as absence". `FileSecretStore.get`
does exactly that masquerade, which is why round 9 gave that store its own
`getStrict`. The fallback stays (the seam is optional and doubles must
compile) but the comment no longer claims it is equivalent for a store that
can fail.
**The spec documented the performance bug rather than the fix.** It still said
`getMany` decrypts once per server, which was the shape round 13 replaced —
and describing the earlier bug as the design is worse than saying nothing.
The remaining suppressed comment is the hand-off's TOCTOU delete: the
cross-process guarantee, decided against for this PR and tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
No new inline comments this round, but two of the four suppressed ones were
real defects rather than restatements — both in code I added, both silently
wrong in the safe-looking direction.
**A symlinked secrets directory read as unmounted.** `isOnMountPoint` walks
to the nearest existing ancestor *lexically* and then string-prefixes it
against mountinfo, so `~/.mcp-inspector -> /data/inspector` with `/data`
mounted matched no mount point. The container then selected the session-only
store on a box that had explicitly arranged for persistence, and nothing said
so — the mount was there, the volume was there, the secrets evaporated on
exit. Resolved through symlinks now, tolerantly: a path that cannot be
resolved falls back to the lexical comparison, which is what this did before
and is correct whenever no symlink is involved. The `statSync` fallback branch
already followed links, so this also makes the two paths agree.
**The bulk read did not fix the case it was written for.** `getMany` batched
*one server's* fields, but both rehydration callers iterate servers — so a
catalog of 20 servers holding one secret each still paid 20 serialized scrypt
derivations. The same ~450ms stall the seam was introduced to remove, reached
one server at a time instead of one field at a time. Measuring the wrong unit
and then optimizing it is worse than not optimizing, because the number in the
commit message says it is fixed.
The seam is cross-server now — `getMany(requests: SecretBulkRequest[])` — and
both callers pass the whole catalog in one call. `FileSecretStore` decrypts
once for the entire rehydration; the keychain fallback still issues
independent parallel `get`s, which is what it wants. A test asserts one
derivation for four servers, next to the existing one asserting one for three
fields, so the property is pinned at both axes.
The other two suppressed comments are the cross-process guarantee — `mutate`'s
residual and the hand-off's TOCTOU delete — which remain decided against for
this PR and tracked in #2082.
Full `npm run ci` green.
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>
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>
Two changes, interleaved in the same files because both are about the
same read-modify-write path.
## The lock is gone (maintainer decision on the round-5/7/8 finding)
Three consecutive review rounds found a real race in the `mkdir` lock, and
the last one is not patchable with what Node exposes: claiming a stale lock
needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`).
Without it, `canSteal` inspects one lock but `rename` acts on whatever is at
that path *later*, so a waiter that lost the first race can move the
winner's fresh lock aside and enter alongside it.
Rather than patch it a fourth time, writers now collide and the loser
notices. `mutate` reads `M0`, applies the mutation to get `M1`, writes it,
then reads back `M2`; if they differ, someone wrote in between, so it
re-applies onto what they left and retries, bounded.
The comparison is over the **whole map**, and that is the load-bearing
detail. Checking only the entry we touched passes in exactly the case that
loses data — ours is present, and it is the *other* writer's that is gone.
With A and B from the same `M0`: A writes `MA`, B writes `MB` and clobbers
A; B verifies and correctly sees `MB`; A verifies, sees `M2 !== MA`,
re-applies onto `MB`, writes again. Both survive, and the writer that was
clobbered is the one that repairs it.
Two things this deliberately does not claim:
- It is **not mutual exclusion**. A process that crashes between its write
and its verifying read cannot notice it was clobbered, and nothing else
will. That window is strictly narrower than the lock's — which lost
updates with every participant alive — and needs no primitive Node lacks.
Stated in the code rather than left to be rediscovered in review.
- Non-convergence **fails loudly**. After five lost rounds `set` throws
rather than returning, because returning would reintroduce the silent
loss with extra steps. `delete` stays silent, per the interface contract.
`serialize` is now keyed on the resolved path **process-wide** rather than
per instance. Two `FileSecretStore`s on one file are ordinary — the resolved
store holds one and `absorbFileSecretsIntoKeyring` builds another — and a
per-instance queue does not order them against each other at all. The new
convergence test caught this on its first run: two instances racing `set`
lost an entry. In-process is now correct by construction; the verify covers
what it cannot see, a second Inspector process.
The hand-off's read → copy → delete was the lock's other user. Its delete is
now conditional: the file is re-read immediately before removal and compared
against what was copied, and it declines to delete when they differ. Leaving
the file costs nothing (keychain-wins makes the next run idempotent) while
deleting on a stale read costs a secret.
## Round 9 (two inline, two suppressed)
- **`KeyringSecretStore.getStrict` let raw binding errors escape.** Both
plaintext migrations keep their source file only when they catch
`SecretStoreUnavailableError`, so a transient keychain failure 500'd
`GET /api/servers` instead of abandoning the migration. Wrapped, as `set`
already does.
- **`FileSecretStore` had no strict read at all**, so `secretStoreGetStrict`
fell back to the tolerant `get` and mapped every read failure to `null` —
leaving the exact inversion the seam was introduced to prevent open on the
store it was introduced for.
- **`readOnDiskEncryption` reported "encrypted" for an envelope it cannot
open.** Naming the cipher is not the same as being openable: a file with no
`kdf`/`data`, or a `data` that is not `iv.tag.ciphertext`, produced the
quiet reassuring footer for a file whose next save was guaranteed to fail.
- **`/api/config` kept a stale descriptor when the resolver returned
`undefined`**, because the conditional spread left the startup value
standing. Assigned unconditionally now; `c.json` omits the key.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
The lock finding from this round is deliberately not addressed here — see
below.
- **My round-6 strict-read fix was inert in production.** `DeferredSecretStore`
is what `defaultSecretStore()` returns, and it did not forward `getStrict`
— so `secretStoreGetStrict` fell through to the tolerant `get` for every
real caller, and the strictness existed only in tests that injected a
concrete store. Forwarded now, along with `getMany`.
- **The same tolerant-read bug remained in both pre-existing migrations.**
I added `getStrict` for the new hand-off in round 6 and never applied it to
the mcp.json and client.json migrations, which have the identical
shape: decide from `get`, then write on `null`. A transient keychain read
failure therefore let an older plaintext value overwrite a newer keychain
one — and the disk copy was then stripped, so the newer value was gone. Both
use the strict read now; the throw is a `SecretStoreUnavailableError`, which
both call sites already handle as "abandon the migration, keep the file".
- **The permission check made a false statement on Windows.** `stat` returns
synthetic bits there, and interpreting them yields a confident falsehood
either way — "0666 — not owner-only" about a file the ACL may restrict, or
"owner-only" about one it does not. The ACL is the real answer and this does
not inspect it, so Windows now reports `unknown`.
- **Rehydration cost one full decryption per field.** `get` reads and decrypts
the entire file, and both rehydration paths ask field by field, serialized
behind the store's own queue. Measured on this machine, scrypt at N=16384 is
~23ms per derivation, so a 20-secret encrypted catalog spent **~450ms of
pure key derivation on every `GET /api/servers`** — a visible stall, not a
micro-optimization. Added an optional `getMany` seam: `FileSecretStore`
decrypts once per server, everything else falls back to the parallel
per-field `get` that already suited it. A test counts derivations directly:
1 for a three-field `getMany`, 3 for the three `get`s it replaced.
Also broadens a log message that still said "Keychain unavailable" for a
store that may not be a keychain.
Full `npm run ci` green — 5,762 tests under the coverage gate.
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>
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>
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>
Found on a re-read of the round-4 liveness work, not by review.
The heartbeat refreshed the lock directory's mtime unconditionally. After
a steal — possible when this process looked dead, or across hosts where
the pid check cannot apply — the directory belongs to someone else, and
touching it keeps *their* lock looking alive. If they then die, the stale
timestamp nobody is maintaining is being maintained by us, and a third
writer can never break in: a stale lock turned unbreakable, which is the
permanent outage the stealing exists to avoid.
The heartbeat now re-reads the owner stamp and clears itself when the
token no longer matches, so it only ever refreshes a lock we still hold.
The release path already checked the token; this closes the same hole on
the other side.
Tested by stamping a different owner onto a held lock and aging it: the
mtime stays old across several heartbeat intervals, and release leaves the
thief's lock intact.
Signed-off-by: cliffhall <cliff@futurescale.com>
Four findings (two inline, two suppressed), all taken. Three are the lock,
which is where the remaining sharp edges were.
- **A lock could be created and then never stamped.** If `mkdir` succeeded
but writing the owner token failed, the catch treated it as "cannot lock
here", returned the no-op release, and left the directory behind. The
mutation then ran unlocked *and* every later writer queued behind an
orphan nobody could deliberately release, until it aged out. The
acquisition now tracks whether it created the directory, removes it on a
failed stamp, and refuses rather than entering the section unlocked.
- **An old mtime was being treated as proof the owner was dead.** It is
not: a holder that is SIGSTOPed, starved by a blocked event loop, or on a
machine that suspended is alive and not running, and expiring it on
elapsed time alone takes the lock out from under a live critical section
— the lost update the lock exists to prevent, arrived at through the
machinery meant to prevent it. The heartbeat narrowed the window; it
could not close it, because the holder that most needs protecting is
precisely the one not running its timers.
The owner stamp now carries `pid` and `host`, and staleness only *opens*
the question — on the same host a waiter asks the OS whether that pid
still exists (`kill(pid, 0)`, which sends no signal; `EPERM` counts as
alive, so a holder running as another user is never stolen from). Only a
genuinely absent process is stolen from. Where the stamp cannot be
trusted — another machine over a shared filesystem, or a missing/corrupt
owner file — the mtime remains the answer, because the alternative is a
lock nothing can ever break, turning one dead process into a permanent
outage.
- **Podman was classified as a host.** `/.dockerenv` is Docker's marker and
rootless Podman has no equivalent of it, while its cgroup paths say
`libpod-<id>.scope` rather than the literal "podman" the pattern looked
for. Such a box fell back to a file in the ephemeral writable layer and
reported it as durable — the wrong way round to be wrong.
`/run/.containerenv` and `libpod` are both recognized now.
- **Two tests floated a second `acquireLock`**, so the test could finish and
`afterEach` could remove the temp directory while the acquisition was
still running. Both hold and settle it now.
Separately, a defect of my own found while re-reading round 3:
`describeFileStore` called `tightenSecretFilePermissions`, so the function
whose job is to *describe* the store issued a `chmod` on every
`GET /api/config` — once per page load, on a file another process may be
mid-write on. Split out a read-only `readSecretFilePermissions`; the repair
stays at startup, where it happens once and its outcome is announced.
Tests: a stale lock whose owner is alive is not stolen (and survives the
attempt), one whose pid is gone is, one stamped by another host is; a
failed ownership stamp reports and leaves nothing behind; Podman via both
signals; and the read-only permission check leaves a 0644 file at 0644.
Full `npm run ci` green — 5,711 tests under the coverage gate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Nine findings (six inline, three suppressed), all taken. Two were serious.
**The memory fallback made the migrations destructive.** Both migrations —
mcp.json in the remote server, client.json in `node-persistence` — lift a
plaintext secret off disk, write it to the secret store, and then delete it
from the file. That trade is safe only while the store is at least as
durable as the disk, which held by construction while `InMemorySecretStore`
was a test double. Making it a production fallback broke it: on an
unmounted container a plain `GET /api/servers` would have moved a user's
existing secrets into RAM and lost them at exit — destroying data on a
read.
`SecretStore` gains an optional `isDurable()`, absent meaning durable, and
both migrations withhold the disk delete when it answers false (the value
is still loaded, so the session behaves normally). The production fallback
is now a named `SessionSecretStore` rather than the bare
`InMemorySecretStore`, so the suite's many doubles keep standing in for a
*working keychain* — which is what they are there to be — and only code
that deliberately chooses RAM in production says so.
**Round 2's per-call descriptor never reached anyone.** Both web servers
resolved it once at startup and baked it into `initialConfig`, which
`/api/config` serves unchanged — so the freshness fix was inert for its
only consumer. The route now takes a `secretStorageResolver` and calls it
per request.
The lock needed three more fixes, and they compound:
- **Release removed whatever directory was at the path.** A holder whose
work outran the stale threshold would delete the *next* owner's lock,
letting a third writer into a section two processes believed they held.
Ownership is now stamped with a random token and release only removes a
lock still carrying it.
- **The timeout fell through to an unlocked write** — under contention,
the one moment another writer is provably active. It now throws
`SecretFileLockTimeoutError`, which `set` is allowed to do.
- **The first write of every process bypassed the lock**, because the
secrets directory does not exist yet and `mkdir` failed ENOENT. The
parent is created first. First writes are exactly when two processes are
most likely to start together.
Fixing the timeout exposed a fourth: with stale (10s) below timeout, a
waiter stole from a holder that was merely *slow* rather than dead —
reinstating the lost update. A holder now heartbeats its lock, so
"stale" means the holder is gone, and the timeout is what a live-but-slow
holder produces. Lock timings are injectable so the tests can exercise
contention, staleness, and timeout without spending the real intervals.
Also: the loose-permission state is carried into `SecretStorageInfo` as
`looseMode` and rendered as the caveat (it outranks the encryption caveat —
a file others can read is a live exposure, while "unencrypted" is a
property of a file only its owner can open), rather than living in a
startup log the browser cannot see; and `usableSecretStorage` validates
`pendingEncryption` and `looseMode` as the optional booleans/numbers they
are (`pendingEncryption: "false"` is truthy and would have printed advice
that is the opposite of the truth) and rejects file-only fields on a
keychain or memory descriptor.
Tests: both migrations against a session store with a durable control
alongside, so the guard is visibly the difference; `/api/config` serving a
changing resolver and falling back without one; lock ownership after a
steal, the timeout under a live holder, no-steal from a slow holder, and a
locked first write; `looseMode` through the descriptor, the caveat
ordering, and the tone; and five more rejected payload shapes. Full
`npm run ci` green — 5,704 tests under the coverage gate.
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>
#1954 wired the list-load error off the aggregate managed stores only.
With `paginatedLists` on, those stores deliberately skip their all-page
walk, so their error is permanently null — while the paged stores that
actually drive the sidebar had no error state at all and `loadPage` had
a try/finally with no catch. A failing list showed an empty panel with
no alert and no Retry, and the connect-time `void loadPage(undefined)`
left an unhandled rejection.
- Give PagedTools/Prompts/ResourcesState observable error state: an
`errorChange` event plus `getError()`, set in a new catch in
`loadPage`, cleared on the next success and on disconnect. The
rejection is still re-thrown, matching ManagedListState — callers'
auth-recovery wrappers key off it to detect a 401.
- Catch the connect-time load's rejection (`.catch(() => {})`) now that
the failure is recorded as state rather than lost.
- Select the error by mode in `usePaginatedList`, the same way `items`
already is, and read it in App.tsx from the pagination model.
- Rename `useManagedListError` to `useListError`: both store families
expose an identical `errorChange`, so the hook declares that one-event
contract itself instead of importing ManagedListEventMap.
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. The helper's contract is exactly-once teardown
and the test asserted it must not lean on Hono's own re-entry guard, but
neither the implementation nor the test actually enforced that: the
listener had no local guard and the test aborted only once.
Signed-off-by: cliffhall <cliff@futurescale.com>
Both SSE handlers installed their subscriber, primed the stream so
Firefox's fetch() resolves (#1858), and only then registered onAbort.
Hono's onAbort has no already-aborted replay and abort() fires only the
subscribers registered at that moment, so a client disconnecting while
the priming write was in flight fired nothing: the session event
consumer / server-list subscriber stayed installed, the mcp.json watcher
was never stopped, and the handler parked forever on a promise nothing
would resolve.
Add primeAndHoldSseStream, which registers one listener before the first
await — it both cleans up and releases the hold, collapsing the previous
duplicate onAbort registrations. Priming still happens after the
subscriber is installed, since callers treat the stream's first bytes as
proof they are subscribed.
Closes#1999
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-19 review on #2035 -- a divergence introduced by
my own round-14 guard rather than by the original code.
The SDK validated a value inside `encodeValue`, i.e. only after looking one
up by a DECLARED name. The restored guard checked the caller's whole map, so
an unrelated entry failed a template that never mentions it. Measured on
9caac43d:
tryExpandUriTemplate("x://{id}", { id: "7", stale: "a".repeat(1_000_001) })
=> error: The value for "stale" exceeds the 1000000-character limit
RFC 6570 ignores an extra variable, so this refused a perfectly valid read.
`valueLengthError` now takes the names to consider and both callers pass the
template's declared set.
Copilot's second half is worth taking on its own: strict expansion parsed
the template THREE times -- inside `templateError`, again for the name list,
again to expand -- and the template is server-controlled and may be 1 MB.
It is now parsed once and reused, with validation split into
`sdkTemplateError` (the constructor's unclosed-brace and length/count
limits) and `partsError` (this module's own grammar), so `templateError` and
strict expansion share both halves rather than duplicating either.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-18 review on #2035. Two suppressed comments, both
holes in my own recent fixes rather than in the original code.
1. Round 16's grouping keyed off the length-announcing lead byte alone, so
it accepted sequences RFC 3629 forbids -- and the comment claiming they
fell back to per-triplet counting was simply wrong. Measured on 05bbfe50:
x{+v:1} "%C0%80x" => "x%C0%80" overlong
"%E0%80%80x" => "x%E0%80%80" overlong
"%ED%A0%80x" => "x%ED%A0%80" UTF-16 surrogate
"%F4%90%80%80x" => "x%F4%90%80%80" past U+10FFFF
The lead-byte ranges are now the well-formed ones -- C2-DF, E0 only with
A0-BF, ED only with 80-9F, F0 only with 90-BF, F4 only with 80-8F -- so
each of the above falls back to one triplet, which is what the fallback
was always documented to do. The legal boundary cases (%E0%A0%80,
%ED%9F%BF, %F4%8F%BF%BF) are tested alongside, so tightening the ranges
cannot quietly start splitting valid characters.
2. The preview never ran the read's template validation. It expands
expression by expression, so `x://{a}}/{b}` with both fields filled
previewed `x://1}/2` -- a URI whose stray `}` makes every read refuse.
Validation moved into an exported `templateError` that both callers
consult, so they cannot drift. It subsumes the narrower rule round 12
added here ("leave the invalid part standing"), which still let the REST
of such a template expand: the refusal is per template, not per
expression, so the preview now shows the template verbatim -- literals
unencoded included, since half-normalizing an unreadable template invents
a string the server never published. Two existing tests asserted the
weaker behavior and now assert this one.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-16 review on #2035. Two suppressed comments, both
real, and both refinements of fixes from the previous two rounds rather than
defects in the original code.
1. Round 11 settled on "a pct-encoded triplet counts as one character", but
a multi-octet character spans several triplets -- `%C3%A9` is one `é`.
Measured on a3dd8563:
x{+v:1} v = "%C3%A9x" => "x%C3" a lone lead byte, decoding to nothing
`prefixUnits` now recognizes a well-formed pct-encoded UTF-8 sequence from
its lead byte (RFC 3629: C2-DF two octets, E0-EF three, F0-F4 four;
continuations 80-BF), longest alternative first so a four-octet sequence
is not read as a two-octet one followed by loose triplets.
A malformed sequence -- a lead byte with too few continuations, or a stray
continuation -- matches no sequence alternative and falls back to
per-triplet counting. That is the conservative direction: it counts as
MORE units, so truncation keeps less rather than emitting octets that
were never in the value.
2. Round 14's per-value ceiling only covered the read. The panel calls
`previewUriTemplate` on every keystroke, and that path expands expression
by expression, so it reached the allocation-heavy encoders with the same
oversized value -- submission refused while the UI froze on the paste
anyway. Both paths now share one exported `valueLengthError`, checked
before any encoding happens.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-15 review on #2035.
1. `readResourceFromTemplate` had no regression distinguishing this module
from the SDK expander it replaced: the existing test uses `report.txt`
and the transport tests expand separately and call `readResource`, so all
of them pass either way.
Writing that test corrected something I had asserted twice in this PR.
The first version used `foo/bar` -- and verified by reverting the wiring,
it still passed. `encodeURIComponent` DOES encode `/`, so the SDK expands
`foobar://events/{topic}` to `foobar://events/foo%2Fbar` exactly as this
module does. #1919 was the WEB PANEL's own `String.replace`, not the SDK
expander, which the TUI path was using correctly all along.
The test now uses `a!b`. `!` is a sub-delim: RFC 6570 requires it
encoded, `encodeURIComponent` leaves it bare, so the SDK returns
`foobar://events/a!b` and the assertion fails. Re-verified by reverting
the wiring -- 2 of the 10 tests fail (this one and the malformed-template
refusal), where before the change none did.
2. The module summary said "shared by every client" a dozen lines above the
paragraph explaining the CLI is not a consumer. Now says web and TUI.
Also records, on MAX_VALUE_LENGTH, the audit prompted by round 14: the
per-value ceiling was the ONLY guard lost with `UriTemplate.expand`. The
template-length (1e6), expression-count (1e4) and variable-name-length (1e6)
limits all live in the SDK's `parse()`, which runs from the constructor --
and strict expansion still constructs a `UriTemplate`, so they remain
enforced. The restored limit is the SDK's `MAX_VARIABLE_LENGTH`, read from
the pinned dist rather than assumed.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-14 review on #2035. Three suppressed comments,
all real; each measured on 1c595e96 before acting.
1. A brace surviving in a literal is an unmatched one, and not a legal
literal character. The SDK's constructor rejects an unclosed `{` but
reads a stray `}` as text, and this parser did the same:
expandUriTemplateStrict("x://a}", {}) => "x://a}"
-- a "URI" carrying a brace, with the panel enabling the read on it.
Both halves are now refused the same way, so the withhold behavior
covers the whole class rather than the half the SDK happened to catch.
2. Replacing `UriTemplate.expand` also removed its 1,000,000-character
per-value ceiling. Nothing else bounded what reached the
allocation-heavy encoders, so a pasted or programmatically supplied
megabyte-plus value went straight through. Restored at the SDK's own
limit -- the guard it was, not a new policy -- and the error names the
offending variable.
3. `templateVariables` re-scanned an expression's whole varspec list for
every one of its names to compute `conforming`, which is quadratic in a
multi-name expression. Templates are server-controlled and the SDK
admits one up to 1 MB, so a single expression can carry thousands of
short comma-separated names, and this runs on the render thread.
Iterating the varspecs supplies the name and its conformance in one
pass.
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>
Addresses Copilot's round-11 review on #2035, which corrects my own round-9
fix rather than the original code.
That fix grouped pct-triplets only under `+` and `#`, on the reasoning that
every other operator escapes a `%` anyway, so `%61` is three characters
rather than one. Wrong, in a way the narrower rule made visible one operator
over -- measured on 2e06b369:
x/{v:1} v = "%61%62" => "x/%25" truncated to a `%` that was never
a character of the value
RFC 6570 2.4.1 counts characters and says a pct-encoded triplet counts as a
single one, without reference to the operator. Grouping per-operator also
made the same input mean two different things: `%61` indivisible under `+`,
three loose characters under `{v}`. What the operator decides is how a
retained unit is *encoded* afterwards -- a simple expansion still escapes
the kept triplet's `%`, so `%61` goes out as `%2561` -- not what counts as
one character.
`prefixUnits` no longer takes an operator, and the test that codified the
old split now asserts the uniform rule across simple, named, and reserved
expansion.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-10 review on #2035. One inline comment, one
suppressed; both real.
1. Literals were copied through verbatim, but RFC 6570 3.1 requires them
pct-encoded on expansion -- the conformance case `café/{var}` expands to
`caf%C3%A9/value`. Measured on ef3f5e74 it produced `café/value`, raw
UTF-8 in the path; since this module replaced the SDK's expander for both
clients, nothing else was going to encode it. (The pinned SDK does not
either -- it returns `café/value` too -- so this is a gap we inherited
rather than introduced.)
`encodeLiteral` keeps reserved delimiters (a literal's `/`, `?` and `#`
are structure) and existing pct-triplets, and encodes the rest. A BRACE
passes through: it can only reach a literal from a malformed template,
where an unclosed `{` becomes trailing text, and `%7B` would leave a
string resembling neither the template nor anything the server can match.
This is also what removed the preview's placeholder machinery. No token
can survive a round trip now that literals are encoded, so the preview
assembles part by part instead -- `encodeLiteral` for literal runs,
`expandTemplateExpression` for filled expressions, `part.source` for the
rest. Both halves still come from the shared expander, so the preview
cannot promise a URI that submitting would not send, and the question of
what token could never collide with a server-supplied template is gone.
2. On the varname set I took the structural point rather than the
conclusion. `VARCHAR` is RFC-exact again; `-` and `~` moved into a
separately named `TOLERATED_VARCHAR` tier, and a name needing it parses
with `conforming: false`, surfaced on `TemplateVariable`.
So the grammar in the code says what the RFC says and the tolerance is
explicit and separately tested -- but the Inspector still expands
`{default-graph-uri}`, because real servers publish hyphenated names and
the SDK's matcher round-trips them. Refusing to read a resource that
demonstrably works is a worse failure for a debugging tool than expanding
a name the conformance suite would reject. A caller wanting RFC-exact
behavior refuses on the flag rather than re-deriving the grammar.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-9 review on #2035.
RFC 6570 2.4.1 counts the prefix in *characters* and is explicit that a
pct-encoded triplet counts as a single one. Truncation went by code point,
so it cut a triplet in half -- measured on 6acaff70:
x{+v:5} v = "%61%62%63%64%65%66" => "x%61%256"
The orphaned "%6" is not a triplet, so encodeAllowReserved then (correctly)
escaped its `%`, and the value went out carrying a byte the caller never
asked for. Now "x%61%62%63%64%65".
Triplets are units only under `+` and `#`. Under every other operator the
encoder escapes a `%` anyway (`%61` -> `%2561`), so the three characters
really are three and grouping them would mis-count the prefix -- asserted
both ways. Astral code points remain single units under both, which is what
`Array.from` was there for.
Also corrects the comment on `readResourceFromTemplate`, which still said
the shared helper drops blank values. It has not since 6acaff70: `params`
passes through as given, so only an absent key omits its expression and a
direct API caller can request `?q=` deliberately. A caller whose values come
from a form drops its own blanks first.
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>
Addresses Copilot's round-7 review on #2035. Two suppressed comments, both
real; each reproduced against this branch before acting.
1. `parseVarSpec` was a "strip the first `*`, trim, split on `:`" pass, so
it accepted shapes RFC 6570's varspec production does not admit --
measured on the previous commit:
x://{*id} name "id" => "x://abcdef" explode is a TRAILING modifier
x://{id*:3} name "id" => "x://abc" the two modifiers are exclusive
x://{ id } name "id" => "x://abcdef" whitespace is not a varchar
x://{a b} name "a b" => "x://1" ditto -- and it emits into the URI
`{id*:3}` is the worst of them: it silently truncated the value through a
combination the grammar forbids. Parsing is now one anchored regex --
a varname, then at most one trailing `*` or `:1-9999`.
The name set is `varchar` widened by `-` and `~`, deliberately. A name is
the one thing expansion emits into the URI WITHOUT encoding it (`;`, `?`
and `&` write `name=value`), so the rule it has to satisfy is "needs no
encoding" -- RFC 3986 unreserved plus pct-encoded, of which the RFC's own
varchar is a subset. Strict varchar would refuse `{user-id}`, which real
servers publish and which the SDK's matcher round-trips; refusing to read
a resource that demonstrably works is a worse outcome for a debugging
tool than a name one character outside the production. Everything now
rejected is a shape we would otherwise have to guess at.
2. The TUI built its "Missing required template variable(s): ..." list with
its own bare `values[name]` filter while gating on `hasRequiredValues`,
which reads own properties only. For a variable legitimately named
`constructor` the two disagreed: `({})["constructor"]` is `Object`, whose
`.length` is 1, so the list came out empty and the error named no field
at all while the submit stayed blocked. Both now come from one exported
`unmetRequiredGroups`, with `hasRequiredValues` defined in terms of it.
Also renames a test whose title still said multi-name query expressions were
left "to the SDK, which handles them" -- expansion has been owned here since
935113d1; the behavior it asserts is naming, not delegation.
npm run ci passes.
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>
Two suppressed review findings.
With an override configured, the wrapper cloned and parsed every
successful application/json response — every tools/call result and every
resource payload on the direct CLI/TUI transports — only to conclude it
was not a metadata document. It now tests the request URL for the RFC
8414 / OIDC well-known paths first, so ordinary traffic costs one string
test and is returned untouched.
The read path also trusted StoredMCPServer's `string` type for the two
override fields, which a hand-edited mcp.json does not honor: the CLI and
TUI read the file directly and then `.trim()` these, so a number would
crash the server load. Guarded with cleanEndpointOverride, mirroring
cleanAuthorizationParams.
Signed-off-by: cliffhall <cliff@futurescale.com>
Addresses Copilot's round-6 review on #2035.
`toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC
6570 variable names -- `varname` allows ALPHA / DIGIT / `_` / pct-encoded
-- and every lookup here was a bare `values[name]`, which finds
`Object.prototype`'s member for each of them. Measured: `({})["toString"]
!== undefined` is true with typeof "function", and `({}["constructor"] ??
"").length` is 1.
So a *blank* `{?toString}` field read as supplied and expanded a function
body into the URI instead of omitting the expression, and
`hasRequiredValues([["constructor"]], {})` returned true because
`Object.length` is 1 -- a required group satisfied by nothing.
All reads now go through an own-property `readValue`, and the web
preview's filled-check uses `Object.hasOwn`. `expandExpression` carries
the value through its filter rather than re-reading it afterwards, so the
"is it defined" test and the read cannot disagree and nothing needs a
non-null assertion.
The remaining six findings were documentation drifting behind the code,
which matters here because the comments are the argument for why this
module exists:
- The web helper still described the SDK's second-`?`-to-`&` rewrite as
something the preview relies on. That was removed last round.
- Three places still named the CLI as a consumer. It has no template form
and passes an already-expanded `--uri` to `readResource`.
- The PR description still described the superseded architecture
(expansion delegated to the SDK), i.e. the opposite of what now ships.
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot round 2 (PR #2039) surfaced no new visible comments but six
suppressed ones. Four were actionable.
`clients/tui/src/App.tsx` — a stale-completion race in the new banner:
start disconnecting server A, switch to B, then let A's disconnect
reject, and A's failure appears under B. Rapid same-server retries had
the same shape, and the stale attempt is the one most likely to reject.
Claim a monotonic attempt token before awaiting and publish only if it
is still current and the server has not changed; `handleConnect` bumps
the same counter so a connect retires an in-flight disconnect. The
rejection is still owned when superseded — just not published. The
regression test drives a deferred rejection across a server switch, and
fails without the guard.
`tsconfig.lint.json` — the root type-aware block matches
`test-servers/src/**/*.tsx` but the parser project omitted that
extension, so such a file would have failed with "not found in any of
the provided project(s)" instead of being linted. Added, and AGENTS.md
now states the invariant: the include must stay a superset of the lint
globs.
`clients/web/server/{server,vite-hono-plugin}.ts` — the `open()`
rejection branch was untested, and duplicated. Both were fixed by
removing the duplication: `openBrowser` now lives beside
`printServerBanner` in `web-server-config.ts`, with one implementation
to test. Two copies of a one-line `.catch` is two chances for one to
drift back to a bare `open(url)`. The dev plugin is not booted in tests
— it returns early under VITEST by design — but it calls the same
tested helper.
`core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close
path, the entire reason the helper exists, had no test. Exported it (as
this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`)
and covered both legs, asserting no unhandledRejection fires.
Also corrects AGENTS.md, which said each client lists "both" tsconfig
projects — true for cli/tui/launcher, but web lists four leaf projects.
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>
Addresses Copilot's round-5 review on #2035 — eight suppressed comments,
all of them real.
The consequential one overturns behavior I copied from the SDK and
defended in two earlier rounds. RFC 6570 expands each expression
independently; it carries no cross-expression state. The SDK tracks
whether a query expression has already emitted and rewrites a later
`{?two}`'s `?` to `&`. That looks friendlier and is wrong, and its own
matcher says so: for `x{?one}{?two}` it expands to `x?one=1&two=2`, and
`match("x?one=1&two=2")` on that same template returns **null**, while
`match("x?one=1?two=2")` returns both variables. So the rewrite emitted a
URI the advertised template cannot match — #1919 one level up. A server
that wants a continuation advertises `{?one}{&two}`, which expands to `&`
and matches; producing that shape is the server's choice to declare, not
ours to infer.
Also:
- `{id:abc}` was silently treated as `{id}`, and a test codified it. RFC
6570's max-length is `%x31-39 0*3DIGIT`, so `{id:}`, `{id:0}`,
`{id:abc}` and `{id:10000}` are invalid templates rather than templates
with an ignorable modifier — and the SDK's constructor accepts them all,
so nothing else rejects them. Strict expansion now throws; lenient
returns the raw template. Discovery stays lenient so the panel still
renders rather than going blank.
- The TUI could submit `{a,b}` completely blank while the web panel
blocked the same request: ink-form has no way to express "any one of
these", so its members are left optional and the modal needs its own
group check. Added, with tests — it was also below the coverage gate
without them (functions 76.92%).
- Added the integration test the config never had: it resolves the
checked-in config and drives all four reads over a real transport, so a
misspelt preset fails there rather than only when someone runs the repro
by hand.
- "All three clients" was false. The CLI has no template form and its
`resources/read` passes the already-expanded `--uri` straight to
`readResource`, so nothing here runs for it. Corrected in the README,
AGENTS.md, and the module comment, and dropped a docblock still
describing the SDK-delegation path removed last round.
Signed-off-by: cliffhall <cliff@futurescale.com>
Two review findings on the fetch wrapper:
Reading the body with `response.text()` meant every successful JSON
response was rebuilt while overrides were configured, even when it was
not a metadata document. That dropped native properties a synthesized
`Response` cannot carry (`url`, `redirected`, `type`) from responses the
wrapper has no business altering, and threw outright on a body-less
204/205, whose status Fetch forbids a body on — so an unrelated transport
response could fail. It now parses a clone and returns the caller's own
response unless a document is actually patched.
`new URL` also accepts embedded credentials while Fetch rejects a request
URL carrying them, so `https://user:pass@host/token` passed both the form
and the runtime and then failed deep in the OAuth exchange.
`oauthEndpointUrlError` now rejects a username or password.
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>
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>
The `/api/servers/events` reader in `useServers` searched for `\n\n` and
consumed exactly two characters as the separator. SSE permits CRLF line
endings, so a `event: change\r\n\r\n` frame stayed buffered forever:
`isSseDataFrame()` was never reached and no background refresh fired,
leaving the server list stale until a manual refresh or an LF-delimited
event arrived.
Frame boundaries are now located with a CRLF-aware matcher that reports
the separator's real length, and frame lines split on either terminator.
A bare CR is deliberately not accepted as a terminator: it cannot be
distinguished from the first half of a CRLF straddling a chunk boundary,
so a buffer ending in `…\r\n\r` would read as a completed frame and the
`\n` arriving next would open a bogus one.
Two regressions cover it, both verified failing against the old parser.
Each uses a referentially stable `fetchFn` and holds the frame behind a
promise released after the disk write — with an inline `fetchFn` the hook
re-subscribes every render and that re-subscription's own mount refresh
picks the edit up regardless, so the test would pass unfixed.
Closes#2006
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>
cliffhall