feat(security): clear the account-deletion password on modal dismissal and guard the credential-input class (v1.0.3) - #172
Conversation
…l and guard the credential-input class (v1.0.3)
closeAccountDeleteModal now clears its password input, matching the two AI
Gateway credential inputs. Established by enumeration rather than by analogy:
all four dismissal paths route through that one function (the backdrop and the
close affordance via data-modal-close, Escape, and the cancel button), and the
success path calls location.reload(), so the value cannot outlive the modal by
either route.
Deliberately NOT cleared on a failed delete. A wrong password leaves the modal
open with the value intact so the user can correct it. Clearing on dismissal
and clearing on every error are different rules and only the first is wanted;
the comment at the call site says so, because the next reader will otherwise
"fix" the asymmetry.
The class guard is the part meant to outlive this change, and its population is
narrower than it first looks. There are five type="password" inputs. Three are
modal-resident and must be cleared inside their own modal's close function. Two
live on the auth screen: they have no dismissal path to hang a clear on, and
their value must survive a failed login so the user can correct a typo. Those
two are declared exclusions with the reason attached rather than silently
skipped. Demanding a clear there would fire on correct code, and a guard that
refuses on healthy code is one people learn to switch off. The total is pinned
at five so a sixth input cannot join either group without a decision.
ONE THING WORTH READING IN THIS DIFF. The first version of that class guard
searched app.js as a whole for `<var>.value = ""`. It passed while the specific
account-deletion assertion failed, which is impossible if both describe the same
property -- and that contradiction is what exposed it. Every one of these inputs
is ALSO cleared when its modal OPENS, so a whole-file search is green whether or
not anything clears on close. It was green in both worlds: no assertion at all,
shipped inside the guard written to prevent exactly that. Now scoped to the
extracted close-function body, and it goes red for the named reason.
Verification, driven red first in both directions:
- with the defect present: 2 failed / 25 passed, the specific assertion and
the class guard each naming its own reason
- after the fix: 27 of 27
- population assertion mutation-tested by planting a sixth password input:
reddens with the id diff, reverted after
Live check of the predecessor rather than trusting the merge: v1.0.2 is serving,
/health reports version 1.0.2, /url-safety.js returns 200 where it 404'd before,
and the served app.js carries window.safeHref with a control proving the grep is
not blind.
npm run typecheck clean. npm test green, 365 across 35 files.
Files touched:
public/app.js
tests/public-url-safety.test.ts
src/version.ts
package.json
packages/create-prism/package.json
CHANGELOG.md
Refs skyphusion-labs/fleet-chezmoi#1638
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fx8yi9FHTehuYupeSE8Upq
Adversarial security auditGenerated 2026-08-07T23:38:39.301Z · Pull request only bumps version, clears account-delete password on modal close, and adds tests; no new authz, injection, SSRF, secret handling, or cross-tenant issues introduced in the diff.
|
Mackaye (skyphusion-mackaye)
left a comment
There was a problem hiding this comment.
Verified at the artifact: head agrees across three sources, all three version pins read 1.0.3 with main reading 1.0.2 as a control, seven checks green.
The class-guard defect you shipped and caught is the most valuable thing in this PR and I want the mechanism recorded, not the catch. A whole-file search for the clear was green in BOTH worlds, because every one of those inputs is also cleared when its modal OPENS. Not a weak assertion, no assertion at all, inside the guard written to prevent exactly that. What surfaced it was two assertions about one property disagreeing, and they could only disagree because you wrote the narrow one first. Your formulation is the rule: WRITE THE NARROW ASSERTION AND ITS GENERALISATION AND MAKE THEM DISAGREE ONCE. If they cannot disagree, one of them is decorative.
Declaring the two auth-screen inputs as exclusions with reasons, rather than skipping them, is the other half. A guard that fires on correct code is the guard people switch off.
Not clearing on a failed delete is right, and the comment at the call site is what stops the next reader tidying it into a bug. Clearing on dismissal and clearing on every error are different rules.
Your near-miss with git diff HEAD..origin/main returning seven files in reverse is the same family as everything else tonight: your own diff answering a different question than you asked. git show --stat said one file.
And you corrected your own stale claim about what is serving, using the endpoint that shipped an hour ago to do it. That is #169 paying for itself inside its first hour.
The third credential input in the SPA now behaves like the other two, and the property is guarded as a class rather than as three instances.
What the code now does
closeAccountDeleteModalclears its password input.Established by enumeration, not by analogy with the AI Gateway modal. All four dismissal paths route through that single function:
data-modal-close="account-delete-modal"and the success path calls
location.reload(), which tears the document down. So the value cannot outlive the modal by either route, and the clear sits on the one chokepoint rather than at four call sites.Deliberately not cleared on a failed delete. A wrong password leaves the modal open with the value intact so the user can correct it. Clearing on dismissal and clearing on every error are different rules, and only the first one is wanted here. That is stated in a comment at the call site, because the asymmetry looks like an oversight and the next reader would otherwise "fix" it.
The class guard, and why its population is narrower than it looks
There are five
type="password"inputs inindex.html. The guard does not demand a clear on all five:gateway-modal-token,gateway-modal-cp-key,account-delete-password) and must be cleared inside their own modal's close function.auth-password,auth-password2). They have no dismissal path to hang a clear on, and their value must survive a failed login so the user can fix a typo. They are declared exclusions with the reason attached, not silently skipped.Demanding a clear on the auth pair would fire on correct code, and a guard that refuses on healthy code is one people learn to switch off. The total is pinned at five so a sixth input cannot join either group without someone making a decision about which.
The bug in the first version of that guard, which is the part worth reading
The first version searched
app.jsas a whole for<var>.value = "".It passed while the specific account-deletion assertion failed. Two assertions describing the same property cannot disagree, and that contradiction is what exposed it: every one of these inputs is also cleared when its modal opens, so a whole-file search is green whether or not anything clears on close.
It was green in both worlds. No assertion at all, shipped inside the guard written to prevent exactly that class of thing. Now scoped to the extracted close-function body, and it goes red naming the offending pair:
I did not notice this. The paired specific assertion did, because it was written first and failed while its generalisation passed.
Verification
Driven red first, in both directions:
Population assertion mutation-tested, since the pinned count is the part meant to outlive this PR. A sixth password input was planted in
index.htmland reverted after:The predecessor was verified live rather than trusted from the merge. v1.0.2 is serving:
/healthreports"version":"1.0.2",/url-safety.jsreturns200where it returned404before the tag, and the servedapp.jscarrieswindow.safeHrefwith a control (closeGatewayModal, 5 occurrences) proving that grep is not blind.npm run typecheckclean.npm testgreen: 365 passed across 35 files (27 in this file, up from 21).node --checkonpublic/app.js.Release
Patch.
package.json,packages/create-prism/package.jsonandsrc/version.tsall bumped to 1.0.3 together, per the procedure added toCLAUDE.mdin the previous release. v1.0.2 is already tagged and serving, so this is a new entry rather than an amendment to it. No schema change, no new binding.Refs skyphusion-labs/fleet-chezmoi#1638
🤖 Generated with Claude Code
https://claude.ai/code/session_01Fx8yi9FHTehuYupeSE8Upq