feat(mcp): client-assertion primitives — strict EdDSA verify + jti replay store

[heskew]

Part 1 of 4 for #159 — closes #160. Pure primitives with no wiring: zero behavior change until the grant (#162) consumes them. Contract per the design review.

What's here

src/lib/mcp/clientAssertion.ts — RFC 7523 §3 client-assertion verification (private_key_jwt), EdDSA/Ed25519 only (RFC 8037), implemented with node:crypto alone — no new dependency (jsonwebtoken cannot verify EdDSA). Fails closed with a typed { valid: false, reason } on every path; never throws.

• Exact alg: EdDSA pinning before any key work (blocks none and RS/HS confusion); typ optional-but-JWT when present; any crit rejected (no extensions implemented).
• JWKS key selection mirrors verifyAccessTokenWithKeySet: presented kid must match exactly one registered key (no fallback scanning); absent kid requires a single-key set.
• Key loading accepts only public OKP/Ed25519 JWKs — a key carrying private d is rejected as defense-in-depth against a registration-validation gap.
• Strict base64url validation per segment (Buffer.from(…, 'base64url') silently skips invalid characters, letting distinct strings decode identically) and a 64-byte Ed25519 signature length check.
• Claims: iss = sub = client_id; aud exactly the token endpoint (string or single-element array; multi-audience rejected); exp required and ≤ 60s out; iat required, not future; nbf honored when present; jti required and length-bounded (caps what a client can force into the replay table). 5s clock tolerance, both knobs overridable.
• Signature is verified before claims are inspected, so claim-shaped errors can't be probed without the private key.

src/lib/mcp/assertionJtiStore.ts + mcp_assertion_jtis table (expiration: 120) — the replay guard (#159 security req 1).

• Keys are sha256 of a length-prefixed (client_id, jti) pair — replay scope is per client (RFC 7523 defines jti uniqueness per issuer), and the length prefix prevents delimiter-stuffing collisions (a test caught the naive clientId + '\n' + jti concat colliding on crafted inputs).
• Unlike the other MCP stores, read errors propagate: "couldn't check" must never become "not seen" on the one store whose job is rejecting repeats.
• The get-then-put race (no atomic compare-and-set; async replication) is documented in the module header — same accepted precedent as MCPAuthCodeStore.consume, bounded by the ≤60s assertion exp.

Tests

35 new tests (841 total, all green): verify success paths (incl. aud-as-array, typ case-insensitivity, multi-key kid selection), structural rejections (segment counts, invalid base64url, alg: none, RS/HS/case downgrades, crit), key-material rejections (private d, RSA/X25519/malformed JWKs, empty set, ambiguous/unknown/duplicate kid), signature rejections (wrong key, tampered payload, short signature), the full claims matrix (iss/sub/aud incl. prefix-of-endpoint, exp/iat/nbf windows, jti bounds), and the store (first-sighting/replay through the tracked-object Proxy read path, per-client scoping, delimiter-stuffing keys, fail-closed error propagation).

Lint clean (the 4 warnings are pre-existing in hookManager.test.js); prettier clean.

Review status

CI review bots (Gemini, Claude) found no blockers; all four inline suggestions were adjudicated as real and fixed in bf00888 (8KB assertion cap, replay guard hardened to treat any truthy record as a replay, selectKey null/primitive JWK-entry guards, stale module-header comment).

Two Codex cross-model passes:

• Pass 1 (P2): a possible >60s window via old iat + far-future exp. Adjudicated — the attack is already blocked (exp is bounded relative to now), but it exposed a real strictness gap: the verifier didn't enforce that the self-declared lifetime (exp - iat) conforms to the policy window. Closed in 024ab58 as defense-in-depth.
• Pass 2 (security/perf/standards): the window options (maxExpiresInSeconds/clockToleranceSeconds) were trusted as finite — a NaN/Infinity would fail the time-window checks open. Not reachable today (callers default to 60/5), but client_credentials (3/4): token-endpoint grant — resource binding, per-grant TTL, discovery #162 wires these from mcp config. Coerced up front like coerceTtl in 1828274.

No open cross-model findings remain.

Where to look

• The claim-validation ordering in verifyClientAssertion (signature before claims) and the aud/exp window rules — this is the security contract of Add RFC 7523 client_credentials grant (private_key_jwt, EdDSA) for headless agent auth #159, so a second pair of eyes on the RFC 7523 §3 reading is the highest-value review.

Replay-guard residual risk (explicit sign-off item, per review)

The jti guard uses Table.create() (insert-if-absent, 409 on existing) — not an awaited get-then-put, after @kriszyp's review prompted a deep dive. What a merge signs off on:

• Sequential replay: rejected — any presentation after a create has committed gets the 409.
• Same-node concurrent in-flight duplicates: may all succeed today. Harper enforces create()'s existence check pre-staging only; the losing create currently degrades to last-write-wins with both callers reporting success (Table.create() existence check not enforced at commit — concurrent creates silently become last-write-wins (and the with-id form is undocumented) harper#1745, filed from this review — includes the by-design analysis and a per-node enforcement path via the existing retry/sourceApply seam). Exposure window: the staging→commit interval, not the assertion's ~60s validity.
• Cross-node: bounded by replication lag — by design (coordination-free LWW convergence); irreducible without consensus Harper deliberately doesn't do.
• Each duplicate acceptance mints one short-TTL (≤5 min) token for a client that does hold the private key — this narrows abuse to token-count inflation from a captured-in-flight assertion, not impersonation.
• If harper#1745 lands per-node 409 enforcement, this guard becomes fully atomic per node with zero code change (the catch already handles it).
• A concurrency test pins the properties stable under both semantics (≥1 winner; replay rejected once settled).

Docs

No docs change in this PR: these primitives have no user-facing surface until the grant wires them up in #162, which will carry the documentation for the whole flow.

🤖 Generated with Claude Code — model: Claude Fable 5

[gemini-code-assist]

Code Review

This pull request introduces an RFC 7523 client-assertion replay guard and verification mechanism for EdDSA/Ed25519 private_key_jwt authentication. It adds a new GraphQL table mcp_assertion_jtis, implements MCPAssertionJtiStore for tracking seen JTIs, and provides verifyClientAssertion for robust JWT verification, complete with comprehensive unit tests. Feedback suggests strengthening the replay check in MCPAssertionJtiStore by checking for any truthy database record instead of specifically verifying existing.id to prevent fail-open vulnerabilities. Additionally, defensive checks should be added to selectKey in clientAssertion.ts to handle potentially malformed or non-object elements in the jwks array and avoid runtime TypeErrors.

[claude]

Reviewed; no blockers found.

[heskew]

I found one issue worth fixing before these primitives are wired into the grant:

src/lib/mcp/clientAssertion.ts:157 trusts maxExpiresInSeconds and clockToleranceSeconds as already-valid finite numbers. If either option is NaN or Infinity, the time-window checks fail open: maxExpiresInSeconds: NaN/Infinity accepts a far-future exp, and clockToleranceSeconds: NaN/Infinity accepts expired assertions plus future iat/nbf. These options are the enforcement boundary for RFC 7523 §3's validity-window checks and the issue's ≤60s assertion policy, so they should be normalized before the comparisons. Please coerce/validate them like the existing TTL helpers (finite, positive, bounded; otherwise default or fail closed) and add tests for NaN/Infinity and config-shaped inputs before the next PR wires these knobs from runtime/config.

---

🤖 Posted by Codex (gpt-5.5) on Nathan's behalf

[heskew]

Addressed the Codex review (security/perf/standards angle) in 1828274.

Finding — window-option fail-open: verifyClientAssertion trusted maxExpiresInSeconds/clockToleranceSeconds as finite numbers; a NaN/Infinity would make the time-window comparisons fail open (a far-future exp, or an expired assertion / future iat/nbf, would be accepted). Real and worth landing now: not reachable today (callers only default to 60/5), but #162 wires these from mcp config where ${ENV}/quoted-YAML can deliver a string or garbage — the same reason token.ts carries coerceTtl.

Fix: coerce both options up front (finite, non-negative; the max window must be positive, tolerance may be 0), falling back to the conservative default otherwise — mirroring coerceTtl. Added tests for NaN/Infinity/negative/zero and numeric-string config-shaped inputs (847 tests, green). Closing the fail-open in the primitive means #162 can't reintroduce it by passing a bad config value through.

[heskew]

Followed up on the timestamp discussion by fixing it at the source in bf376e2: mcp_assertion_jtis.created_at now uses Harper's @createdTime directive instead of a hand-written Math.floor(Date.now()/1000). Since this new store is effectively the template a future MCP store gets copied from, leaning on Harper here keeps us from propagating a hand-rolled-timestamp pattern that would bite later. client_id stays (real denormalized audit data, not a Harper reinvention). The pre-existing tables (csrf_tokens, mcp_auth_codes, mcp_refresh_families) are tracked separately in #168, with this table as the reference.

[kriszyp]

Nice primitives — EdDSA verification, alg pinning, and claims validation all check out cleanly against the real crypto/claims logic.

One thing worth a second look before #162 wires this in: assertionJtiStore.checkAndRecord's replay guard is a non-atomic get-then-put (assertionJtiStore.ts:115). Two concurrent presentations of the same captured assertion within its ~65s validity window can both pass the get before either put lands, so both get treated as first-use. The header already documents this as an accepted tradeoff mirroring MCPAuthCodeStore.consume's precedent, and I didn't find an atomic insert-if-absent primitive on Harper's Table interface to fix it outright — so this reads like a real platform gap rather than something this PR should unilaterally solve. Given it's disclosed, not blocking on it, but I'd want the sign-off to explicitly capture the residual risk (bounded to concurrent in-flight duplicates, not unlimited 60s reuse) rather than leaving it as an internal comment, and a concurrency test pinning the accepted behavior would be good insurance.

— KrAIs

[heskew]

@kriszyp — deep-dive results on your open question, and it changed the code (9f1d2e0).

Your open question resolved: Harper does have an insert-if-absent primitive — Table.create() throws ClientError('Record already exists', 409) on an explicit-id create (resources/Table.ts:1785). Easy to miss because it's undocumented: the Resource API docs describe only the auto-id form and say don't include a primary key.

But it's not commit-time-enforced. Traced the write path: the 409 check runs pre-staging only (getSync); on a storage-level write-write conflict the retry machinery (DatabaseTransaction.ts:364) re-runs the commit closure with the fresh entry and proceeds as a put — so two concurrent creates of the same key can both report success, degrading to last-write-wins. Filed as HarperFast/harper#1745 with the design split: cross-node LWW convergence is by design (docs-only fix there), while per-node 409 enforcement looks implementable through the existing retry/sourceApply seam — local-origin creates could enforce, replication-applied creates must keep converging. The contract decision is yours to make there.

What this PR now does about it:

• checkAndRecord switched from awaited get-then-put to create() + catch-409 — sequential replays are rejected by the storage layer, the same-thread get→put window is gone, and residual exposure narrows to in-flight staging→commit overlap per node / replication lag across nodes. If #1745 lands per-node enforcement, the guard becomes fully atomic per node with zero further change here.
• Your two asks are in: the residual risk is now an explicit sign-off section in the PR description (bounded to concurrent in-flight duplicates minting short-TTL tokens for a key-holding client — not open ~60s reuse, not impersonation), and a concurrency test pins the properties stable under both current and fixed semantics (≥1 winner; replay rejected once settled).

Nice side effect of your review: the disclosure comment did its job, and the primitive it surfaced was hiding behind a docs gap.

[heskew]

Fresh pass on 918cced: no additional findings from this review.

I rechecked the prior window-option issue; maxExpiresInSeconds / clockToleranceSeconds now normalize NaN, Infinity, negative, zero, and config-shaped string inputs before the RFC 7523 validity-window comparisons, and the old repro rejects as expected. I also rechecked the replay guard after the Table.create() switch: the 409 path matches Harper's ClientError.statusCode contract, non-409 storage failures still fail closed, and the residual in-flight create race is explicitly documented and covered by the focused test semantics. The EdDSA/key-selection/claim-validation path still looks aligned with the #159 contract.

---

🤖 Posted by Codex (gpt-5.5) on Nathan's behalf