fix(auth): address Copilot review round 8 (four inline findings)

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>
This commit is contained in:
cliffhall 2026-08-22 17:41:56 -04:00
commit 37faf1be88
2 changed files with 351 additions and 24 deletions

View file

@ -452,6 +452,50 @@ describe("tightenSecretFilePermissions", () => {
expect((await fs.stat(filePath())).mode & 0o777).toBe(0o600);
});
it("declines to interpret Windows' synthetic mode bits", async () => {
// Windows does not model POSIX modes. `stat` still returns bits, and
// reading them as a permission statement is 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.
const store = new FileSecretStore({ filePath: filePath() });
await store.set("alpha", "env:A", "1");
const real = process.platform;
Object.defineProperty(process, "platform", {
value: "win32",
configurable: true,
});
try {
expect(await readSecretFilePermissions(filePath())).toEqual({
state: "unknown",
detail: "not checked on Windows",
});
} finally {
Object.defineProperty(process, "platform", {
value: real,
configurable: true,
});
}
});
it("still reports absent on Windows when there is no file", async () => {
const real = process.platform;
Object.defineProperty(process, "platform", {
value: "win32",
configurable: true,
});
try {
expect(
await readSecretFilePermissions(path.join(tmpDir, "nope.json")),
).toEqual({ state: "absent" });
} finally {
Object.defineProperty(process, "platform", {
value: real,
configurable: true,
});
}
});
it("reports unknown, not absent, when an existing file cannot be inspected", async () => {
// Collapsing every stat failure into "absent" is what made it dangerous:
// the descriptor then omits the warning and the footer states mode 0600
@ -700,6 +744,84 @@ async function writeEncryptedFixtureWithPayload(
);
}
describe("getMany", () => {
it("returns a server's fields from a single read", async () => {
const store = new FileSecretStore({
filePath: filePath(),
passphrase: "hunter2",
});
await store.set("srv", SECRET_FIELD_OAUTH_CLIENT_SECRET, "s1");
await store.set("srv", "env:TOKEN", "s2");
await store.set("other", "env:TOKEN", "nope");
expect(
await store.getMany("srv", [
SECRET_FIELD_OAUTH_CLIENT_SECRET,
"env:TOKEN",
"env:ABSENT",
]),
).toEqual({ [SECRET_FIELD_OAUTH_CLIENT_SECRET]: "s1", "env:TOKEN": "s2" });
});
it("derives the key once for the whole set, not once per field", async () => {
// The reason the seam exists: `get` reads and decrypts the *entire* file,
// so rehydrating field-by-field cost one scrypt derivation per field,
// serialized behind this store's queue. Counting derivations is the
// measurement that matters — scrypt is the expensive part, deliberately
// so.
const store = new FileSecretStore({
filePath: filePath(),
passphrase: "hunter2",
});
await store.set("srv", "env:A", "1");
await store.set("srv", "env:B", "2");
await store.set("srv", "env:C", "3");
let derivations = 0;
vi.resetModules();
vi.doMock("node:crypto", async () => {
const actual =
await vi.importActual<typeof import("node:crypto")>("node:crypto");
return {
...actual,
default: actual,
scrypt: (...args: Parameters<typeof actual.scrypt>) => {
derivations += 1;
return actual.scrypt(...args);
},
};
});
try {
const mod =
await import("@inspector/core/auth/node/file-secret-store.js");
const fresh = new mod.FileSecretStore({
filePath: filePath(),
passphrase: "hunter2",
});
await fresh.getMany("srv", ["env:A", "env:B", "env:C"]);
expect(derivations).toBe(1);
// And the shape it replaced, for contrast: three fields, three
// derivations.
derivations = 0;
await fresh.get("srv", "env:A");
await fresh.get("srv", "env:B");
await fresh.get("srv", "env:C");
expect(derivations).toBe(3);
} finally {
vi.doUnmock("node:crypto");
vi.resetModules();
}
});
it("is tolerant like get: an unreadable store yields no fields", async () => {
await fs.writeFile(filePath(), "{ not json", "utf-8");
const store = new FileSecretStore({ filePath: filePath() });
expect(await store.getMany("srv", ["env:A"])).toEqual({});
});
});
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.

View file

@ -545,6 +545,45 @@ describe("defaultSecretFilePath", () => {
});
});
describe("DeferredSecretStore forwards the optional seams", () => {
// This is what `defaultSecretStore()` returns, so a seam it drops is a
// seam that does not exist in production — however well the tests that
// inject a concrete store behave.
it("forwards getStrict rather than degrading to the tolerant get", async () => {
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 store = mod.defaultSecretStore();
expect(typeof store.getStrict).toBe("function");
const { secretStoreGetStrict } =
await import("@inspector/core/auth/node/secret-store.js");
// The file store's strict read is its ordinary read; the point here is
// that the call reaches the target at all.
expect(await secretStoreGetStrict(store, "srv", "env:A")).toBe(null);
await store.set("srv", "env:A", "1");
expect(await secretStoreGetStrict(store, "srv", "env:A")).toBe("1");
});
it("forwards getMany", async () => {
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 store = mod.defaultSecretStore();
await store.set("srv", "env:A", "1");
await store.set("srv", "env:B", "2");
const { secretStoreGetMany } =
await import("@inspector/core/auth/node/secret-store.js");
expect(await secretStoreGetMany(store, "srv", ["env:A", "env:B"])).toEqual({
"env:A": "1",
"env:B": "2",
});
});
});
describe("absorbFileSecretsIntoKeyring", () => {
// The transition: a box without libsecret falls back to a file, the user
// saves secrets there, then installs libsecret. Without a hand-off the next