fix(auth): address Copilot review round 16
Three findings. The first is the third consecutive round in which my previous fix introduced the next defect, all in the same ~15 lines of `useInitialConfig`. **The request token did not subsume the unmount guard, and I said it did.** Bumping the generation on effect teardown invalidates loads already *in flight*; a `refresh()` called after unmount claims the next token, is therefore current, and commits into a dead hook. That is a live path — the callers fire `refresh()` from an async settings persist that can outlive `App`. Mounted state is tracked separately again and `stale()` checks both. The test for this was also wrong, in the way that matters: it called `refresh()` after unmount but never settled the fetch, so it could not observe the commit it existed to prevent. It settles the promise now. **A tmpfs mount was reported as durable.** `docker run --tmpfs /home/node/.mcp-inspector` puts a real entry in mountinfo, so a mount-table lookup answered "durable", selected the file store, and published `durable: true` for a file that disappears when the container stops — the exact false promise the in-memory fallback exists to avoid, reached by trusting the mount table over the filesystem type. `tmpfs`/`ramfs` are excluded now, read from the field after the ` - ` separator rather than by position, since the optional fields before it are variable in number. **A malformed envelope was diagnosed as a changed passphrase.** A truncated payload, a wrong-length IV or tag, or an out-of-range KDF parameter all threw `SecretFileKeyMismatchError`, whose 503 tells the user to restore a passphrase that cannot repair a structurally broken file. The read path now runs the same `encryptedEnvelopeProblem` check the descriptor path already ran, and reserves the key-mismatch error for an envelope that is well-formed and fails authentication — which is the case where that advice is right. Two existing tests asserted the old behavior and encoded the bug; both updated, and a new one pins that a well-formed envelope with the wrong key still gets the passphrase advice. The remaining suppressed comment is the hand-off's check-to-delete race, tracked in #2082. Full `npm run ci` green. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
0ab542cbee
commit
52087f7739
2 changed files with 160 additions and 18 deletions
|
|
@ -306,7 +306,24 @@ describe("FileSecretStore failure handling", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("set reports a truncated ciphertext payload", async () => {
|
||||
it("blames the file, not the passphrase, for a malformed envelope", async () => {
|
||||
// A truncated payload or an out-of-range KDF parameter used to surface as
|
||||
// `SecretFileKeyMismatchError`, whose 503 tells the user to restore a
|
||||
// passphrase — advice that cannot repair a structurally broken file.
|
||||
await writeEncryptedFixture();
|
||||
const parsed = JSON.parse(await fs.readFile(filePath(), "utf-8"));
|
||||
parsed.kdf.N = 3;
|
||||
await fs.writeFile(filePath(), JSON.stringify(parsed), "utf-8");
|
||||
const store = new FileSecretStore({
|
||||
filePath: filePath(),
|
||||
passphrase: "right-key",
|
||||
});
|
||||
await expect(store.set("alpha", "env:A", "1")).rejects.toThrow(
|
||||
/Restoring a passphrase will not repair it/,
|
||||
);
|
||||
});
|
||||
|
||||
it("set reports a truncated ciphertext payload as a structural fault", async () => {
|
||||
await writeEncryptedFixture();
|
||||
const parsed = JSON.parse(await fs.readFile(filePath(), "utf-8"));
|
||||
parsed.data = "only-one-part";
|
||||
|
|
@ -315,16 +332,31 @@ describe("FileSecretStore failure handling", () => {
|
|||
filePath: filePath(),
|
||||
passphrase: "right-key",
|
||||
});
|
||||
await expect(store.set("alpha", "env:A", "1")).rejects.toThrow(
|
||||
/encrypted payload is not iv\.tag\.ciphertext/,
|
||||
);
|
||||
});
|
||||
|
||||
it("still blames the passphrase when the envelope is well-formed", async () => {
|
||||
// The other half: structure fine, authentication fails. That *is* a key
|
||||
// problem and must keep the advice that fits it — the round-16 change
|
||||
// must not have turned every failure into a structural one.
|
||||
await writeEncryptedFixture();
|
||||
const store = new FileSecretStore({
|
||||
filePath: filePath(),
|
||||
passphrase: "wrong-key",
|
||||
});
|
||||
await expect(store.set("alpha", "env:A", "1")).rejects.toBeInstanceOf(
|
||||
SecretFileKeyMismatchError,
|
||||
);
|
||||
});
|
||||
|
||||
it("reports a file whose KDF parameters are unusable", async () => {
|
||||
// scrypt rejects on out-of-grammar cost parameters (`N` must be a power of
|
||||
// two > 1). Those parameters come off disk, so this is a corrupt-file case
|
||||
// reachable without touching the crypto call site — and it must land as a
|
||||
// refusal, not as an exception escaping the derivation.
|
||||
// scrypt rejects out-of-grammar cost parameters (`N` must be a power of
|
||||
// two > 1). Those parameters come off disk, so this is a corrupt-file
|
||||
// case — and since round 16 it is diagnosed *as* one: it used to surface
|
||||
// as a key mismatch, whose advice ("restore the passphrase") cannot
|
||||
// repair a malformed file.
|
||||
await writeEncryptedFixture();
|
||||
const parsed = JSON.parse(await fs.readFile(filePath(), "utf-8"));
|
||||
parsed.kdf.N = 3;
|
||||
|
|
@ -336,8 +368,8 @@ describe("FileSecretStore failure handling", () => {
|
|||
expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(
|
||||
null,
|
||||
);
|
||||
await expect(store.set("alpha", "env:A", "1")).rejects.toBeInstanceOf(
|
||||
SecretFileKeyMismatchError,
|
||||
await expect(store.set("alpha", "env:A", "1")).rejects.toThrow(
|
||||
/kdf N is 3, which is not a power of two above 1/,
|
||||
);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -315,6 +315,37 @@ describe("isOnMountPoint via /proc/self/mountinfo", () => {
|
|||
expect(mod.isOnMountPoint("/data/inspector")).toBe(true);
|
||||
});
|
||||
|
||||
it("does not call a tmpfs mount durable", async () => {
|
||||
// `docker run --tmpfs /home/node/.mcp-inspector` puts a real entry in
|
||||
// mountinfo, so believing the mount table alone selects the file store
|
||||
// and publishes `durable: true` for a file that disappears when the
|
||||
// container stops — the same false promise the memory fallback exists to
|
||||
// avoid, reached by trusting the wrong field.
|
||||
const mod = await loadWithMounts(
|
||||
"36 35 0:32 / /home/node/.mcp-inspector rw,relatime - tmpfs tmpfs rw",
|
||||
["/home/node/.mcp-inspector"],
|
||||
);
|
||||
expect(mod.isOnMountPoint("/home/node/.mcp-inspector")).toBe(false);
|
||||
});
|
||||
|
||||
it("does not call a ramfs mount durable either", async () => {
|
||||
const mod = await loadWithMounts(
|
||||
"36 35 0:32 / /data rw,relatime - ramfs ramfs rw",
|
||||
["/data"],
|
||||
);
|
||||
expect(mod.isOnMountPoint("/data")).toBe(false);
|
||||
});
|
||||
|
||||
it("reads the fstype after the optional-fields separator, not by position", async () => {
|
||||
// mountinfo carries a variable number of optional fields before ` - `,
|
||||
// so the type cannot be indexed positionally. This line has two.
|
||||
const mod = await loadWithMounts(
|
||||
"36 35 0:32 / /data rw,relatime shared:1 master:2 - ext4 /dev/sda1 rw",
|
||||
["/data"],
|
||||
);
|
||||
expect(mod.isOnMountPoint("/data")).toBe(true);
|
||||
});
|
||||
|
||||
it("sees a directory that IS the mount point", async () => {
|
||||
const mod = await loadWithMounts(
|
||||
[line("/"), line("/home/node/.mcp-inspector")].join("\n"),
|
||||
|
|
|
|||
Loading…
Reference in a new issue