chore: L1+L2+L3 nit bundle from session code review#7
Merged
Conversation
Three small follow-ups from the sub-agent code review. **L1** — `worker/src/auth/verify-proof.ts:218`. `verifyProof` takes `expectedAudience` but the X.509 branch doesn't use it (certs have no aud claim). The earlier comment documented this in prose only; adding `void expectedAudience;` makes the intent live in the IR where a future refactorer reading just the code can see it. **L2** — `worker/src/key-id.ts`. The migration policy for the keyId widening (32→64 bits) was implicit in a brief sentence. Expanded to a clearer "Migration policy" section: existing 8-hex IDs persist, 5-min cert TTL drains old certs, bundle keyId/prevKeyId fields accept either width, no separate sweeper needed, key rotation naturally handles re-id. **L3** — `worker/src/allowed-audiences.ts` (new) + `worker/worker.ts`. ALLOWED_AUDIENCES was hardcoded; staging vs prod required a code change. Now sourced from `env.ALLOWED_AUDIENCES` (CSV) with the production set as fallback. Empty/missing env → default. Env value fully REPLACES the default so operators declare exactly what their deployment trusts (no implicit production audiences for self-hosted). Extracted to its own file (no DO dependencies) so unit tests can import it without pulling in cloudflare:workers via worker.ts — same pattern as src/key-id.ts and src/auth/redirect-uri.ts. 6 new tests under `getAllowedAudiences`: - missing env → defaults - empty env → defaults - whitespace-only env → defaults - single value → exact match (not merged with defaults) - CSV with whitespace tolerance - sloppy CSV with empty entries → filtered 308/308 vitest green; tsc --noEmit clean. Closes notme-aefa94. 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-aefa94. Three small follow-ups from the session code review.
void expectedAudienceinverifyProofx509 branch — intent in IR not just prosekey-id.tsALLOWED_AUDIENCESnow env-overridable viaenv.ALLOWED_AUDIENCES(CSV); production set is the fallbackL3 is the only behavior change — staging deployments can now pin different audiences without a code change. Env value fully replaces the default (no implicit production audiences for self-hosted).
Test plan
cd worker && npx vitest run→ 308/308 (was 302; +6 for getAllowedAudiences)npx tsc --noEmit→ clean🤖 Generated with Claude Code