fix(auth): address Copilot review on the secret-store PR
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>
This commit is contained in:
parent
bc408f5869
commit
3452a63101
1 changed files with 411 additions and 75 deletions
|
|
@ -61,9 +61,19 @@ import {
|
|||
/** Env var naming an explicit store, bypassing the probe entirely. */
|
||||
export const SECRET_STORE_ENV = "MCP_INSPECTOR_SECRET_STORE";
|
||||
|
||||
/** Env var overriding the secrets file location. */
|
||||
/** Env var overriding the secrets file location outright. */
|
||||
export const SECRET_FILE_ENV = "MCP_INSPECTOR_SECRET_FILE";
|
||||
|
||||
/**
|
||||
* Env var relocating the Inspector's durable state directory. Already
|
||||
* honored by the web backend for OAuth tokens and `client.json`; the
|
||||
* secrets file follows it for the same reason, and because the mount check
|
||||
* below asks whether *that* directory survives. A container mounting only
|
||||
* the configured storage directory would otherwise be judged unmounted,
|
||||
* fall back to `memory`, and lose secrets the user had arranged to keep.
|
||||
*/
|
||||
export const STORAGE_DIR_ENV = "MCP_STORAGE_DIR";
|
||||
|
||||
const KINDS: SecretStoreKind[] = ["keyring", "file", "memory"];
|
||||
|
||||
export interface ResolvedSecretStore {
|
||||
|
|
@ -89,10 +99,16 @@ export function parseSecretStoreEnv(
|
|||
return undefined;
|
||||
}
|
||||
|
||||
/** Default secrets file: beside the catalog, in `~/.mcp-inspector`. */
|
||||
/**
|
||||
* Default secrets file: `MCP_INSPECTOR_SECRET_FILE` if named outright,
|
||||
* otherwise `secrets.json` in the configured storage directory, otherwise
|
||||
* beside the catalog in `~/.mcp-inspector`.
|
||||
*/
|
||||
export function defaultSecretFilePath(): string {
|
||||
const override = process.env[SECRET_FILE_ENV]?.trim();
|
||||
if (override) return path.resolve(override);
|
||||
const storageDir = process.env[STORAGE_DIR_ENV]?.trim();
|
||||
if (storageDir) return path.resolve(storageDir, "secrets.json");
|
||||
const homeDir = process.env.HOME || process.env.USERPROFILE || ".";
|
||||
return path.join(homeDir, ".mcp-inspector", "secrets.json");
|
||||
}
|
||||
|
|
@ -172,11 +188,11 @@ export function isOnMountPoint(dir: string): boolean {
|
|||
}
|
||||
|
||||
/** Build the store named by `kind`, plus the descriptor that explains it. */
|
||||
function buildStore(
|
||||
async function buildStore(
|
||||
kind: SecretStoreKind,
|
||||
reason: SecretStorageInfo["reason"],
|
||||
detail?: string,
|
||||
): ResolvedSecretStore {
|
||||
): Promise<ResolvedSecretStore> {
|
||||
if (kind === "keyring") {
|
||||
return {
|
||||
store: new KeyringSecretStore(),
|
||||
|
|
@ -196,13 +212,24 @@ function buildStore(
|
|||
// every failure), and blocking selection on a chmod would make an
|
||||
// unwritable file a startup problem rather than a `set`-time one.
|
||||
void tightenSecretFilePermissions(filePath);
|
||||
// Report the FILE, not the intent. `store.encrypted` is what the next write
|
||||
// will do; a file already on disk in the clear stays readable until that
|
||||
// write happens, and saying "File (encrypted)" in the meantime is the one
|
||||
// false reassurance this footer must never give. With no file yet (or one we
|
||||
// cannot parse) the policy is all there is to report, and it is then
|
||||
// accurate — the first write creates the file in that mode.
|
||||
const onDisk = await store.readOnDiskEncryption();
|
||||
const plaintext = onDisk === null ? !store.encrypted : onDisk === "none";
|
||||
return {
|
||||
store,
|
||||
info: {
|
||||
kind,
|
||||
reason,
|
||||
path: filePath,
|
||||
plaintext: !store.encrypted,
|
||||
plaintext,
|
||||
// Only meaningful while the two disagree; omitted otherwise so the
|
||||
// payload does not carry a flag that says nothing.
|
||||
...(plaintext && store.encrypted ? { pendingEncryption: true } : {}),
|
||||
durable: true,
|
||||
detail,
|
||||
},
|
||||
|
|
@ -260,7 +287,7 @@ export function resolveSecretStore(): Promise<ResolvedSecretStore> {
|
|||
resolved ??= (async (): Promise<ResolvedSecretStore> => {
|
||||
const configured = parseSecretStoreEnv(process.env[SECRET_STORE_ENV]);
|
||||
if (configured) {
|
||||
const result = buildStore(configured, "configured");
|
||||
const result = await buildStore(configured, "configured");
|
||||
warnAboutSecretStorage(result.info);
|
||||
return result;
|
||||
}
|
||||
|
|
@ -271,7 +298,7 @@ export function resolveSecretStore(): Promise<ResolvedSecretStore> {
|
|||
container: isContainer(),
|
||||
mounted: isOnMountPoint(path.dirname(defaultSecretFilePath())),
|
||||
});
|
||||
const result = buildStore(kind, "fallback", probe.detail);
|
||||
const result = await buildStore(kind, "fallback", probe.detail);
|
||||
warnAboutSecretStorage(result.info);
|
||||
return result;
|
||||
})();
|
||||
|
|
|
|||
Loading…
Reference in a new issue