inspector/clients/web/server/server.ts
cliffhall b143dd6cbd fix(auth): address Copilot review round 3 on the secret-store PR
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>
2026-08-22 13:19:56 -04:00

170 lines
6 KiB
TypeScript

/**
* Hono production server. Export startHonoServer(config) for in-process use by
* the runner. The server is started programmatically via `runWeb` (in
* `run-web.ts`), which is the bundled `build/index.js` entry the `mcp-inspector`
* bin invokes — there is no separate standalone server entry.
*/
import { readFileSync } from "node:fs";
import { join, dirname } from "node:path";
import { fileURLToPath } from "node:url";
import { randomBytes } from "node:crypto";
import { serve } from "@hono/node-server";
import { serveStatic } from "@hono/node-server/serve-static";
import { Hono } from "hono";
import type { Context } from "hono";
import { createRemoteApp } from "../../../core/mcp/remote/node/server.ts";
import { formatHostForUrl } from "../../../core/node/hostUrl.ts";
import { createSandboxController } from "./sandbox-controller.js";
import { injectAuthToken } from "./inject-auth-token.js";
import type { WebServerConfig } from "./web-server-config.js";
import { getSecretStorageInfo } from "../../../core/auth/node/secret-store-selection.ts";
import {
webServerConfigToInitialPayload,
printServerBanner,
openBrowser,
} from "./web-server-config.js";
import type { WebServerHandle } from "./types.js";
export type { WebServerHandle };
const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);
/**
* Start the Hono production server in-process. Returns a handle that closes sandbox then HTTP server.
* Caller owns SIGINT/SIGTERM; do not register signal handlers here.
*/
export async function startHonoServer(
config: WebServerConfig,
): Promise<WebServerHandle> {
const sandboxController = createSandboxController({
port: config.sandboxPort,
host: config.sandboxHost,
allowedOrigins: config.allowedOrigins,
});
await sandboxController.start();
const resolvedAuthToken =
config.authToken ||
(config.dangerouslyOmitAuth ? "" : randomBytes(32).toString("hex"));
const rootPath = config.staticRoot ?? __dirname;
// Resolve the secret store before the API is built so the descriptor the
// browser reads (`/api/config`) and the one the banner prints come from the
// same resolution — the selection is cached, so this is also the only place
// the keychain probe actually runs.
const secretStorage = await getSecretStorageInfo();
const { app: apiApp, close: closeApi } = createRemoteApp({
authToken: config.dangerouslyOmitAuth ? undefined : resolvedAuthToken,
dangerouslyOmitAuth: config.dangerouslyOmitAuth,
storageDir: config.storageDir,
mcpConfigPath: config.mcpConfigPath,
writable: config.writable,
initialServers: config.initialServers ?? undefined,
allowedOrigins: config.allowedOrigins,
sandboxUrl: sandboxController.getUrl() ?? undefined,
logger: config.logger,
initialConfig: webServerConfigToInitialPayload(config, secretStorage),
// The startup value above is what the banner printed; the route
// re-resolves per request, because the first write under a newly-set
// passphrase encrypts a pre-existing plaintext file and a captured
// descriptor would keep reporting the old state until a restart.
secretStorageResolver: getSecretStorageInfo,
});
const app = new Hono();
app.use("/api/*", async (c) => {
return apiApp.fetch(c.req.raw);
});
// Serve index.html with the API token injected so a reload at any bare URL
// (no `?MCP_INSPECTOR_API_TOKEN=…`) still authenticates against /api/*.
// No-op when auth is dangerously omitted (empty token). The dev Vite plugin
// applies the same injection via `transformIndexHtml`. `Cache-Control:
// no-store` keeps a browser/proxy from serving a page that carries a stale
// token after a server restart regenerates it (randomBytes per start).
const serveIndexHtml = (c: Context) => {
const indexPath = join(rootPath, "index.html");
const html = readFileSync(indexPath, "utf-8");
c.header("Cache-Control", "no-store");
return c.html(injectAuthToken(html, resolvedAuthToken));
};
app.get("/", async (c) => {
try {
return serveIndexHtml(c);
} catch (error) {
console.error("Error serving index.html:", error);
return c.notFound();
}
});
// Real static assets (paths with a file extension). Missing files fall
// through to the SPA fallback below via `next()`.
app.use("/*", serveStatic({ root: rootPath }));
// SPA deep-link fallback: any non-/api route that didn't resolve to a static
// asset (e.g. `/oauth/callback`, the OAuth landing URL) serves the *injected*
// index.html — not the raw file — so bookmarks and hand-typed reloads at
// those paths get the token global too, matching the `/` route.
app.get("*", async (c) => {
if (c.req.path.startsWith("/api")) {
return c.notFound();
}
try {
return serveIndexHtml(c);
} catch (error) {
console.error("Error serving index.html:", error);
return c.notFound();
}
});
const httpServer = serve(
{
fetch: app.fetch,
port: config.port,
hostname: config.hostname,
},
(info) => {
const sandboxUrl = sandboxController.getUrl();
const url = printServerBanner(
config,
info.port,
resolvedAuthToken,
sandboxUrl ?? undefined,
secretStorage,
);
if (config.autoOpen) {
// Never a bare `open(url)`: this callback is synchronous, so a
// rejection would escape it. See openBrowser.
openBrowser(url);
}
},
);
httpServer.on("error", (err: Error) => {
if (err.message.includes("EADDRINUSE")) {
console.error(
`MCP Inspector PORT IS IN USE at http://${formatHostForUrl(config.hostname)}:${config.port}`,
);
process.exit(1);
} else {
throw err;
}
});
return {
async close(): Promise<void> {
await closeApi();
await sandboxController.close();
if ("closeAllConnections" in httpServer) {
httpServer.closeAllConnections();
}
await new Promise<void>((resolve, reject) => {
httpServer.close((err) => (err ? reject(err) : resolve()));
});
},
};
}