docs: record the outstanding backend issues in TODO.md
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user