test(e2e): address Alex's review feedback on the PKCE and desk-clash specs - #484
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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 <noreply@anthropic.com>
camreeves
force-pushed
the
fix/e2e-477-review-followup
branch
from
August 5, 2026 17:41
f7bda30 to
7aa3a71
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/placeos?upgradeToPro=build-rate-limit |
…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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #477. Alex's review arrived after that PR was merged, so these are separate commits rather than changes to it. All three points were valid.
1. Bind the PKCE challenge to the verifier (P1)
The spec checked that a
code_challengeand acode_verifierwere each present, but never that they were related. That passes even if the client stops deriving the challenge from the verifier — precisely the regression the test exists to catch.It now asserts
SHA-256(verifier)in base64url equals the challenge, and that the challenge is 43 base64url characters (32 bytes unpadded), so acode_challenge_method=S256parameter that lies about the value gets caught.Worth noting: the verifier is sent in the token request's query string, not the body — reading it from
postDatareturns nothing.2. Require 409, not
>= 400(P1)Exactly as Alex said. A 500 satisfied the old assertion while proving nothing about clash detection, and booking POSTs have a documented way of returning 500 under concurrency (REG-09 / PPT-2642). Both clash assertions are now
toBe(409), with the received status in the failure message.3. Clean up the second user's bookings (P2)
GET /bookingsis caller-scoped, so a booking the second user unexpectedly succeeds in creating is invisible to the owner'sreleaseAsset— the desk would stay held for every later run. Anything that user creates is now recorded and deleted as that user, before their request context is disposed.Verification
🤖 Generated with Claude Code