fix(library): scope show + scan-status by user library access (bookshelf-q14o) #644

Merged

zombor merged 1 commit from bd-bookshelf-q14o into main 2026-06-19 20:22:25 +00:00

Conversation 2 Commits 1 Files changed 3 +155 -5

zombor commented 2026-06-19 15:09:14 +00:00

Owner

Copy link

Summary

GET /libraries/{id} (showHandler) and GET /libraries/{id}/scan-status (scanStatusHandler) were gated only by AuthMiddleware — any authenticated user could fetch library detail (including filesystem paths) and scan-status for any library ID, even libraries not in their user_library_mapping.

Changes

• Add ownership check to showHandler: resolve user's library IDs via GetUserLibraryIDs(stubUserID), return ErrNotFound if the requested ID is not in the set. Fail-closed on empty set.
• Add same ownership check to scanStatusHandler with matching pattern.
• Import slices in handler.go (already used in health_handler.go).
• Update existing handler_test.go and scan_handler_test.go tests to provide GetUserLibraryIDs stub in Deps where needed.
• Add new test contexts for: user lacks access (→ 404), GetUserLibraryIDs returns error (→ 500), empty access set (→ 404 fail-closed).
• Filed bookshelf-evu5 as follow-up for the still-unscoped GET /libraries list endpoint.

Mirrors the pattern established by healthHandler in #633.

Test Plan

• All existing unit tests updated and passing (no panic on nil GetUserLibraryIDs)
• New ownership test cases for showHandler: user lacks access → 404, DB error → 500, empty set → 404
• New ownership test cases for scanStatusHandler: user lacks access → 404, DB error → 500, empty set → 404
• make test green
• make coverage green (100% gate passes)

Closes bead bookshelf-q14o on merge.

## Summary `GET /libraries/{id}` (showHandler) and `GET /libraries/{id}/scan-status` (scanStatusHandler) were gated only by AuthMiddleware — any authenticated user could fetch library detail (including filesystem paths) and scan-status for any library ID, even libraries not in their `user_library_mapping`. ## Changes - Add ownership check to `showHandler`: resolve user's library IDs via `GetUserLibraryIDs(stubUserID)`, return `ErrNotFound` if the requested ID is not in the set. Fail-closed on empty set. - Add same ownership check to `scanStatusHandler` with matching pattern. - Import `slices` in `handler.go` (already used in `health_handler.go`). - Update existing `handler_test.go` and `scan_handler_test.go` tests to provide `GetUserLibraryIDs` stub in `Deps` where needed. - Add new test contexts for: user lacks access (→ 404), `GetUserLibraryIDs` returns error (→ 500), empty access set (→ 404 fail-closed). - Filed `bookshelf-evu5` as follow-up for the still-unscoped `GET /libraries` list endpoint. Mirrors the pattern established by `healthHandler` in #633. ## Test Plan - [x] All existing unit tests updated and passing (no panic on nil `GetUserLibraryIDs`) - [x] New ownership test cases for showHandler: user lacks access → 404, DB error → 500, empty set → 404 - [x] New ownership test cases for scanStatusHandler: user lacks access → 404, DB error → 500, empty set → 404 - [x] `make test` green - [x] `make coverage` green (100% gate passes) Closes bead bookshelf-q14o on merge.

zombor added 1 commit 2026-06-19 15:09:16 +00:00

fix(library): scope show + scan-status by user library access (bookshelf-q14o) ... dab1f1ff0c

GET /libraries/{id} and GET /libraries/{id}/scan-status were gated only by
AuthMiddleware — any authenticated user could fetch detail (including library
filesystem paths) and scan-status for any library ID, even libraries not in
their user_library_mapping.

Add an ownership check to both handlers: resolve the user's library IDs via
GetUserLibraryIDs (stubUserID=1 until real auth lands, bookshelf-4op), then
return ErrNotFound if the requested ID is not in the set — fail-closed on
empty set too. Mirrors the pattern established by healthHandler (#633).

Also adds tests for the access-denied, GetUserLibraryIDs-error, and
empty-set cases for both endpoints.

Follow-up bookshelf-evu5 tracks the still-unscoped GET /libraries list.

zombor commented 2026-06-19 15:17:33 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Phase 0: DEMO Verification

No DEMO block is present in the bead. The spec is a pure ownership/scoping fix with no interactive demo surface (the change is a security gate, not a new feature). The review proceeds on the diff.

Phase 1: Spec Compliance

All stated requirements verified:

• showHandler ownership check fires before handler.WantsJSON branch — both JSON and HTML paths are gated. The HTML redirect path (http.Redirect(w, r, "/", http.StatusSeeOther)) is only reachable after slices.Contains passes. Confirmed.
• scanStatusHandler ownership check fires before d.GetScanStatus — no scan data is returned before the gate. Confirmed.
• Not-found path does not leak existence: fmt.Errorf("library show: library %d: %w", id, middleware.ErrNotFound) wraps middleware.ErrNotFound, same as a genuinely missing library. Confirmed.
• Fail-closed on empty set: slices.Contains([]int64{}, id) is always false. Confirmed by dedicated "empty set" test contexts in both test files.
• stubUserID comes from internal/library/bulk_handler.go:17 (const stubUserID = int64(1)) — a package constant, not from a request body or query param. Confirmed.
• Pattern matches healthHandler in internal/library/health_handler.go exactly: same slices.Contains guard, same middleware.ErrNotFound wrap, same stubUserID reference, same var (...) / libraryIDs top-of-block declaration.
• GetUserLibraryIDs dep wired into Deps struct and used correctly — no interface leakage.
• Follow-up bead evu5 for the unscoped GET /libraries list is correctly out of scope and not flagged.

Phase 2: Code Quality

Conventions:

• var block at top of handler function with libraryIDs []int64 declared before use — correct per project style.
• Curried-function pattern: d.GetUserLibraryIDs injected via Deps, not a global or interface. Correct.
• slices.Contains import added cleanly.

Test conventions:

• stubUserLibraryIDs helper defined once in handler_test.go at package scope, reused across both test files (same package). Clean.
• Each new Context has its own BeforeEach with a complete Deps struct, no shared mutable state leaking between contexts.
• Each It block has exactly one Expect. Correct.
• BeforeEach does setup; JustBeforeEach does invocation (server + request). Correct separation.
• Comment in the invalid-ID test block correctly explains why GetUserLibraryIDs is not needed (parseID fails before the ownership check).

Security:

• Both handlers scope filesystem-path-bearing data behind the ownership check before any data is read from DB or returned to the client.
• No per-user data reaches the wire before the slices.Contains gate in either handler.

No issues found.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

CODE REVIEW: APPROVED ## Phase 0: DEMO Verification No DEMO block is present in the bead. The spec is a pure ownership/scoping fix with no interactive demo surface (the change is a security gate, not a new feature). The review proceeds on the diff. ## Phase 1: Spec Compliance All stated requirements verified: - `showHandler` ownership check fires before `handler.WantsJSON` branch — both JSON and HTML paths are gated. The HTML redirect path (`http.Redirect(w, r, "/", http.StatusSeeOther)`) is only reachable after `slices.Contains` passes. Confirmed. - `scanStatusHandler` ownership check fires before `d.GetScanStatus` — no scan data is returned before the gate. Confirmed. - Not-found path does not leak existence: `fmt.Errorf("library show: library %d: %w", id, middleware.ErrNotFound)` wraps `middleware.ErrNotFound`, same as a genuinely missing library. Confirmed. - Fail-closed on empty set: `slices.Contains([]int64{}, id)` is always false. Confirmed by dedicated `"empty set"` test contexts in both test files. - `stubUserID` comes from `internal/library/bulk_handler.go:17` (`const stubUserID = int64(1)`) — a package constant, not from a request body or query param. Confirmed. - Pattern matches `healthHandler` in `internal/library/health_handler.go` exactly: same `slices.Contains` guard, same `middleware.ErrNotFound` wrap, same `stubUserID` reference, same `var (...) / libraryIDs` top-of-block declaration. - `GetUserLibraryIDs` dep wired into `Deps` struct and used correctly — no interface leakage. - Follow-up bead evu5 for the unscoped `GET /libraries` list is correctly out of scope and not flagged. ## Phase 2: Code Quality **Conventions:** - `var` block at top of handler function with `libraryIDs []int64` declared before use — correct per project style. - Curried-function pattern: `d.GetUserLibraryIDs` injected via `Deps`, not a global or interface. Correct. - `slices.Contains` import added cleanly. **Test conventions:** - `stubUserLibraryIDs` helper defined once in `handler_test.go` at package scope, reused across both test files (same package). Clean. - Each new `Context` has its own `BeforeEach` with a complete `Deps` struct, no shared mutable state leaking between contexts. - Each `It` block has exactly one `Expect`. Correct. - `BeforeEach` does setup; `JustBeforeEach` does invocation (server + request). Correct separation. - Comment in the invalid-ID test block correctly explains why `GetUserLibraryIDs` is not needed (`parseID` fails before the ownership check). **Security:** - Both handlers scope filesystem-path-bearing data behind the ownership check before any data is read from DB or returned to the client. - No per-user data reaches the wire before the `slices.Contains` gate in either handler. **No issues found.** REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-19 15:17:35 +00:00

Author

Owner

Copy link

SECURITY REVIEW — PR #644 (bookshelf-q14o)

Scope: IDOR fix on GET /libraries/{id} (showHandler) and GET /libraries/{id}/scan-status (scanStatusHandler).

---

Findings

[MAJOR] internal/library/handler.go — Write handlers lack GetUserLibraryIDs ownership check (out-of-scope but noted)

updateHandler (PUT/PATCH), deleteHandler, addPathHandler, deletePathHandler, editModalHandler, and triggerScanHandler (POST /scan) operate on an arbitrary library ID taken from the URL path without calling GetUserLibraryIDs. They are gated by the ManipulateLibrary permission middleware (403 on lacking the permission flag), but that is a capability gate, not an ownership gate — a user with permission_manipulate_library can update, delete, or trigger a scan on any library they do not own. This is the same class of IDOR as the one this PR fixes on the read endpoints.

This is pre-existing debt, not introduced by this PR. However the PR description says the read endpoints "expose filesystem paths" — the write endpoints modify or delete those libraries, which is at least equally sensitive. Flagging as MAJOR so a follow-up bead is filed; it should not block merging THIS diff which is a net improvement.

Suggested fix: apply the same GetUserLibraryIDs + slices.Contains guard to each write handler before the mutation call, following the pattern established here.

---

This PR — verified clean on:

• Ownership check placement in showHandler: called before the WantsJSON branch (line ~140), so both the JSON response path and the HTML redirect path are guarded. Neither branch can reach d.Get without passing the check. ✓
• Ownership check placement in scanStatusHandler: called before d.GetScanStatus (line ~672). ✓
• userID source: stubUserID is a package-level constant int64(1) defined in bulk_handler.go:17 — not from path/query/body. ✓
• Fail-closed (empty set): slices.Contains([], id) returns false → ErrNotFound. Test "when the user has no library access (empty set)" asserts 404. ✓
• No existence leak: unauthorized ID returns fmt.Errorf("...: %w", middleware.ErrNotFound) — identical error class as a missing record. The middleware translates both to 404 with no distinguishing body. ✓
• DB error path: GetUserLibraryIDs error returns a wrapped error (500), not a data leak. Test "when GetUserLibraryIDs returns an error" asserts 500. ✓
• Test coverage: three new Context blocks per endpoint (lacks-access→404, empty-set→404, db-error→500). Existing happy-path tests updated with stubUserLibraryIDs(id) so they still pass the check. ✓
• Content-negotiation HTML branch of showHandler: HTML path redirects to / after the ownership check — the redirect is gated, not a bypass. ✓

---

REVIEW VERDICT: 0 blocker, 1 major, 0 minor

## SECURITY REVIEW — PR #644 (bookshelf-q14o) **Scope:** IDOR fix on `GET /libraries/{id}` (showHandler) and `GET /libraries/{id}/scan-status` (scanStatusHandler). --- ### Findings **[MAJOR] internal/library/handler.go — Write handlers lack GetUserLibraryIDs ownership check (out-of-scope but noted)** `updateHandler` (PUT/PATCH), `deleteHandler`, `addPathHandler`, `deletePathHandler`, `editModalHandler`, and `triggerScanHandler` (POST /scan) operate on an arbitrary library ID taken from the URL path without calling `GetUserLibraryIDs`. They are gated by the `ManipulateLibrary` permission middleware (403 on lacking the permission flag), but that is a *capability* gate, not an *ownership* gate — a user with `permission_manipulate_library` can update, delete, or trigger a scan on *any* library they do not own. This is the same class of IDOR as the one this PR fixes on the read endpoints. This is pre-existing debt, not introduced by this PR. However the PR description says the read endpoints "expose filesystem paths" — the write endpoints modify or delete those libraries, which is at least equally sensitive. Flagging as MAJOR so a follow-up bead is filed; it should not block merging THIS diff which is a net improvement. Suggested fix: apply the same `GetUserLibraryIDs` + `slices.Contains` guard to each write handler before the mutation call, following the pattern established here. --- ### This PR — verified clean on: - **Ownership check placement in showHandler:** called *before* the `WantsJSON` branch (line ~140), so both the JSON response path and the HTML redirect path are guarded. Neither branch can reach `d.Get` without passing the check. ✓ - **Ownership check placement in scanStatusHandler:** called *before* `d.GetScanStatus` (line ~672). ✓ - **userID source:** `stubUserID` is a package-level constant `int64(1)` defined in `bulk_handler.go:17` — not from path/query/body. ✓ - **Fail-closed (empty set):** `slices.Contains([], id)` returns false → `ErrNotFound`. Test "when the user has no library access (empty set)" asserts 404. ✓ - **No existence leak:** unauthorized ID returns `fmt.Errorf("...: %w", middleware.ErrNotFound)` — identical error class as a missing record. The middleware translates both to 404 with no distinguishing body. ✓ - **DB error path:** `GetUserLibraryIDs` error returns a wrapped error (500), not a data leak. Test "when GetUserLibraryIDs returns an error" asserts 500. ✓ - **Test coverage:** three new `Context` blocks per endpoint (lacks-access→404, empty-set→404, db-error→500). Existing happy-path tests updated with `stubUserLibraryIDs(id)` so they still pass the check. ✓ - **Content-negotiation HTML branch of showHandler:** HTML path redirects to `/` after the ownership check — the redirect is gated, not a bypass. ✓ --- REVIEW VERDICT: 0 blocker, 1 major, 0 minor

zombor force-pushed bd-bookshelf-q14o from dab1f1ff0c to c4859ddc4f 2026-06-19 19:27:05 +00:00 Compare

zombor force-pushed bd-bookshelf-q14o from c4859ddc4f to 4bd8e4cb28 2026-06-19 20:15:00 +00:00 Compare

zombor merged commit 7bee7af473 into main 2026-06-19 20:22:25 +00:00

zombor referenced this pull request from a commit 2026-06-19 20:22:28 +00:00

Merge pull request 'fix(library): scope show + scan-status by user library access (bookshelf-q14o)' (#644) from bd-bookshelf-q14o into main

zombor referenced this pull request from a commit 2026-06-20 00:56:48 +00:00

fix(library): complete stubUserID removal in showHandler + scanStatusHandler

zombor referenced this pull request from a commit 2026-06-20 01:02:47 +00:00

fix(library): complete stubUserID removal in showHandler + scanStatusHandler

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!644

No description provided.