d69091d5adccc085db0997f2b9b02a3af4a0283c
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 synAdminandClaude Opus 5
|
36dbf18916 |
feat(passkeys): a password reset takes the passkeys with it (#42)
The last issue in the passkeys project, and the only one that adds no capability. It is the safety net, and it exists because this is the piece most likely to be skipped and most expensive to discover missing. A reset is the recovery path, and recovery has to be complete. The reset already deletes every session on the account, on the reasoning that a reset prompted by a compromise must not leave an intruder signed in for the remaining thirty days of their cookie. A passkey an intruder registered has no expiry at all. Leaving those behind would mean a customer can recover their password and still not have their account back. The obvious objection is that this hands whoever controls the mailbox a way to strip a customer's passkeys. It does, and it costs nothing: anyone who can complete a reset already controls the email address and therefore already controls the account. The passkeys were not protecting anything by that point. Anything in flight goes too. An intruder who pressed add a passkey moments before the reset could otherwise finish that ceremony afterwards and put a credential straight back onto the account the reset had just cleared. Changing a password deliberately does not do this, and the asymmetry is the point. A change requires the current password from someone already signed in, so nothing about it suggests a lockout or a compromise, and it already spares the current session for the same reason. A customer who suspects one particular device revokes that device by name from the account page, which is a better tool than deleting everything. A reset has no idea which credential is the problem, so it takes all of them. The customer is told twice. Before, in the reset email and on the reset form, unconditionally — that form has no session and is never told whether the account has passkeys, because answering that would make the reset page an oracle for it, so the wording has to read the same to someone who has none. After, with a count, and only when the count is not zero. That moment is the only one where the count can be reported: the rows are gone by the time anyone could go and look. A customer told two were removed who only remembers registering one has just learned something they could not otherwise find out. That notice is a panel that waits to be dismissed rather than a toast, because a toast dismisses itself and this is the message a customer needs to still be looking at while they decide what to do about it. The other question this issue asks — whether a reset ends a session established by a passkey — turns out to need no code, because #39 made both paths call one createSession. But "it falls out for free" is a claim, so there is now a test that establishes a session through that exact function and watches the reset end it. docs/ops/account-recovery.md records the whole policy, including the two answers that are not code. Losing an authenticator is not a lockout: the customer signs in with their password and revokes the lost credential themselves, which is why this change implements nothing for it. Losing the email address is a lockout, and there is deliberately no self-service route out — this shop holds no second proof of identity, and anything invented to fill that gap would be a weaker credential than the one it replaced. The manual route runs through the shop owner verifying against order history, and its third step, changing the address from the admin screen, does not exist yet. That is written down as a gap with its own notification and audit questions rather than smuggled in here. Verified: backend tsc clean for src and tests, 521 unit tests pass, lint clean apart from warnings that predate this branch; frontend tsc, lint and build clean. The integration and end-to-end suites need a database this machine has no Docker for, so CI is what proves those. Closes #42 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
 bermudalamb
|
5c907fcf9a |
test(e2e): add page objects, fixtures and a typed test build (#137)
Foundation only. No spec is converted in this commit, so the suite behaves exactly as before — the conversions follow in themed batches, each leaving the suite green. The suite had grown by copy-paste. Registering a customer was implemented nine times, as `register` in five files and `registerCustomer` in four more, each carrying its own re-explanation of the same bcrypt wait and the same "the header, not the URL, proves the session exists" reasoning. `uniqueEmail` was reinvented per file with a different prefix and a different encoding each time. Thirteen locators reached into antd's internals — `.ant-tabs-tab-active .ant-tabs-tab-btn`, `.ant-select-item-option[title=...]`, `.ant-col` — spread across seven files, so an antd upgrade breaks tests that have nothing to do with it. Page objects hold named locators and the actions that operate on them; assertions stay in the specs, so a test reads as its own statement of what it verifies. The exception is an action waiting for its own completion — registering waits for the account button, opening an admin tab waits for its panel — because that wait is the action's contract, and pushing it to callers would recreate the duplication being removed. Fixtures carry the setup rather than the specs. `customer` registers through the API rather than the form: nine specs drove the registration form purely to arrive at a signed-in session, so a broken form failed a hundred tests that were not about it, and each paid for a bcrypt round-trip through the UI. `page.request` shares the browser context's cookie jar, so the session belongs to `page`. The specs that are genuinely about registration drive the form properly through `authModal`. `adminApi` takes its base URL from the Playwright config. One spec built its own request context against a hardcoded http://localhost:5173, so changing the port in the config would have moved every test except that one. The inline pg.Client in password-reset.spec.ts moves to support/db.ts. The reasoning for reading the database directly is unchanged and still right — an endpoint that returns a reset token for an arbitrary address is account takeover if it is ever reachable, and an environment gate is a thin thing to stand between that and production — but it no longer sits in a spec where it can be copied into the next one wanting a shortcut. Its default port becomes 55500, the local stack's, rather than 55432: that is the integration suite's disposable Postgres, a different database with different credentials that the app under test is not connected to, and it is Hyper-V-reserved on at least one machine here, so the spec failed with a bare ECONNREFUSED naming a port nobody had chosen. tsconfig.test.json type-checks the tree and runs as part of `npm run build`. It is separate from tsconfig.json rather than widening its `include`, because scripts/check-sonar-tsconfig.js compares the two configs' include arrays, and pulling the Playwright suite into SonarQube's analysis program is a different decision from type-checking it. The whole existing suite type-checks clean on the first run. Lint now covers tests/ with `project` rather than `projectService` — the service resolves a file to the nearest tsconfig.json, which for tests/ is the one that excludes them, and every file then errors as not part of a project. no-floating-promises is an error here: Playwright's API is almost entirely promises, and a missing await on an assertion does not fail, it passes having asserted nothing. Four rule families are switched off for tests rather than left as warnings. Bringing these files in scope added 45, of which none were defects, and #60's argument is that a gate nobody reads is not a gate. There is no React in this directory, and the hooks rules fire on ordinary functions whose parameter is named `use` — which Playwright fixtures are, by its own API. Test credentials are the point of a test and the project's own rule is that they live only in test paths, which is here. Math.random builds unique fixture names so parallel workers do not collide, and a cryptographic generator would say something untrue about what the value is for. The count is back to the 30 that src carried before. Refs #137 |