fix(opds): enforce permission_access_opds and permission_download (bookshelf-t582g.4.1) #1249

Merged

zombor merged 1 commit from bd-bookshelf-t582g.4.1 into main 2026-07-27 00:20:41 +00:00

Conversation 3 Commits 1 Files changed 4 +436 -30

zombor commented 2026-07-26 19:10:25 +00:00

Owner

Copy link

Summary

• Any valid OPDS credential previously bypassed permission checks: permission_access_opds and permission_download were surfaced in the admin UI and OIDC group mappings but never enforced in handler.go
• Added requireOpdsAccess (→ permission_access_opds) and requireDownload (→ permission_download) guards as shared helpers in handler.go; both return middleware.ErrForbidden (403) for authenticated users lacking the permission
• RootHandler and BooksHandler gate on permission_access_opds; DownloadHandler and CoverHandler gate on permission_download
• wire.go: wires users.GetAdminUserPermissions(d.Q.GetUserPermissions) once and passes it to all four handlers; Wire function held at exactly 60 lines (funlen gate)

Test plan

• New permissions_test.go covers all 4 handlers × (has-perm / lacks-perm / db-error) = 12 new specs
• All 146 opds unit tests pass
• All new/modified handler functions remain at 100% statement coverage
• make lint clean (no opds-specific findings)
• Full make test suite green

Closes bead bookshelf-t582g.4.1 on merge.

## Summary - Any valid OPDS credential previously bypassed permission checks: `permission_access_opds` and `permission_download` were surfaced in the admin UI and OIDC group mappings but never enforced in `handler.go` - Added `requireOpdsAccess` (→ `permission_access_opds`) and `requireDownload` (→ `permission_download`) guards as shared helpers in `handler.go`; both return `middleware.ErrForbidden` (403) for authenticated users lacking the permission - `RootHandler` and `BooksHandler` gate on `permission_access_opds`; `DownloadHandler` and `CoverHandler` gate on `permission_download` - `wire.go`: wires `users.GetAdminUserPermissions(d.Q.GetUserPermissions)` once and passes it to all four handlers; `Wire` function held at exactly 60 lines (funlen gate) ## Test plan - [x] New `permissions_test.go` covers all 4 handlers × (has-perm / lacks-perm / db-error) = 12 new specs - [x] All 146 opds unit tests pass - [x] All new/modified handler functions remain at 100% statement coverage - [x] `make lint` clean (no opds-specific findings) - [x] Full `make test` suite green Closes bead bookshelf-t582g.4.1 on merge.

zombor added 1 commit 2026-07-26 19:10:25 +00:00

fix(opds): enforce permission_access_opds and permission_download gates (bookshelf-t582g.4.1) ... 96f74a78c8

Previously any valid OPDS credential could access the full catalog and download
files regardless of assigned permissions.  This was an authz bypass: the admin
UI and OIDC group-mapping surfaces exposed permission_access_opds /
permission_download, but the OPDS handler never checked them.

Fix: after Basic-Auth credential verification succeeds, load the user's
permissions via GetAdminUserPermissions and enforce:
- permission_access_opds → RootHandler, BooksHandler (catalog/feed access)
- permission_download    → DownloadHandler, CoverHandler (file/image downloads)

A missing permission returns 403 Forbidden (middleware.ErrForbidden), not a
Basic-Auth 401 challenge, so OPDS clients with valid credentials but insufficient
grants receive the correct HTTP status rather than a re-prompt loop.

The two guards (requireOpdsAccess / requireDownload) live in handler.go as
shared helpers so future sync-protocol handlers (Kobo, KOReader) can reuse the
same pattern without inlining.

Wire.go: wire getUserPermissions via users.GetAdminUserPermissions once and
pass it to all four handlers.  Wire function kept at exactly 60 lines (gate).

Tests: new permissions_test.go covers all four handlers × (has-perm / lacks-perm /
db-error) = 12 new specs; all handlers remain at 100% statement coverage.

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

zombor commented 2026-07-26 19:34:48 +00:00

Author

Owner

Copy link

Security review — PR #1239 (bd-bookshelf-cs2zl.2)

New POST /books/bulk/filter/scan-file (LLM scan-file over all filter-matching / whole-library books) + library kebab trigger.

Multi-user scoping — PASS (fail-closed). bulkScanFileFilterRequest reuses the shared bulkFilterHandler: userID is taken from the session (userIDFromRequest(r)), never the body; library_id/shelf_id/status are re-scoped to that userID (ShelfUserID/StatusUserID); magic_shelf_id is ownership-checked via checkMagicShelfAccess (404 on miss). Book-ID resolution re-resolves userLibraryIDs per ContinueAsNew epoch in buildListFilteredIDsPageFn and passes them to ListFilteredBookIDsPage, whose predicate is fail-closed (internal/books/filter_predicates.go:104 → library_id IN (...); non-nil empty → 1=0). users.GetUserLibraryIDs normalizes nil → []int64{}, so a zero-library user takes the 1=0 branch — a body-supplied library_id the user cannot access yields no rows, not a cross-user leak.

Auth — PASS. Route is gated g.BulkScanFile(...) → BookBulkScanFileRequired → users.PermissionRequired(..., PermissionBulkAutoFetchMetadata) (internal/app/app.go:355), same real permission as the sibling by-IDs endpoint. Not "any logged-in user."

Resource-exhaustion / cost DoS — PASS. Fan-out is bounded single-digit (defaultFanOutConcurrency = 4, internal/wfengine/fanout.go:19) and sub-workflows route to the LLM queue (scanFileFanOutOptions) so vision activities respect the GPU/concurrency cap; the kebab entry is gated behind {{if $.LLMVisionAvailable}} and a dialog.confirm count prompt.

Injection — PASS. All SQL is sqlc/parameterized; view_query validated via ParseViewQueryFilter at the boundary; status/format/metadata filters allowlist-validated; audit action is a constant.

Workflow versioning — SAFE (no gate needed). The new case BulkFilterOpScanFile in bulkByFilterApplyOp is selected by the per-instance-immutable input.Op; in-flight instances carry a different Op and keep their original command sequence, so replay does not diverge.

Findings

[MINOR] templates/layouts/base.html:200 — count-confirmation shows 0 for the largest libraries
data-...-book-count-value="{{if .HasCount}}{{.Count}}{{else}}0{{end}}" falls back to 0 when the count is unavailable (HasCount false) — which per the Scale convention is exactly the large/unfiltered libraries where the "you are about to scan N files … uses your LLM budget" confirmation matters most. The most expensive case shows the least alarming number, weakening the secondary cost guard. Authorization/scoping/permission gates still fully protect the operation, so this is UX-quality, not a vulnerability. Suggest an indeterminate message ("all files in this library") when HasCount is false rather than "0 files".

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security review — PR #1239 (`bd-bookshelf-cs2zl.2`) New `POST /books/bulk/filter/scan-file` (LLM scan-file over all filter-matching / whole-library books) + library kebab trigger. **Multi-user scoping — PASS (fail-closed).** `bulkScanFileFilterRequest` reuses the shared `bulkFilterHandler`: `userID` is taken from the session (`userIDFromRequest(r)`), never the body; `library_id`/`shelf_id`/`status` are re-scoped to that userID (`ShelfUserID`/`StatusUserID`); `magic_shelf_id` is ownership-checked via `checkMagicShelfAccess` (404 on miss). Book-ID resolution re-resolves `userLibraryIDs` per ContinueAsNew epoch in `buildListFilteredIDsPageFn` and passes them to `ListFilteredBookIDsPage`, whose predicate is fail-closed (`internal/books/filter_predicates.go:104` → `library_id IN (...)`; non-nil empty → `1=0`). `users.GetUserLibraryIDs` normalizes `nil → []int64{}`, so a zero-library user takes the `1=0` branch — a body-supplied `library_id` the user cannot access yields no rows, not a cross-user leak. **Auth — PASS.** Route is gated `g.BulkScanFile(...)` → `BookBulkScanFileRequired` → `users.PermissionRequired(..., PermissionBulkAutoFetchMetadata)` (`internal/app/app.go:355`), same real permission as the sibling by-IDs endpoint. Not "any logged-in user." **Resource-exhaustion / cost DoS — PASS.** Fan-out is bounded single-digit (`defaultFanOutConcurrency = 4`, `internal/wfengine/fanout.go:19`) and sub-workflows route to the LLM queue (`scanFileFanOutOptions`) so vision activities respect the GPU/concurrency cap; the kebab entry is gated behind `{{if $.LLMVisionAvailable}}` and a `dialog.confirm` count prompt. **Injection — PASS.** All SQL is sqlc/parameterized; `view_query` validated via `ParseViewQueryFilter` at the boundary; status/format/metadata filters allowlist-validated; audit action is a constant. **Workflow versioning — SAFE (no gate needed).** The new `case BulkFilterOpScanFile` in `bulkByFilterApplyOp` is selected by the per-instance-immutable `input.Op`; in-flight instances carry a different `Op` and keep their original command sequence, so replay does not diverge. ### Findings [MINOR] templates/layouts/base.html:200 — count-confirmation shows 0 for the largest libraries `data-...-book-count-value="{{if .HasCount}}{{.Count}}{{else}}0{{end}}"` falls back to 0 when the count is unavailable (`HasCount` false) — which per the Scale convention is exactly the large/unfiltered libraries where the "you are about to scan N files … uses your LLM budget" confirmation matters most. The most expensive case shows the least alarming number, weakening the secondary cost guard. Authorization/scoping/permission gates still fully protect the operation, so this is UX-quality, not a vulnerability. Suggest an indeterminate message ("all files in this library") when `HasCount` is false rather than "0 files". REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-07-26 19:35:18 +00:00

Author

Owner

Copy link

Code Review: OPDS Permission Enforcement (bd-bookshelf-t582g.4.1)

Summary

This PR implements enforcement of permission_access_opds on catalog/feed endpoints and permission_download on file/cover download routes, closing a previously unchecked authorization gap.

Findings

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Verification Results

✓ Coverage of all OPDS entry points: All 5 routes are properly gated

• GET /opds (RootHandler) → requires permission_access_opds
• GET /opds/books (BooksHandler) → requires permission_access_opds
• GET /opds/books/{id}/file/{fileID} (DownloadHandler) → requires permission_download
• GET /opds/books/{id}/cover (CoverHandler) → requires permission_download
• GET /opds/books/{id}/thumbnail (CoverHandler) → requires permission_download

✓ Enforcement order (authentication → permission check): All handlers correctly call authenticate() first, then requireOpdsAccess() or requireDownload() immediately after, before any business logic.

✓ Fail-closed behavior: Permission checks return middleware.ErrForbidden (→ 403) when permission is absent, and wrap/return any DB errors as 500s. No fallthrough to allow when permission load fails.

✓ Shared helpers clean: Helper functions requireOpdsAccess() and requireDownload() are concise, properly wrap errors with context, and correctly check the respective permission fields (AccessOpds, Download) from the UserPermissions struct.

✓ Curried dependency injection: getUserPermissions is bound once at wire time via users.GetAdminUserPermissions(d.Q.GetUserPermissions) and passed to each handler, following the projects functional-arg pattern.

✓ Wire format unchanged for permitted users: No changes to XML response structures, element names, or link generation. OPDS feed format remains wire-compatible.

✓ Black-box test coverage: permissions_test.go uses package opds_test and exercises only exported functions and types. Three test cases per endpoint:

• User with permission → 200 OK
• User without permission → 403 Forbidden
• Permission load error → 500 Internal Server Error

✓ Existing tests updated: All pre-existing tests in handler_test.go now pass the allPermsGranted stub to prevent permission checks from interfering with their test assertions (proper test isolation).

All requirements satisfied. No security vulnerabilities, no correctness issues, no convention violations detected.

## Code Review: OPDS Permission Enforcement (bd-bookshelf-t582g.4.1) ### Summary This PR implements enforcement of `permission_access_opds` on catalog/feed endpoints and `permission_download` on file/cover download routes, closing a previously unchecked authorization gap. ### Findings **REVIEW VERDICT: 0 blocker, 0 major, 0 minor** #### Verification Results ✓ **Coverage of all OPDS entry points:** All 5 routes are properly gated - GET /opds (RootHandler) → requires `permission_access_opds` - GET /opds/books (BooksHandler) → requires `permission_access_opds` - GET /opds/books/{id}/file/{fileID} (DownloadHandler) → requires `permission_download` - GET /opds/books/{id}/cover (CoverHandler) → requires `permission_download` - GET /opds/books/{id}/thumbnail (CoverHandler) → requires `permission_download` ✓ **Enforcement order (authentication → permission check):** All handlers correctly call `authenticate()` first, then `requireOpdsAccess()` or `requireDownload()` immediately after, before any business logic. ✓ **Fail-closed behavior:** Permission checks return `middleware.ErrForbidden` (→ 403) when permission is absent, and wrap/return any DB errors as 500s. No fallthrough to allow when permission load fails. ✓ **Shared helpers clean:** Helper functions `requireOpdsAccess()` and `requireDownload()` are concise, properly wrap errors with context, and correctly check the respective permission fields (`AccessOpds`, `Download`) from the `UserPermissions` struct. ✓ **Curried dependency injection:** `getUserPermissions` is bound once at wire time via `users.GetAdminUserPermissions(d.Q.GetUserPermissions)` and passed to each handler, following the projects functional-arg pattern. ✓ **Wire format unchanged for permitted users:** No changes to XML response structures, element names, or link generation. OPDS feed format remains wire-compatible. ✓ **Black-box test coverage:** `permissions_test.go` uses `package opds_test` and exercises only exported functions and types. Three test cases per endpoint: - User with permission → 200 OK - User without permission → 403 Forbidden - Permission load error → 500 Internal Server Error ✓ **Existing tests updated:** All pre-existing tests in `handler_test.go` now pass the `allPermsGranted` stub to prevent permission checks from interfering with their test assertions (proper test isolation). **All requirements satisfied. No security vulnerabilities, no correctness issues, no convention violations detected.**

zombor commented 2026-07-26 19:36:02 +00:00

Author

Owner

Copy link

Security Review — PR #1249 (OPDS authz gate, bd-bookshelf-t582g.4.1)

Adversarial focus: OPDS route gating, fail-closed, byte-fetch prevention, enumeration leak, multi-user scoping.

Route enumeration — all 5 OPDS routes are registered solely via opds.Wire -> RegisterRoutes (internal/opds/routes.go); opds.Wire (internal/app/app.go:88) is the only registration site and now passes getUserPermissions to every handler:

• GET /opds -> RootHandler -> requireOpdsAccess (permission_access_opds) — GATED
• GET /opds/books -> BooksHandler -> requireOpdsAccess — GATED
• GET /opds/books/{id}/file/{fileID} -> DownloadHandler -> requireDownload (permission_download) — GATED
• GET /opds/books/{id}/cover -> CoverHandler -> requireDownload — GATED
• GET /opds/books/{id}/thumbnail -> CoverHandler -> requireDownload — GATED

There is no search/acquisition route beyond these. No un-gated OPDS handler remains — the bypass is closed.

Five focus areas:

1. Byte-fetch prevention: in DownloadHandler and CoverHandler the gate is placed immediately after authenticate, BEFORE getFile/openFile. It blocks reading the file bytes, not merely hiding the acquisition link.
2. Fail-closed on permissions-load error: requireOpdsAccess/requireDownload return a wrapped error on getUserPermissions failure -> error middleware -> 500 (no content served). GetAdminUserPermissions (internal/users/admin_service.go:384-391) returns empty UserPermissions{} on sql.ErrNoRows (missing user -> all-false -> 403) and a wrapped error otherwise. Fail-closed in both directions.
3. middleware.ErrForbidden maps to 403 (internal/middleware/error_mapper.go:99).
4. No user-enumeration/existence leak: bad credentials get a 401 challenge before the gate is reached; 403 (unpermissioned) and 500 (DB error) are identical regardless of user — no differential signal.
5. Multi-user scoping preserved and additive: getUserLibraryIDs + getContentRestrictions (feed) and checkBookAccess (download/cover) all remain; the permission gate is IN ADDITION TO, not instead of, library scoping.

Tests cover has/lacks/DB-error (403/500) for all four handlers, black-box package opds_test, one-Expect-per-It.

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1249 (OPDS authz gate, `bd-bookshelf-t582g.4.1`) Adversarial focus: OPDS route gating, fail-closed, byte-fetch prevention, enumeration leak, multi-user scoping. **Route enumeration — all 5 OPDS routes are registered solely via `opds.Wire` -> `RegisterRoutes` (internal/opds/routes.go); `opds.Wire` (internal/app/app.go:88) is the only registration site and now passes `getUserPermissions` to every handler:** - `GET /opds` -> RootHandler -> `requireOpdsAccess` (permission_access_opds) — GATED - `GET /opds/books` -> BooksHandler -> `requireOpdsAccess` — GATED - `GET /opds/books/{id}/file/{fileID}` -> DownloadHandler -> `requireDownload` (permission_download) — GATED - `GET /opds/books/{id}/cover` -> CoverHandler -> `requireDownload` — GATED - `GET /opds/books/{id}/thumbnail` -> CoverHandler -> `requireDownload` — GATED There is no search/acquisition route beyond these. No un-gated OPDS handler remains — the bypass is closed. **Five focus areas:** 1. Byte-fetch prevention: in DownloadHandler and CoverHandler the gate is placed immediately after `authenticate`, BEFORE `getFile`/`openFile`. It blocks reading the file bytes, not merely hiding the acquisition link. 2. Fail-closed on permissions-load error: `requireOpdsAccess`/`requireDownload` return a wrapped error on `getUserPermissions` failure -> error middleware -> 500 (no content served). `GetAdminUserPermissions` (internal/users/admin_service.go:384-391) returns empty `UserPermissions{}` on `sql.ErrNoRows` (missing user -> all-false -> 403) and a wrapped error otherwise. Fail-closed in both directions. 3. `middleware.ErrForbidden` maps to 403 (internal/middleware/error_mapper.go:99). 4. No user-enumeration/existence leak: bad credentials get a 401 challenge before the gate is reached; 403 (unpermissioned) and 500 (DB error) are identical regardless of user — no differential signal. 5. Multi-user scoping preserved and additive: `getUserLibraryIDs` + `getContentRestrictions` (feed) and `checkBookAccess` (download/cover) all remain; the permission gate is IN ADDITION TO, not instead of, library scoping. Tests cover has/lacks/DB-error (403/500) for all four handlers, black-box `package opds_test`, one-Expect-per-It. No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-t582g.4.1 from 96f74a78c8 to d34baaf22f 2026-07-27 00:13:28 +00:00 Compare

zombor merged commit 94c56bfb46 into main 2026-07-27 00:20:41 +00:00

zombor referenced this pull request from a commit 2026-07-27 00:20:43 +00:00

Merge pull request 'fix(opds): enforce permission_access_opds and permission_download (bookshelf-t582g.4.1)' (#1249) from bd-bookshelf-t582g.4.1 into main

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

No description provided.