fix(auth): land the round-20 UI wiring that never applied, and nonce the staging name

Three of round 21's four findings say the same thing: **the UI half of round 20
was never applied, and I reported it as done.**

The edit script aborted partway on a regex error, after writing one file and
before writing the other two. So `ServerConfigModal` gained the footer prop
but `App.tsx` never passed it — the footer was `undefined` in production and
rendered nothing, visible only to the unit test that passes a descriptor
directly — the extra refresh call sites never landed, and the footer's doc
comment still claimed two secret-entry dialogs while the code had three.

`npm run ci` passed throughout, because every test I added exercises the piece
that *did* land. This is the same failure as round 17's uncommitted files with
a different cause: I verified intent (green gate, clean typecheck) instead of
the artifact. Each edit now asserts it changed the file, and the result is
grepped afterwards — the check costs a second and is the only one that
describes what is actually on disk.

Landed now, and verified in the files rather than inferred:

- `App.tsx` passes `secretStorage` to `ServerConfigModal`, so the disclosure
  the previous commit claimed to add actually renders.
- Every secret-bearing write refreshes the descriptor — config submit and both
  import flows, not just the two settings saves. Any of them can perform the
  pending plaintext-to-encrypted upgrade, after which the footer was reporting
  "Plaintext file" until reload.
- The footer's doc names all three dialogs. The wrong count there was not a
  description of the bug, it *was* the bug: the omitted dialog was the one
  taking secrets with no disclosure.

Also, the fourth finding: **the staging name was reusable across restarts.**
`recoverOrphanedSnapshots` deliberately leaves an orphan when a live
`secrets.json` also exists, and a pid-only name repeats (pid 1 on every
container start) — so the next claim would `rename` straight over that orphan
and permanently discard secrets it may uniquely hold. A per-attempt nonce
makes a staging path unrepeatable.

Full `npm run ci` green, staged tree verified first.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-22 22:30:21 -04:00
commit 8f14d371ad
3 changed files with 28 additions and 8 deletions

View file

@ -4591,21 +4591,31 @@ function App() {
initialConfig={configModalTarget?.config}
existingIds={existingIds}
onClose={() => setConfigModal(null)}
onSubmit={onConfigSubmit}
// Wrapped like the settings persists: this submit carries stdio `env`
// values, so it can perform the pending plaintext-to-encrypted
// upgrade and change the descriptor the footer reports.
onSubmit={refreshingPersist(onConfigSubmit, refreshInitialConfig)}
secretStorage={secretStorage}
/>
<ServerImportConfigModal
opened={importConfigOpen}
existingIds={existingIds}
onClose={() => setImportConfigOpen(false)}
onFetchSource={importSource}
onAddServer={addServerHighlighted}
onUpdateServer={updateServer}
onAddServer={refreshingPersist(
addServerHighlighted,
refreshInitialConfig,
)}
onUpdateServer={refreshingPersist(updateServer, refreshInitialConfig)}
/>
<ServerImportJsonModal
opened={importJsonOpen}
existingIds={existingIds}
onClose={() => setImportJsonOpen(false)}
onAddServer={addServerHighlighted}
onAddServer={refreshingPersist(
addServerHighlighted,
refreshInitialConfig,
)}
/>
<ServerSettingsModal
// Remount per open (and per target server) so the accordion resets to

View file

@ -226,9 +226,13 @@ function footerTooltip(info: SecretStorageInfo): string | undefined {
* The permanent "here is where your secrets go" strip at the bottom of the
* Client Settings and Server Settings modals (#1950).
*
* Those two dialogs are the only places the Inspector accepts a secret —
* an OAuth client secret, an enterprise IdP client secret, a stdio `env:`
* value — so they are where the answer belongs. The alternatives were
* It appears wherever the Inspector accepts a secret — Client Settings (the
* enterprise IdP client secret), Server Settings (the per-server OAuth
* client secret and stdio `env:` values), and Server Config (stdio `env:`
* values again). This comment named only the first two until review round
* 20, and the dialog it omitted was the one taking secrets with no
* disclosure at all, so the wrong count here was the bug rather than a
* description of it. The alternatives were
* considered and rejected: a startup banner is seen once, by whoever
* started the process, possibly hours before anyone types anything; a
* toast is seen once and then gone; a dismissible banner is, by design,

View file

@ -39,6 +39,7 @@
* stating that this directory is meant to survive.
*/
import { randomUUID } from "node:crypto";
import * as fsSync from "node:fs";
import * as fs from "node:fs/promises";
import * as path from "node:path";
@ -555,7 +556,12 @@ export async function absorbFileSecretsIntoKeyring(
// The staged name carries the pid so two Inspectors starting together
// cannot claim the same destination; whichever wins the rename does the
// migration and the loser sees ENOENT and returns.
const staged = `${filePath}.migrating-${process.pid}`;
// A per-attempt nonce, not just the pid. `recoverOrphanedSnapshots`
// deliberately leaves an orphan in place when a live `secrets.json` also
// exists — and a pid-only name is reusable across restarts (pid 1 on every
// container start), so the next claim would `rename` straight over that
// orphan and permanently discard secrets it may uniquely hold.
const staged = `${filePath}.migrating-${process.pid}-${randomUUID()}`;
try {
await fs.rename(filePath, staged);
} catch (err) {