test(worker): add real concurrent-flow stomp test for passkey#4
Merged
Conversation
…passkey challenge
Code-review sub-agent (aa0878f69907df240) flagged M3 — the existing
"regression guard" test at passkey.test.ts:279 only proved
verifyAuthentication doesn't throw on an empty challenges table. It
never exercised the actual fix: lookup by exact challenge value
instead of ORDER BY created_at DESC LIMIT 1.
This test seeds TWO challenges with different values + timestamps
(alice earlier, bob later) and submits an assertion with alice's
challenge. The mock records every SQL call. After
verifyAuthentication runs, we find the SELECT against
passkey_challenges that filters by type='authentication' and assert:
1. The query text matches `challenge = ?` (the fix's WHERE clause)
2. The query text does NOT match `ORDER BY created_at DESC` (the
bug's ordering)
3. The first parameter is "alice-challenge" — meaning the lookup
binds to the submitted-challenge value, not the most-recent row
If the order-by-desc bug ever returns, all three assertions fail.
Adds a new makeRecordingSqlForChallenges helper alongside the
existing createMockSql — kept separate because the M3 test needs
pre-seeded rows + call recording, while existing tests are happy
with the simpler mock.
303/303 vitest green; tsc --noEmit clean.
Closes notme-ae9c1b.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jamestexas
added a commit
that referenced
this pull request
Apr 29, 2026
Code-review sub-agent flagged that passkey.ts:248 filters expired
challenges at READ time but never deletes them. Abandoned flows
(user closes the browser tab, network failure, etc.) leave dead
rows forever — the table grows unbounded.
Add a `sweepExpiredChallenges(sql)` helper that issues
`DELETE FROM passkey_challenges WHERE created_at < datetime('now',
'-1 hour')`. Wired into both `authenticationOptions` and
`registrationOptions` so the cleanup amortizes against real auth
traffic — no separate alarm job needed.
Why 1 hour instead of 5 minutes (the lookup-window cutoff):
defense in depth. The lookup already rejects anything past 5 min,
so 1-hour rows are dead weight at most. 1 hour gives a clear margin
above the lookup window so a clock-skew edge case can't sweep a
row the lookup would still want.
2 new tests under `passkey.challenge.sweeper`:
- authenticationOptions: DELETE issued with `-1 hour` cutoff,
runs BEFORE the INSERT (clock-skew protection)
- registrationOptions: same shape
Recording-mock pattern same as the M3 test in PR #4.
THREAT_MODEL.md `challenge expiry` row updated to mention the sweep.
304/304 vitest green; tsc --noEmit clean.
Closes notme-ae81c9.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jamestexas
added a commit
that referenced
this pull request
May 10, 2026
…protocol alignment) — replaces #10 (#11) * docs: ADR-010 — cross-language canonical encoding alignment with signet protocol Closes notme-803923 (split into three properly-scoped beads — see ADR for the restructure). The schema docstring claim "Cap'n Proto's deterministic binary format guarantees this [cross-language byte equality]" was wrong as written. Capnp is the type sync language in this stack, not the wire format. Wire formats live in implementations and are governed by signet protocol ADRs. ADR-010 commits to: - Transport / storage / HTTP API: JSON (matches signet's own https_fetcher.go, KV layout, all 139 JSON call sites in worker/). - Cryptographic canonical bytes (Ed25519 sign/verify input): canonical CBOR per RFC 8949 §4.2, integer-keyed map matching signet/pkg/revocation/checker.go:168-188 byte-for-byte. This is a tiny code change (one function changes its body), but a load-bearing protocol-conformance decision: notme becomes a real reference impl of signet protocol, with cross-impl bundle verifiability. Bead restructure (replaces notme-803923): notme-b92141 (P0) — TS adopts canonical CBOR for signing canonicalization. Implements this ADR. notme-b96b4c (P1) — schema sync drift; capnp-to-ts.ts silent degradation on unknown features. Type-sync correctness, independent of wire format. notme-b9938a (P3) — dedupe bundleCanonical (copy-pasted between revocation.ts and signing-authority.ts). Falls out of the P0 work. Phase 1 (this commit): docs + schema docstring fix only. Phase 2 (next commit): cbor-x dependency, function body change, cross-runtime fixture suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * worker: switch CABundle signing to canonical CBOR (ADR-010) Closes notme-b92141 (P0 — TS adopts canonical CBOR for CABundle signing canonicalization). Closes notme-b9938a (P3 — dedupe bundleCanonical, signing-authority.ts duplicate now imports from revocation.ts). Per ADR-010 + signet ADR-002 §2.3: the bytes Ed25519-signed for CABundle MUST be canonical CBOR (RFC 8949 §4.2), integer-keyed map matching signet/pkg/revocation/checker.go:168-188 byte-for-byte. Wire format unchanged outside the signing input: - HTTP API: JSON - KV storage: JSON - DO blob columns: JSON - DPoP/JWT: JSON-in-base64 - Signing canonical: CBOR canonical (this change) Implementation: - Added cbor-x ^1.6.4 to worker/package.json — pure JS, runs on Cloudflare Workers V8. Encoder configured with explicit options: mapsAsObjects: false (preserve Map → CBOR map; integer keys) useRecords: false (no cbor-x record extension) tagUint8Array: false (RFC 8949 plain bytes, not RFC 8746 typed-array tag — required to match fxamacker/cbor's []byte encoding) - Inner Keys map: explicit RFC 8949 §4.2 canonical key ordering (length-then-bytewise lex on UTF-8 encoded keys). - base64 → Uint8Array conversion for the keys map values, so CBOR encodes as bytes (major type 2) matching signet's []byte type. - Output normalized to Uint8Array — cbor-x returns Buffer in Node. Tests at src/__tests__/bundle-canonical.test.ts (5 cases, all green): - 6-entry CBOR map header (signature field excluded) - Determinism (same input → same bytes) - TS field-insertion-order independence - prevKeyId='' encoded as empty string (matches Go zero value) - Hand-computed CBOR fixture: 25 bytes, byte-for-byte locked. Future cross-runtime gate: signet Go-side test produces same fixture bytes; this test becomes the cross-runtime check. Out of scope (separate beads filed): - notme-b96b4c (P1) — capnp-to-ts.ts silent degradation. Type sync, independent of wire format. - notme-c38bb6 (P1) — worker/src/revocation.test.ts is dead; uses cloudflare:test which the vitest config doesn't wire up. Discovered while working on this. Workaround: pure-function tests at src/__tests__/bundle-canonical.test.ts exercise the canonical-bytes path. Verification: npx tsc --noEmit clean npx vitest run — 337/337 pass (was 332; +5 new bundleCanonical tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs+code: address Copilot review on PR #10 10 findings, all addressed. (Original PR #10 was auto-closed when its base branch chore/notme-housekeeping was deleted on the squash-merge of PR #9; this branch is the cherry-picked equivalent targeting fresh main.) Code: 1. (#3, #5) bundleCanonical() parameter type → Omit<CABundle, "signature">. Drops the cast in signing-authority.ts that was needed because the previous CABundle parameter required a signature field even though the function ignored it. 2. (#2) Removed orphaned "Canonical JSON for bundle signature verification" JSDoc above isBundleStale (revocation.ts:129-134). The block had been stranded above the wrong function after the CBOR work moved bundleCanonical to a new location, leaving two adjacent JSDoc blocks back-to-back. 3. (#6) Replaced "notme-XXX" placeholder in worker/src/__tests__/bundle-canonical.test.ts:14 with the actual bead ID notme-c38bb6 (the dead-revocation-tests bead). 4. (#10) Added a multi-key sort test exercising sortStringKeysCanonical with two entries chosen specifically to expose the canonical ordering rule: "b" (length 1) and "ab" (length 2). Naive alphabetical puts "ab" first; canonical puts "b" first per RFC 8949 §4.2 length-then-bytewise. The single-key fixture in the prior test couldn't catch a regression to alphabetical-only sort; this one does. Docs: 5. (#7) schema/README.md — replaced the "open issue: TS-side canonical-bytes encoder is wrong" section with "TS-side canonical-bytes encoder — alignment shipped". The Mermaid diagram's edge labels are corrected from "signs canonical capnp bytes" / "verifies JSON-of-sorted-keys" to "signs canonical CBOR" / "verifies canonical CBOR (matches byte-for-byte)" — what the system actually does post-PR. Removed the warn class from gen_ts since the gen output is no longer the wire-format mismatch site (only capnp_to_ts retains the warn class for the type-sync drift, which is a separate open bead). 6. (#1, #8) docs/design/010 migration path expanded. Strategy (A) "dual-encode for one rotation cycle" remains documented as the conservative path for production systems; strategy (B) "hard-cutover with bounded staleness window" is the new chosen path with rationale: notme is currently pre-production; the SigningAuthority alarm cycle (4min) bounds the unverifiable-bundle window; revocation checks already fail closed during bundle staleness so behavior is consistent. Re-evaluation trigger documented for if/when notme has production-tier persistent signed bundles. Investigated, filed, deferred: 7. (#4) generateBundle() drops prevKeyId — confirmed pre-existing rotation-grace-period bug. Filed as notme-54f84b (P1). Out of scope for this PR (the CBOR work doesn't introduce or affect this; the encoder handles prevKeyId ?? "" correctly cross-runtime). Will be addressed in its own PR. Acknowledged (no code change): 8. (#9) cbor-x's optional cbor-extract native dep — pulls in prebuilds via install scripts. cbor-x has a pure-JS fallback when cbor-extract isn't installed (e.g. under --ignore-scripts builds). Workers deployment doesn't hit the native path; CI sandboxing concerns are addressable by --ignore-scripts without losing functionality. No code change needed. Verification: npx vitest run — 338/338 pass (was 337; +1 multi-key sort test) npx tsc --noEmit clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(adr-010): address Copilot review on PR #11 (See also section) Two findings, both legitimate: 1. (#1) "See also" section used relative paths like ../../../signet/... which don't resolve from within the notme repo. Switched all cross-repo references to absolute github.com URLs (signet ADR-002/011/checker.go/https_fetcher.go, ley-line-open T8 dossier). 2. (#2) Bullets describing in-repo references (bundleCanonical, signing-authority duplicate, fixture suite) were written in pre-landing tense ("function to be replaced", "duplicate to be removed"). Updated to post-landing tense — bundleCanonical is the shipped CBOR encoder; the signing-authority inline duplicate was removed; the bundle-canonical.test.ts fixture file now exists and is referenced. The cross-runtime fixture suite is the only remaining "still to be created" item; called out explicitly as a follow-up. Also reorganized the section into "external (absolute URLs)" vs "in-repo (relative paths)" subsections so the convention is visible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <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.
Summary
Closes notme-ae9c1b. Sub-agent code review flagged that the existing "regression guard" test for the concurrent-flow challenge fix only checked the empty-table no-throw path — it never exercised two real concurrent flows.
What's new
Test seeds TWO challenges (alice + bob, different timestamps), submits alice's assertion, and asserts via a recording mock SQL that:
challenge = ?ORDER BY created_at DESC(the bug)alice-challenge(not the most-recent bob)If the bug ever returns, the test fails.
Test plan
cd worker && npx vitest run→ 303/303 (was 302; +1)npx tsc --noEmit→ clean🤖 Generated with Claude Code