From 277a28083a42e38736d09d7c0df9d845ad387bf5 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Mon, 3 Aug 2026 18:05:22 +1000 Subject: [PATCH 1/4] test(e2e): assert the PKCE handshake on the wire (AUTH-E2E-01) 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) --- E2E_USER_STORIES.md | 2 +- apps/workplace/e2e/local/pkce.spec.ts | 94 +++++++++++++++++++++++++++ e2e/support/login.ts | 31 ++++++++- 3 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 apps/workplace/e2e/local/pkce.spec.ts diff --git a/E2E_USER_STORIES.md b/E2E_USER_STORIES.md index f372908bb4..e5a5761abe 100644 --- a/E2E_USER_STORIES.md +++ b/E2E_USER_STORIES.md @@ -133,7 +133,7 @@ environment, data or real-client gap that unit specs could not see. | ID | P | Story | Status | |----|---|-------|--------| -| AUTH-E2E-01 | P0 | Authorization-code + PKCE exchange in a real browser: no `client_secret` anywhere, `S256` challenge, token is a JWT. | **partial** — `login.spec.ts` asserts the exchange and token shape; the explicit no-secret / challenge-recomputation assertions still live in `tasks/PPT-2536/e2e/backoffice-login.spec.js` and should move here. | +| AUTH-E2E-01 | P0 | Authorization-code + PKCE exchange in a real browser: no `client_secret` anywhere, `S256` challenge, token is a JWT. | **done** — `local/pkce.spec.ts`. Asserts on the wire, not the response: a client that leaked a secret or dropped PKCE would still return a valid-looking token, so the response cannot tell you the handshake was sound. Also checks the password never appears in a URL and only ever reaches `/auth/signin`. Ported from `tasks/PPT-2536/e2e/backoffice-login.spec.js`. | | AUTH-E2E-02 | P0 | A refreshed token keeps its scope, is rotated, preserves `sub`, and is still accepted by rest-api. | **done** — `login.spec.ts`. This is the exact 2026-07-23 revert (403 on `/oauth_apps` after refresh). | | AUTH-E2E-03 | P1 | A refresh chain survives N sequential refreshes without degrading scope or access. | todo — covered API-only by `tasks/PPT-2536/integration/` (RF-03); wanted in-browser. | | AUTH-E2E-04 | P1 | A stale/incompatible session cookie from a previous auth implementation does not break sign-in. | todo — verified manually (SC-01); needs automating. | diff --git a/apps/workplace/e2e/local/pkce.spec.ts b/apps/workplace/e2e/local/pkce.spec.ts new file mode 100644 index 0000000000..467a2808f0 --- /dev/null +++ b/apps/workplace/e2e/local/pkce.spec.ts @@ -0,0 +1,94 @@ +/** + * AUTH-E2E-01 — the PKCE handshake, asserted on the wire. + * + * `login.spec.ts` proves a login works and that the resulting token is usable. + * This proves the handshake that produced it was actually safe, which is a + * different question: a client that leaked a secret, or quietly dropped PKCE, + * would still end up with a perfectly valid token and a perfectly working app. + * Nothing about the response tells you the exchange was sound. You have to look + * at what the browser sent. + * + * These assertions previously lived in tasks/PPT-2536/e2e/backoffice-login.spec.js, + * written during the auth.cr migration. They belong with the suite rather than in + * a task folder, where nothing runs them. + */ +import { test, expect } from '../../../../e2e/support/fixtures'; +import { loginViaUI } from '../../../../e2e/support/login'; +import { APP_URL, roleFor } from '../../../../e2e/support/env'; +import { clientId, redirectUriFor } from '../../../../e2e/support/auth'; + +// The subject is the handshake itself, so start with no credentials. +test.use({ storageState: undefined }); + +test.describe('PKCE handshake', () => { + test('the browser performs a public-client PKCE exchange with no secret', async ({ page }) => { + const { requests, token } = await loginViaUI(page, roleFor('admin')); + + const authorize = requests.find((r) => r.url.includes('/oauth/authorize')); + const exchange = requests.find( + (r) => r.url.includes('/oauth/token') && !r.url.includes('refresh_token'), + ); + + expect(authorize, 'an /oauth/authorize request should have been made').toBeTruthy(); + expect(exchange, 'an /oauth/token request should have been made').toBeTruthy(); + + // --- the authorize leg ------------------------------------------------- + // 302 is the redirect back to the app carrying ?code=. A 200 here would + // mean we were served a page instead, i.e. the handshake never completed. + expect(authorize!.status, 'authorize redirects back with a code').toBe(302); + + expect(authorize!.url, 'PKCE must use S256, never "plain"').toContain( + 'code_challenge_method=S256', + ); + + const challenge = new URL(authorize!.url).searchParams.get('code_challenge') ?? ''; + expect(challenge.length, 'a real code_challenge was sent').toBeGreaterThan(20); + // base64url, so it must not contain the standard-base64-only characters. + expect(challenge, 'the challenge is base64url encoded').not.toMatch(/[+/=]/); + + // --- the token leg ----------------------------------------------------- + expect(exchange!.status, 'token exchange succeeds').toBe(200); + + // The whole point of a public client: the browser holds no secret, so it + // must never send one. If this ever fails, a credential is sitting in a + // shipped bundle where anyone can read it. + const wire = `${exchange!.url} ${exchange!.postData ?? ''}`; + expect(wire, 'no client_secret anywhere in the token request').not.toContain( + 'client_secret', + ); + + // The verifier is what proves this client started the flow. Without it, + // an intercepted code could be redeemed by anyone. + expect(wire, 'the code_verifier was sent').toContain('code_verifier'); + + // And it is the client we registered, not something else. + expect(wire, 'the expected client_id was used').toContain( + clientId(redirectUriFor(APP_URL)), + ); + + expect(token.access_token, 'the exchange produced a token').toBeTruthy(); + }); + + test('no credentials are exposed in any /auth request', async ({ page }) => { + const { requests } = await loginViaUI(page, roleFor('admin')); + const password = roleFor('admin').password; + + // The password legitimately appears once, in the sign-in POST body. It must + // not turn up anywhere else, and above all never in a URL, where it would be + // captured by browser history, proxies and server logs. + for (const r of requests) { + expect(r.url, `password must never appear in a URL (${r.method} ${r.url})`).not.toContain( + password, + ); + expect(r.url, `no client_secret in a URL (${r.method} ${r.url})`).not.toContain( + 'client_secret', + ); + } + + const bodies_with_password = requests.filter((r) => (r.postData ?? '').includes(password)); + expect( + bodies_with_password.every((r) => r.url.includes('/auth/signin')), + 'the password should only ever be sent to /auth/signin', + ).toBeTruthy(); + }); +}); diff --git a/e2e/support/login.ts b/e2e/support/login.ts index e6f3a35bf9..82449ee9d5 100644 --- a/e2e/support/login.ts +++ b/e2e/support/login.ts @@ -17,9 +17,26 @@ import { Page, expect } from '@playwright/test'; import { APP_URL, Role } from './env'; +/** One request the browser made to /auth/*, as observed on the wire. */ +export interface AuthRequest { + url: string; + method: string; + status: number; + postData: string | null; +} + export interface LoginResult { /** The token-endpoint response the browser actually received. */ token: { access_token?: string; refresh_token?: string; scope?: string; expires_in?: number }; + /** + * Every `/auth/*` request the browser made during the login, in order. + * + * Captured so specs can assert on what was actually sent rather than on what + * the SDK claims it sent. The distinction matters: a client that leaks a + * secret, or silently drops PKCE, still returns a perfectly valid-looking + * token, so the response alone cannot tell you the handshake was safe. + */ + requests: AuthRequest[]; } /** @@ -36,8 +53,18 @@ export interface LoginResult { */ export async function loginViaUI(page: Page, role: Role): Promise { let token: LoginResult['token'] = {}; + const requests: AuthRequest[] = []; page.on('response', async (res) => { - if (res.url().includes('/oauth/token') && res.status() === 200) { + const url = res.url(); + if (!url.includes('/auth/')) return; + const req = res.request(); + requests.push({ + url, + method: req.method(), + status: res.status(), + postData: req.postData(), + }); + if (url.includes('/oauth/token') && res.status() === 200) { try { token = await res.json(); } catch { @@ -64,7 +91,7 @@ export async function loginViaUI(page: Page, role: Role): Promise { await page.waitForURL(new RegExp(escapeRe(new URL(APP_URL).host)), { timeout: 30_000 }); await expect(page.locator('topbar')).toBeVisible({ timeout: 30_000 }); - return { token }; + return { token, requests }; } function escapeRe(s: string): string { From 46eb63047d86c331364efb2863f98c696efc296a Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Mon, 3 Aug 2026 18:11:14 +1000 Subject: [PATCH 2/4] test(e2e): one user's bookings are not another user's business 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) --- E2E_USER_STORIES.md | 8 +- .../e2e/local/booking-scoping.spec.ts | 173 ++++++++++++++++++ 2 files changed, 177 insertions(+), 4 deletions(-) create mode 100644 apps/workplace/e2e/local/booking-scoping.spec.ts diff --git a/E2E_USER_STORIES.md b/E2E_USER_STORIES.md index e5a5761abe..d2f0b25af7 100644 --- a/E2E_USER_STORIES.md +++ b/E2E_USER_STORIES.md @@ -116,9 +116,9 @@ the PR gate. | WP-E2E-06 | P1 | A deleted booking disappears from the listing (teardown really tears down). | **done** — `local/desk-booking.spec.ts` | | WP-E2E-03 | P1 | Building/level selectors are populated from seeded zones, and changing them re-scopes what is bookable. | todo | | WP-E2E-07 | P1 | "Your bookings" lists the user's own booking; cancelling it moves it out of the upcoming list. | todo | -| WP-E2E-08 | P1 | A booking made by one user is **not** visible in another user's "your bookings" (per-user scoping). | todo — see AUTH-E2E-05 | -| WP-E2E-09 | P1 | Booking a **locker** end to end. Same metadata + per-worker-asset + sweep pattern as desks. | todo | -| WP-E2E-10 | P1 | Booking a **parking** space end to end. | todo | +| WP-E2E-08 | P1 | A booking made by one user is **not** visible in another user's listing, and cannot be deleted by them. | **done** — `local/booking-scoping.spec.ts`. Red-checked: the other user's listing really is empty while the booking exists. | +| WP-E2E-09 | P1 | Booking a **locker** end to end. | todo — **more setup than desks**, not the same pattern. Lockers come from locker *banks* then lockers within them (`loadLockerResources`), so seeding is two-level. Budget accordingly. | +| WP-E2E-10 | P1 | Booking a **parking** space end to end. | todo — **more setup than desks**. Needs a level zone tagged `parking` plus spaces created through the parking API (`queryParkingSpacesForZones`), not Zone metadata. | | WP-E2E-11 | P2 | Inviting a **visitor** end to end. | todo | | WP-E2E-12 | P2 | Directory / colleagues search returns seeded users. | todo | | WP-E2E-13 | P2 | The explore/map view renders for a seeded level and reflects availability. | todo — needs map metadata seeded | @@ -137,7 +137,7 @@ environment, data or real-client gap that unit specs could not see. | AUTH-E2E-02 | P0 | A refreshed token keeps its scope, is rotated, preserves `sub`, and is still accepted by rest-api. | **done** — `login.spec.ts`. This is the exact 2026-07-23 revert (403 on `/oauth_apps` after refresh). | | AUTH-E2E-03 | P1 | A refresh chain survives N sequential refreshes without degrading scope or access. | todo — covered API-only by `tasks/PPT-2536/integration/` (RF-03); wanted in-browser. | | AUTH-E2E-04 | P1 | A stale/incompatible session cookie from a previous auth implementation does not break sign-in. | todo — verified manually (SC-01); needs automating. | -| AUTH-E2E-05 | P1 | A non-admin cannot read or mutate another user's bookings; an admin's own listing does not leak others'. | todo — **and it matters**: `GET /bookings` is caller-scoped, which we only learned by getting a leak check wrong. | +| AUTH-E2E-05 | P1 | A non-admin cannot read or mutate another user's bookings. | **done** — `local/booking-scoping.spec.ts`, same spec as WP-E2E-08. Covers both halves: the listing excludes it, and a delete attempt is rejected. Includes a control asserting you *can* see your own, so "nobody sees anything" can't pass as success. | | AUTH-E2E-06 | P2 | Token expiry mid-session recovers without stranding the SPA. | todo | | AUTH-E2E-07 | P2 | Malformed and hostile `/auth/*` requests return 4xx, never 5xx and never a backtrace. | todo — covered by auth.cr unit specs (SEC-01); browser-level coverage optional. | | AUTH-E2E-08 | P1 | `SameSite` behaviour in a genuine third-party/iframe context. | **blocked** — Playwright Chromium cannot create a true third-party context. Known untested incident class (B.7). | diff --git a/apps/workplace/e2e/local/booking-scoping.spec.ts b/apps/workplace/e2e/local/booking-scoping.spec.ts new file mode 100644 index 0000000000..6d3256138e --- /dev/null +++ b/apps/workplace/e2e/local/booking-scoping.spec.ts @@ -0,0 +1,173 @@ +/** + * WP-E2E-08 / AUTH-E2E-05 — one user's bookings are not another user's business. + * + * This locks down a property we learned about the hard way. `GET /bookings` is + * scoped to the caller, which is easy to assume but was not obvious: an early + * leak-check written as an admin reported zero bookings while the database + * plainly held one, because the admin was only ever being shown their own. + * + * That behaviour is load-bearing in two directions. It is a privacy boundary + * (your colleagues cannot enumerate where you sit), and it is a trap for anyone + * writing tooling against the API. Worth a test either way, because a regression + * here would leak quietly rather than fail loudly. + */ +import { request } from '@playwright/test'; +import { test, expect } from '../../../../e2e/support/fixtures'; +import { BACKEND_URL, WORKERS, roleFor } from '../../../../e2e/support/env'; +import { mintToken } from '../../../../e2e/support/auth'; +import { APP_URL } from '../../../../e2e/support/env'; +import { + STAFF_API, + deleteBooking, + listBookings, + releaseAsset, + uniqueTitle, + zonesWithTag, +} from '../../../../e2e/support/api'; +import type { APIRequestContext } from '@playwright/test'; +import { deskFor } from '../../../../e2e/support/env'; + + +/** + * The org/building/level zone ids a booking carries, matching what the UI sends. + * Always queried by tag: `GET /zones` with no `tags` parameter comes back empty. + */ +async function bookingZones(api: APIRequestContext): Promise { + const groups = await Promise.all( + ['org', 'building', 'level'].map((tag) => zonesWithTag(api, tag)), + ); + return groups.flat().map((z) => z.id); +} + +const DAY = 86_400; +// Window comfortably wider than any booking these specs create. Keep it that +// way: a booking placed exactly on the boundary is not returned by the listing, +// which reads as "scoping is broken" rather than "the window was too tight". +const from = () => Math.floor(Date.now() / 1000) - 3 * DAY; +const to = () => Math.floor(Date.now() / 1000) + 3 * DAY; + +test.describe('booking visibility between users', () => { + test('another user cannot see or delete your booking', async ({ staffApi }, testInfo) => { + const mine = testInfo.parallelIndex; + // A genuinely different seeded user. With one worker there is nobody else + // to compare against, so the spec would be meaningless. + const theirs = (mine + 1) % WORKERS; + test.skip(theirs === mine, 'needs at least two workers to have two distinct users'); + + const desk = deskFor(mine); + const title = uniqueTitle('E2E Scoping'); + await releaseAsset(staffApi, 'desk', desk.id, from(), to()); + + // Create as *this* worker's user, through the API rather than the UI — + // the subject here is authorisation, not the booking form. + const me = await (await staffApi.get('/api/engine/v2/users/current')).json(); + const zones = await bookingZones(staffApi); + const start = Math.floor(Date.now() / 1000) + DAY; + + const created = await staffApi.post(`${STAFF_API}/bookings`, { + data: { + booking_type: 'desk', + asset_id: desk.id, + booking_start: start, + booking_end: start + 3600, + timezone: 'Etc/UTC', + user_email: me.email, + user_id: me.id, + user_name: me.name, + title, + zones, + }, + }); + expect(created.ok(), `creating the booking should succeed: ${created.status()}`).toBeTruthy(); + const booking = await created.json(); + + // A second, genuinely different user. + const other_role = roleFor('staff', theirs); + const other_mint = await mintToken( + BACKEND_URL, + APP_URL, + other_role.email, + other_role.password, + ); + const other = await request.newContext({ + baseURL: BACKEND_URL, + ignoreHTTPSErrors: true, + extraHTTPHeaders: { Authorization: `Bearer ${other_mint.accessToken}` }, + }); + + try { + // Sanity: the two identities really are different, or everything below + // would pass for the wrong reason. + const them = await (await other.get('/api/engine/v2/users/current')).json(); + expect(them.email, 'the second user must be a different person').not.toBe(me.email); + + // The privacy boundary. + const their_view = await listBookings(other, 'desk', from(), to()); + expect( + their_view.map((b) => b.id), + "another user's default listing must not include your booking", + ).not.toContain(booking.id); + + // And they cannot remove it. A 2xx here would mean anyone can cancel + // anyone's desk, which is worse than merely being able to see it. + const their_delete = await other.delete(`${STAFF_API}/bookings/${booking.id}`); + expect( + their_delete.status(), + `another user must not be able to delete your booking ` + + `(got ${their_delete.status()})`, + ).toBeGreaterThanOrEqual(400); + + // Still there afterwards, from the owner's point of view. + const still_mine = await listBookings(staffApi, 'desk', from(), to()); + expect( + still_mine.map((b) => b.id), + 'the booking should survive the other user attempting to delete it', + ).toContain(booking.id); + } finally { + await other.dispose(); + await deleteBooking(staffApi, booking.id); + } + }); + + test('you can see your own booking in the listing', async ({ staffApi }, testInfo) => { + // The control for the test above. Without it, "they cannot see it" would + // also pass if nobody could see anything. + const desk = deskFor(testInfo.parallelIndex); + const title = uniqueTitle('E2E Scoping Control'); + await releaseAsset(staffApi, 'desk', desk.id, from(), to()); + + const me = await (await staffApi.get('/api/engine/v2/users/current')).json(); + const zones = await bookingZones(staffApi); + const start = Math.floor(Date.now() / 1000) + DAY; + + const created = await staffApi.post(`${STAFF_API}/bookings`, { + data: { + booking_type: 'desk', + asset_id: desk.id, + booking_start: start, + booking_end: start + 3600, + timezone: 'Etc/UTC', + user_email: me.email, + user_id: me.id, + user_name: me.name, + title, + zones, + }, + }); + expect( + created.ok(), + `creating the booking should succeed: ${created.status()} ${await created.text()}`, + ).toBeTruthy(); + const booking = await created.json(); + + try { + const mine = await listBookings(staffApi, 'desk', from(), to()); + expect( + mine.map((b) => b.id), + 'you must be able to see your own booking', + ).toContain(booking.id); + } finally { + await deleteBooking(staffApi, booking.id); + } + }); +}); From b4a1f5acf46d2c90ddd0952b2b01ab305240a5b4 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Mon, 3 Aug 2026 18:17:11 +1000 Subject: [PATCH 3/4] test(e2e): two people cannot hold the same desk (REG-02) 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) --- E2E_USER_STORIES.md | 2 +- apps/workplace/e2e/local/desk-clash.spec.ts | 156 ++++++++++++++++++++ 2 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 apps/workplace/e2e/local/desk-clash.spec.ts diff --git a/E2E_USER_STORIES.md b/E2E_USER_STORIES.md index d2f0b25af7..3d86e12c1f 100644 --- a/E2E_USER_STORIES.md +++ b/E2E_USER_STORIES.md @@ -150,7 +150,7 @@ task that found it, so the row can be traced. | ID | P | Story | Source | Status | |----|---|-------|--------|--------| | REG-01 | P0 | Scope is not lost on token refresh; downstream authorisation still passes. | PPT-2536, 2026-07-23 revert | **done** — AUTH-E2E-02 | -| REG-02 | P1 | An overlapping desk booking is rejected rather than silently accepted. | `2607.1` "Fix rejecting overlapping bookings on desk assignment" | todo | +| REG-02 | P1 | An overlapping desk booking is rejected rather than silently accepted. | `2607.1` "Fix rejecting overlapping bookings on desk assignment" | **done** — `local/desk-clash.spec.ts`. Identical and partially-overlapping slots both refused (409), attempted as a *second* user so a per-user-only check would fail. Includes a control that a non-overlapping slot is accepted, and that the desk frees up after deletion. Red-checked. | | REG-03 | P1 | A clash check uses the **current** `booking_end`, not a stale one. | `2607.1` "Fix stale booking_end being used for clash check" | todo | | REG-04 | P1 | Desk booking status displays correctly in the booking list. | `2606.1` "Fix status display for desk bookings" | todo | | REG-05 | P2 | The authorised-user check has no race on boot (no flash of unauthorised). | `2607.1` "Fix race condition for authorised check" | todo | diff --git a/apps/workplace/e2e/local/desk-clash.spec.ts b/apps/workplace/e2e/local/desk-clash.spec.ts new file mode 100644 index 0000000000..9ae90ef48b --- /dev/null +++ b/apps/workplace/e2e/local/desk-clash.spec.ts @@ -0,0 +1,156 @@ +/** + * REG-02 — two people cannot hold the same desk at the same time. + * + * Maps to a real shipped fix: "Fix rejecting overlapping bookings on desk + * assignment" (release 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. + * + * Deliberately attempted as a SECOND user, because that's the real scenario and + * because a clash check that only looked at your own bookings would still pass a + * single-user version of this test. + */ +import { request } from '@playwright/test'; +import { test, expect } from '../../../../e2e/support/fixtures'; +import { APP_URL, BACKEND_URL, WORKERS, deskFor, roleFor } from '../../../../e2e/support/env'; +import { mintToken } from '../../../../e2e/support/auth'; +import { + STAFF_API, + deleteBooking, + releaseAsset, + uniqueTitle, + zonesWithTag, +} from '../../../../e2e/support/api'; +import type { APIRequestContext } from '@playwright/test'; + +const DAY = 86_400; +const from = () => Math.floor(Date.now() / 1000) - 3 * DAY; +const to = () => Math.floor(Date.now() / 1000) + 3 * DAY; + +async function bookingZones(api: APIRequestContext): Promise { + const groups = await Promise.all( + ['org', 'building', 'level'].map((tag) => zonesWithTag(api, tag)), + ); + return groups.flat().map((z) => z.id); +} + +async function book( + api: APIRequestContext, + asset_id: string, + start: number, + end: number, + zones: string[], +) { + const me = await (await api.get('/api/engine/v2/users/current')).json(); + return api.post(`${STAFF_API}/bookings`, { + data: { + booking_type: 'desk', + asset_id, + booking_start: start, + booking_end: end, + timezone: 'Etc/UTC', + user_email: me.email, + user_id: me.id, + user_name: me.name, + title: uniqueTitle('E2E Clash'), + zones, + }, + }); +} + +test.describe('desk double-booking', () => { + test('a second person cannot book a desk that is already taken', async ({ + staffApi, + }, testInfo) => { + const mine = testInfo.parallelIndex; + const theirs = (mine + 1) % WORKERS; + test.skip(theirs === mine, 'needs at least two workers to have two distinct users'); + + const desk = deskFor(mine); + const zones = await bookingZones(staffApi); + await releaseAsset(staffApi, 'desk', desk.id, from(), to()); + + // A fixed, future window. Not "now", so the test can't be tripped by the + // clock crossing a boundary mid-run. + const start = Math.floor(Date.now() / 1000) + DAY; + const end = start + 3600; + + const first = await book(staffApi, desk.id, start, end, zones); + expect( + first.ok(), + `the first booking should succeed: ${first.status()} ${await first.text()}`, + ).toBeTruthy(); + const booking = await first.json(); + + const other_role = roleFor('staff', theirs); + const other_mint = await mintToken( + BACKEND_URL, + APP_URL, + other_role.email, + other_role.password, + ); + const other = await request.newContext({ + baseURL: BACKEND_URL, + ignoreHTTPSErrors: true, + extraHTTPHeaders: { Authorization: `Bearer ${other_mint.accessToken}` }, + }); + + try { + // Exactly the same slot. + const exact = await book(other, desk.id, start, end, zones); + expect( + exact.status(), + `an identical slot must be refused, got ${exact.status()}`, + ).toBeGreaterThanOrEqual(400); + + // And a partial overlap, which is the case a naive check misses: it + // starts before the existing booking ends. + const partial = await book(other, desk.id, start + 1800, end + 1800, zones); + expect( + partial.status(), + `an overlapping slot must be refused, got ${partial.status()}`, + ).toBeGreaterThanOrEqual(400); + + // Control: a slot that genuinely doesn't overlap is fine. Without this, + // a backend that rejected everything would pass the two checks above. + const clear = await book(other, desk.id, end + 3600, end + 7200, zones); + expect( + clear.ok(), + `a non-overlapping slot should be accepted: ${clear.status()} ${await clear.text()}`, + ).toBeTruthy(); + const clear_booking = await clear.json(); + await deleteBooking(other, clear_booking.id); + } finally { + await other.dispose(); + await deleteBooking(staffApi, booking.id); + } + }); + + test('the desk frees up once the booking is deleted', async ({ staffApi }, testInfo) => { + // Guards a nastier version of the same bug: a cancelled booking that still + // blocks the desk. Users would see it as free and be unable to book it, + // which is harder to diagnose than a straightforward double-booking. + const desk = deskFor(testInfo.parallelIndex); + const zones = await bookingZones(staffApi); + await releaseAsset(staffApi, 'desk', desk.id, from(), to()); + + const start = Math.floor(Date.now() / 1000) + 2 * DAY; + const end = start + 3600; + + const first = await book(staffApi, desk.id, start, end, zones); + expect(first.ok(), `first booking: ${first.status()}`).toBeTruthy(); + const booking = await first.json(); + + const blocked = await book(staffApi, desk.id, start, end, zones); + expect(blocked.status(), 'the slot is taken while the booking exists').toBeGreaterThanOrEqual(400); + + await deleteBooking(staffApi, booking.id); + + const after = await book(staffApi, desk.id, start, end, zones); + expect( + after.ok(), + `the same slot should be bookable again once freed: ${after.status()} ${await after.text()}`, + ).toBeTruthy(); + await deleteBooking(staffApi, (await after.json()).id); + }); +}); From 64b40c07e62eeb910a6f34b3aca740a1efb3918c Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Wed, 5 Aug 2026 22:06:05 +1000 Subject: [PATCH 4/4] test(e2e): keep the PPT-2642 client-abort diagnostic 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 --- e2e/support/repro/reg09-client-abort.ts | 89 +++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 e2e/support/repro/reg09-client-abort.ts diff --git a/e2e/support/repro/reg09-client-abort.ts b/e2e/support/repro/reg09-client-abort.ts new file mode 100644 index 0000000000..f44243eb7b --- /dev/null +++ b/e2e/support/repro/reg09-client-abort.ts @@ -0,0 +1,89 @@ +/** + * PPT-2642 diagnostic — does a client that disconnects mid-request leave a + * transaction open on the server? + * + * The burst reproducer leaves a backend `idle in transaction` for as long as the + * process lives, with pg-orm 2.2.3's discard handling compiled in. That means a + * `BEGIN` was issued and neither committed nor rolled back — which is what you + * would expect if the request fiber died between the two, rather than unwinding + * through the `ensure` that returns the connection. + * + * The burst always aborted a couple of requests client-side, so this isolates + * that one variable: no concurrency, no serialization failure, just a POST that + * is cut off after the transaction has certainly started. + * + * e2e/stack/up.sh + * docker compose -p placeos-e2e restart staff-api # clean pool + * ABORT_MS=60 N=5 bunx tsx e2e/support/repro/reg09-client-abort.ts + * + * Then, with no other traffic: + * docker exec placeos-e2e-postgres-1 psql -U placeos -d placeos \ + * -c "SELECT pid, state, now()-state_change AS idle_for FROM pg_stat_activity + * WHERE state = 'idle in transaction';" + * + * A backend still `idle in transaction` seconds later means the abort is the + * trigger, and the fix belongs where the request fiber is torn down. + */ +import { mintToken } from '../auth'; + +const B = 'https://localhost:9443'; +const ABORT_MS = Number(process.env.ABORT_MS ?? 60); +const N = Number(process.env.N ?? 5); + +process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; + +(async () => { + const m = await mintToken( + B, + `${B}/backoffice`, + 'support@place.tech', + 'development', + ); + const auth = { Authorization: `Bearer ${m.accessToken}` }; + + const zones = ( + await (await fetch(`${B}/api/engine/v2/zones?limit=100`, { headers: auth })).json() + ).map((z: any) => z.id); + const me = await ( + await fetch(`${B}/api/engine/v2/users/current`, { headers: auth }) + ).json(); + + const base = Math.floor(Date.now() / 1000) + 86400 * 30; + let aborted = 0; + let completed = 0; + + for (let i = 0; i < N; i++) { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), ABORT_MS); + try { + await fetch(`${B}/api/staff/v1/bookings`, { + method: 'POST', + signal: controller.signal, + headers: { ...auth, 'Content-Type': 'application/json' }, + body: JSON.stringify({ + booking_type: 'desk', + asset_id: `e2e-desk-${i % 5}`, + booking_start: base + i * 7200, + booking_end: base + i * 7200 + 3600, + timezone: 'Etc/UTC', + user_email: me.email, + user_id: me.id, + user_name: me.name, + title: `REG09-abort ${i}`, + zones, + }), + }); + completed += 1; + } catch { + aborted += 1; + } finally { + clearTimeout(timer); + } + // let the server finish whatever it is doing before the next one + await new Promise((r) => setTimeout(r, 300)); + } + + console.log(`\n ${N} POSTs, aborted after ${ABORT_MS}ms`); + console.log(` aborted client-side: ${aborted}, completed: ${completed}`); + console.log(' now check pg_stat_activity for `idle in transaction`'); +})();