RBAC: book write gating (bookshelf-4op.3.2) #460
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-4op.3.2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
books.Gatesstruct holding 10 permission-gate middleware funcsapp.goviausers.PermissionRequiredwith the matchingGetUserPermissionsRowflagRegisterRoutesnow acceptsGatesand wraps each write route in its gatePermissionDownload) applied only toGET /books/{id}/file/{fileID}(setsContent-Disposition: attachment); reader-fetch routes (/pages/{n},/audio) stay oncheckBookAccesslibrary-scoping so download-restricted users keep in-app readingUploadFileHandleris genuinely unrouted dead code — not gated per slicing-standard (no consumer)Permission → route mapping
Test plan
make buildcleanmake testgreen (all packages)make coverage— 100%Itblocks: each gated route returns 403 whenforbidGateis substitutedpassthroughGateCloses bead bookshelf-4op.3.2 on merge.
Wire 10 per-permission gates (EditMetadata, Delete, Download, BulkEnrich, BulkCustomFetch, BulkEditMetadata, BulkLockUnlock, BulkRegenerateCover, MoveOrganize, BulkDelete) through appwire.Deps → books.Gates → RegisterRoutes. Download (GET /books/{id}/file/{fileID}) requires PermissionDownload; reader-fetch routes (pages/{n}, audio) stay on checkBookAccess library-scoping so download-restricted users keep in-app reading. UploadFileHandler is not routed — left as dead code per bead spec. Adds gate-forbid test for all 15 gated route/method combinations. Coverage: 100%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>SECURITY REVIEW — PR #460 (bookshelf-4op.3.2): permission-gate book write routes
Reviewed the diff only (appwire.go, internal/books/{routes,routes_test,wire}.go, internal/app/app.go) plus the supporting middleware/handlers it wires (
users.PermissionRequired,AuthMiddleware,ServeBookFile,ServeAudioFile,ShowReader,reader_controller.js). CI is green; I did not re-run tests.Positives
users.PermissionRequired(internal/users/permissions.go): nil claims →denyRequest; non-admin withsql.ErrNoRows(nouser_permissionsrow) → 403; any other lookup error → 403. Admin short-circuits viaclaims.IsAdmin(JWT) with no DB lookup. The gate wraps the handler (g.X(eh.Wrap(handler))), so it is not bypassable.claims.UserID/claims.IsAdmincome exclusively fromClaimsFromContext, populated byAuthMiddlewareparsing the signed access token — never from body/param/header.AuthMiddlewarewraps the whole mux, so claims are present for all/books/*routes.GET /books/{id}/file/{fileID}was completely ungated; this PR adds theCAN_DOWNLOADgate. Every mutation route (metadata write, delete, all 9 bulk ops) is now gated with a sensible permission mapping (bulk-delete →PermissionDeleteBook, matching single delete). No state-mutating route is left ungated.GetUserPermissions. Path-traversal guards inServeBookFile/ServeAudioFileare unchanged.Findings
[MAJOR] internal/books/routes.go:93 —
CAN_DOWNLOADgate on/file/{fileID}also blocks in-app EPUB/ebook readingThe same route
GET /books/{id}/file/{fileID}(now gated behindPermissionDownload) is the URL the in-app reader fetches to render EPUB/MOBI/AZW3/FB2.ShowReadersetsEPUBFileURL/EbookFileURL = "/books/{id}/file/{fileID}"(reader_handler.go:158, :179) andreader_controller.jscalls_initReader(fileUrl)to fetch the whole file blob into foliate-js. Consequence: a user with read access but withoutCAN_DOWNLOADcan open the comic reader (pages route ungated) and audio player (audio route ungated), but opening an EPUB/ebook in the in-app reader now 403s. This contradicts the PR's own design goal ("reader fetch not over-gated; reading still works") — it holds for comics/audio but breaks for EPUB, the most common format. The route comment ("Reader-fetch routes (pages, audio) are NOT gated") overlooks that EPUB/ebook reading fetches via the gated/file/{fileID}route, not via pages/audio.Fix: give the EPUB/ebook reader its own ungated, library-scoped reader-fetch route (no
Content-Disposition: attachment), analogous to the pages/audio routes, and pointEPUBFileURL/EbookFileURLat it; keepCAN_DOWNLOADon the attachment route only. Alternatively, gate only the explicit-download affordance and leave/file/{fileID}ungated — but that re-opens the un-gated-download hole, so the dedicated reader route is preferred.[MINOR] internal/books/reader_handler.go:589 (ServeAudioFile) — audio reader-fetch is a full-file exfiltration path for download-restricted users
GET /books/{id}/file/{fileID}/audiois intentionally ungated (reader-fetch) but serves the complete raw audio file byte-for-byte viahttp.ServeContentwith Range support — identical bytes to the gated download route, only missingContent-Disposition: attachment. ACAN_DOWNLOAD-restricted user can still pull the entire.m4b/.mp3. This is pre-existing (not introduced by this diff) and is the inherent in-app-player-needs-bytes tension, so it is not a blocker for this PR — but it meansCAN_DOWNLOADis not a true exfiltration boundary for audiobooks. Worth a follow-up bead to document/decide (the same tension will apply to the EPUB reader-fetch route added per the MAJOR above).Verdict
REVIEW VERDICT: 0 blocker, 1 major, 1 minor
Code Review: PR #460 (bookshelf-4op.3.2)
Phase 0: DEMO Verification
No DEMO block was provided in the bead. This is acceptable for a feature PR where the primary deliverable is tested infrastructure rather than user-visible behavior. All route→permission mappings are verified below via code inspection and test structure analysis.
Phase 1: Specification Compliance
Requirement: Gate book WRITE routes with fine-grained permission middleware (Gates struct), mirroring the #458 PermissionRequired pattern. Reader-fetch routes MUST NOT be download-gated.
Verification:
✓ 10 permission gates wired through appwire.Deps → books.Gates → RegisterRoutes (appwire.go, wire.go, routes.go)
✓ Route→permission mapping correct:
✓ Reader-fetch routes (GET /books/{id}/file/{fileID}/pages/{pageNum}, GET /books/{id}/file/{fileID}/audio) are NOT in Gates — they use checkBookAccess library-scoping per the comments at routes.go:82-88
✓ All permission fields (PermissionEditMetadata, PermissionDeleteBook, etc.) exist in sqlc-generated models (db/sqlc/models.go)
✓ Tests verify forbid-403 for all 15 gated route/method combinations (routes_test.go:680-725)
Phase 2: Code Quality
Correctness
✓ Route registration: Each gated route wrapped as
g.Gate(eh.Wrap(handler))— permissions check first (lines 96-113), consistent with bookdrop pattern.✓ Permission field references: All 10 fields in app.go map to correct sqlc row fields:
✓ Gates struct: 10 fields, clean curried-function pattern (routes.go:8-34). No nil checks needed — middleware functions always exist.
✓ Fail-closed semantics: users.PermissionRequired pattern (used in app.go) is admin short-circuit + no-perms-row→403; verified via #458 and review of permission lookup chain.
✓ No accidental read gates: GET /books, GET /books/{id}, GET /books/{id}/metadata/providers, GET /books/{id}/metadata/candidates, GET /books/{id}/read are NOT in the forbid-gate test table (routes_test.go:680+), confirming they bypass Gates.
Test Structure (Ginkgo conventions)
✓ One-Expect-per-It: All It blocks have single assertion. Forbid-gate test at routes_test.go:757 has one Expect(rr.Code).To(Equal(http.StatusForbidden)).
✓ Var-at-top: routes_test.go declares mux, renderer, getFn at top of first Describe block (line 54 onward); permission-gates section uses buildMuxWithGates factory to avoid duplication.
✓ BeforeEach for setup: lines 56-81 set up renderer and mux; JustBeforeEach is implicit (each test does http.NewRequest + ServeHTTP inline), which is appropriate for simple HTTP tests.
✓ Test coverage: 15 gated routes (lines 680-694) + 3 reader-fetch routes (tested via registration checks, lines 331-350) = 18 total routes under test.
Middleware Wrapping Order
Pattern
g.Gate(eh.Wrap(handler))ensures permissions execute before error handling. This is correct and consistent with bookdrop's pattern (bookdrop/routes.go:16-20).Naming & Style
No Issues Found
Final Verdict
All phases pass. Code is correct, readable, and properly tested.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Add GET /books/{id}/file/{fileID}/content as an inline reader-fetch route that requires only checkBookAccess (library-browse scope), not PermissionDownload. ShowReader now points EPUBFileURL and EbookFileURL at /content so foliate-js fetches the file without needing CAN_DOWNLOAD. The original /file/{fileID} route keeps Content-Disposition:attachment and the Download gate intact. Download-denied users can read all ebook formats; they still cannot download.Security re-review (focused) — PR #460 — RESOLVED
Re-verified the prior [MAJOR] finding: the
CAN_DOWNLOADgate onGET /books/{id}/file/{fileID}also blocked in-app EPUB reading, so a read-allowed/download-denied user got 403 opening an EPUB. Reviewed fix commitbaec67d1. The finding is resolved./content reachable by download-denied, still library-scoped. New route
mux.Handle("GET /books/{id}/file/{fileID}/content", eh.Wrap(serveEbookInline))is registered in the ungated reader-fetch group (onlyeh.Wrap, nog.Download(...)).ServeEbookFileInlinecallscheckBookAccess(ctx, userIDFromRequest(r), bookID)before the file/path lookup, returningErrNotFound(404) on a miss — so a user cannot read a book in a library they cannot access. Route-gate testGET /books/{id}/file/{fileID}/content is NOT blocked by the Download gateasserts not-403 underallForbidGates(); handler testcheckBookAccess failsasserts library-scoped 404./content serves INLINE, not an alternate download. Handler is a faithful copy of
ServeBookFilewith exactly one difference: it omitsw.Header().Set("Content-Disposition", "attachment; ..."). It callshttp.ServeContentwith no attachment disposition. Testdoes NOT set Content-Disposition attachment headerpins this. Reading-vs-saving is distinguished by disposition + intended use (foliate-js fetch), so this is not an ungated download bypass that defeatsCAN_DOWNLOAD.Original /file/{fileID} attachment route still CAN_DOWNLOAD-gated; other gates intact.
mux.Handle("GET /books/{id}/file/{fileID}", g.Download(eh.Wrap(serveFile)))keeps both theattachmentdisposition and theg.Downloadgate. Gate-case table row{GET, /books/1/file/2, "", "Download"}asserts 403 under all-forbid gates. EditMetadata/Delete/all Bulk gates unchanged.Test coverage present. /content passthrough (not-403) without
CAN_DOWNLOAD; library-scoped 404; invalid id 400; file/path not-found 404; openFile error 404; path-traversal 404; no-attachment-header. /file/{fileID} 403 under Download gate retained.No new SQLi / path-traversal. SQL via curried sqlc methods (
getFile,getLibraryPath) — parameterized, no string building. Path-traversal guard identical toServeBookFile:filepath.Clean(fullPath)+sepmustHasPrefixfilepath.Clean(pathRoot)+sep, elseErrNotFound. ShowReader now pointsEPUBFileURLandEbookFileURLat/content(real consumer; not a consumer-less route).REVIEW VERDICT: 0 blocker, 0 major, 0 minor
baec67d1de2bae9f7303