fix(dedup): gate destructive endpoints behind permission middleware (bookshelf-u5q4) #889
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-u5q4"
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
Gatesstruct tointernal/dedupmirroring the pattern ininternal/libraryandinternal/books.permission_move_organize_files; delete-non-selected →permission_delete_book.appwire.Deps.BookMoveOrganizeRequiredandBookDeleteRequiredindedup.Wire.Test plan
internal/dedup/routes_test.gowith black-box 403 specs for all three mutating routes when gate forbidspassthroughGates()— no behavior change for non-permission testsmake testpasses;make coverageshows 100% gateCloses bead bookshelf-u5q4 on merge.
Workflow Detail page screenshot (wf-detail-older-execution)
Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.
Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)
Security Review — PR #889 (bookshelf-u5q4)
[BLOCKER] internal/dedup/routes.go:52-53 — Merge and MergeAll hard-delete source books but are gated only on permission_move_organize_files, not permission_delete_book
The Merge service function (dedup/service.go:401-402) runs deleteBooksRaw which issues DELETE FROM book WHERE id IN (...) as step 5 of every merge. BulkDedupMergeAllWorkflow calls the same Merge activity. Both POST /libraries/{id}/dedup/merge and POST /libraries/{id}/dedup/merge-all are gated exclusively by g.MergeRequired (permission_move_organize_files). A user who holds permission_move_organize_files but NOT permission_delete_book can permanently destroy book records via these endpoints, bypassing the delete permission gate entirely. The delete-non-selected route is correctly gated by g.DeleteRequired (permission_delete_book); the merge paths need the same guard (either require both permissions, or gate merge routes through a combined checker that asserts both flags). The test in routes_test.go uses forbidGates() which forbids ALL gates at once — it proves the middleware is invoked but does not catch that the wrong single permission is used for a route that deletes books.
REVIEW VERDICT: 1 blocker, 0 major, 0 minor
Code Review — bookshelf-u5q4
Phase 0: DEMO
No DEMO block in the bead (permission-gate enforcement is tested via unit assertions rather than a runnable CLI demo). Acceptable for a middleware-gating fix.
Phase 1 & 2 Findings
[MINOR] internal/dedup/routes_test.go:72,80,87 — negative assertions on ungated routes
Expect(rec.Code).NotTo(Equal(http.StatusForbidden))would pass even if the handler returned 500, since nopModal/nopScan/nopMergeAllStatus all explicitly write 200. A positiveExpect(rec.Code).To(Equal(http.StatusOK))is more precise and is consistent with the one-assertion-per-It idiom the rest of the test suite uses. Low risk because the nop stubs are trivially correct, but the assertion does not pin the behaviour it is advertising.What was checked
Correctness of wrapping (vs library/routes.go)
POST .../dedup/merge→g.MergeRequired(eh.Wrap(merge))✓POST .../dedup/merge-all→g.MergeRequired(eh.Wrap(mergeAll))✓POST .../dedup/delete-non-selected→g.DeleteRequired(eh.Wrap(deleteNonSelected))✓GET .../dedup,POST .../dedup/scan,GET .../dedup/merge-all/{instanceID}/status) intentionally ungated — matches bead intent and the scan-is-read-only invariant. ✓Permission correctness
d.BookMoveOrganizeRequired(PermissionMoveOrganizeFiles) — correct; mirrors howbooks.Gates.MoveOrganizegates/books/bulk/move. ✓d.BookDeleteRequired(PermissionDeleteBook) — correct; same permission used bybooks.Gates.Deleteandbooks.Gates.Detach. ✓Gates struct convention
dedup.Gatesis structurally identical tobooks.Gatesandlibrary.Gates: a value type carrying namedfunc(http.Handler) http.Handlerfields, passed by value toRegisterRoutes, constructed inline inwire.go. This is the established pattern. No inconsistency. ✓Nil-safety
g.MergeRequired(...)panics. This is the same contract thatbooks.Gatesandlibrary.Gatesaccept.app.gosets bothBookMoveOrganizeRequiredandBookDeleteRequiredunconditionally at startup;wire.goreads (does not reassign) the Deps fields, so the by-value-Deps trap (bookshelf-deps-nil #633) does not apply here. Risk consistent with the rest of the codebase. ✓passthroughGates() accuracy
func(next) { return next }. Correctly models "user has all permissions" for handler-behaviour tests, which test handler logic, not permission logic. Permission enforcement is tested separately inroutes_test.gowithforbidGates(). The helper is accurate and not a bypass. ✓Test quality
package dedup_test(black-box) ✓forbidGates()tests 403 for all three gated routes, oneExpectperIt✓NotTo(Equal(http.StatusForbidden))) function correctly but are weaker than positive assertions (see MINOR above)Ordered+BeforeAllis valid Ginkgo for shared mux setup; the specs are independently safe ✓audit_test.goupdated to passpassthroughGates()throughdoMergeWithAudit✓Wire.go
Gates{MergeRequired: d.BookMoveOrganizeRequired, DeleteRequired: d.BookDeleteRequired}inserted as the second-to-last arg, immediately befored.EH— matches theRegisterRoutessignature. ✓REVIEW VERDICT: 0 blocker, 0 major, 1 minor
468e50506f2f1a2a3d42Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)
Workflow Detail page screenshot (wf-detail-older-execution)
Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.