fix: only download covers from http(s) URLs, never local paths #18

Merged

jfro merged 1 commit from jtk/cover-url-local-path into main

2026-10-18 16:14:54 +00:00

jfro commented

2026-10-18 15:55:29 +00:00

Owner

Fixes #5 (critical): any logged-in user could post book[cover_url]=/etc/passwd (or a config/env file) to /books/add-from-lookup, scan_barcode or batch_add, and the server copied the file into /uploads/covers/<uuid>.jpg, which is served without auth.

Changes

Storage.download_and_store_cover/1 only accepts http/https URLs with a host. Local paths, file://, other schemes and non-strings return {:error, :unsupported_url} and log a warning.
New Storage.store_synced_cover/1, used only by ExternalLibrarySync: http(s) URLs are downloaded, anything else must be a regular file (checked with lstat, so no symlinks) inside the configured Calibre library_path. store_local_cover/2 now takes the allowed roots.
Storage.store_cover/2 sniffs magic bytes (JPEG/PNG/GIF/WebP) and refuses anything else (:not_an_image); the stored type and extension come from the bytes, not the declared content type or the upload's filename.
Logging: download host, size, type and duration; refused URLs/paths at warning. Error reasons are inspected (interpolating a Req exception struct would have raised).

SSRF (#6) is a separate PR on top of this.

Tests

test/fuzzy_catalog/storage_test.exs (local paths and non-http schemes refused, non-images refused, Calibre covers inside the library accepted, .., sibling-prefix dirs and symlinks refused) and a controller test that posts cover_url=/etc/passwd and checks no cover is stored. mix precommit: 172 tests pass.

Fixes #5 (critical): any logged-in user could post `book[cover_url]=/etc/passwd` (or a config/env file) to `/books/add-from-lookup`, `scan_barcode` or `batch_add`, and the server copied the file into `/uploads/covers/<uuid>.jpg`, which is served without auth. ## Changes - `Storage.download_and_store_cover/1` only accepts `http`/`https` URLs with a host. Local paths, `file://`, other schemes and non-strings return `{:error, :unsupported_url}` and log a warning. - New `Storage.store_synced_cover/1`, used only by `ExternalLibrarySync`: http(s) URLs are downloaded, anything else must be a regular file (checked with `lstat`, so no symlinks) inside the configured Calibre `library_path`. `store_local_cover/2` now takes the allowed roots. - `Storage.store_cover/2` sniffs magic bytes (JPEG/PNG/GIF/WebP) and refuses anything else (`:not_an_image`); the stored type and extension come from the bytes, not the declared content type or the upload's filename. - Logging: download host, size, type and duration; refused URLs/paths at warning. Error reasons are `inspect`ed (interpolating a Req exception struct would have raised). SSRF (#6) is a separate PR on top of this. ## Tests `test/fuzzy_catalog/storage_test.exs` (local paths and non-http schemes refused, non-images refused, Calibre covers inside the library accepted, `..`, sibling-prefix dirs and symlinks refused) and a controller test that posts `cover_url=/etc/passwd` and checks no cover is stored. `mix precommit`: 172 tests pass.

jfro added 1 commit

2026-10-18 15:55:29 +00:00

fix: only download covers from http(s) URLs, never local paths

CI / test (pull_request) Successful in 34s

Details

d7fa451198

download_and_store_cover took any string that existed on disk as a local
file, so a logged-in user could post book[cover_url]=/etc/passwd and get
the file copied into the public covers directory.

- download_and_store_cover only accepts http/https URLs
- local covers (Calibre sync) go through store_synced_cover/1, which only
  reads regular files inside the configured Calibre library path
- store_cover checks the image's magic bytes and picks the type and
  extension from them, ignoring the declared type

Fixes #5

jfro referenced this pull request

2026-10-18 15:57:36 +00:00

fix: block cover downloads from private hosts and cap their size #19

jfro reviewed

2026-10-18 16:14:46 +00:00

jfro left a comment

Review by Nye (PR review / audit) — APPROVE

download_and_store_cover only takes http(s) URLs with a non-empty host; the cover_url path in book_controller can no longer reach the filesystem.
Calibre local covers go through store_synced_cover, confined to calibre.library_path (same config source CalibreProvider uses, so no behaviour gap). Path.expand normalises ...
store_cover sniffs magic bytes, so a non-image can't be stored.
Tests cover local path, non-http schemes, outside-library, symlink, no-library.

Non-blocking follow-up (filed as low): lstat only checks the final path component, so a symlinked directory inside the Calibre library could still point outside it. Fix later by resolving the real path. The library is admin-controlled, so not a blocker.

**Review by Nye (PR review / audit) — APPROVE** - `download_and_store_cover` only takes http(s) URLs with a non-empty host; the `cover_url` path in `book_controller` can no longer reach the filesystem. - Calibre local covers go through `store_synced_cover`, confined to `calibre.library_path` (same config source `CalibreProvider` uses, so no behaviour gap). `Path.expand` normalises `..`. - `store_cover` sniffs magic bytes, so a non-image can't be stored. - Tests cover local path, non-http schemes, outside-library, symlink, no-library. Non-blocking follow-up (filed as low): `lstat` only checks the final path component, so a symlinked *directory* inside the Calibre library could still point outside it. Fix later by resolving the real path. The library is admin-controlled, so not a blocker.

jfro merged commit 017e19bb54 into main

2026-10-18 16:14:54 +00:00

jfro deleted branch jtk/cover-url-local-path

2026-10-18 16:14:54 +00:00

jfro referenced this pull request from a commit

2026-10-18 16:14:54 +00:00

fix: only download covers from http(s) URLs, never local paths (#18)

jfro referenced this pull request

2026-10-18 16:23:37 +00:00

Calibre cover path check only lstats the final path component #28