test(e2e): PKCE-on-the-wire, per-user scoping, and desk clash coverage - #477
Conversation
Closes the last outstanding P0. login.spec.ts proves a login works and that the token is usable; this proves the handshake that produced it was safe, which is a different question. A client that leaked a secret or quietly dropped PKCE would still end up with a valid token and a working app, so nothing in the response tells you the exchange was sound. You have to look at what the browser sent. loginViaUI now records every /auth/* request it observes, and the new spec asserts S256 (never "plain"), a real base64url challenge, a code_verifier, the expected client_id, and no client_secret anywhere. Plus: the password never appears in a URL, and only ever reaches /auth/signin. Red-checked. These assertions previously lived in tasks/PPT-2536/, where nothing ran them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers WP-E2E-08 and AUTH-E2E-05. A second seeded user cannot see the booking in their listing, and cannot delete it; the owner still can. Includes a control asserting you can see your own booking, otherwise "nobody sees anything" would pass as success. This locks down something we learned the hard way: GET /bookings is scoped to the caller. An early leak check written as an admin reported zero bookings while the database plainly held one. It is both a privacy boundary and a trap for anyone writing tooling, and a regression would leak quietly rather than fail loudly. Red-checked: inverting the assertion shows the other user's listing really is empty while the booking exists. Also corrects two rows in the contract. Lockers and parking do NOT follow the desk metadata pattern as claimed - lockers come from banks then lockers within them, parking needs level zones tagged `parking` plus a separate spaces API. Both are more setup than desks, and the contract now says so rather than implying they are quick wins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Maps to a shipped fix, "Fix rejecting overlapping bookings on desk assignment" (2607.1). Double-booking is the kind of regression that doesn't announce itself: nothing errors, nobody notices, and two people turn up to the same desk on Tuesday. Attempted as a SECOND user deliberately, because that's the real scenario and because a clash check that only consulted your own bookings would still pass a single-user version of this. Covers the identical slot and a partial overlap, which is the case a naive check misses. Two controls, so the test can't pass for the wrong reason: a genuinely non-overlapping slot must still be accepted (otherwise a backend that rejected everything would look correct), and the desk must free up once the booking is deleted (a cancelled booking that still blocks the desk is harder to diagnose than a plain double-booking). Red-checked: the API really does return 409. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
MrYuion
left a comment
MrYuion
left a comment
There was a problem hiding this comment.
Review summary
This is valuable coverage, and the controls around the happy paths are thoughtful. I found two false-positive paths that weaken the guarantees these specs claim to provide, plus one cleanup issue that can poison retries.
Changes requested
-
P1 — Bind the PKCE challenge to its verifier
pkce.spec.ts:44–62The current assertions only establish that a syntactically plausible challenge and some verifier were sent. If the client sent unrelated values and the backend stopped enforcing PKCE, the exchange could return 200 and this test would still pass—the security regression this spec is intended to catch.
Please parse
code_verifierfromexchange.postData, calculate its SHA-256 base64url value, and compare it exactly withchallenge. An S256 challenge should also be exactly 43 base64url characters. -
P1 — Assert
409 Conflict, not every error response
desk-clash.spec.ts:100–112·desk-clash.spec.ts:144–145The workflow already documents intermittent
DB::ConnectionLost500s on booking POSTs. With>= 400, one of those infrastructure failures is treated as successful clash detection. The PR notes that the real behavior was red-checked as 409, so these assertions should usetoBe(409). -
P2 — Clean up unexpectedly accepted second-user bookings
desk-clash.spec.ts:98–125If either
exactorpartialreturns 2xx, its assertion throws before the returned booking ID is captured. Thefinallyblock deletes only the owner's first booking. Because booking listings are caller-scoped, the next attempt'sreleaseAsset(staffApi, ...)cannot see or delete the leaked second-user booking, so the retry fails during setup.Please retain IDs from any successful second-user responses and delete them through
otherbefore disposing the context.
The advisory E2E check is green, but these issues are specifically false-positive and failure-path problems, so a successful run does not exercise them.
PPT-2643 again. #478 fixed `newForm`'s deferred re-entry, but that branch is not the one the booking flows take. The current user is restored from the localStorage cache within about 50ms of bootstrap, whereas every flow calls its form lifecycle only after org data lands — `NewDeskFlowComponent.ngOnInit` awaits `waitUntilInitialised()` plus a 300ms settle, then calls `loadForm` and, for a fresh booking, `newForm`. So `currentUserIsLoaded()` is already true, the deferral never fires, and the captured-edits replay never runs. `loadForm` had no capture at all, and it is the first of the two resets. Its `model.set(...)` restores defaults — `all_day` false, a truthy `secondary_resource` that re-checks "Require locker" — over whatever the user typed into a form that has been interactive since first paint. Capture in `loadForm` too, and replay over the loaded booking before `applyDurationSettings` so a restored `all_day` still drives the time-sync window. The capture merges rather than replaces, because `form().reset()` clears the dirty flags the capture reads, so the `newForm` that follows in the same tick would otherwise overwrite a real capture with an empty one. The stash is released on a microtask, which is late enough for that chained reset and early enough that it cannot reach an unrelated form. Two specs, both seen red first, driving the ordinary path with no mocking and no runtime probe neutralised: input entered before initialisation survives `loadForm` + `newForm`, and it is not resurrected in a later form. Fixes PPT-2643 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Switching between booking forms without leaving the booking area does not reset the form, so edits captured for the initialisation replay follow the user across. That is a consequence of the fix worth stating rather than discovering later: only fields the user actually edited move, isCrossTypeEdit still discards the previous booking's identity, and leaving the section calls clearForm(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix(bookings): keep form input across the reset flows actually run (PPT-2643)
test(e2e): local-backend Playwright suite for workplace, with advisory CI
Rules out client disconnects as the trigger for the connection poisoning: aborting requests mid-flight strands nothing, while a burst with no aborts at all does. Kept next to the burst reproducer because a diagnostic that lives only in a ticket comment rots. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/placeos?upgradeToPro=build-rate-limit |
2 participants
Stacked on #476 — please review that one first, this branches off it.
Adds three specs to the workplace e2e suite. All P0s in the coverage contract are now done.
pkce.spec.tsplain), a real base64url challenge, acode_verifier, the expectedclient_id, noclient_secretanywhere, and that the password never appears in a URL. Ported fromtasks/PPT-2536/, where nothing ran it.booking-scoping.spec.tsGET /bookingsis caller-scoped, and an early leak check written as an admin reported zero bookings while the database held one.desk-clash.spec.ts2607.1. Attempted as a second user, since a check that only consulted your own bookings would pass a single-user version.13 specs, green locally and on the runner.
Every assertion is red-checked
Each was deliberately inverted and observed to fail against real data, because a test that has never been seen to fail is a guess rather than a guard:
409 Conflict[]while the booking existscode_challenge_method=S256and base64url challenge on the wireControls, so nothing passes for the wrong reason
Also corrects the contract
Lockers and parking were listed as following the desk metadata pattern. They don't. Lockers come from banks and then lockers within them; parking needs level zones tagged
parkingplus a separate spaces API. Both are materially more setup than desks, and the rows now say so rather than implying quick wins.🤖 Generated with Claude Code