fix(auth): address Copilot review round 7 (five of six findings)
Five taken; one rejected with evidence, below. - **The keychain hand-off ran outside the file lock.** Read → copy → delete has to be one transaction: another Inspector completing a `set` between the read and the delete had its brand-new secret removed without ever being copied — a write that reported success and then vanished, which is the loss the lock exists to prevent, reached through the migration meant to preserve things. The whole sequence now runs under the same cross-process lock every mutation takes. - **An unmigratable entry was skipped and the file deleted anyway.** A key that is not `serverId:field` cannot be addressed through the store API, so it cannot be copied — which makes the hand-off incomplete by definition, and the complete-success rule then forbids removing the source. It was discarding that value permanently in order to tidy up a migration that had failed. The copyable entries still go across; the file stays, and says why. - **`readOnDiskEncryption` accepted any format version.** A version-2 file was reported as a usable plaintext or encrypted store even though `readMap` refuses it and every save fails. Checked before the mode now, matching `readMap`. - **The loose-mode warning at selection was encryption-blind**, claiming a reader could read the secrets of a file that is ciphertext, and it duplicated the caveat printed moments later by `warnAboutSecretStorage`. Selection now only repairs; the descriptor path does the reporting, because it is the one that knows whether the file is encrypted. - **The footer's tooltip offered to copy the path without showing it.** The band stopped printing it and the accessible name carries it for screen-reader users, which left a sighted user unable to see where a custom destination points without pasting it somewhere else. **Not taken: "`writeStoreFile` is not atomic."** It is. The finding reads `store-io.ts` line 84 as a direct write, but `writeFile` there is imported from the `atomically` package, not `node:fs`. Verified rather than argued: writing over an existing file changes its inode (so a rename happened) and leaves mode `0600`. I had started implementing the suggested temp+rename before checking the import, which would have replaced a tested library with a hand-rolled duplicate. That check corrected something of mine, though: `tightenSecretFilePermissions` justified itself with "a rename over a 0644 file keeps the loose mode forever", which is false — the temp carries `mode: 0o600` and the rename replaces the inode, so every *write* re-establishes it. The function is still needed for the case it actually addresses: a file that exists and is never written. Comment corrected to say so. Full `npm run ci` green — 5,753 tests under the coverage gate. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
fdeffa1f00
commit
6efc65d2af
2 changed files with 160 additions and 17 deletions
|
|
@ -173,4 +173,28 @@ describe("SecretStorageFooter", () => {
|
|||
expect(band).toHaveTextContent("Mode 0644 — not owner-only.");
|
||||
expect(band).toHaveAttribute("data-tone", "warn");
|
||||
});
|
||||
|
||||
it("puts the path in the tooltip, not just an offer to copy it", async () => {
|
||||
// The band no longer prints the path and the accessible name carries it
|
||||
// for screen-reader users, which left a sighted user unable to see where
|
||||
// a custom destination points without copying it out somewhere else.
|
||||
renderWithMantine(
|
||||
<SecretStorageFooter
|
||||
info={{
|
||||
kind: "file",
|
||||
reason: "fallback",
|
||||
durable: true,
|
||||
plaintext: false,
|
||||
path: "/custom/place/secrets.json",
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
// Hover the control the tooltip is attached to, not the band around it.
|
||||
await userEvent.hover(
|
||||
screen.getByRole("button", { name: /Copy secrets file path/ }),
|
||||
);
|
||||
const tip = await screen.findByRole("tooltip");
|
||||
expect(tip).toHaveTextContent("/custom/place/secrets.json");
|
||||
expect(tip).toHaveTextContent("Click to copy.");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -206,7 +206,12 @@ function footerTooltip(info: SecretStorageInfo): string | undefined {
|
|||
`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");
|
||||
// The path itself, not just an offer to copy it. The band no longer prints
|
||||
// it, and the accessible name carries it for screen-reader users — which
|
||||
// left a sighted user unable to see *where* a custom destination points
|
||||
// without copying it out to somewhere else first.
|
||||
parts.push(info.path);
|
||||
parts.push("Click to copy.");
|
||||
return parts.join(" ");
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue