fix(auth): address Copilot review round 2 on the secret-store PR
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>
This commit is contained in:
cliffhall
parent
3452a63101
commit
de7795ca54
4 changed files with 716 additions and 96 deletions
|
|
@ -32,7 +32,7 @@ export interface SecretStorageFooterProps {
|
|||
// header, body and footer as one. `nowrap` keeps it to one line at any modal
|
||||
// width; the truncating detail text on the right absorbs a narrow modal.
|
||||
const FooterBand = Group.withProps({
|
||||
variant: "secretStorageFooter",
|
||||
variant: "stickyModalFooter",
|
||||
h: SECRET_STORAGE_FOOTER_HEIGHT,
|
||||
px: "md",
|
||||
gap: 6,
|
||||
|
|
|
|||
|
|
@ -7,11 +7,13 @@
|
|||
* not contain the plaintext — and a mocked filesystem can be made to
|
||||
* agree with any of those without them being true.
|
||||
*/
|
||||
import { describe, it, expect, beforeEach, afterEach } from "vitest";
|
||||
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
|
||||
import * as crypto from "node:crypto";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import {
|
||||
acquireLock,
|
||||
FileSecretStore,
|
||||
SecretFileKeyMismatchError,
|
||||
tightenSecretFilePermissions,
|
||||
|
|
@ -447,10 +449,52 @@ describe("tightenSecretFilePermissions", () => {
|
|||
expect((await fs.stat(filePath())).mode & 0o777).toBe(0o600);
|
||||
});
|
||||
|
||||
it("is a no-op for a file that doesn't exist yet (the first-run case)", async () => {
|
||||
it("reports absent, not a failure, for a file that doesn't exist yet", async () => {
|
||||
// The first-run case. Nothing to tighten and nothing to warn about —
|
||||
// `writeStoreFile` creates the file 0600.
|
||||
await expect(
|
||||
tightenSecretFilePermissions(path.join(tmpDir, "nope.json")),
|
||||
).resolves.toBeUndefined();
|
||||
).resolves.toEqual({ state: "absent" });
|
||||
});
|
||||
|
||||
it("reports a mode it could not fix, instead of claiming 0600", async () => {
|
||||
// The case the caller has to hear about: the docs and the footer both say
|
||||
// the file is 0600, so a file we failed to tighten must be surfaced rather
|
||||
// than silently described as protected.
|
||||
//
|
||||
// Driven through a fresh module whose `chmod` rejects — a test cannot
|
||||
// readily own a file it lacks permission to chmod, and vitest cannot spy
|
||||
// on a `node:fs/promises` export because the ESM namespace is not
|
||||
// configurable.
|
||||
const store = new FileSecretStore({ filePath: filePath() });
|
||||
await store.set("alpha", "env:A", "1");
|
||||
await fs.chmod(filePath(), 0o644);
|
||||
|
||||
vi.resetModules();
|
||||
vi.doMock("node:fs/promises", async () => {
|
||||
const actual =
|
||||
await vi.importActual<typeof import("node:fs/promises")>(
|
||||
"node:fs/promises",
|
||||
);
|
||||
return {
|
||||
...actual,
|
||||
default: actual,
|
||||
chmod: vi.fn(() =>
|
||||
Promise.reject(new Error("EPERM: operation not permitted")),
|
||||
),
|
||||
};
|
||||
});
|
||||
try {
|
||||
const mod =
|
||||
await import("@inspector/core/auth/node/file-secret-store.js");
|
||||
expect(await mod.tightenSecretFilePermissions(filePath())).toEqual({
|
||||
state: "loose",
|
||||
mode: 0o644,
|
||||
});
|
||||
} finally {
|
||||
vi.doUnmock("node:fs/promises");
|
||||
vi.resetModules();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -498,3 +542,179 @@ describe("readOnDiskEncryption", () => {
|
|||
expect(await keyless.readOnDiskEncryption()).toBe("aes-256-gcm");
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Write a genuinely-encrypted secrets file whose decrypted payload is
|
||||
* `payload` — including payloads the store would never produce.
|
||||
*
|
||||
* Hand-rolled against the documented format rather than driven through the
|
||||
* store, because the whole point is to reach a state the store's own writer
|
||||
* cannot create: an authentic ciphertext (right passphrase, valid GCM tag)
|
||||
* carrying the wrong shape. Anything less would test the key check instead
|
||||
* of the shape check.
|
||||
*/
|
||||
async function writeEncryptedFixtureWithPayload(
|
||||
payload: unknown,
|
||||
passphrase: string,
|
||||
): Promise<void> {
|
||||
const salt = crypto.randomBytes(16);
|
||||
const kdf = {
|
||||
algorithm: "scrypt",
|
||||
salt: salt.toString("base64"),
|
||||
N: 16384,
|
||||
r: 8,
|
||||
p: 1,
|
||||
};
|
||||
const key: Buffer = await new Promise((resolve, reject) => {
|
||||
crypto.scrypt(
|
||||
passphrase,
|
||||
salt,
|
||||
32,
|
||||
{ N: kdf.N, r: kdf.r, p: kdf.p },
|
||||
(err, k) => (err ? reject(err) : resolve(k)),
|
||||
);
|
||||
});
|
||||
const iv = crypto.randomBytes(12);
|
||||
const cipher = crypto.createCipheriv("aes-256-gcm", key, iv);
|
||||
const body = Buffer.concat([
|
||||
cipher.update(JSON.stringify(payload), "utf-8"),
|
||||
cipher.final(),
|
||||
]);
|
||||
const data = [iv, cipher.getAuthTag(), body]
|
||||
.map((b) => b.toString("base64"))
|
||||
.join(".");
|
||||
await fs.writeFile(
|
||||
filePath(),
|
||||
JSON.stringify({ version: 1, encryption: "aes-256-gcm", kdf, data }),
|
||||
"utf-8",
|
||||
);
|
||||
}
|
||||
|
||||
describe("payload shape validation", () => {
|
||||
// GCM proves who wrote the bytes, not what shape they parse to, and the
|
||||
// plaintext branch proves nothing at all — so neither may be cast.
|
||||
it("refuses a plaintext file whose secrets field is an array", async () => {
|
||||
// The array is the case a cast cannot survive: it passes `typeof
|
||||
// "object"`, takes the named assignment, and then `JSON.stringify` drops
|
||||
// every named property — so `set` would resolve having written a file
|
||||
// without the secret it was handed.
|
||||
await fs.writeFile(
|
||||
filePath(),
|
||||
JSON.stringify({ version: 1, encryption: "none", secrets: [] }),
|
||||
"utf-8",
|
||||
);
|
||||
const store = new FileSecretStore({ filePath: filePath() });
|
||||
await expect(store.set("alpha", "env:A", "1")).rejects.toThrow(/an array/);
|
||||
expect(await store.get("alpha", "env:A")).toBe(null);
|
||||
});
|
||||
|
||||
it("refuses a plaintext file holding a non-string value", async () => {
|
||||
await fs.writeFile(
|
||||
filePath(),
|
||||
JSON.stringify({
|
||||
version: 1,
|
||||
encryption: "none",
|
||||
secrets: { "alpha:env:A": { nested: true } },
|
||||
}),
|
||||
"utf-8",
|
||||
);
|
||||
const store = new FileSecretStore({ filePath: filePath() });
|
||||
await expect(store.set("alpha", "env:B", "1")).rejects.toThrow(
|
||||
/non-string value for "alpha:env:A"/,
|
||||
);
|
||||
});
|
||||
|
||||
it("refuses an authentic ciphertext that decodes to the wrong shape", async () => {
|
||||
// Encrypted with the right passphrase, so it decrypts cleanly and the
|
||||
// refusal can only come from the shape check. It must NOT surface as a key
|
||||
// mismatch, which would send the user to fix a passphrase that is working.
|
||||
await writeEncryptedFixtureWithPayload([], "hunter2");
|
||||
const store = new FileSecretStore({
|
||||
filePath: filePath(),
|
||||
passphrase: "hunter2",
|
||||
});
|
||||
expect(await store.get("alpha", "env:A")).toBe(null);
|
||||
const err = await store
|
||||
.set("alpha", "env:A", "1")
|
||||
.then(() => null)
|
||||
.catch((e: unknown) => e);
|
||||
expect(err).toBeInstanceOf(SecretStoreUnavailableError);
|
||||
expect(err).not.toBeInstanceOf(SecretFileKeyMismatchError);
|
||||
expect((err as Error).message).toMatch(/an array/);
|
||||
});
|
||||
});
|
||||
|
||||
describe("acquireLock", () => {
|
||||
it("excludes a second holder while the first is live", async () => {
|
||||
const target = path.join(tmpDir, "locked.json");
|
||||
const release = await acquireLock(target);
|
||||
let secondTaken = false;
|
||||
const second = acquireLock(target).then((r) => {
|
||||
secondTaken = true;
|
||||
return r;
|
||||
});
|
||||
// Still held: the waiter must not have been let through.
|
||||
await new Promise((r) => setTimeout(r, 60));
|
||||
expect(secondTaken).toBe(false);
|
||||
await release();
|
||||
await (
|
||||
await second
|
||||
)();
|
||||
expect(secondTaken).toBe(true);
|
||||
});
|
||||
|
||||
it("steals a lock left behind by a killed process", async () => {
|
||||
// A container killed with SIGKILL leaves the directory. A lock only a
|
||||
// clean exit can release would turn that into permanently broken writes —
|
||||
// strictly worse than the race it prevents.
|
||||
const target = path.join(tmpDir, "stale.json");
|
||||
const lockDir = `${target}.lock`;
|
||||
await fs.mkdir(lockDir);
|
||||
const old = new Date(Date.now() - 60_000);
|
||||
await fs.utimes(lockDir, old, old);
|
||||
const release = await acquireLock(target);
|
||||
await release();
|
||||
});
|
||||
|
||||
it("proceeds unlocked rather than failing when the lock cannot be created", async () => {
|
||||
// A path whose parent does not exist: mkdir fails with ENOENT, not
|
||||
// EEXIST. Refusing to write here would deny a real user a real secret to
|
||||
// prevent a race with a second process that may not exist.
|
||||
const release = await acquireLock(
|
||||
path.join(tmpDir, "no-such-dir", "secrets.json"),
|
||||
);
|
||||
await release();
|
||||
});
|
||||
|
||||
it("serializes concurrent writers through two independent store instances", async () => {
|
||||
// Two `FileSecretStore` objects have separate in-process queues, so this
|
||||
// is the cross-process race in miniature: without the file lock the second
|
||||
// write drops the first one's entry.
|
||||
const target = path.join(tmpDir, "shared.json");
|
||||
const a = new FileSecretStore({ filePath: target });
|
||||
const b = new FileSecretStore({ filePath: target });
|
||||
await Promise.all([
|
||||
a.set("alpha", "env:A", "1"),
|
||||
b.set("alpha", "env:B", "2"),
|
||||
]);
|
||||
expect(await a.get("alpha", "env:A")).toBe("1");
|
||||
expect(await a.get("alpha", "env:B")).toBe("2");
|
||||
});
|
||||
});
|
||||
|
||||
describe("tightenSecretFilePermissions reports what it could not fix", () => {
|
||||
it("says absent when there is no file yet", async () => {
|
||||
expect(await tightenSecretFilePermissions(filePath())).toEqual({
|
||||
state: "absent",
|
||||
});
|
||||
});
|
||||
|
||||
it("says ok once the file is 0600", async () => {
|
||||
const store = new FileSecretStore({ filePath: filePath() });
|
||||
await store.set("alpha", "env:A", "1");
|
||||
await fs.chmod(filePath(), 0o644);
|
||||
expect(await tightenSecretFilePermissions(filePath())).toEqual({
|
||||
state: "ok",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -221,6 +221,105 @@ describe("isOnMountPoint", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("isOnMountPoint via /proc/self/mountinfo", () => {
|
||||
/**
|
||||
* Load the module with a synthetic mount table, so the container layouts
|
||||
* that matter can be exercised on a developer's laptop. `existsSync` is
|
||||
* stubbed alongside it because the paths in these tables do not exist here.
|
||||
*/
|
||||
async function loadWithMounts(mountinfo: string | null, existing: string[]) {
|
||||
vi.resetModules();
|
||||
vi.doMock("node:fs", async () => {
|
||||
const actual = await vi.importActual<typeof import("node:fs")>("node:fs");
|
||||
return {
|
||||
...actual,
|
||||
existsSync: (p: string) => existing.includes(p),
|
||||
readFileSync: (p: string, enc?: unknown) => {
|
||||
if (p === "/proc/self/mountinfo") {
|
||||
if (mountinfo === null) throw new Error("ENOENT");
|
||||
return mountinfo;
|
||||
}
|
||||
return actual.readFileSync(p, enc as never);
|
||||
},
|
||||
};
|
||||
});
|
||||
return import("@inspector/core/auth/node/secret-store-selection.js");
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
vi.doUnmock("node:fs");
|
||||
vi.resetModules();
|
||||
});
|
||||
|
||||
// Field 5 is the mount point; the rest is real mountinfo shape.
|
||||
const line = (point: string) =>
|
||||
`36 35 0:32 / ${point} rw,relatime - overlay overlay rw`;
|
||||
|
||||
it("sees a directory that IS the mount point", async () => {
|
||||
const mod = await loadWithMounts(
|
||||
[line("/"), line("/home/node/.mcp-inspector")].join("\n"),
|
||||
["/home/node/.mcp-inspector"],
|
||||
);
|
||||
expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(true);
|
||||
});
|
||||
|
||||
it("sees a directory INSIDE a mounted volume", async () => {
|
||||
// The case a device-boundary test gets wrong: the volume is at
|
||||
// /home/node, so .mcp-inspector is an ordinary subdirectory sharing its
|
||||
// parent's device. Judging it unmounted demotes a durable setup to
|
||||
// session-only memory.
|
||||
const mod = await loadWithMounts(
|
||||
[line("/"), line("/home/node")].join("\n"),
|
||||
["/home/node/.mcp-inspector"],
|
||||
);
|
||||
expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(true);
|
||||
});
|
||||
|
||||
it("is false on the container's writable layer", async () => {
|
||||
// Only `/` is mounted, and `/` is deliberately excluded — it is the
|
||||
// writable layer this question exists to distinguish against.
|
||||
const mod = await loadWithMounts(line("/"), ["/home/node/.mcp-inspector"]);
|
||||
expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(false);
|
||||
});
|
||||
|
||||
it("does not mistake a sibling prefix for a parent mount", async () => {
|
||||
// `/home/node-2` starts with `/home/node` as a string but is not under
|
||||
// it, which is why the check appends the separator.
|
||||
const mod = await loadWithMounts(
|
||||
[line("/"), line("/home/node")].join("\n"),
|
||||
["/home/node-2/.mcp-inspector"],
|
||||
);
|
||||
expect(mod.isOnMountPoint("/home/node-2/.mcp-inspector")).toBe(false);
|
||||
});
|
||||
|
||||
it("decodes a space-escaped mount point", async () => {
|
||||
const mod = await loadWithMounts(
|
||||
[line("/"), line("/mnt/my\\040volume")].join("\n"),
|
||||
["/mnt/my volume/secrets"],
|
||||
);
|
||||
expect(mod.isOnMountPoint("/mnt/my volume/secrets")).toBe(true);
|
||||
});
|
||||
|
||||
it("answers from the nearest existing ancestor, not the missing leaf", async () => {
|
||||
// First run: the secrets directory has not been created inside the
|
||||
// mounted volume yet.
|
||||
const mod = await loadWithMounts(
|
||||
[line("/"), line("/home/node")].join("\n"),
|
||||
["/home/node"],
|
||||
);
|
||||
expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(true);
|
||||
});
|
||||
|
||||
it("falls back to the device comparison where mountinfo is unreadable", async () => {
|
||||
// macOS, Windows, or a /proc-less container. The fallback compares
|
||||
// against `/` rather than the immediate parent, so a subdirectory of a
|
||||
// mount is still recognized — but a real path is needed for `stat`, so
|
||||
// this only asserts it does not throw and answers the ordinary case.
|
||||
const mod = await loadWithMounts(null, [tmpDir]);
|
||||
expect(mod.isOnMountPoint(tmpDir)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("warnAboutSecretStorage", () => {
|
||||
const base: SecretStorageInfo = {
|
||||
kind: "keyring",
|
||||
|
|
@ -452,6 +551,43 @@ describe("the descriptor reports the file, not the write policy", () => {
|
|||
expect(info.pendingEncryption).toBeUndefined();
|
||||
});
|
||||
|
||||
it("stops saying unencrypted once this process performs the upgrading write", async () => {
|
||||
// The staleness trap: the selection is cached for the process, so a
|
||||
// descriptor cached alongside it would keep serving "still unencrypted"
|
||||
// from `/api/config` for the rest of the session — describing bytes this
|
||||
// very process had just changed.
|
||||
const filePath = path.join(tmpDir, "secrets.json");
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
|
||||
process.env.MCP_INSPECTOR_SECRET_STORE = "file";
|
||||
vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
|
||||
const plain = await loadWithProbe(false);
|
||||
await plain.defaultSecretStore().set("alpha", "env:A", "1");
|
||||
|
||||
process.env.MCP_INSPECTOR_SECRET_KEY = "hunter2";
|
||||
const mod = await loadWithProbe(false);
|
||||
|
||||
// Same module instance throughout — this is one running process.
|
||||
expect((await mod.getSecretStorageInfo()).plaintext).toBe(true);
|
||||
await mod.defaultSecretStore().set("alpha", "env:B", "2");
|
||||
const after = await mod.getSecretStorageInfo();
|
||||
expect(after.plaintext).toBe(false);
|
||||
expect(after.pendingEncryption).toBeUndefined();
|
||||
});
|
||||
|
||||
it("keeps returning the same store while re-deriving the descriptor", async () => {
|
||||
// The selection itself must stay cached — the banner, /api/config, and
|
||||
// the store doing the writing have to agree on *which* store it is.
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json");
|
||||
process.env.MCP_INSPECTOR_SECRET_STORE = "file";
|
||||
vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const mod = await loadWithProbe(false);
|
||||
const first = await mod.resolveSecretStore();
|
||||
const second = await mod.resolveSecretStore();
|
||||
expect(second.store).toBe(first.store);
|
||||
expect((await mod.getSecretStorageInfo()).kind).toBe("file");
|
||||
});
|
||||
|
||||
it("falls back to the write policy when there is no file yet", async () => {
|
||||
// Nothing on disk to describe, and the policy is then accurate: the first
|
||||
// write creates the file in exactly that mode.
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@ export const ThemeGroup = Group.extend({
|
|||
return {};
|
||||
},
|
||||
styles: (_theme, props) => {
|
||||
// `secretStorageFooter` is the permanent band at the bottom of the
|
||||
// `stickyModalFooter` is the permanent band at the bottom of the
|
||||
// Client/Server Settings modals naming where secrets are stored (#1950).
|
||||
// Flat properties, so they belong here rather than in App.css.
|
||||
//
|
||||
|
|
@ -27,7 +27,11 @@ export const ThemeGroup = Group.extend({
|
|||
// this is supposed to be, so it sticks to the bottom edge instead, with
|
||||
// the same opaque body background and z-index the sticky header uses so
|
||||
// content passes underneath rather than through it.
|
||||
if (props.variant === "secretStorageFooter") {
|
||||
// Structural, not domain: this says "a band pinned to the bottom of a
|
||||
// modal", which is all the theme layer should know. What the band *means*
|
||||
// — that it names where your secrets go — belongs to `SecretStorageFooter`,
|
||||
// the element that owns those semantics.
|
||||
if (props.variant === "stickyModalFooter") {
|
||||
return {
|
||||
root: {
|
||||
position: "sticky",
|
||||
|
|
|
|||
Loading…
Reference in a new issue