From 7aa3a71086a30cd0f596d07f530632205d5d217d Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Thu, 6 Aug 2026 02:35:47 +1000 Subject: [PATCH 1/2] test(e2e): address review feedback on the PKCE and desk-clash specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Alex's review on #477 landed after that PR was merged, so these are follow-ups rather than changes to it. - pkce: assert the challenge is actually SHA-256(verifier) in base64url, and that it is 43 base64url characters. Previously the spec only checked that a challenge and a verifier were each present, which would pass even if the two were unrelated — the exact state a client that stopped deriving the challenge correctly would leave things in. The verifier is read from the token request's query string, which is where ts-client puts it. - desk-clash: require 409 rather than any >= 400. A 500 from an unhealthy backend satisfied the old check while proving nothing about clash detection, and booking POSTs have a known way of returning 500 under load (REG-09). - desk-clash: delete anything the second user unexpectedly succeeds in creating, as that user, before their context is disposed. GET /bookings is caller-scoped, so the owner's releaseAsset cannot see those rows and the desk would stay held for later runs. Full suite green locally: 14 passed. Both new assertions red-checked. Co-Authored-By: Claude Opus 5 --- apps/workplace/e2e/local/desk-clash.spec.ts | 78 +++++++++++---- apps/workplace/e2e/local/pkce.spec.ts | 102 +++++++++++++++----- 2 files changed, 137 insertions(+), 43 deletions(-) diff --git a/apps/workplace/e2e/local/desk-clash.spec.ts b/apps/workplace/e2e/local/desk-clash.spec.ts index 9ae90ef48b..2b8984a58b 100644 --- a/apps/workplace/e2e/local/desk-clash.spec.ts +++ b/apps/workplace/e2e/local/desk-clash.spec.ts @@ -10,10 +10,8 @@ * 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 type { APIRequestContext } from '@playwright/test'; +import { request, type APIResponse } from '@playwright/test'; import { STAFF_API, deleteBooking, @@ -21,7 +19,15 @@ import { uniqueTitle, zonesWithTag, } from '../../../../e2e/support/api'; -import type { APIRequestContext } from '@playwright/test'; +import { mintToken } from '../../../../e2e/support/auth'; +import { + APP_URL, + BACKEND_URL, + WORKERS, + deskFor, + roleFor, +} from '../../../../e2e/support/env'; +import { expect, test } from '../../../../e2e/support/fixtures'; const DAY = 86_400; const from = () => Math.floor(Date.now() / 1000) - 3 * DAY; @@ -64,7 +70,10 @@ test.describe('desk double-booking', () => { }, testInfo) => { const mine = testInfo.parallelIndex; const theirs = (mine + 1) % WORKERS; - test.skip(theirs === mine, 'needs at least two workers to have two distinct users'); + test.skip( + theirs === mine, + 'needs at least two workers to have two distinct users', + ); const desk = deskFor(mine); const zones = await bookingZones(staffApi); @@ -92,41 +101,73 @@ test.describe('desk double-booking', () => { const other = await request.newContext({ baseURL: BACKEND_URL, ignoreHTTPSErrors: true, - extraHTTPHeaders: { Authorization: `Bearer ${other_mint.accessToken}` }, + extraHTTPHeaders: { + Authorization: `Bearer ${other_mint.accessToken}`, + }, }); + // Anything the second user unexpectedly succeeds in creating has to be + // cleaned up by *them*: `GET /bookings` is caller-scoped, so the owner's + // `releaseAsset` in the next run cannot see it, and the desk stays held. + const leaked: number[] = []; + const capture = async (res: APIResponse) => { + if (res.ok()) leaked.push((await res.json()).id); + return res; + }; + try { // Exactly the same slot. - const exact = await book(other, desk.id, start, end, zones); + // + // 409 specifically, not "any error". A 5xx from an unhealthy backend + // would satisfy `>= 400` while proving nothing about clash detection — + // and booking POSTs have a known way of returning 500 under load + // (REG-09), which is exactly the failure this spec must not absorb. + const exact = await capture( + await book(other, desk.id, start, end, zones), + ); expect( exact.status(), - `an identical slot must be refused, got ${exact.status()}`, - ).toBeGreaterThanOrEqual(400); + `an identical slot must be refused with 409, got ${exact.status()}`, + ).toBe(409); // 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); + const partial = await capture( + await book(other, desk.id, start + 1800, end + 1800, zones), + ); expect( partial.status(), - `an overlapping slot must be refused, got ${partial.status()}`, - ).toBeGreaterThanOrEqual(400); + `an overlapping slot must be refused with 409, got ${partial.status()}`, + ).toBe(409); // 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); + 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); + leaked.push(clear_booking.id); } finally { + // Delete as `other`, before the context is disposed — the owner + // cannot see these. + for (const id of leaked) + await deleteBooking(other, id).catch(() => undefined); await other.dispose(); await deleteBooking(staffApi, booking.id); } }); - test('the desk frees up once the booking is deleted', async ({ staffApi }, testInfo) => { + 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. @@ -142,7 +183,10 @@ test.describe('desk double-booking', () => { 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); + expect( + blocked.status(), + 'the slot is taken while the booking exists', + ).toBeGreaterThanOrEqual(400); await deleteBooking(staffApi, booking.id); diff --git a/apps/workplace/e2e/local/pkce.spec.ts b/apps/workplace/e2e/local/pkce.spec.ts index 467a2808f0..fb361239b6 100644 --- a/apps/workplace/e2e/local/pkce.spec.ts +++ b/apps/workplace/e2e/local/pkce.spec.ts @@ -12,39 +12,60 @@ * 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 { createHash } from 'node:crypto'; + import { clientId, redirectUriFor } from '../../../../e2e/support/auth'; +import { APP_URL, roleFor } from '../../../../e2e/support/env'; +import { expect, test } from '../../../../e2e/support/fixtures'; +import { loginViaUI } from '../../../../e2e/support/login'; // 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 }) => { + 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 authorize = requests.find((r) => + r.url.includes('/oauth/authorize'), + ); const exchange = requests.find( - (r) => r.url.includes('/oauth/token') && !r.url.includes('refresh_token'), + (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(); + 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!.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(/[+/=]/); + const challenge = + new URL(authorize!.url).searchParams.get('code_challenge') ?? ''; + // SHA-256 is 32 bytes, which is exactly 43 characters of unpadded + // base64url. Anything else is not an S256 challenge, whatever the + // `code_challenge_method` parameter claims. + expect( + challenge, + 'an S256 challenge is 43 base64url characters', + ).toMatch(/^[A-Za-z0-9\-_]{43}$/); // --- the token leg ----------------------------------------------------- expect(exchange!.status, 'token exchange succeeds').toBe(200); @@ -53,23 +74,48 @@ test.describe('PKCE handshake', () => { // 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', - ); + 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'); + // + // Asserting the two values are *related* is the whole point. Checking + // only that a challenge and a verifier were both present would pass even + // if they had nothing to do with each other — which is precisely the + // state a broken client, or a backend that stopped enforcing PKCE, would + // leave things in. + // ts-client puts the exchange parameters in the query string, but has + // also form-encoded them in the body across versions, so read either. + const body = exchange!.postData ?? ''; + const verifier = + new URL(exchange!.url).searchParams.get('code_verifier') || + new URLSearchParams(body).get('code_verifier') || + ''; + expect(verifier, 'a code_verifier was sent').not.toBe(''); + const derived = createHash('sha256') + .update(verifier) + .digest('base64url'); + expect(derived, 'the challenge is SHA-256(verifier), base64url').toBe( + challenge, + ); // 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(); + expect( + token.access_token, + 'the exchange produced a token', + ).toBeTruthy(); }); - test('no credentials are exposed in any /auth request', async ({ page }) => { + test('no credentials are exposed in any /auth request', async ({ + page, + }) => { const { requests } = await loginViaUI(page, roleFor('admin')); const password = roleFor('admin').password; @@ -77,15 +123,19 @@ test.describe('PKCE handshake', () => { // 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', - ); + 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)); + 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', From 11b693cdac6bf29691be938436d45b296cbbb1df Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Thu, 6 Aug 2026 03:49:44 +1000 Subject: [PATCH 2/2] fix(workplace): cancel the group-booking timer when the success page is destroyed `ngOnInit` scheduled a bare 100ms `setTimeout` that reads localStorage and writes to the component's signals. Nothing cancelled it, so leaving the page inside that window ran the callback against a component that no longer exists. In CI it fails the whole workplace test run: the timer outlives the test environment and raises `ReferenceError: localStorage is not defined` as an unhandled error, which vitest counts as a failure even though all 428 tests pass. It only shows on the slower runner, which is why it reads as flaky. Co-Authored-By: Claude Opus 5 --- .../book/desk-flow/desk-flow-success.component.ts | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/apps/workplace/src/app/book/desk-flow/desk-flow-success.component.ts b/apps/workplace/src/app/book/desk-flow/desk-flow-success.component.ts index 7ae5e464b1..99e327cdf8 100644 --- a/apps/workplace/src/app/book/desk-flow/desk-flow-success.component.ts +++ b/apps/workplace/src/app/book/desk-flow/desk-flow-success.component.ts @@ -3,6 +3,7 @@ import { Component, computed, inject, + OnDestroy, OnInit, signal, } from '@angular/core'; @@ -277,7 +278,7 @@ interface GroupBookingListItem { UserAvatarComponent, ], }) -export class NewDeskFlowSuccessComponent implements OnInit { +export class NewDeskFlowSuccessComponent implements OnInit, OnDestroy { private _org = inject(OrganisationService); private _state = inject(BookingFormService); private _settings = inject(SettingsService); @@ -369,6 +370,8 @@ export class NewDeskFlowSuccessComponent implements OnInit { return this._settings.time_format; } + private _group_bookings_timer?: ReturnType; + public async ngOnInit() { await this._org.waitUntilInitialised(); this.last_event.set(this._state.last_success); @@ -384,11 +387,18 @@ export class NewDeskFlowSuccessComponent implements OnInit { this.building.set(this._building_pipe.transform(event.zones)); // Load group bookings if this is a group booking - setTimeout(async () => { + this._group_bookings_timer = setTimeout(async () => { if (this.is_group()) await this._loadGroupBookings(); }, 100); } + public ngOnDestroy() { + // The callback reads localStorage and writes to this component's + // signals, so leaving it pending past destruction does work on a + // component nobody is looking at any more. + clearTimeout(this._group_bookings_timer); + } + private async _loadGroupBookings() { const stored_ids = localStorage.getItem( 'PLACEOS.last_group_booking_ids',