Skip to content

fix(security): report OCSP/CRL cache disposition via target.loadedFromSource#1586

Merged
kriszyp merged 1 commit into
kris/loadedfromsource-target-onlyfrom
fix/cert-cache-disposition
Jul 8, 2026
Merged

fix(security): report OCSP/CRL cache disposition via target.loadedFromSource#1586
kriszyp merged 1 commit into
kris/loadedfromsource-target-onlyfrom
fix/cert-cache-disposition

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 3, 2026

Copy link
Copy Markdown
Member

Summary

The OCSP and CRL certificate-verification paths reported the cached result field (and the trace log line) by calling wasLoadedFromSource?.() on the record returned by the caching table's get(). That value is a plain frozen record with no resource methods, so the call was always undefined and cached was hard-wired to true — every check logged/reported "cache hit" regardless of whether the certificate status was actually fetched from source. wasLoadedFromSource() was removed from the Resource base class in #1576, and the Context.loadedFromSource mirror is removed in #1590.

This observes cache disposition on the RequestTarget of the get, which #1590 makes the sole disposition signal.

Changes

  • ocspVerification.ts / crlVerification.ts — pass a RequestTarget as the get id and read target.loadedFromSource after the get. The target is built with the empty ctor + target.id = cacheKey to avoid URL-parsing cache keys that may contain / or ?. Cert data still travels in the context (2nd) arg, which the source reads as requestContext, so the source fetch is unchanged. CRL is aligned to the same 2-arg get(target, context) convention as OCSP.
  • certificateVerificationSource.ts — read performCRLCheck/performOCSPCheck off the live module object (.default) with memoized, per-method dynamic imports. See "Where to look".
  • crlVerification.ts — import the PKI.js Ed25519/Ed448 patch directly (see "Where to look").
  • Unit tests for both OCSP and CRL: cached:false on a fresh source fetch, cached:true on the subsequent cached read.

Where to look

  • loadAsInstance mode (the subtle correctness point). The cert cache table is left in the default instance mode (loadAsInstance is unset, i.e. !== false), not loadAsInstance=false. Verified empirically that target.loadedFromSource is set true on a miss / false on a hit anyway: getResource routes loadAsInstance !== false tables through _loadRecord, which calls setLoadedFromSource(target, false) on a cache hit and setLoadedFromSource(target, true) on a source fetch (via getFromSource). I did not set loadAsInstance:false — it isn't needed here.
  • certificateVerificationSource.ts — live .default read + memoized imports. A CJS module's ESM-import namespace snapshots its named exports at first import(), so caching the resolved function reference would pin a stale implementation (e.g. a test double past its restore — which leaked across test files and broke the OCSP source before this). Reading off .default (live module.exports) each call fixes it; the memoized per-method import avoids re-hitting the module loader on cache misses.
  • crlVerification.ts — PKI.js Ed25519 patch import (from cross-model review). CRL signature verification uses PKI.js but previously received the Ed25519/Ed448 patch only as a side effect of loading the OCSP module. With the per-method import above, a CRL-only path no longer loads OCSP, so the CRL module now imports the patch itself — otherwise Ed25519/Ed448 issuers would degrade to unknown/failure. Verified that loading only the CRL module applies the patch.

Dependency / base

Stacked on #1590 (kris/loadedfromsource-target-only), which makes target.loadedFromSource the sole cache-disposition signal and removes the Context mirror. Base is set to that branch; once #1590 lands on v5.1 this rebases onto v5.1. Both #1590 and this target v5.1, not main — needs a forward-port to main later.

Notes

  • OCSP and CRL keep their own small cache-read blocks rather than a shared helper (they differ in method/log-tag/config-shape). Deliberate.
  • Cross-model review: Codex flagged the Ed25519 patch coupling (fixed here); Gemini/agy validated the approach — including that instance-mode is correct and loadAsInstance:false was unnecessary — with no blockers.

Note: earlier revisions of this PR read context.loadedFromSource; it was reworked to target.loadedFromSource after the plan changed to #1590 (dropping the Context mirror). Force-pushed.

🤖 Generated with Claude Code (model: Claude Opus 4.8)

@kriszyp kriszyp requested review from DavidCockerill and heskew July 3, 2026 12:41

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the lazy loading of certificate verification modules to avoid stale implementations when testing, and updates the CRL and OCSP verification flows to pass a context object to the cache table, tracking cache disposition via context.loadedFromSource. Additionally, new unit tests have been added to verify cache hits and misses. The review feedback suggests optimizing module loading by caching the dynamic import promises and lazily loading only the required verification module on-demand based on the verification method.

Comment thread security/certificateVerification/certificateVerificationSource.ts Outdated
Comment thread security/certificateVerification/certificateVerificationSource.ts Outdated
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kriszyp kriszyp marked this pull request as ready for review July 3, 2026 13:12
…mSource

The OCSP and CRL verification paths reported the `cached` field (and the trace
log) by calling `wasLoadedFromSource?.()` on the record returned by the caching
table's `get()`. That plain frozen record has no resource methods, so the call
was always `undefined` and `cached` was always `true` (always "cache hit").
`wasLoadedFromSource()` was removed from the Resource base class in #1576, and
the Context mirror of the flag is removed in #1590.

Observe cache disposition on the RequestTarget of the get instead (#1590 makes
`target.loadedFromSource` the sole signal, marked on both hit and miss by the
`_loadRecord` path that serves this instance-mode table).

- ocsp / crl: pass a RequestTarget as the get id (empty ctor + `target.id =
  cacheKey` to avoid URL-parsing keys that may contain '/' or '?'); the cert
  data still travels in the context arg, which the source reads as
  requestContext. Read `target.loadedFromSource` after the get. CRL is aligned
  to the same 2-arg `get(target, context)` convention as OCSP.
- certificateVerificationSource: read performCRLCheck/performOCSPCheck off the
  live module object (`.default`) with memoized, per-method dynamic imports. A
  CJS module's ESM-import namespace snapshots its named exports at first import,
  so a cached function reference would honor a stale implementation forever —
  e.g. a test double pinned past its restore, which leaked across test files.
- crl: import the PKI.js Ed25519/Ed448 patch directly. CRL signature
  verification uses PKI.js but previously got the patch only as a side effect of
  loading the OCSP module; with the per-method import above, a CRL-only path no
  longer loads OCSP, so it must own its patch dependency (else Ed25519/Ed448
  issuers degrade to unknown/failure).

Adds unit tests covering fresh-fetch cached:false and cached-read cached:true
for both OCSP and CRL. Verified the cert cache table marks target.loadedFromSource
true on miss / false on hit despite being instance mode (loadAsInstance unset),
and that loading only the CRL module applies the Ed25519 patch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kriszyp kriszyp force-pushed the fix/cert-cache-disposition branch from f293860 to a391cb8 Compare July 3, 2026 14:55
@kriszyp kriszyp changed the base branch from v5.1 to kris/loadedfromsource-target-only July 3, 2026 14:55
@kriszyp kriszyp changed the title fix(security): report OCSP/CRL cache disposition from context.loadedFromSource fix(security): report OCSP/CRL cache disposition via target.loadedFromSource Jul 3, 2026
@kriszyp kriszyp merged commit 22a1f0c into kris/loadedfromsource-target-only Jul 8, 2026
46 checks passed
@kriszyp kriszyp deleted the fix/cert-cache-disposition branch July 8, 2026 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant