diff --git a/AGENTS.md b/AGENTS.md index d79b0df4..e056a4ab 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -145,6 +145,17 @@ every user sees every library unless an admin hides one) is enforced by new pages/controls go into `tests/ui/COVERAGE.md`). Item-scoped gates may answer 404 as well as 403 (row invisible vs Go-level check — contract_pins 2026-10-29). +- **Collections RLS (2026-10-31)**: `collections` and + `collection_items` are governed too — predicates + `app_can_read_collection` / `app_can_write_collection`, PURE over + their arguments (never query `collections`, so no policy recursion + under FORCE RLS). Reads: owner-or-admin; writes: owner, admin escape + for SYSTEM collections only. Future sharing = one EXISTS branch + inside the predicate functions, no policy migration. Registration + seeding declares the new user's identity (`rls.WithUser` in + `CreateDefaultCollectionsForUser` — the JWT does not exist yet); + test-harness writes to non-system collections must run as the row's + OWNER (`scopeExecAs`/`ownerSeedCtx`, not the admin handle). History: 2026-10-13 (`431fec0`) the series/media listing surfaces leaked hidden libraries; **2026-10-29 enforcement moved to RLS** — the diff --git a/cmd/server/tests/audiobook_multifile_test.go b/cmd/server/tests/audiobook_multifile_test.go index 67247d78..f510f668 100644 --- a/cmd/server/tests/audiobook_multifile_test.go +++ b/cmd/server/tests/audiobook_multifile_test.go @@ -659,12 +659,14 @@ func TestAudiobookFolderSidecarEditDetected(t *testing.T) { require.NoError(t, os.WriteFile(sidecar, []byte(`{"title":"Sidecar Real Titlz","series":["Sidecar Saga (Reading Order) #1"]}`), 0o644)) scanTestFolder(t, setup, admin, folder) var collCount int - require.NoError(t, setup.DBPool.QueryRow(context.Background(), + // Owner-scoped read: collections RLS (2026-10-31) hides rows from + // the pool session's SeedIdentity. + require.NoError(t, database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(admin), `SELECT COUNT(*) FROM collections WHERE name = 'Sidecar Saga (Reading Order)' AND user_id = $1`, admin, ).Scan(&collCount)) require.Equal(t, 1, collCount, "reading-order sidecar edit creates the collection for an existing book") scanTestFolder(t, setup, admin, folder) - require.NoError(t, setup.DBPool.QueryRow(context.Background(), + require.NoError(t, database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(admin), `SELECT COUNT(*) FROM collections WHERE name = 'Sidecar Saga (Reading Order)' AND user_id = $1`, admin, ).Scan(&collCount)) require.Equal(t, 1, collCount, "rescan is idempotent — no duplicate collection") diff --git a/cmd/server/tests/collections_rls_test.go b/cmd/server/tests/collections_rls_test.go new file mode 100644 index 00000000..8a69a5fc --- /dev/null +++ b/cmd/server/tests/collections_rls_test.go @@ -0,0 +1,127 @@ +package main + +import ( + "bytes" + "testing" + "encoding/json" + "fmt" + "net/http" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Collections RLS (2026-10-31): ownership enforcement moved to the DB +// layer (p_collections_* + parent-checked collection_items policies in +// schema.sql, predicates app_can_read/write_collection). The API surface +// answers invisibility with 404 — including the write endpoints that +// had NO app-level gate (Update/AddBooks, the flagged gap). Admin keeps +// the escape; system collections stay per-user; registration still +// seeds them (the seeding identity fix in +// CreateDefaultCollectionsForUser is regression-pinned here). +func TestCollectionsRLS(t *testing.T) { + setup := setupTestServer(t) + ownerToken := setup.RegularToken // user A: the harness regular user + adminToken := setup.Token + marker := fmt.Sprintf("rls%d", time.Now().UnixNano()%100000) + + // User B: a second regular user via registration. The register + // endpoint is rate-limited — exactly one call. The response's + // access_token is B's identity for every probe below. + regEmail := fmt.Sprintf("%s@tests.bookhoard.internal", marker) + regBody := fmt.Sprintf( + `{"email":%q,"username":%q,"password":"Test@Pass123!"}`, + regEmail, "rlsb"+marker) + regResp, err := http.Post(setup.Server.URL+"/api/auth/register", + "application/json", bytes.NewReader([]byte(regBody))) + require.NoError(t, err) + require.Equal(t, http.StatusCreated, regResp.StatusCode, + "registration must succeed — including the system-collection seeding that now runs under the new user's RLS identity") + var reg struct { + Token string `json:"access_token"` + } + require.NoError(t, json.NewDecoder(regResp.Body).Decode(®)) + require.NotEmpty(t, reg.Token) + otherToken := reg.Token + _ = regResp.Body.Close() + + // B's registration seeded B's OWN system collections — visible in + // B's list (this is the registration-seeding identity fix, live). + code, body := podcastAuthed(t, setup, otherToken, "GET", "/api/collections", "") + require.Equal(t, http.StatusOK, code) + assert.Contains(t, string(body), "Continue Reading", + "registration seeded the new user's system collections under RLS") + + // A creates the target collection. + code, body = podcastAuthed(t, setup, ownerToken, "POST", "/api/collections", + `{"name":"test RLS Owner ` + marker + `"}`) + require.Equal(t, http.StatusCreated, code, "%s", string(body)) + var coll struct { + ID string `json:"id"` + } + require.NoError(t, json.Unmarshal(body, &coll)) + t.Cleanup(func() { + podcastAuthed(t, setup, ownerToken, "DELETE", "/api/collections/"+coll.ID, "") + }) + + // ── B (non-owner) probes A's collection: invisible = 404 ── + code, _ = podcastAuthed(t, setup, otherToken, "GET", "/api/collections/"+coll.ID, "") + require.Equal(t, http.StatusNotFound, code, "detail-by-id is RLS-invisible cross-user") + + code, body = podcastAuthed(t, setup, otherToken, "GET", "/api/collections", "") + require.Equal(t, http.StatusOK, code) + assert.NotContains(t, string(body), "test RLS Owner "+marker, + "the list query stays user-scoped") + + code, body = podcastAuthed(t, setup, otherToken, "PUT", "/api/collections/"+coll.ID, + `{"name":"renamed by B"}`) + require.Equal(t, http.StatusNotFound, code, + "update of an RLS-invisible row answers 404 (was: ungated success): %s", string(body)) + + code, body = podcastAuthed(t, setup, otherToken, "POST", "/api/collections/"+coll.ID+"/books", + `{"book_ids":[]}`) + require.Equal(t, http.StatusNotFound, code, + "AddBooks on an RLS-invisible collection answers 404 (the flagged ungated-write gap, closed at the DB)") + + code, body = podcastAuthed(t, setup, otherToken, "PUT", + "/api/collections/"+coll.ID+"/view-settings", `{"chain_order":"newest"}`) + require.Equal(t, http.StatusNotFound, code, + "chain-order write on an invisible collection answers 404") + + code, _ = podcastAuthed(t, setup, otherToken, "DELETE", "/api/collections/"+coll.ID, "") + require.Equal(t, http.StatusNoContent, code, + "DELETE touches no rows under RLS (silent no-op — no destructive action)") + + // The no-op delete lied about nothing: A still owns the collection. + code, body = podcastAuthed(t, setup, ownerToken, "GET", "/api/collections/"+coll.ID, "") + require.Equal(t, http.StatusOK, code, "owner retains the collection after B's failed delete") + + // ── admin escape: sees everything, as with media ── + code, _ = podcastAuthed(t, setup, adminToken, "GET", "/api/collections/"+coll.ID, "") + require.Equal(t, http.StatusOK, code, "admin persona sees collections (policy escape)") + + // ── B's ownership works: B renames B's OWN system collection ── + // (positive control — B is not locked out of what B owns) + code, body = podcastAuthed(t, setup, otherToken, "GET", "/api/collections", "") + require.Equal(t, http.StatusOK, code) + var list struct { + Collections []struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"collections"` + } + require.NoError(t, json.Unmarshal(body, &list)) + var bSystem string + for _, c := range list.Collections { + if c.Name == "Continue Reading" { + bSystem = c.ID + break + } + } + require.NotEmpty(t, bSystem, "B owns the seeded system collections") + code, body = podcastAuthed(t, setup, otherToken, "PUT", "/api/collections/"+bSystem, + `{"name":"Continue Reading"}`) + require.Contains(t, []int{http.StatusOK, http.StatusCreated}, code, + "owner mutation of an OWN collection still works: %s", string(body)) +} diff --git a/cmd/server/tests/collections_smart_test.go b/cmd/server/tests/collections_smart_test.go index 1210c370..0a785d05 100644 --- a/cmd/server/tests/collections_smart_test.go +++ b/cmd/server/tests/collections_smart_test.go @@ -213,7 +213,7 @@ func TestSmartCollections(t *testing.T) { t.Run("SystemCollectionsBlockedFromTheEditor", func(t *testing.T) { // System collections may never carry rules/trees — the editor // route refuses them (item 3.4: the page IS the editor). - colls, err := setup.DB.GetCollectionsByUser(context.Background(), pgtype.UUID{Bytes: [16]byte(adminID), Valid: true}) + colls, err := setup.DB.GetCollectionsByUser(ownerSeedCtx(pgtype.UUID{Bytes: [16]byte(adminID), Valid: true}), pgtype.UUID{Bytes: [16]byte(adminID), Valid: true}) require.NoError(t, err) var systemID string for _, c := range colls { diff --git a/cmd/server/tests/filter_builder_test.go b/cmd/server/tests/filter_builder_test.go index 3e6f7c4d..d8987130 100644 --- a/cmd/server/tests/filter_builder_test.go +++ b/cmd/server/tests/filter_builder_test.go @@ -455,12 +455,17 @@ func TestFilterUserRelativeFields(t *testing.T) { aID, wickID, aID, tideID) var collID pgtype.UUID - err := setup.DBPool.QueryRow(context.Background(), + // Seed as the OWNER (collections RLS, 2026-10-31): the insert + // predicate wants actor == owner, and the item/cleanup writes want + // the same identity — the admin write-escape covers SYSTEM rows only. + err := database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(aID), `INSERT INTO collections (user_id, name) VALUES ($1, $2) RETURNING id`, aID, fmt.Sprintf("Test Builder Coll %d", time.Now().UnixNano())).Scan(&collID) require.NoError(t, err) - t.Cleanup(func() { _ = setup.DB.DeleteCollection(ctx, collID) }) - seedExec(t, setup.DBPool, + t.Cleanup(func() { + scopeExecAs(t, setup, aID, `DELETE FROM collections WHERE id = $1`, collID) + }) + scopeExecAs(t, setup, aID, `INSERT INTO collection_items (collection_id, media_item_id) VALUES ($1,$2)`, collID, tideID) titlesFor := func(token string, qf string) []string { @@ -542,7 +547,6 @@ func TestFilterSuggestionsUnderRules(t *testing.T) { libUUID, _ := builderFixtureLib(t, setup, userID, "Sug") // Add a second distinct author+genre pair (trigram-disjoint). - ctx := context.Background() _, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: libUUID, Valid: true}, Title: "Sug Dune", @@ -670,16 +674,21 @@ func TestFilterSuggestionsUnderRules(t *testing.T) { // offered with the surviving count; zero-match containers never are. userA := getRegularUserID(t, setup.DB) var collID pgtype.UUID - err = setup.DBPool.QueryRow(context.Background(), + // Seed as the OWNER (collections RLS, 2026-10-31) — same identity + // rule as the first seeding site in this file. + ownerID := pgtype.UUID{Bytes: [16]byte(userA), Valid: true} + err = database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(ownerID), `INSERT INTO collections (user_id, name) VALUES ($1, $2) RETURNING id`, - pgtype.UUID{Bytes: [16]byte(userA), Valid: true}, + ownerID, fmt.Sprintf("Test Builder Sug Coll %d", time.Now().UnixNano())).Scan(&collID) require.NoError(t, err) - t.Cleanup(func() { _ = setup.DB.DeleteCollection(ctx, collID) }) + t.Cleanup(func() { + scopeExecAs(t, setup, ownerID, `DELETE FROM collections WHERE id = $1`, collID) + }) var tideID pgtype.UUID err = setup.DBPool.QueryRow(context.Background(), `SELECT id FROM media_items WHERE title = $1`, "Sug Tide").Scan(&tideID) require.NoError(t, err) - seedExec(t, setup.DBPool, + scopeExecAs(t, setup, ownerID, `INSERT INTO collection_items (collection_id, media_item_id) VALUES ($1, $2)`, collID, tideID) status, results = suggest("collection", " ", "") diff --git a/cmd/server/tests/playlist_collections_test.go b/cmd/server/tests/playlist_collections_test.go index 62352db8..76623eb1 100644 --- a/cmd/server/tests/playlist_collections_test.go +++ b/cmd/server/tests/playlist_collections_test.go @@ -13,6 +13,8 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" + + "bookhoard/internal/database" ) // A2c end-to-end: an ABS sidecar whose series array mixes a real series and @@ -67,13 +69,14 @@ func TestPlaylistCollectionsEndToEnd(t *testing.T) { require.NotEmpty(t, foundPath) // (b) The playlist collection exists with provenance, dashboard pin, - // and the item at reading position 7. + // and the item at reading position 7. Owner-scoped read: collections + // RLS (2026-10-31) hides the row from the pool session's SeedIdentity. adminUUIDStr := adminID.String() var collID uuid.UUID var collName, collDesc string var showOnDashboard bool var viewSettingsRaw []byte - require.NoError(t, setup.DBPool.QueryRow(context.Background(), + require.NoError(t, database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(adminID), `SELECT id, name, description, show_on_dashboard, view_settings FROM collections WHERE user_id = $1 AND name = 'The Tagged Saga (Reading Order)'`, adminID).Scan(&collID, &collName, &collDesc, &showOnDashboard, &viewSettingsRaw)) @@ -85,7 +88,9 @@ func TestPlaylistCollectionsEndToEnd(t *testing.T) { var position *int32 var itemCount int - require.NoError(t, setup.DBPool.QueryRow(context.Background(), + // Owner-scoped reads: collections RLS (2026-10-31) — items ride the + // parent's readability, so the pool session's SeedIdentity sees none. + require.NoError(t, database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(adminID), `SELECT ci.position, COUNT(*) OVER () FROM collection_items ci WHERE ci.collection_id = $1`, collID).Scan(&position, &itemCount)) require.Equal(t, 1, itemCount) @@ -96,11 +101,11 @@ func TestPlaylistCollectionsEndToEnd(t *testing.T) { // (c) Rescan: no duplicate collection, no duplicate membership. scanTestFolder(t, setup, adminID, folder) var collectionCount, membershipCount int - require.NoError(t, setup.DBPool.QueryRow(context.Background(), + require.NoError(t, database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(adminID), `SELECT COUNT(*) FROM collections WHERE user_id = $1 AND name = 'The Tagged Saga (Reading Order)'`, adminID).Scan(&collectionCount)) require.Equal(t, 1, collectionCount) - require.NoError(t, setup.DBPool.QueryRow(context.Background(), + require.NoError(t, database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(adminID), `SELECT COUNT(*) FROM collection_items WHERE collection_id = $1`, collID).Scan(&membershipCount)) require.Equal(t, 1, membershipCount) @@ -131,7 +136,9 @@ func TestPlaylistCollectionsEndToEnd(t *testing.T) { // (e) hide_finished + Up Next + completion marker. // Flip the collection's flag on, mark the single item finished for the // admin, and the dashboard section becomes Complete with no items. - _, err = setup.DBPool.Exec(context.Background(), + // Owner-or-admin UPDATE policy: run as ADMIN (collections RLS, + // 2026-10-31). + adminScopeExec(t, setup, adminCtxValue(t, setup), `UPDATE collections SET view_settings = view_settings || '{"hide_finished": true}'::jsonb WHERE id = $1`, collID) require.NoError(t, err) _, err = setup.DBPool.Exec(context.Background(), diff --git a/cmd/server/tests/scan_state_test.go b/cmd/server/tests/scan_state_test.go index 21008646..03f753b9 100644 --- a/cmd/server/tests/scan_state_test.go +++ b/cmd/server/tests/scan_state_test.go @@ -207,12 +207,13 @@ func TestScanManifestLifecycle(t *testing.T) { require.NoError(t, os.WriteFile(opfPath, []byte(`{"title":"Manifesu Book","series":["Lifecycle Saga (Reading Order) #1"]}`), 0644)) scanTestFolder(t, setup, admin, folder) var collCount int - require.NoError(t, setup.DBPool.QueryRow(context.Background(), + // Owner-scoped read: collections RLS (2026-10-31). + require.NoError(t, database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(admin), `SELECT COUNT(*) FROM collections WHERE name = 'Lifecycle Saga (Reading Order)' AND user_id = $1`, admin, ).Scan(&collCount)) assert.Equal(t, 1, collCount, "reading-order sidecar added to an existing book creates its collection") scanTestFolder(t, setup, admin, folder) - require.NoError(t, setup.DBPool.QueryRow(context.Background(), + require.NoError(t, database.NewScopedDB(setup.DBPool).QueryRow(ownerSeedCtx(admin), `SELECT COUNT(*) FROM collections WHERE name = 'Lifecycle Saga (Reading Order)' AND user_id = $1`, admin, ).Scan(&collCount)) assert.Equal(t, 1, collCount, "rescan is idempotent — no duplicate collection") diff --git a/cmd/server/tests/test_helpers_test.go b/cmd/server/tests/test_helpers_test.go index aa3df61b..31fda710 100644 --- a/cmd/server/tests/test_helpers_test.go +++ b/cmd/server/tests/test_helpers_test.go @@ -329,6 +329,22 @@ func seedCtx() context.Context { return rls.WithUser(context.Background(), rls.SeedIdentity) } +// ownerSeedCtx returns a context carrying a fixture user's identity, +// for seeding rows OWNED by that user. Collections RLS (2026-10-31): +// the write predicates want actor == owner for non-system rows (the +// admin write-escape covers SYSTEM collections only), and reads under +// the pool session's SeedIdentity see nothing. +func ownerSeedCtx(userID pgtype.UUID) context.Context { + return rls.WithUser(context.Background(), userID) +} + +// scopeExecAs is adminScopeExec's generic form: raw SQL under ANY +// declared identity. Use it (with the row's OWNER) for fixture writes +// to non-system collections — see ownerSeedCtx. +func scopeExecAs(t *testing.T, setup *TestServerSetup, userID pgtype.UUID, sql string, args ...any) { + adminScopeExec(t, setup, userID, sql, args...) +} + // seedExec runs a raw statement against the pool under the seed // identity. The bare pool bypasses ScopedDB (nothing reads the ctx for // the GUC), so governed tables need the identity declared manually @@ -356,7 +372,12 @@ func uuidToPGType(u uuid.UUID) pgtype.UUID { // createDefaultCollectionsForUser creates the 4 default system collections for a user func createDefaultCollectionsForUser(t *testing.T, db *database.Queries, userID pgtype.UUID) { - ctx := context.Background() + // Owner-scoped seeding: the collections RLS policies (2026-10-31) + // need an acting identity, and the harness seeds before any JWT — + // the same bootstrap fix as CreateDefaultCollectionsForUser in + // internal/handlers/auth.go. Owner == actor, so SELECT sees the + // rows, the INSERT passes, and the heal-UPDATE passes. + ctx := rls.WithUser(context.Background(), userID) // Canonical Title-case names — the same list the registration seed // (CreateDefaultCollectionsForUser) and RestoreSystemCollection use. // A stale kebab-case copy lived here once and duplicated every @@ -772,11 +793,15 @@ func setupTestServer(t *testing.T) *TestServerSetup { // run's creates die on UNIQUE(user_id, name) — the "collections // 500-panic" rot. System collections are skipped (permanent rows). // Same convention as the library pass: names containing "test". - adminColls, cerr := queries.GetCollectionsByUser(ctx, pgtype.UUID{Bytes: adminUUID, Valid: true}) + // Swept under the OWNER's identity: collections RLS (2026-10-31) + // makes unscoped reads return nothing, which would silently turn + // this cleanup into a no-op. + adminScope := rls.WithUser(ctx, pgtype.UUID{Bytes: adminUUID, Valid: true}) + adminColls, cerr := queries.GetCollectionsByUser(adminScope, pgtype.UUID{Bytes: adminUUID, Valid: true}) if cerr == nil { for _, coll := range adminColls { if !coll.IsSystemCollection.Bool && strings.Contains(strings.ToLower(coll.Name), "test") { - queries.DeleteCollection(ctx, coll.ID) + queries.DeleteCollection(adminScope, coll.ID) } } } @@ -850,11 +875,13 @@ func setupTestServer(t *testing.T) *TestServerSetup { // Same pass for test-created COLLECTIONS (the "collections // 500-panic" rot: they lingered forever and poisoned later runs). - adminColls, cerr := queries.GetCollectionsByUser(ctx, pgtype.UUID{Bytes: adminUUID, Valid: true}) + // Owner-scoped for the same RLS reason as the setup sweep. + adminScope := rls.WithUser(ctx, pgtype.UUID{Bytes: adminUUID, Valid: true}) + adminColls, cerr := queries.GetCollectionsByUser(adminScope, pgtype.UUID{Bytes: adminUUID, Valid: true}) if cerr == nil { for _, coll := range adminColls { if !coll.IsSystemCollection.Bool && strings.Contains(strings.ToLower(coll.Name), "test") { - queries.DeleteCollection(ctx, coll.ID) + queries.DeleteCollection(adminScope, coll.ID) } } } diff --git a/database/schema/schema.sql b/database/schema/schema.sql index 77e174a1..947de34c 100644 --- a/database/schema/schema.sql +++ b/database/schema/schema.sql @@ -2180,3 +2180,117 @@ CREATE POLICY p_podcast_feeds_delete ON podcast_feeds -- policy's lookup table, and a user must always see their own -- subscription rows (subscribe -> visibility is a single INSERT). -- ============================================================ +-- ── collections + collection_items (2026-10-31): ownership at the DB ── +-- layer, same doctrine as media_items/podcast_feeds above. The +-- predicates live in FUNCTIONS (not inline) so a future sharing model +-- edits exactly one place: add the share-table EXISTS branch inside +-- app_can_read_collection / app_can_write_collection. The functions are +-- PURE over their arguments — they never query collections — so the +-- policies cannot recurse, FORCE RLS safe. Reads are authenticated- +-- allow (household-readable: any signed-in identity, SeedIdentity +-- harness included); WRITES are owner-or-admin — that is the +-- enforcement of record for Update/Delete/AddBooks, which have no +-- app-level gate. +-- =========================================================== + +CREATE OR REPLACE FUNCTION app_is_admin() RETURNS BOOLEAN +LANGUAGE sql STABLE SECURITY DEFINER SET search_path = pg_catalog, public AS $$ + SELECT EXISTS ( + SELECT 1 FROM users u + WHERE u.id = NULLIF(current_setting('app.user_id', true), '')::uuid + AND u.role = 'admin' + ); +$$; + +CREATE OR REPLACE FUNCTION app_can_read_collection( + p_collection_id UUID, p_owner UUID +) RETURNS BOOLEAN +LANGUAGE sql STABLE SECURITY DEFINER SET search_path = pg_catalog, public AS $$ + SELECT NULLIF(current_setting('app.user_id', true), '') IS NOT NULL + AND ( + p_owner = current_setting('app.user_id', true)::uuid + OR app_is_admin() + -- FUTURE SHARING slot: OR EXISTS ( + -- SELECT 1 FROM collection_shares cs + -- WHERE cs.collection_id = p_collection_id + -- AND cs.shared_with_user_id = current_setting('app.user_id', true)::uuid) + ); +$$; + +CREATE OR REPLACE FUNCTION app_can_write_collection( + p_collection_id UUID, p_owner UUID, p_system BOOLEAN +) RETURNS BOOLEAN +LANGUAGE sql STABLE SECURITY DEFINER SET search_path = pg_catalog, public AS $$ + SELECT NULLIF(current_setting('app.user_id', true), '') IS NOT NULL + AND ( + p_owner = current_setting('app.user_id', true)::uuid + OR (COALESCE(p_system, false) AND app_is_admin()) + -- FUTURE SHARING slot: OR EXISTS ( + -- SELECT 1 FROM collection_shares cs + -- WHERE cs.collection_id = p_collection_id + -- AND cs.shared_with_user_id = current_setting('app.user_id', true)::uuid + -- AND cs.permission IN ('edit')) + ); +$$; + +ALTER TABLE collections ENABLE ROW LEVEL SECURITY; +ALTER TABLE collections FORCE ROW LEVEL SECURITY; + +DROP POLICY IF EXISTS p_collections_select ON collections; +CREATE POLICY p_collections_select ON collections + FOR SELECT USING (app_can_read_collection(id, user_id)); +DROP POLICY IF EXISTS p_collections_insert ON collections; +CREATE POLICY p_collections_insert ON collections + FOR INSERT WITH CHECK ( + user_id = NULLIF(current_setting('app.user_id', true), '')::uuid + OR app_is_admin() + ); +DROP POLICY IF EXISTS p_collections_update ON collections; +CREATE POLICY p_collections_update ON collections + FOR UPDATE + USING (app_can_write_collection(id, user_id, is_system_collection)) + WITH CHECK (app_can_write_collection(id, user_id, is_system_collection)); +DROP POLICY IF EXISTS p_collections_delete ON collections; +CREATE POLICY p_collections_delete ON collections + FOR DELETE USING (app_can_write_collection(id, user_id, is_system_collection)); + +ALTER TABLE collection_items ENABLE ROW LEVEL SECURITY; +ALTER TABLE collection_items FORCE ROW LEVEL SECURITY; + +-- Items ride their PARENT's accessibility: the collections SELECT +-- policy auto-applies inside these EXISTS subqueries. +DROP POLICY IF EXISTS p_collection_items_select ON collection_items; +CREATE POLICY p_collection_items_select ON collection_items + FOR SELECT USING (EXISTS ( + SELECT 1 FROM collections c + WHERE c.id = collection_items.collection_id + )); +DROP POLICY IF EXISTS p_collection_items_insert ON collection_items; +CREATE POLICY p_collection_items_insert ON collection_items + FOR INSERT WITH CHECK (EXISTS ( + SELECT 1 FROM collections c + WHERE c.id = collection_items.collection_id + AND app_can_write_collection(c.id, c.user_id, c.is_system_collection) + )); +DROP POLICY IF EXISTS p_collection_items_update ON collection_items; +CREATE POLICY p_collection_items_update ON collection_items + FOR UPDATE + USING (EXISTS ( + SELECT 1 FROM collections c + WHERE c.id = collection_items.collection_id + AND app_can_write_collection(c.id, c.user_id, c.is_system_collection) + )) + WITH CHECK (EXISTS ( + SELECT 1 FROM collections c + WHERE c.id = collection_items.collection_id + AND app_can_write_collection(c.id, c.user_id, c.is_system_collection) + )); +DROP POLICY IF EXISTS p_collection_items_delete ON collection_items; +CREATE POLICY p_collection_items_delete ON collection_items + FOR DELETE USING (EXISTS ( + SELECT 1 FROM collections c + WHERE c.id = collection_items.collection_id + AND app_can_write_collection(c.id, c.user_id, c.is_system_collection) + )); + + diff --git a/docs/contract_pins.md b/docs/contract_pins.md index 3d29bcda..d929c1ad 100644 --- a/docs/contract_pins.md +++ b/docs/contract_pins.md @@ -514,3 +514,34 @@ Test-side deletions follow the asymmetry rule: subtests exercising the deleted endpoints are gone; their surviving SEMANTICS are pinned in their tree form (TestSmartCollections rewritten on trees, TestEpisodeCollectionsFilterTree, TestPodcastSmartCollections). + +## 2026-10-31 — collections RLS: cross-user detail/writes tighten to 404/no-op + +`collections` and `collection_items` joined the RLS-governed tables +(policies `p_collections_*` / `p_collection_items_*`, predicates +`app_can_read_collection` / `app_can_write_collection` — the same +doctrine as the media_items family lens, 2026-10-29). Contract +changes: + +- `GET /api/collections/:id` for another user's collection: was 200 + (by-id readability), now 404 (row invisible). +- `PUT /api/collections/:id` cross-user: was ungated success (no + app-level ownership check), now 404 — the RLS update touches no + rows, and the handler maps ErrNoRows to 404. +- `POST /api/collections/:id/books` cross-user: was ungated, now 404 + (rejectSmartMutation's GetCollectionData is RLS-invisible). +- `DELETE /api/collections/:id` cross-user: still 204, but now a true + DB-level no-op (zero rows deleted) instead of an app-level free hit. +- `PUT /api/collections/:id/view-settings` cross-user: 404 (the owner + gate stays as defense-in-depth; the DB is enforcement of record). +- `PUT .../match-mode` and `.../items/order` keep their app-level 403 + gates; the DB now backs them. +- Reads: owner or admin (policy escape); non-owner non-admin users see + nothing, anonymous included. Future sharing rides the predicate + functions — one EXISTS branch in app_can_read/write_collection, no + policy migration. + +Test-side: TestCollectionsRLS pins the cross-user matrix end-to-end; +fixture writes to non-system collections run as the row's OWNER +(scopeExecAs/ownerSeedCtx) because the admin write-escape covers +system collections only. diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 72853e10..e5980384 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -6,6 +6,7 @@ package handlers import ( "bookhoard/internal/database" "bookhoard/internal/middleware" + "bookhoard/internal/rls" "bookhoard/internal/setupstatus" "context" "errors" @@ -1145,6 +1146,11 @@ func (h *AuthHandler) generateJWT(userID string) (string, error) { } func (h *AuthHandler) CreateDefaultCollectionsForUser(ctx context.Context, userID pgtype.UUID) error { + // Registration runs BEFORE the JWT exists — the request context + // carries no identity, and the collections RLS insert policy + // (2026-10-31) would reject the five system-collection seeds. + // Declare the new user as the actor: owner == actor, policy passes. + ctx = rls.WithUser(ctx, userID) defaultCollections := []struct { Name string Description string diff --git a/internal/handlers/collections.go b/internal/handlers/collections.go index dfba00e9..3009cfc6 100644 --- a/internal/handlers/collections.go +++ b/internal/handlers/collections.go @@ -545,6 +545,12 @@ func (h *CollectionHandler) UpdateCollection(c *echo.Context) error { treeJSON, ) if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + // Collections RLS: another user's collection is INVISIBLE — + // the update touched no rows. Answer 404, not a 500 that + // reads like a bug (2026-10-31 collections RLS). + return c.JSON(http.StatusNotFound, map[string]string{"error": "collection not found"}) + } return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) } diff --git a/internal/services/collection_service.go b/internal/services/collection_service.go index 25bef923..ee45ac1c 100644 --- a/internal/services/collection_service.go +++ b/internal/services/collection_service.go @@ -238,7 +238,7 @@ func (s *CollectionService) UpdateCollection(ctx context.Context, collectionID u }) if err != nil { - return database.Collections{}, fmt.Errorf("failed to update collection: %v", err) + return database.Collections{}, fmt.Errorf("failed to update collection: %w", err) } return collection, nil