fix(auth): address Copilot review round 14
One inline and three of the four suppressed. Two of the four are cases where
an earlier fix of mine was applied to one branch and not its twin.
**The footer went stale after the write that changed it.** `useInitialConfig`
fetched once, and `secretStorage` is the one field it carries that describes
state *this app mutates*: the first save under a newly-set passphrase
re-encrypts a pre-existing plaintext file. The backend re-derives the
descriptor per request — deliberately, and there is a test for that — but the
client had no way to ask again, so the footer said "Plaintext file" for the
rest of the session about a file that no longer was. Exactly the misreport
this band exists to prevent, arrived at from the client side.
The hook now exposes `refresh()`, and both persist paths that can carry a
secret call it: server settings (OAuth client secret, stdio `env:` values)
and client settings (the enterprise IdP client secret). The refresh tracks
its own liveness through a ref, since unlike the mount fetch it has no effect
teardown to cancel against.
**The plaintext branch skipped the payload validation the encrypted branch
does.** Round 12 tightened the encrypted envelope check and left this one
behind, so `{ "encryption": "none", "secrets": [] }` — trivial to hand-write —
described a healthy plaintext store whose next save `readMap` refuses. Both
branches validate now.
**`secretStoreGetStrict`'s doc contradicted the code it was written for.** It
said falling back to `get` is right "for the in-memory and file stores —
neither can fail in a way that masquerades as absence". `FileSecretStore.get`
does exactly that masquerade, which is why round 9 gave that store its own
`getStrict`. The fallback stays (the seam is optional and doubles must
compile) but the comment no longer claims it is equivalent for a store that
can fail.
**The spec documented the performance bug rather than the fix.** It still said
`getMany` decrypts once per server, which was the shape round 13 replaced —
and describing the earlier bug as the design is worse than saying nothing.
The remaining suppressed comment is the hand-off's TOCTOU delete: the
cross-process guarantee, decided against for this PR and tracked in #2082.
Full `npm run ci` green.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
26d2f58784
commit
0c52172379
2 changed files with 152 additions and 9 deletions
|
|
@ -356,7 +356,23 @@ export class FileSecretStore implements SecretStore {
|
|||
detail: `format version ${String(parsed.version)}, expected ${FORMAT_VERSION}`,
|
||||
};
|
||||
}
|
||||
if (parsed.encryption === "none") return { state: "plaintext" };
|
||||
if (parsed.encryption === "none") {
|
||||
// Symmetry with the encrypted branch: a `secrets` that is not a
|
||||
// string map is refused by `readMap`, so returning "plaintext" here
|
||||
// describes a healthy store whose next save is guaranteed to fail —
|
||||
// `{ "encryption": "none", "secrets": [] }` being the easy case to
|
||||
// hand-write. The encrypted branch was tightened in round 12 and this
|
||||
// one was left behind.
|
||||
try {
|
||||
asSecretMap(parsed.secrets ?? {}, this.filePath);
|
||||
} catch (err) {
|
||||
return {
|
||||
state: "unreadable",
|
||||
detail: err instanceof Error ? err.message : String(err),
|
||||
};
|
||||
}
|
||||
return { state: "plaintext" };
|
||||
}
|
||||
if (parsed.encryption === CIPHER) {
|
||||
// Naming the cipher is not the same as being openable, and a partial
|
||||
// check is its own trap: three dot-separated strings satisfy
|
||||
|
|
|
|||
|
|
@ -540,9 +540,16 @@ export class KeyringSecretStore implements SecretStore {
|
|||
*/
|
||||
/**
|
||||
* Read `store` intolerantly: throw rather than answer `null` when the store
|
||||
* itself cannot be read. Falls back to `get` for a store that does not
|
||||
* distinguish the two, which is right for the in-memory and file stores —
|
||||
* neither can fail in a way that masquerades as absence.
|
||||
* itself cannot be read.
|
||||
*
|
||||
* Falls back to `get` only for stores that genuinely cannot fail that way —
|
||||
* in practice `InMemorySecretStore`, whose reads are a `Map` lookup. It is
|
||||
* **not** a safe fallback for the file store: `FileSecretStore.get`
|
||||
* deliberately converts filesystem, format and key failures to `null`, which
|
||||
* is exactly the masquerade this helper exists to prevent, and is why that
|
||||
* store implements `getStrict` itself. The fallback stays because the seam is
|
||||
* optional and third-party/test doubles must keep compiling — not because
|
||||
* `get` is equivalent for anything that can fail.
|
||||
*/
|
||||
export async function secretStoreGetStrict(
|
||||
store: SecretStore,
|
||||
|
|
|
|||
Loading…
Reference in a new issue