Improve CRL retrieval and fix poisoned cache - #894
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new MockHTTPClient and the background refresh scheduling in CRLService have correctness/performance gaps that can cause misleading test results and unintended high-priority refresh work.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves the LCP Certificate Revocation List (CRL) fetching/caching pipeline by preloading CRLs ahead of license validation, refreshing expired CRLs in the background, and preventing “poisoned” caches (eg captive portal HTML cached as CRL) from blocking LCP publication opening.
Changes:
- Refactors
CRLServiceinto an actor, adds CRL payload validation (DER/X.509 shape check), and refreshes expired CRLs asynchronously while still serving the cached value. - Preloads the CRL cache when initializing
LCPServiceto avoid delaying LCP publication opening on first validation. - Adds dedicated CRL unit tests plus a reusable
MockHTTPClienttest helper; updatesCHANGELOG.md.
File summaries
| File | Description |
|---|---|
| Tests/LCPTests/Services/CRLServiceTests.swift | Adds Swift Testing coverage for CRL DER validation and cache/refresh behavior (including poisoned cache scenarios). |
| Tests/LCPTests/MockHTTPClient.swift | Introduces an HTTPClient mock used to deterministically test CRL fetch/refresh flows. |
| Sources/LCP/Services/CRLService.swift | Implements preloading + background refresh and validates cached/fetched CRL payloads before use/caching. |
| Sources/LCP/LCPService.swift | Kicks off CRL preloading during LCPService initialization and reuses the same CRLService instance for license validation. |
| CHANGELOG.md | Documents the CRL preloading, background refresh, and poisoned-cache fix. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2d2dba7 to
148b045
Compare
148b045 to
f964a56
Compare
| // Short timeout to avoid blocking the License, since we can always fall back on the cached CRL. | ||
| let timeout: TimeInterval? = (localCRL == nil) ? nil : 8 | ||
| /// Starts a CRL refresh, or returns the one already in flight. | ||
| private func refresh() -> Task<String, Error> { |
There was a problem hiding this comment.
If a caller cancels their task while waiting for refresh().value, refreshTask remains until the Task finishes. Subsequent calls will get the cancelled Task instead of starting a new one.
There was a problem hiding this comment.
What do you mean by cancelled task in this context? The Task stored in refreshTask is unstructured and cannot be cancelled (by design, we want the fetch to go through).
| let url = HTTPURL(string: "http://crl.edrlab.telesec.de/rl/EDRLab_CA.crl")! | ||
|
|
||
| let response = try await httpClient.fetch(HTTPRequest(url: url, timeoutInterval: timeout)) | ||
| let response = try await httpClient.fetch(HTTPRequest(url: url)) |
There was a problem hiding this comment.
Why remove the timeoutInterval?
There was a problem hiding this comment.
The CRL fetch used to be blocking because it occurred while opening the first publication. That’s why we used to set a short timeout: to display an error quickly if the server didn’t return the CRL.
The CRL is now preloaded when creating the LCPService, so we no longer need the timeout. Removing it also gives the server more leeway.
| .get() | ||
|
|
||
| guard !response.body.isEmpty else { | ||
| guard CRLService.isX509CRL(response.body) else { |
There was a problem hiding this comment.
Should we check that the response is successful here?
There was a problem hiding this comment.
It's already handled, httpClient.fetch() throws if it is not successful.
|
Cross-reference with ThDk edrlab/thorium-reader#3865 |
Changed
LCP
LCPService, and an expired one is refreshed in the background instead of making the user wait for the response.Fixed
LCP
200 OKstatus, which was then cached for seven days and prevented opening LCP publications. An invalid CRL cached by a previous version is now ignored instead of waiting for its expiration.Twin Kotlin PR: readium/kotlin-toolkit#833