patrick
5305d3bb5e
Scripted EPUBs are a regression from the foliate migration, not a pre-existing gap: epub.js sandboxed without allow-scripts by default, foliate sets it unconditionally. Records grimmory's fix — CSP on per-entry responses rather than app-wide — which also sidesteps the mode-watcher blocker, plus the whole-file buffering it would remove.
167 lines
7.9 KiB
Markdown
167 lines
7.9 KiB
Markdown
# TODO
|
||
|
||
Known issues and deferred work. Agent-facing notes belong in the `AGENTS.md` files;
|
||
this is for things that are broken or missing and not yet scheduled.
|
||
|
||
## Backend
|
||
|
||
### Identifier extraction drops most ISBNs
|
||
|
||
`backend/src/chitai/services/metadata_extractor.py` — `EpubExtractor._extract_identifiers`
|
||
|
||
The EPUB path validates `DC:identifier` values verbatim:
|
||
|
||
```python
|
||
for id in epub.get_metadata("DC", "identifier"):
|
||
if is_valid_isbn(id[0]):
|
||
...
|
||
```
|
||
|
||
`is_valid_isbn` branches on `len(isbn)` being exactly 10 or 13, so anything carrying
|
||
formatting fails. Two consequences:
|
||
|
||
- **Hyphenated ISBNs are silently dropped.** `978-0-486-28211-4` is 17 characters, so it
|
||
never reaches the checksum. Most EPUBs write ISBNs hyphenated, so the majority are lost.
|
||
The PDF path already does this correctly — `_extract_isbns` calls
|
||
`match.replace("-", "")` before validating.
|
||
- **`urn:isbn:` prefixes are dropped** for the same reason. This is a common EPUB form.
|
||
|
||
Fix: normalise before validating — strip a leading `urn:isbn:`, then remove everything
|
||
that isn't `0-9` or `X`. Reuse the PDF path's approach rather than duplicating it.
|
||
|
||
Note this only runs at upload, so fixing it changes nothing for books already imported.
|
||
A backfill would need to re-read the files on disk.
|
||
|
||
### Non-ISBN identifiers are discarded
|
||
|
||
Same function. Anything that isn't a valid ISBN is thrown away, including values EPUBs
|
||
routinely carry: `urn:uuid:…`, `calibre:…`, Google Books volume IDs and ASINs.
|
||
|
||
The `Identifier` model is already generic (`name` + `value`, unique per book), so storing
|
||
them needs no schema change — only the extractor decides what survives. The frontend
|
||
already renders ISBN, ASIN and DOI as links and shows unknown types as plain values, so
|
||
anything stored will display sensibly.
|
||
|
||
Worth adding at the same time:
|
||
|
||
- A DOI regex (`10.\d{4,9}/\S+`) alongside the ISBN scan in `PdfExtractor._extract_isbns`
|
||
— academic PDFs carry one and it is the most useful identifier they have.
|
||
- Deriving ISBN-10 from ISBN-13 when only the latter is present. It is a pure checksum
|
||
conversion and doubles the chance of an external lookup matching.
|
||
|
||
## Frontend
|
||
|
||
### Scripted EPUBs run against the app origin
|
||
|
||
**This is a regression from the foliate-js migration, not a pre-existing gap.**
|
||
|
||
The old epub.js reader never passed `allowScriptedContent`. epub.js defaults it to
|
||
`false`, which sets `iframe.sandbox = "allow-same-origin"` — no `allow-scripts` — so
|
||
script inside a book never ran. The vendored foliate-js sets, unconditionally:
|
||
|
||
```js
|
||
// paginator.js — and the same in fixed-layout.js
|
||
// `allow-scripts` is needed for events because of WebKit bug
|
||
this.#iframe.setAttribute("sandbox", "allow-same-origin allow-scripts");
|
||
```
|
||
|
||
`allow-same-origin` together with `allow-scripts` is the combination that makes the
|
||
sandbox attribute do nothing. Sections are served as same-origin `blob:` URLs, so script
|
||
in a book can reach `/api/*` with the session cookie attached. foliate's README says as
|
||
much and tells you to use a CSP instead; we have not added one.
|
||
|
||
This is not theoretical. Audiobookshelf shipped the same combination and got
|
||
**CVE-2024-35236** — scripted EPUB plus an unrestricted upload gave remote code
|
||
execution; fixed in 2.10.0 by making scripted content a per-library opt-in, off by
|
||
default. Kavita (CVE-2024-39307) and Jellyfin (fixed 10.9.8) are variations on it.
|
||
Write-up: <https://gebir.ge/blog/every-trick-in-the-book/>.
|
||
|
||
**An app-wide CSP is the wrong shape.** `kit.csp` with `script-src: ['self']` also blocks
|
||
`mode-watcher`'s inline `setInitialMode`, which sets the dark class before first paint —
|
||
SvelteKit only nonces the bootstrap script it injects itself, so every page load would
|
||
flash the light theme. Pinning a hash of a third-party inline script breaks silently on
|
||
upgrade.
|
||
|
||
**Grimmory solves it properly**, and it runs foliate-js too. Rather than handing foliate
|
||
the whole file, it serves each EPUB entry from its own endpoint and puts the strict
|
||
policy on that response:
|
||
|
||
```java
|
||
// EpubReaderController.java
|
||
response.setHeader("Content-Security-Policy", "script-src 'none'");
|
||
```
|
||
|
||
The app shell keeps its own, more permissive policy. That works because each section is
|
||
then a real same-origin document with its own header, rather than a `blob:` — and a
|
||
`blob:` inherits the CSP of the document that created it, which is exactly why a header
|
||
on `/api/books/download/…` would achieve nothing today.
|
||
|
||
Two ways forward:
|
||
|
||
1. **Cheap.** Patch the vendored `sandbox` attribute to drop `allow-scripts`, restoring
|
||
what epub.js gave us. Cost is the WebKit bug the upstream comment cites: events inside
|
||
the iframe get swallowed, which would likely break touch/swipe paging and possibly the
|
||
in-iframe keyboard handling in `foliate-view.svelte`. Needs testing before trusting.
|
||
2. **Right.** Follow grimmory: a backend endpoint serving individual EPUB entries with
|
||
`script-src 'none'`, and drive foliate through its `{ loadText, loadBlob, getSize }`
|
||
loader hooks instead of a whole-file blob.
|
||
|
||
Option 2 also fixes the memory cost below, which is why it is worth more than it looks.
|
||
|
||
### Book files are buffered whole, twice
|
||
|
||
`frontend/src/routes/api/[...path]/+server.ts` reads every response with
|
||
`await response.arrayBuffer()` and forwards no `Range` header, so opening a book pulls
|
||
the entire file into the node process and then again into the browser. A large art PDF
|
||
or a comic archive is tens of megabytes each time, per reader.
|
||
|
||
Serving EPUB entries individually (option 2 above) removes it for EPUB. PDFs would still
|
||
want range support in the proxy, which is what the vendored pdf.js viewer expects and
|
||
currently never gets.
|
||
|
||
### Remove the epub.js locations-cache purge
|
||
|
||
`frontend/src/lib/reader/legacy-cache.ts` — `purgeLegacyLocationCache`
|
||
|
||
The epub.js reader cached generated locations in `localStorage` under
|
||
`${bookId}-locations`, a few hundred KB of JSON per long book. foliate computes
|
||
progress from section byte sizes at open time, so nothing writes those keys any
|
||
more, but existing browsers still hold them — and a reader near the 5–10 MB
|
||
origin quota would make the new reader-settings write throw `QuotaExceededError`.
|
||
|
||
The reader clears them once per browser, behind a `chitai:locations-purged` flag.
|
||
Delete the module, its call in `epub-reader.svelte` and the flag once deployments
|
||
have had a release or two to run it — after roughly 2026-12.
|
||
|
||
### Cover dimensions are unknown until load
|
||
|
||
`book-cover.svelte` renders covers at a fixed height with natural width so nothing is
|
||
cropped or distorted. Because the intrinsic size is not known ahead of time, the text
|
||
beside the cover settles once when the image loads. `min-width` bounds the movement but
|
||
does not remove it.
|
||
|
||
Removing it properly means storing cover dimensions at ingest and emitting them as
|
||
`width`/`height` attributes so the browser reserves exact space. That is a model change
|
||
plus a migration.
|
||
|
||
### No series navigation
|
||
|
||
`Book` has `series` and `series_position`, and the detail page shows both, but there is no
|
||
way to reach the other volumes. The books list endpoint filters by author, publisher, tag,
|
||
shelf and progress — a `series` filter would need adding in
|
||
`backend/src/chitai/services/filters/book.py` and wiring through
|
||
`services/dependencies.py`, following the existing `AuthorFilter` pattern.
|
||
|
||
### Book pages are sparse for most books
|
||
|
||
EPUB metadata is thin: most imports arrive with a title, an author and nothing else. Two
|
||
independent directions, neither started:
|
||
|
||
- **Use what exists.** "More by this author" (the list endpoint already accepts
|
||
`authors=`), and exposing `Book.created_at` and `BookProgress.updated_at` — both are in
|
||
the database, neither is on `BookRead` / `BookProgressRead`.
|
||
- **Fetch from outside.** Open Library or Google Books lookup by ISBN for descriptions and
|
||
covers. This is what actually fixes the sparseness, but it needs outbound requests, rate
|
||
limiting, a manual-vs-automatic decision, and a rule for not clobbering hand-edited
|
||
metadata.
|