fix: stop book params from setting cover_image_key and confine storage keys #20

Merged

jfro merged 3 commits from jtk/cover-key-traversal into main 2026-10-18 17:10:28 +00:00

Conversation 0 Commits 3 Files changed 6 +148 -5

jfro commented 2026-10-18 15:59:23 +00:00

Owner

Copy link

Fixes #7 (high): cover_image_key was cast from book params, so a user could set book[cover_image_key]=../../../<file> on create/update, then delete the book or send remove_cover=true, and LocalBackend.delete/1 would File.rm that path (the same key also fed exists?/retrieve_url).

Independent of #18/#19 (branched from main).

Changes

• BookController: cover_image_key is dropped from request params (create, update, add-from-lookup, scan, batch add) and logged at warning if sent. Only the server sets it after storing an upload or download.
• Book.changeset: cover_image_key must look like covers/<uuid>.(jpg|png|gif|webp), the only shape Storage.store_cover produces. Internal callers (sync, the ebook worker) keep working; existing rows aren't revalidated.
• LocalBackend: keys go through Path.safe_relative/1; absolute keys, empty/nil keys and any .. that leaves base_path return {:error, :invalid_key} (or false from exists?) and log a warning, instead of touching the filesystem. Pages with a bad stored key show no cover rather than crashing.

Tests

test/fuzzy_catalog/storage/local_backend_test.exs (traversal, absolute, empty and nil keys refused for store/delete/exists?/retrieve_url; a file outside uploads survives), test/fuzzy_catalog/catalog/book_test.exs (key format), and controller tests that send a traversal key on create and update. mix precommit: 167 tests pass.

Fixes #7 (high): `cover_image_key` was cast from book params, so a user could set `book[cover_image_key]=../../../<file>` on create/update, then delete the book or send `remove_cover=true`, and `LocalBackend.delete/1` would `File.rm` that path (the same key also fed `exists?`/`retrieve_url`). Independent of #18/#19 (branched from main). ## Changes - **BookController**: `cover_image_key` is dropped from request params (create, update, add-from-lookup, scan, batch add) and logged at warning if sent. Only the server sets it after storing an upload or download. - **Book.changeset**: `cover_image_key` must look like `covers/<uuid>.(jpg|png|gif|webp)`, the only shape `Storage.store_cover` produces. Internal callers (sync, the ebook worker) keep working; existing rows aren't revalidated. - **LocalBackend**: keys go through `Path.safe_relative/1`; absolute keys, empty/nil keys and any `..` that leaves `base_path` return `{:error, :invalid_key}` (or `false` from `exists?`) and log a warning, instead of touching the filesystem. Pages with a bad stored key show no cover rather than crashing. ## Tests `test/fuzzy_catalog/storage/local_backend_test.exs` (traversal, absolute, empty and nil keys refused for store/delete/exists?/retrieve_url; a file outside uploads survives), `test/fuzzy_catalog/catalog/book_test.exs` (key format), and controller tests that send a traversal key on create and update. `mix precommit`: 167 tests pass.

jfro added 1 commit 2026-10-18 15:59:23 +00:00

fix: stop book params from setting cover_image_key and confine storage keys ... 7240fdff08

A user could send book[cover_image_key]=../../../<file> on create or
update, then delete the book or remove its cover, and LocalBackend would
File.rm that path.

- BookController drops cover_image_key from request params; only the
  server sets it after storing an upload or download
- Book.changeset only accepts keys shaped like covers/<uuid>.<image ext>
- LocalBackend refuses absolute keys and any .. that leaves base_path
  (Path.safe_relative/1) for store, delete, exists? and retrieve_url

Fixes #7

jfro added 1 commit 2026-10-18 16:29:10 +00:00

Merge remote-tracking branch 'origin/main' into jtk/cover-key-traversal ... 5727a04dca

# Conflicts:
#	test/fuzzy_catalog_web/controllers/book_controller_test.exs

jfro reviewed 2026-10-18 17:02:04 +00:00

jfro left a comment

Copy link

Review by Nye (PR review / audit) — APPROVE @5727a04 (originally @7240fdf)

Closes #7: controller drops cover_image_key from params on create/update and add-from-lookup; Book.changeset validate_format only accepts covers/<uuid>.(jpg|png|gif|webp) (validate_* runs only on changes, so legacy rows still save); LocalBackend uses Path.safe_relative for store/delete/exists?. Every server path that writes keys (store_cover, download_and_store_cover, upload, sync, #17's ebook thumbnails) produces exactly that format, so no regressions. Tests cover changeset, backend and controller. Re-checked after main was merged in: the lib/ diff is identical to 7240fdf, #19's test is kept, CI green.

Merge note: will conflict with #17 in book.ex: keep both (#17's rating validation and this PR's cover_image_key handling).

**Review by Nye (PR review / audit) — APPROVE** @5727a04 (originally @7240fdf) Closes #7: controller drops `cover_image_key` from params on create/update and add-from-lookup; `Book.changeset` `validate_format` only accepts `covers/<uuid>.(jpg|png|gif|webp)` (validate_* runs only on changes, so legacy rows still save); LocalBackend uses `Path.safe_relative` for store/delete/exists?. Every server path that writes keys (store_cover, download_and_store_cover, upload, sync, #17's ebook thumbnails) produces exactly that format, so no regressions. Tests cover changeset, backend and controller. Re-checked after main was merged in: the lib/ diff is identical to 7240fdf, #19's test is kept, CI green. Merge note: will conflict with #17 in book.ex: keep both (#17's rating validation and this PR's cover_image_key handling).

jfro added 1 commit 2026-10-18 17:08:19 +00:00

Merge remote-tracking branch 'origin/main' into jtk/cover-key-traversal ... da1831802e

# Conflicts:
#	lib/fuzzy_catalog/catalog/book.ex

jfro merged commit 9e1c3b9018 into main 2026-10-18 17:10:28 +00:00

jfro deleted branch jtk/cover-key-traversal 2026-10-18 17:10:28 +00:00

jfro referenced this pull request from a commit 2026-10-18 17:10:28 +00:00

fix: stop book params from setting cover_image_key and confine storage keys (#20)

jfro referenced this pull request 2026-10-18 17:24:08 +00:00

fix: keep rating, tags and custom_metadata out of the book form #39

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

  

critical

  

ebook-branch

  

high

  

low

  

main

  

medium

  

release-blocker

  

security

No labels

bug

critical

ebook-branch

high

low

main

medium

release-blocker

security

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

jfro/fuzzy_catalog!20

No description provided.