fix(books): enforce library access on file-delete write path (bookshelf-ubad) #629

Merged

zombor merged 1 commit from bd-bookshelf-ubad into main 2026-06-19 02:21:48 +00:00

Conversation 2 Commits 1 Files changed 6 +67 -29

zombor commented 2026-06-19 02:11:16 +00:00

Owner

Copy link

Summary

• DeleteBookFile service now accepts a checkAccess(ctx, userID, bookID) error func and calls it before fetching the file record; returns ErrNotFound on miss (no existence leak)
• DeleteBookFileHandler now accepts userIDFromRequest func(*http.Request) int64 (replacing extractUser tmpl.UserExtractor) and threads userID into the service call
• wire.go injects checkBookAccess (same func used by all read paths) and userIDFromRequest
• Tests updated: service test covers the new checkAccess failure case; handler test asserts the userID is passed through

Security fix

Previously a user with PermissionDeleteBook could delete a book_file from a book in a library they had no access to — GetBookFileForDelete only checked bf.id=? AND bf.book_id=? AND b.deleted=0 with no library scope. The detach path's same gap is being fixed separately in jmn7.6/#628.

Test plan

• make test passes (all internal/ packages green)
• DeleteBookFile service test: user without library access gets ErrNotFound, no file is fetched or deleted
• DeleteBookFileHandler test: userID is passed to service
• No coverage regressions

Closes bead bookshelf-ubad on merge.

## Summary - `DeleteBookFile` service now accepts a `checkAccess(ctx, userID, bookID) error` func and calls it before fetching the file record; returns `ErrNotFound` on miss (no existence leak) - `DeleteBookFileHandler` now accepts `userIDFromRequest func(*http.Request) int64` (replacing `extractUser tmpl.UserExtractor`) and threads `userID` into the service call - `wire.go` injects `checkBookAccess` (same func used by all read paths) and `userIDFromRequest` - Tests updated: service test covers the new `checkAccess` failure case; handler test asserts the userID is passed through ## Security fix Previously a user with `PermissionDeleteBook` could delete a `book_file` from a book in a library they had no access to — `GetBookFileForDelete` only checked `bf.id=? AND bf.book_id=? AND b.deleted=0` with no library scope. The detach path's same gap is being fixed separately in jmn7.6/#628. ## Test plan - [x] `make test` passes (all `internal/` packages green) - [x] `DeleteBookFile` service test: user without library access gets `ErrNotFound`, no file is fetched or deleted - [x] `DeleteBookFileHandler` test: userID is passed to service - [x] No coverage regressions Closes bead bookshelf-ubad on merge.

zombor added 1 commit 2026-06-19 02:11:16 +00:00

fix(books): enforce library access on file-delete write path (bookshelf-ubad) ... c9f6cfd27b

DeleteBookFile now accepts a checkAccess(ctx, userID, bookID) func and calls
it before fetching the file record, returning ErrNotFound on miss (no
existence leak). DeleteBookFileHandler now accepts userIDFromRequest instead
of extractUser and threads userID into the service call. Wire.go injects
checkBookAccess (same func used by all read paths) + userIDFromRequest.

Previously a user with PermissionDeleteBook could delete a book_file from
a book in a library they had no access to — the query had no library_id
IN user_library_mapping scope.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-06-19 02:16:06 +00:00

Author

Owner

Copy link

Code Review — bookshelf-ubad

Phase 0: DEMO Verification

No DEMO block exists in this bead (this is a security fix, not a user-facing feature change). Proceeding to spec review per instructions.

---

Phase 1: Spec Compliance

The PR does exactly what the spec asks:

• checkAccess (wired as checkBookAccess) is injected into DeleteBookFile as the first curried dep.
• The check fires before the getFile ownership check and before the transaction — correct placement.
• Returns middleware.ErrNotFound on miss (no existence leak: the error is fmt.Errorf("delete book file: %w", ErrNotFound), unwrappable via errors.Is).
• Handler already handles ErrNotFound at delete_file_handler.go:68 — no change needed there.
• wire.go:677 correctly threads the existing checkBookAccess closure (same one used by reads/LLM/reading sessions) — single source of truth.
• The old nil-safe extractUser path is replaced with userIDFromRequest, consistent with all other handlers.

---

Phase 2: Code Quality

Correctness of the guard:

• CheckBookAccess (service.go:660) skips the check when userID == 0. The old handler defaulted to userID = 0 when extractUser was nil. The new userIDFromRequest always returns d.ExtractUser(r).ID, which is the stub user ID (1) until real auth lands. The bypass is gone — correct.
• Content-restriction denials from checkAccess produce ErrForbidden, which is wrapped at delete_file_service.go:47 but not caught by the handler switch (delete_file_handler.go:63-70). It falls to the generic fmt.Errorf("delete book file: %w", err) path, yielding a 500 instead of 403. This is pre-existing behavior shared by all other paths that use checkBookAccess — the handler switch never checks ErrForbidden. It is not introduced by this PR and is consistent with how the read path behaves today. No finding for this PR.

Test quality:

[MINOR] internal/books/delete_file_service_test.go:93-101 — "does not fetch the file record" assertion tests the wrong observable
The It asserts removedPaths is empty, but removedPaths is populated by removeFile (the disk-removal step), not by getFile. The comment in the test acknowledges this weakness and correctly notes the real guard is the ErrNotFound assertion above. The name is misleading — removedPaths being empty proves the disk-removal did not run, not that getFile was skipped. Suggest renaming to "does not remove the file from disk" (which matches what is actually asserted) or adding a boolean-capture to the getFile stub. No correctness impact — the ErrNotFound It above already catches any regression.

Conventions:

• Curried-function pattern: correct throughout.
• var (...) declarations at top of closures: maintained.
• One Expect per It: maintained.
• BeforeEach / JustBeforeEach separation: correct.
• routes_test.go updated in both RegisterRoutes and RegisterRoutes permission gates suites: complete.

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review — bookshelf-ubad ### Phase 0: DEMO Verification No DEMO block exists in this bead (this is a security fix, not a user-facing feature change). Proceeding to spec review per instructions. --- ### Phase 1: Spec Compliance The PR does exactly what the spec asks: - `checkAccess` (wired as `checkBookAccess`) is injected into `DeleteBookFile` as the first curried dep. - The check fires **before** the `getFile` ownership check and before the transaction — correct placement. - Returns `middleware.ErrNotFound` on miss (no existence leak: the error is `fmt.Errorf("delete book file: %w", ErrNotFound)`, unwrappable via `errors.Is`). - Handler already handles `ErrNotFound` at `delete_file_handler.go:68` — no change needed there. - `wire.go:677` correctly threads the existing `checkBookAccess` closure (same one used by reads/LLM/reading sessions) — single source of truth. - The old `nil`-safe `extractUser` path is replaced with `userIDFromRequest`, consistent with all other handlers. --- ### Phase 2: Code Quality **Correctness of the guard:** - `CheckBookAccess` (service.go:660) skips the check when `userID == 0`. The old handler defaulted to `userID = 0` when `extractUser` was nil. The new `userIDFromRequest` always returns `d.ExtractUser(r).ID`, which is the stub user ID (1) until real auth lands. The bypass is gone — correct. - Content-restriction denials from `checkAccess` produce `ErrForbidden`, which is wrapped at `delete_file_service.go:47` but **not** caught by the handler switch (`delete_file_handler.go:63-70`). It falls to the generic `fmt.Errorf("delete book file: %w", err)` path, yielding a 500 instead of 403. This is pre-existing behavior shared by all other paths that use `checkBookAccess` — the handler switch never checks `ErrForbidden`. It is **not introduced by this PR** and is consistent with how the read path behaves today. No finding for this PR. **Test quality:** [MINOR] internal/books/delete_file_service_test.go:93-101 — "does not fetch the file record" assertion tests the wrong observable The `It` asserts `removedPaths` is empty, but `removedPaths` is populated by `removeFile` (the disk-removal step), not by `getFile`. The comment in the test acknowledges this weakness and correctly notes the real guard is the `ErrNotFound` assertion above. The name is misleading — `removedPaths` being empty proves the disk-removal did not run, not that `getFile` was skipped. Suggest renaming to `"does not remove the file from disk"` (which matches what is actually asserted) or adding a boolean-capture to the `getFile` stub. No correctness impact — the `ErrNotFound` `It` above already catches any regression. **Conventions:** - Curried-function pattern: correct throughout. - `var (...)` declarations at top of closures: maintained. - One `Expect` per `It`: maintained. - `BeforeEach` / `JustBeforeEach` separation: correct. - `routes_test.go` updated in both `RegisterRoutes` and `RegisterRoutes permission gates` suites: complete. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-06-19 02:17:17 +00:00

Author

Owner

Copy link

Security Review — bookshelf-ubad (PR #629)

Reviewed per .claude/rules/review-standard.md.

Scope

The PR adds a checkAccess (library-access) guard to the single DELETE /books/{id}/files/{fileID} path. No other delete-file paths exist in the router.

Finding: check position and completeness

Access check position: checkAccess is invoked at the top of DeleteBookFile before getFile, deleteFileTx, and the disk removeFile. This is correct — no mutation occurs before the ownership/library check.

userID source: userIDFromRequest is derived from d.ExtractUser(r).ID, the authenticated session, consistent with every other access-checked handler in the package. Not request-body or query-param derived.

Existence leak — library-access (ErrNotFound): when the user's library set does not include the book's library, CheckBookAccess returns ErrNotFound (wrapped). DeleteBookFile re-wraps and propagates it. The handler's switch passes it straight through (return err), yielding HTTP 404. No existence leak.

Existence leak — content restriction (ErrForbidden): when a content restriction blocks access, CheckBookAccess returns ErrForbidden. DeleteBookFile wraps it. The handler switch does not intercept ErrForbidden; it falls through to return fmt.Errorf("delete book file: %w", err). errors.Is unwraps transitively, so the error mapper returns HTTP 403. This is consistent with the read-path Show handler, which also surfaces ErrForbidden as 403 rather than normalising to 404. A user with delete permission can infer that a 403 means the book exists but is content-restricted, while a 404 means it does not exist or is library-inaccessible. This is an intentional design choice shared across the read and write paths; it mirrors the existing Show handler semantics and is not a regression introduced by this PR.

Adversarial path — can a delete-permitted user delete a file in a library they can't access? No. checkAccess runs before any mutation. CheckBookAccess is fail-closed: an authenticated user with no library mappings sees ErrNotFound for every book; a user whose library set does not contain the book's library also sees ErrNotFound. There is no bypass.

userID == 0 bypass: CheckBookAccess skips the check when userID == 0. The auth middleware guards DELETE routes before the handler is reached, so an unauthenticated request does not arrive with userID = 0 in production. The pre-existing behaviour is unchanged and out of scope for this PR.

Wire-up verified: wire.go on branch bd-bookshelf-ubad passes checkBookAccess (the same CheckBookAccess instance used by Get, reader handlers, etc.) as the first argument to DeleteBookFile, and userIDFromRequest to DeleteBookFileHandler. No stale d.ExtractUser remains on the DeleteBookFileHandler call site.

Single route: routes.go registers exactly one DELETE /books/{id}/files/{fileID} route. No "delete-formats" or bulk-file-delete secondary path exists.

Test coverage

• delete_file_service_test.go adds a Describe("when the user does not have access to the book's library") block covering ErrNotFound return and removedPaths empty assertion.
• delete_file_handler_test.go adds It("passes the user ID to the delete service") and verifies receivedUserID == 42.
• The "does not fetch the file record" It is noted as structurally weak in the comment (asserts removedPaths empty, not that getFile was not called), but the ErrNotFound sibling It is the real guard; the comment is accurate. No correction needed.

No injection / no new leak

No SQL built from request parameters; no new surface area beyond what was previously guarded by PermissionDeleteBook.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — bookshelf-ubad (PR #629) Reviewed per `.claude/rules/review-standard.md`. ### Scope The PR adds a `checkAccess` (library-access) guard to the single `DELETE /books/{id}/files/{fileID}` path. No other delete-file paths exist in the router. ### Finding: check position and completeness **Access check position:** `checkAccess` is invoked at the top of `DeleteBookFile` *before* `getFile`, `deleteFileTx`, and the disk `removeFile`. This is correct — no mutation occurs before the ownership/library check. **userID source:** `userIDFromRequest` is derived from `d.ExtractUser(r).ID`, the authenticated session, consistent with every other access-checked handler in the package. Not request-body or query-param derived. **Existence leak — library-access (ErrNotFound):** when the user's library set does not include the book's library, `CheckBookAccess` returns `ErrNotFound` (wrapped). `DeleteBookFile` re-wraps and propagates it. The handler's switch passes it straight through (`return err`), yielding HTTP 404. No existence leak. **Existence leak — content restriction (ErrForbidden):** when a content restriction blocks access, `CheckBookAccess` returns `ErrForbidden`. `DeleteBookFile` wraps it. The handler switch does not intercept `ErrForbidden`; it falls through to `return fmt.Errorf("delete book file: %w", err)`. `errors.Is` unwraps transitively, so the error mapper returns HTTP 403. This is consistent with the read-path `Show` handler, which also surfaces `ErrForbidden` as 403 rather than normalising to 404. A user with delete permission *can* infer that a 403 means the book exists but is content-restricted, while a 404 means it does not exist or is library-inaccessible. This is an **intentional design choice** shared across the read and write paths; it mirrors the existing `Show` handler semantics and is not a regression introduced by this PR. **Adversarial path — can a delete-permitted user delete a file in a library they can't access?** No. `checkAccess` runs before any mutation. `CheckBookAccess` is fail-closed: an authenticated user with no library mappings sees ErrNotFound for every book; a user whose library set does not contain the book's library also sees ErrNotFound. There is no bypass. **userID == 0 bypass:** `CheckBookAccess` skips the check when `userID == 0`. The auth middleware guards `DELETE` routes before the handler is reached, so an unauthenticated request does not arrive with `userID = 0` in production. The pre-existing behaviour is unchanged and out of scope for this PR. **Wire-up verified:** `wire.go` on branch `bd-bookshelf-ubad` passes `checkBookAccess` (the same `CheckBookAccess` instance used by `Get`, reader handlers, etc.) as the first argument to `DeleteBookFile`, and `userIDFromRequest` to `DeleteBookFileHandler`. No stale `d.ExtractUser` remains on the `DeleteBookFileHandler` call site. **Single route:** `routes.go` registers exactly one `DELETE /books/{id}/files/{fileID}` route. No "delete-formats" or bulk-file-delete secondary path exists. ### Test coverage - `delete_file_service_test.go` adds a `Describe("when the user does not have access to the book's library")` block covering `ErrNotFound` return and `removedPaths` empty assertion. - `delete_file_handler_test.go` adds `It("passes the user ID to the delete service")` and verifies `receivedUserID == 42`. - The "does not fetch the file record" `It` is noted as structurally weak in the comment (asserts `removedPaths` empty, not that `getFile` was not called), but the ErrNotFound sibling `It` is the real guard; the comment is accurate. No correction needed. ### No injection / no new leak No SQL built from request parameters; no new surface area beyond what was previously guarded by `PermissionDeleteBook`. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor merged commit 7abeba1a9b into main 2026-06-19 02:21:48 +00:00

zombor referenced this pull request from a commit 2026-06-19 02:21:52 +00:00

Merge pull request 'fix(books): enforce library access on file-delete write path (bookshelf-ubad)' (#629) from bd-bookshelf-ubad into main

zombor referenced this pull request 2026-06-19 02:33:29 +00:00

feat(books): replace book content in place (bookshelf-jmn7.7) #632

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

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

zombor/pergamum!629

No description provided.