diff --git a/TODO.md b/TODO.md index 17bc31e..5cf2712 100644 --- a/TODO.md +++ b/TODO.md @@ -49,6 +49,81 @@ Worth adding at the same time: - 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 @@ -102,22 +177,116 @@ Two ways forward: 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. +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. -### Book files are buffered whole, twice +#### What option 2 actually involves -`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. +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. -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. +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