Improve CRL retrieval and fix poisoned cache - #833
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new Crl.kt uses experimental kotlin.io.encoding.Base64 without opting in, and the updated test suite should clear shared preferences between cases to avoid order-dependent failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses LCP publications failing to open when a captive portal response (HTML with HTTP 200) was mistakenly cached as the CRL, by validating CRL payloads as genuine X.509 CRLs before caching and by ignoring previously cached invalid CRLs.
Changes:
- Added a
Crlvalue class to parse/validate DER- and PEM-encoded CRLs usingCertificateFactory. - Updated
CRLServiceto validate the fetched CRL before caching and to ignore invalid cached CRLs. - Expanded Robolectric test coverage for valid/invalid CRLs (including legacy wrapped Base64) and documented the fix in the changelog.
File summaries
| File | Description |
|---|---|
| readium/lcp/src/test/java/org/readium/r2/lcp/service/CRLServiceTest.kt | Adds fixtures and new tests covering valid CRL caching, captive portal HTML rejection, and legacy wrapped Base64 handling. |
| readium/lcp/src/main/java/org/readium/r2/lcp/service/CRLService.kt | Validates fetched/cached CRLs and prevents caching invalid responses; introduces a shared CRL URL constant. |
| readium/lcp/src/main/java/org/readium/r2/lcp/service/Crl.kt | New validated CRL wrapper providing DER→PEM conversion and PEM parsing with X.509 CRL verification. |
| CHANGELOG.md | Adds an LCP entry describing the captive-portal/CRL-cache fix (issue #832). |
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
There is at least one compile-blocking issue (missing opt-in for experimental kotlin.io.encoding.Base64) plus an unresolved lifecycle concern for the newly introduced long-lived coroutine scope.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
readium/lcp/src/main/java/org/readium/r2/lcp/service/Crl.kt:12
kotlin.io.encoding.Base64is still experimental; this file usesBase64.encode/Base64.Mime.decodewithout opting in toExperimentalEncodingApi, which can fail compilation (and diverges from the existing pattern used elsewhere in the module).
import kotlin.io.encoding.Base64
import org.readium.r2.lcp.BuildConfig.DEBUG
- Files reviewed: 5/6 changed files
- Comments generated: 1
- Review effort level: Lite
|
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.