sec(worker): extract /authorize redirect_uri validation + 16 tests#2
Merged
Conversation
The /authorize route inlined the open-redirect defenses directly in
worker.ts (1582-1619): four rules in series — required, parsable,
https-on-public-internet, exact-host allowlist. No unit tests.
Extracted to `worker/src/auth/redirect-uri.ts` as a pure
`validateRedirectUri(uri) → { ok, ... } | { ok: false, status, reason }`.
The route now just maps the result to an HTTP response. ALLOWED_REDIRECT_HOSTS
is exported as a `ReadonlySet<string>` so tests can iterate it.
16 new tests under `authorize.redirect.allowlist`:
- empty / malformed → 400
- allowlisted host (each entry of ALLOWED_REDIRECT_HOSTS) → 200
- non-allowlisted host → 403
- subdomain attack (rosary.bot.attacker.example) → 403
- subdomain not in list (evil.rosary.bot) → 403 ← exact-match check
- http on non-localhost → 400
- http://localhost / https://localhost → 200
- file:// / javascript: / data: → rejected
- userinfo abuse (https://rosary.bot@attacker.example) → 403
- ftp:// even on localhost → 400
- returns parsed URL on success for downstream callers
Maps to THREAT_MODEL.md `authorize.redirect.allowlist` row.
318/318 vitest green; tsc --noEmit clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-match allowlist The row's wording said 'allowlist: *.rosary.bot, *.notme.bot' which implied wildcard subdomain support — but the implementation has always been exact-host. Corrected to enumerate the actual entries and call out the additional defenses (scheme allowlist, userinfo abuse) the new tests cover. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jamestexas
added a commit
that referenced
this pull request
May 10, 2026
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>
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>
7 tasks
jamestexas
added a commit
that referenced
this pull request
May 18, 2026
* chore(deps): comprehensive workspace dep update sweep Workspace-wide dependency update sweep across root + action + worker. Clears all 6 pnpm audit advisories (was 2 high / 4 moderate). | Workspace | Package | From | To | |-----------|----------------------------------|--------------|--------------| | root | @vitest/coverage-v8 | ^4.1.2 | ^4.1.6 | | root | tsx | ^4.21.0 | ^4.22.2 | | root | zod | ^4.3.6 | ^4.4.3 | | root | pnpm.overrides undici | — | <6.24.0 → ^6.24.0 | | root | pnpm.overrides ws | — | <8.20.1 → ^8.20.1 | | action | @types/node | ^25.5.0 | ^25.9.0 | | action | esbuild | ^0.25.0 | ^0.25.12 | | action | typescript | ^5.8.0 | ^5.9.3 | | worker | @cloudflare/workers-types | ^4.20260329.1| ^4.20260518.1| | worker | @cloudflare/vitest-pool-workers | ^0.13.5 | 0.13.5 (PIN) | | worker | @playwright/test | ^1.59.1 | ^1.60.0 | | worker | @types/node | ^25.6.0 | ^25.9.0 | | worker | prettier | ^3.8.1 | ^3.8.3 | | worker | vitest | ^4.1.2 | ^4.1.6 | | worker | wrangler | ^4.78.0 | ^4.92.0 | | worker | zod | ^3.25.0 | ^4.4.3 | | worker | oslo | ^1.2.1 | REMOVED | Notable: - wrangler 4.78 → 4.92: aligns with notme.bot PR #2 baseline; Node 22 runtime - zod 3 → 4 in worker: only one file (gha-oidc.ts) uses zod; uses safeParse + .error.message which are stable across v3/v4. Aligns with root manifest (was already ^4.3.6 there). - oslo removed: deprecated meta-package; zero imports in src/. Worker already uses the successor @oslojs/crypto + @oslojs/encoding directly. - @cloudflare/vitest-pool-workers pinned to exact 0.13.5 (no caret): per rosary-8ae6ab, 0.13.5 has the CF API 10375 issue; we don't yet know if 0.14+ fixes it. Pin makes the constraint explicit. - pnpm.overrides force undici≥6.24.0 and ws≥8.20.1: clears all 5 undici advisories (transitive via @actions/http-client v2) and the ws advisory (transitive via miniflare). Avoids taking the @actions/* major bumps (4.0 is ESM-only — separate refactor). Deferred (need code change or evidence — separate beads): - @actions/core 1 → 3 / @actions/http-client 2 → 4: ESM-only migration; action is currently bundled via esbuild but the ESM-only constraint is a real refactor. Advisories handled via pnpm.overrides instead. - @peculiar/x509 1 → 2: security-sensitive cert API surface; cert-authority.ts + signing-authority.ts need careful review of v2's extension/generator API. - typescript 5 → 6 (action): major TS bump warrants its own pass across the workspace, not bundled in deps sweep. - esbuild 0.25 → 0.28 (action): 0.x bumps frequently change defaults; aligned worker is already at 0.28 — leaving action at 0.25 line for now to avoid bundling-flag drift. Test status: - worker: 425 passed | 6 todo (28 files) — matches baseline - task worker:check: typecheck + tests both green - task schema:check: green - action: pnpm build green, dist/index.js rebuilt and node --check clean - pnpm audit: 0 advisories (was 2 high / 4 moderate) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: trigger run after Actions re-enabled * fix(proxy): collapse redundant nested if in UDS path validator (clippy::collapsible_match) The outer 'contains("..")' check was strictly redundant — the inner 's == ".."' check (the only one that returned) implies it. Collapse the pair into a match guard. No behavior change; UDS paths containing '..' as a substring (e.g. 'foo..bar.sock') are still permitted, only exact '..' components reject. CI for the whole repo couldn't pass while this lint was hot under -D warnings. Unblocks PRs #20/#21/#22 once #23 merges and they rebase. --------- Co-authored-by: Claude Opus 4.7 <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
/authorizeopen-redirect defenses (worker.ts:1582-1619) into a pure helper atworker/src/auth/redirect-uri.ts.Why
The defenses were inlined in the route handler with no test coverage. The matrix has four orthogonal rules (required, parsable, https-on-public-internet, exact-host allowlist) and the only way to validate them was running the worker. Extracting to a pure function lets unit tests pin each row of the matrix directly.
What's tested
ALLOWED_REDIRECT_HOSTSacceptedrosary.bot.attacker.example,evil.rosary.bot) — exact-match defensefile://,javascript:,data:→ rejectedhttps://rosary.bot@attacker.example) → 403Behavior change
None. Route returns the same status codes and reasons; tests pin them.
Test plan
cd worker && npx vitest run→ 318/318 (was 302; +16)npx tsc --noEmit→ clean🤖 Generated with Claude Code