336 lines
16 KiB
Markdown
336 lines
16 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.
|
||
|
||
### Any authenticated user can delete any library or book
|
||
|
||
`backend/src/chitai/database/models/library.py`, `backend/src/chitai/database/models/user.py`
|
||
|
||
There is no authorization tier. `Library` has no owner column, and `User` carries only
|
||
`email` and `password` — no role, no `is_active`. So every authenticated account can create
|
||
and delete libraries, and delete books along with their files on disk. Per-user scoping
|
||
exists only for reading progress and bookshelves, which `provide_book_service` restricts
|
||
correctly.
|
||
|
||
For a single-household deployment that may well be acceptable. The point is that it is
|
||
emergent rather than chosen. The cheapest meaningful step is an `is_admin` flag gating
|
||
library deletion and `delete_books` — a model change plus a migration.
|
||
|
||
### Basic auth answers a malformed header with a 500
|
||
|
||
`backend/src/chitai/middleware/basic_auth.py` — line 22
|
||
|
||
```python
|
||
username, password = b64decode(auth_header.split("Basic ")[1]).decode().split(":")
|
||
```
|
||
|
||
Nothing guards the parse. A `Bearer` token raises `IndexError`, non-base64 raises
|
||
`binascii.Error`, and a credential with no colon raises `ValueError` — as does a password
|
||
that *contains* one, since there is no `maxsplit=1`. Every case surfaces as a 500 on an
|
||
unauthenticated endpoint. All of them should be 401.
|
||
|
||
### An unknown KOSync API key returns 404
|
||
|
||
`backend/src/chitai/middleware/kosync_auth.py` — line 32
|
||
|
||
`KosyncDeviceService.get_by_api_key` uses `get_one`, which raises `NotFoundError`, but the
|
||
middleware catches only `PermissionDeniedException`. The global handler in
|
||
`exceptions/handlers.py` then renders it as a 404, so a device presenting a bad key is told
|
||
the route does not exist rather than that it is unauthorized. The same file still carries a
|
||
leftover `print(exc)`.
|
||
|
||
Worth doing at the same time: `KosyncDeviceService._generate_api_key` uses
|
||
`secrets.token_hex(8)`. 64 bits is thin for a long-lived bearer credential where 32 bytes
|
||
is the convention.
|
||
|
||
### The multi-book download cannot be driven from a test
|
||
|
||
`backend/src/chitai/services/book.py` — `BookService.get_files`
|
||
|
||
`/books/download` is the only handler returning a Litestar `Stream`, and it cannot be
|
||
exercised through `AsyncTestClient`. The request itself succeeds, then fixture teardown
|
||
hangs: the test transport never sends the `http.disconnect` that the streaming response
|
||
waits on, so the app's lifespan shutdown never completes. Coverage therefore sits at the
|
||
service level, on `get_files` directly.
|
||
|
||
Unresolved whether the endpoint also stalls behind a real ASGI server, where that
|
||
disconnect does arrive. Worth one manual check against `litestar run` before relying on it.
|
||
|
||
### The production image runs the development server
|
||
|
||
`backend/Dockerfile` — the final `CMD`
|
||
|
||
```
|
||
CMD ["litestar", "--app-dir", "chitai", "run", "--host", "0.0.0.0", "--port", "8000"]
|
||
```
|
||
|
||
`litestar run` is the CLI development runner. Production should invoke uvicorn or granian
|
||
directly, with a worker count.
|
||
|
||
### Nothing gates formatting, linting or types
|
||
|
||
`ruff format --check src/` reports 27 of 61 files unformatted, and `ruff check src/` finds
|
||
114 errors — 100 of them unused imports, the rest bare `except`, unused variables and
|
||
`== True` comparisons. `ruff check --fix` clears 51 automatically.
|
||
|
||
There is no `[tool.ruff]` section in `pyproject.toml`, so only ruff's default `E4/E7/E9/F`
|
||
rules run, and no type checker is configured at all despite `# type: ignore` comments in
|
||
the tree. Individually these are trivial; collectively they say nothing runs on commit.
|
||
|
||
## 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: serve individual EPUB entries from the backend with
|
||
`script-src 'none'` on each response, and drive foliate through its loader hooks
|
||
instead of a whole-file blob. This is **net-new capability on both sides**, not a
|
||
rewiring of something that exists — see below.
|
||
|
||
Option 2 also fixes the memory cost below, which is why it is worth more than it looks.
|
||
|
||
#### What option 2 actually involves
|
||
|
||
Today the browser fetches the whole `.epub` from `download/{book_id}/{file_id}`, which
|
||
returns a Litestar `File` and knows nothing about the archive's contents. foliate then
|
||
opens the zip **in the browser** (`makeZipLoader` in `view.js`) and turns every chapter,
|
||
image and stylesheet into a `blob:` URL via `Loader.createURL` in `epub.js`. A `blob:`
|
||
carries no headers of its own — it inherits the CSP of the document that created it —
|
||
which is why there is nowhere to attach a policy except the app shell.
|
||
|
||
foliate's parser never touches the zip directly. `EPUB` is constructed with a loader:
|
||
|
||
```js
|
||
// view.js — makeZipLoader is one implementation; makeDirectoryLoader below is another
|
||
return { entries, loadText, loadBlob, getSize };
|
||
```
|
||
|
||
`name` is the **zip entry path**, because `makeZipLoader` keys its map on
|
||
`entry.filename` — so `OEBPS/Text/chapter01.xhtml`, `OEBPS/Images/cover.jpg`. The parser
|
||
resolves hrefs from the OPF manifest into those names and asks the loader for them,
|
||
without caring where the bytes come from. A third implementation that fetches over HTTP
|
||
is the same shape.
|
||
|
||
**Backend.** An endpoint taking a path inside the archive, e.g.
|
||
`GET books/{book_id}/files/{file_id}/entry/{path:path}`, returning a `Stream` over
|
||
`zipfile.ZipFile.open(name)` so an entry never lands in memory whole, with the content
|
||
type from the manifest and `Content-Security-Policy: script-src 'none'` on the response.
|
||
|
||
Two things to get right:
|
||
|
||
- **`path` is caller-supplied.** Resolve it against the archive's `namelist()` and reject
|
||
anything absent, rather than trusting the string — `../` traversal is the hazard.
|
||
- **`getSize` is synchronous** in foliate's loader contract, and it feeds `SectionProgress`,
|
||
which produces the reading percentage. So the endpoint needs a companion that returns
|
||
entry names and sizes up front — one extra call at open — because sizes cannot be
|
||
discovered per request.
|
||
|
||
Opening the zip per request costs a central-directory read each time. Probably fine for
|
||
chapter-sized reads, worth measuring rather than assuming.
|
||
|
||
Note the backend already reads inside EPUBs — `metadata_extractor.py` uses ebooklib at
|
||
ingest for title, authors, identifiers and the cover. What is missing is serving an
|
||
arbitrary entry by path, not the ability to open the archive.
|
||
|
||
**Frontend.** `foliate-view.svelte` stops calling `view.open(file)` and builds an `EPUB`
|
||
around a loader backed by that endpoint. The whole-file fetch in `epub-reader.svelte`
|
||
goes away with it.
|
||
|
||
### The proxy buffers whole files and drops range headers
|
||
|
||
Two separate problems that both live in `frontend/src/routes/api/[...path]/+server.ts`.
|
||
|
||
**Buffering.** Litestar already streams: `ASGIFileResponse` reads in 1 MB chunks
|
||
(`response/file.py`), so the backend never holds a file whole. The proxy then undoes it
|
||
with `await response.arrayBuffer()`, which does not resolve until the last byte arrives —
|
||
so the whole file sits in the node process, per concurrent reader, and the browser gets
|
||
nothing until it completes. Passing `response.body` straight through restores the stream
|
||
and is a small change.
|
||
|
||
**Range.** The proxy forwards only `Content-Type`, `Content-Disposition` and
|
||
`Content-Length`. It never sends the client's `Range` upstream, and would drop
|
||
`Accept-Ranges` and `Content-Range` coming back — a 206 without `Content-Range` is
|
||
broken. So range support cannot work until the proxy is fixed, whatever the backend does.
|
||
|
||
**Litestar has no range support of its own.** In 2.21.1 the only mention of 206 in the
|
||
whole package is the `HTTP_206_PARTIAL_CONTENT` constant; there is no `Accept-Ranges` or
|
||
`Content-Range` handling anywhere. This has to be written.
|
||
|
||
#### What pdf.js actually needs
|
||
|
||
It decides from the **initial 200 response**, not from anything on a 206.
|
||
`validateRangeRequestCapabilities` in `frontend/static/pdfjs/build/pdf.mjs`:
|
||
|
||
```js
|
||
if (responseHeaders.get("Accept-Ranges") !== "bytes") {
|
||
return returnValues; // allowRangeRequests stays false
|
||
}
|
||
```
|
||
|
||
It also needs a parseable `Content-Length`, `Content-Encoding: identity`, and a length
|
||
greater than twice `rangeChunkSize`. Miss any of those and it downloads the whole file
|
||
however good the range support is.
|
||
|
||
So the single highest-value header is **`Accept-Ranges: bytes` on the ordinary 200** —
|
||
that is what makes pdf.js switch to fetching progressively at all.
|
||
|
||
#### Approach
|
||
|
||
Put it on the existing `get_file` handler in `controllers/book.py`, which already resolves
|
||
`book_id`/`file_id` through the service with library scoping and auth:
|
||
|
||
- No `Range` → `Stream` the file with `Accept-Ranges: bytes` and `Content-Length`.
|
||
- `Range` present → parse, seek, `Stream` with 206 and `Content-Range`.
|
||
- Proxy: forward `Range` up; pass `response.body` through; forward `Accept-Ranges`,
|
||
`Content-Range` and the status back.
|
||
|
||
There are `RangeRequestMiddleware` snippets circulating for Litestar that wrap
|
||
`create_static_files_router`. They are the wrong shape here — book files are served by an
|
||
authenticated handler resolving database ids, not by a directory mapping, and using one
|
||
would mean exposing disk paths as URLs and re-solving ownership checks that already
|
||
exist. The common version also only sets `Accept-Ranges` on the 206, so it would not
|
||
switch pdf.js over, and it derives its path with `str.lstrip(prefix)`, which strips a
|
||
character set rather than a prefix — `/static/castle.pdf` becomes `le.pdf`. Worth reading
|
||
its `parse_range_header` for the parsing rules and writing the rest fresh.
|
||
|
||
### 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.
|