fix(auth): address Copilot review round 5, and land the impl its tests need
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>
This commit is contained in:
cliffhall
parent
71cb7f3eae
commit
decc673a5d
5 changed files with 458 additions and 26 deletions
|
|
@ -112,4 +112,26 @@ describe("SecretStorageFooter", () => {
|
|||
renderWithMantine(<SecretStorageFooter info={undefined} />);
|
||||
expect(screen.queryByTestId("secret-storage-footer")).toBeNull();
|
||||
});
|
||||
|
||||
it("says the permissions could not be checked rather than claiming owner-only", async () => {
|
||||
// The third permission state. Falling into the verified-0600 branch here
|
||||
// would have the band assert owner-only having read nothing — the same
|
||||
// bug as claiming 0600 in the caveat, one layer up.
|
||||
renderWithMantine(
|
||||
<SecretStorageFooter
|
||||
info={{
|
||||
kind: "file",
|
||||
reason: "fallback",
|
||||
durable: true,
|
||||
plaintext: true,
|
||||
path: "/home/node/.mcp-inspector/secrets.json",
|
||||
permissionsUnknown: "EACCES",
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
const band = screen.getByTestId("secret-storage-footer");
|
||||
expect(band).toHaveTextContent("Permissions could not be checked.");
|
||||
expect(band).not.toHaveTextContent("Owner-only");
|
||||
expect(band).toHaveAttribute("data-tone", "warn");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -127,6 +127,28 @@ const formatMode = (mode: number): string => mode.toString(8).padStart(4, "0");
|
|||
* and the underlying facts still come from the shared helpers, which is
|
||||
* what keeps them from disagreeing about anything that matters.
|
||||
*/
|
||||
/**
|
||||
* How the band describes the file's permissions.
|
||||
*
|
||||
* Three states, not two, and the third is the point: "Owner-only
|
||||
* permissions." is a claim of *fact*, so it may only be made when the mode
|
||||
* was actually read and found to be 0600. `looseMode` says we looked and it
|
||||
* was wrong; `permissionsUnknown` says we could not look at all. Treating
|
||||
* the second like the first — which a plain `looseMode === undefined` test
|
||||
* does — makes the footer assert owner-only having verified nothing, which
|
||||
* is the failure this band exists to prevent, arrived at by omission rather
|
||||
* than by error.
|
||||
*/
|
||||
function filePermissionsSentence(info: SecretStorageInfo): string {
|
||||
if (info.looseMode !== undefined) {
|
||||
return `Mode ${formatMode(info.looseMode)} — not owner-only.`;
|
||||
}
|
||||
if (info.permissionsUnknown !== undefined) {
|
||||
return "Permissions could not be checked.";
|
||||
}
|
||||
return "Owner-only permissions.";
|
||||
}
|
||||
|
||||
function footerMessage(info: SecretStorageInfo): string {
|
||||
if (info.kind === "keyring") return secretStorageLabel(info);
|
||||
if (info.kind === "memory") {
|
||||
|
|
@ -143,10 +165,7 @@ function footerMessage(info: SecretStorageInfo): string {
|
|||
// is something else and could not be tightened, and saying "owner-only"
|
||||
// there would be the footer asserting the opposite of what is true — the
|
||||
// one failure this whole band exists to prevent.
|
||||
const permissions =
|
||||
info.looseMode === undefined
|
||||
? "Owner-only permissions."
|
||||
: `Mode ${formatMode(info.looseMode)} — not owner-only.`;
|
||||
const permissions = filePermissionsSentence(info);
|
||||
return `${encryption} file. ${permissions}`;
|
||||
}
|
||||
|
||||
|
|
@ -173,6 +192,10 @@ function footerTooltip(info: SecretStorageInfo): string | undefined {
|
|||
parts.push(
|
||||
`Mode ${formatMode(info.looseMode)} could not be tightened to 0600 — anyone who can read the file can read the secrets in it.`,
|
||||
);
|
||||
} else if (info.permissionsUnknown !== undefined) {
|
||||
parts.push(
|
||||
`The file's permissions could not be read (${info.permissionsUnknown}), so it is not known whether others can read it.`,
|
||||
);
|
||||
}
|
||||
parts.push("Click to copy file path");
|
||||
return parts.join(" ");
|
||||
|
|
|
|||
|
|
@ -284,6 +284,7 @@ describe("useInitialConfig", () => {
|
|||
path: "/home/node/.mcp-inspector/secrets.json",
|
||||
pendingEncryption: true,
|
||||
looseMode: 0o644,
|
||||
permissionsUnknown: "EACCES",
|
||||
};
|
||||
const fetchFn = vi
|
||||
.fn()
|
||||
|
|
@ -354,6 +355,50 @@ describe("useInitialConfig", () => {
|
|||
looseMode: "644",
|
||||
},
|
||||
],
|
||||
[
|
||||
"a non-string permissionsUnknown",
|
||||
{
|
||||
kind: "file",
|
||||
reason: "fallback",
|
||||
durable: true,
|
||||
plaintext: true,
|
||||
path: "/x",
|
||||
permissionsUnknown: true,
|
||||
},
|
||||
],
|
||||
[
|
||||
// Each file-only field is rejected on its own, so no single one can
|
||||
// slip through on the back of the others being absent.
|
||||
"permissionsUnknown on a keyring descriptor",
|
||||
{
|
||||
kind: "keyring",
|
||||
reason: "default",
|
||||
durable: true,
|
||||
permissionsUnknown: "EACCES",
|
||||
},
|
||||
],
|
||||
[
|
||||
"looseMode on a keyring descriptor",
|
||||
{ kind: "keyring", reason: "default", durable: true, looseMode: 0o644 },
|
||||
],
|
||||
[
|
||||
"pendingEncryption on a memory descriptor",
|
||||
{
|
||||
kind: "memory",
|
||||
reason: "fallback",
|
||||
durable: false,
|
||||
pendingEncryption: true,
|
||||
},
|
||||
],
|
||||
[
|
||||
"a path on a memory descriptor",
|
||||
{
|
||||
kind: "memory",
|
||||
reason: "fallback",
|
||||
durable: false,
|
||||
path: "/x/secrets.json",
|
||||
},
|
||||
],
|
||||
[
|
||||
// File-only fields on a non-file kind mean the payload was not built
|
||||
// by a backend we understand; rendering a mixture of two stores'
|
||||
|
|
|
|||
|
|
@ -452,6 +452,38 @@ describe("tightenSecretFilePermissions", () => {
|
|||
expect((await fs.stat(filePath())).mode & 0o777).toBe(0o600);
|
||||
});
|
||||
|
||||
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
|
||||
// as fact, having verified nothing.
|
||||
vi.resetModules();
|
||||
vi.doMock("node:fs/promises", async () => {
|
||||
const actual =
|
||||
await vi.importActual<typeof import("node:fs/promises")>(
|
||||
"node:fs/promises",
|
||||
);
|
||||
const denied = Object.assign(new Error("EACCES: permission denied"), {
|
||||
code: "EACCES",
|
||||
});
|
||||
return {
|
||||
...actual,
|
||||
default: actual,
|
||||
stat: vi.fn(() => Promise.reject(denied)),
|
||||
};
|
||||
});
|
||||
try {
|
||||
const mod =
|
||||
await import("@inspector/core/auth/node/file-secret-store.js");
|
||||
expect(await mod.readSecretFilePermissions(filePath())).toEqual({
|
||||
state: "unknown",
|
||||
detail: "EACCES",
|
||||
});
|
||||
} finally {
|
||||
vi.doUnmock("node:fs/promises");
|
||||
vi.resetModules();
|
||||
}
|
||||
});
|
||||
|
||||
it("readSecretFilePermissions reports the mode without changing it", async () => {
|
||||
// The descriptor is rebuilt on every GET /api/config, so the function it
|
||||
// calls must not chmod — once per page load, on a file another process
|
||||
|
|
@ -905,6 +937,40 @@ describe("acquireLock", () => {
|
|||
await fs.rm(lockDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("elects a single winner when two waiters find the same stale lock", async () => {
|
||||
// `rm`-then-`mkdir` is not an election: both waiters judge the lock
|
||||
// stale, the first removes it and takes a fresh one, and the second then
|
||||
// removes *that* — putting both inside the section at once, which is the
|
||||
// lost update the takeover exists to prevent. The rename is atomic, so
|
||||
// exactly one waiter can claim a given stale lock.
|
||||
const target = path.join(tmpDir, "elected.json");
|
||||
const lockDir = `${target}.lock`;
|
||||
await fs.mkdir(lockDir, { recursive: true });
|
||||
await fs.writeFile(
|
||||
path.join(lockDir, "owner"),
|
||||
// A pid that cannot be running, so both waiters agree it is stale.
|
||||
JSON.stringify({ token: "dead", pid: 2 ** 31 - 1, host: os.hostname() }),
|
||||
"utf-8",
|
||||
);
|
||||
const old = new Date(Date.now() - 60_000);
|
||||
await fs.utimes(lockDir, old, old);
|
||||
|
||||
let inside = 0;
|
||||
let maxInside = 0;
|
||||
const timings = { staleMs: 50, timeoutMs: 10_000, pollMs: 5 };
|
||||
const run = async () => {
|
||||
const release = await acquireLock(target, timings);
|
||||
inside += 1;
|
||||
maxInside = Math.max(maxInside, inside);
|
||||
await new Promise((r) => setTimeout(r, 40));
|
||||
inside -= 1;
|
||||
await release();
|
||||
};
|
||||
await Promise.all([run(), run()]);
|
||||
// Never two holders at once.
|
||||
expect(maxInside).toBe(1);
|
||||
});
|
||||
|
||||
it("cleans up and reports when it cannot stamp ownership", async () => {
|
||||
// mkdir succeeded, the owner write did not. Returning a no-op release
|
||||
// here would enter the critical section unlocked *and* leave the
|
||||
|
|
|
|||
|
|
@ -612,29 +612,71 @@ describe("absorbFileSecretsIntoKeyring", () => {
|
|||
|
||||
it("does not delete a file it cannot read", async () => {
|
||||
// An undecryptable file is not an empty one. Deleting it here would lose
|
||||
// exactly the values this whole hand-off exists to preserve.
|
||||
// exactly the values this hand-off exists to preserve.
|
||||
const filePath = path.join(tmpDir, "secrets.json");
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
|
||||
process.env.MCP_INSPECTOR_SECRET_KEY = "right-key";
|
||||
const seeded = await loadWithProbe(false);
|
||||
vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
await new (
|
||||
await import("@inspector/core/auth/node/file-secret-store.js")
|
||||
).FileSecretStore({ filePath, passphrase: "right-key" }).set(
|
||||
|
||||
const { FileSecretStore } =
|
||||
await import("@inspector/core/auth/node/file-secret-store.js");
|
||||
await new FileSecretStore({ filePath, passphrase: "right-key" }).set(
|
||||
"srv",
|
||||
"oauthClientSecret",
|
||||
"encrypted",
|
||||
);
|
||||
void seeded;
|
||||
|
||||
delete process.env.MCP_INSPECTOR_SECRET_KEY;
|
||||
// The passphrase is gone by the time the keychain reappears.
|
||||
const mod = await loadWithProbe(true);
|
||||
const keyring = new InMemorySecretStore();
|
||||
await mod.absorbFileSecretsIntoKeyring(keyring);
|
||||
|
||||
expect(existsSync(filePath)).toBe(true);
|
||||
expect(await keyring.get("srv", "oauthClientSecret")).toBe(null);
|
||||
});
|
||||
|
||||
it("is a no-op for a file that holds no entries", async () => {
|
||||
// An empty map is not a hand-off. Deleting the file here would be
|
||||
// harmless but pointless; the interesting part is that it does not warn
|
||||
// about a migration that never happened.
|
||||
const filePath = await seedFile({});
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const mod = await loadWithProbe(true);
|
||||
await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore());
|
||||
expect(existsSync(filePath)).toBe(true);
|
||||
expect(warn).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("skips an entry whose account key is malformed", async () => {
|
||||
// A hand-edited file can carry a key that is not `serverId:field`. It
|
||||
// cannot be addressed through the store API, so it is passed over rather
|
||||
// than guessed at — but it must not abort the entries around it.
|
||||
const filePath = await seedFile({
|
||||
"no-separator": "orphan",
|
||||
"srv:oauthClientSecret": "from-file",
|
||||
});
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
|
||||
vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const mod = await loadWithProbe(true);
|
||||
const keyring = new InMemorySecretStore();
|
||||
|
||||
await mod.absorbFileSecretsIntoKeyring(keyring);
|
||||
|
||||
expect(existsSync(filePath)).toBe(true);
|
||||
expect(await keyring.get("srv", "oauthClientSecret")).toBe(null);
|
||||
expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file");
|
||||
});
|
||||
|
||||
it("runs the hand-off for an explicitly configured keyring too", async () => {
|
||||
// `MCP_INSPECTOR_SECRET_STORE=keyring` reaches the keychain by a
|
||||
// different path than the probe does, and a leftover file is just as
|
||||
// invisible either way.
|
||||
const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
|
||||
process.env.MCP_INSPECTOR_SECRET_STORE = "keyring";
|
||||
vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const mod = await loadWithProbe(false);
|
||||
// Resolving is what triggers it, via the configured branch.
|
||||
await mod.resolveSecretStore();
|
||||
expect(existsSync(filePath)).toBe(false);
|
||||
});
|
||||
|
||||
it("is a no-op when there is no file", async () => {
|
||||
|
|
@ -646,6 +688,59 @@ describe("absorbFileSecretsIntoKeyring", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("the descriptor reports a mode it could not verify", () => {
|
||||
it("carries permissionsUnknown when the file exists but cannot be inspected", async () => {
|
||||
// The third state. Without it an unverifiable mode fell into the same
|
||||
// branch as a verified 0600, and the footer claimed owner-only
|
||||
// permissions having read nothing.
|
||||
const filePath = path.join(tmpDir, "secrets.json");
|
||||
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
|
||||
process.env.MCP_INSPECTOR_SECRET_STORE = "file";
|
||||
await fs.writeFile(
|
||||
filePath,
|
||||
JSON.stringify({ version: 1, encryption: "none", secrets: {} }),
|
||||
"utf-8",
|
||||
);
|
||||
vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
|
||||
vi.resetModules();
|
||||
vi.doMock("node:fs/promises", async () => {
|
||||
const actual =
|
||||
await vi.importActual<typeof import("node:fs/promises")>(
|
||||
"node:fs/promises",
|
||||
);
|
||||
const denied = Object.assign(new Error("EACCES: permission denied"), {
|
||||
code: "EACCES",
|
||||
});
|
||||
return {
|
||||
...actual,
|
||||
default: actual,
|
||||
stat: vi.fn(() => Promise.reject(denied)),
|
||||
};
|
||||
});
|
||||
vi.doMock("@inspector/core/auth/node/secret-store.js", async () => {
|
||||
const actual = await vi.importActual<
|
||||
typeof import("@inspector/core/auth/node/secret-store.js")
|
||||
>("@inspector/core/auth/node/secret-store.js");
|
||||
return {
|
||||
...actual,
|
||||
probeKeyringAvailable: vi.fn(async () => ({ available: true })),
|
||||
};
|
||||
});
|
||||
try {
|
||||
const mod =
|
||||
await import("@inspector/core/auth/node/secret-store-selection.js");
|
||||
const info = await mod.getSecretStorageInfo();
|
||||
expect(info.permissionsUnknown).toBe("EACCES");
|
||||
expect(info.looseMode).toBeUndefined();
|
||||
} finally {
|
||||
vi.doUnmock("node:fs/promises");
|
||||
vi.doUnmock("@inspector/core/auth/node/secret-store.js");
|
||||
vi.resetModules();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("the descriptor reports a mode it could not tighten", () => {
|
||||
it("carries looseMode into the descriptor and warns once at selection", async () => {
|
||||
// The warning alone was not enough: the caveat under the secret field
|
||||
|
|
|
|||
Loading…
Reference in a new issue