From 8c3d919aa060752060424263983adee4025e94d6 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Thu, 29 Oct 2026 08:44:12 -0400 Subject: [PATCH] wip(rls): engine-side fail-loud + harness seed identity [pin: work interrupted] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design correction discovered by the auth chicken-and-egg: the decorator-level refusal was re-created a convention gate (plan B thinking). ScopedDB now NEVER refuses — it declares the GUC when the context carries an identity and declares nothing otherwise; the ENGINE is the fail-loud layer (policies raise on governed tables, ungoverned tables like auth lookups simply work). rls.WithInternal removed as a dead concept. - search service's raw DBTX path now receives the ScopedDB itself (main.go + harness) — a bare pool bypassed the GUC. - database/schema.go: migration tx declares a synthetic identity (schema.sql's dedup DO block SELECTs media_items; with policies live, boot 2 raised 'unrecognized configuration parameter' — A11). - harness: TestServerSetup.DB is a raw-pool SEED handle (write policies permissive, other seeded tables ungoverned); the server under test keeps the scoped wiring. 33 CreateMediaItem sites use seedCtx() — a seed identity with no visibility rows — because RETURNING requires SELECT visibility of the new row. - invariant test: regex UUID-scraping replaced with a structured JSON decode of the sections payload. INTERRUPTION NOTE: an edit to this same effect (seedCtx) was applied, confirmed by the tool, and subsequently vanished from disk while an 'opencode serve' process (PID 84551) is alive on the machine. This commit pins the tree against further loss; implementation is PAUSED pending the user killing that process. --- cmd/server/main.go | 21 ++++---- cmd/server/tests/collections_preview_test.go | 2 +- cmd/server/tests/collections_smart_test.go | 2 +- .../tests/comic_metadata_display_test.go | 6 +-- cmd/server/tests/comic_metadata_test.go | 14 +++--- cmd/server/tests/contract_pins_test.go | 2 +- cmd/server/tests/facets_integration_test.go | 12 ++--- cmd/server/tests/filter_builder_test.go | 6 +-- cmd/server/tests/ownership_and_admin_test.go | 4 +- cmd/server/tests/phase13_test.go | 2 +- cmd/server/tests/queue_test.go | 2 +- cmd/server/tests/rls_visibility_test.go | 24 ++++++--- cmd/server/tests/scan_state_test.go | 2 +- cmd/server/tests/statistics_test.go | 8 +-- cmd/server/tests/sync_integration_test.go | 2 +- cmd/server/tests/test_helpers_test.go | 30 +++++++++--- internal/database/schema.go | 16 ++++++ internal/database/scoped.go | 49 ++++++++----------- internal/rls/rls.go | 31 ++---------- 19 files changed, 124 insertions(+), 111 deletions(-) diff --git a/cmd/server/main.go b/cmd/server/main.go index ea6db199..9a316f48 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -7,7 +7,6 @@ import ( "bookhoard/internal/handlers" "bookhoard/internal/middleware" ratelimit "bookhoard/internal/middleware" - "bookhoard/internal/rls" "bookhoard/internal/router" "bookhoard/internal/services" "bookhoard/internal/sync" @@ -42,7 +41,12 @@ func main() { } defer dbPool.Close() - queries := database.New(database.NewScopedDB(dbPool)) + // The search service's raw DBTX path must ALSO carry the RLS + // identity — hand it the ScopedDB itself (a DBTX), never the bare + // pool, or search queries bypass the per-user GUC and raise under + // the policies. + scopedDB := database.NewScopedDB(dbPool) + queries := database.New(scopedDB) // Initialize database schema log.Println("🔧 Ensuring database schema is initialized...") @@ -54,10 +58,11 @@ func main() { // Load tunable settings from the DB into the registry. All values fall back // to compiled defaults if a row is missing, so this never blocks startup. - // INTERNAL context: startup has no end-user identity; system_settings - // is not RLS-governed, so the statement runs without the GUC (plan §5). + // (Startup has no end-user identity; system_settings is not RLS- + // governed, and ScopedDB declares nothing when no identity exists — + // the engine only raises on governed tables.) registry := database.NewSettingsRegistry(queries) - if err := registry.Load(rls.WithInternal(ctx)); err != nil { + if err := registry.Load(ctx); err != nil { log.Printf("⚠️ Could not load system settings (using defaults): %v", err) } @@ -68,9 +73,7 @@ func main() { if registry.RegistrationEnabled() { return true } - // INTERNAL: a "how many admins exist" read is not user-scoped; - // users is not RLS-governed (plan §5). - count, err := queries.CountAdmins(rls.WithInternal(context.Background())) + count, err := queries.CountAdmins(context.Background()) if err != nil { return true // fail open, matching setupstatus } @@ -170,7 +173,7 @@ func main() { dashboardHandler := handlers.NewDashboardHandler(queries) seriesHandler := handlers.NewSeriesHandler(queries) filtersHandler := handlers.NewFiltersHandler(queries) - mediaHandler := handlers.NewMediaHandler(queries, dbPool, libraryService, progressService, annotationService) + mediaHandler := handlers.NewMediaHandler(queries, scopedDB, libraryService, progressService, annotationService) matchingHandler := handlers.NewMatchingHandler(queries, connManager) jobsHandler := handlers.NewJobsHandler(queries, worker) diff --git a/cmd/server/tests/collections_preview_test.go b/cmd/server/tests/collections_preview_test.go index 6f346afd..ef3366e8 100644 --- a/cmd/server/tests/collections_preview_test.go +++ b/cmd/server/tests/collections_preview_test.go @@ -379,7 +379,7 @@ func createPreviewTestMediaItems(t *testing.T, db *database.Queries, libraryID s for i := 0; i < count; i++ { genre := genres[i%len(genres)] - media, err := db.CreateMediaItem(ctx, database.CreateMediaItemParams{ + media, err := db.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: libUUID, Valid: true}, Title: "Preview Test Book " + string(rune('A'+i)), Authors: []string{"Test Author " + string(rune('A'+i))}, diff --git a/cmd/server/tests/collections_smart_test.go b/cmd/server/tests/collections_smart_test.go index 77fe5335..21eb69a6 100644 --- a/cmd/server/tests/collections_smart_test.go +++ b/cmd/server/tests/collections_smart_test.go @@ -108,7 +108,7 @@ func smartCreateLibraryAndBooks(t *testing.T, setup *TestServerSetup, userID uui }) for title, genre := range titlesByGenre { - media, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + media, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: libUUID, Valid: true}, Title: title, Authors: []string{"Smart Rule Author"}, diff --git a/cmd/server/tests/comic_metadata_display_test.go b/cmd/server/tests/comic_metadata_display_test.go index 3007ca64..e03e3258 100644 --- a/cmd/server/tests/comic_metadata_display_test.go +++ b/cmd/server/tests/comic_metadata_display_test.go @@ -113,7 +113,7 @@ func TestComicMetadataDisplay_ComicDetailPage(t *testing.T) { params.AlternateInfo = []byte(v) } - mediaItem, err := setup.DB.CreateMediaItem(ctx, params) + mediaItem, err := setup.DB.CreateMediaItem(seedCtx(), params) require.NoError(t, err, "Should create media item") mediaUUID, err := uuid.FromBytes(mediaItem.ID.Bytes[0:16]) @@ -596,7 +596,7 @@ func TestComicMetadataDisplay_ComicDetailPage(t *testing.T) { Isbn: pgtype.Text{String: "9781234567890", Valid: true}, } - mediaItem, err := setup.DB.CreateMediaItem(ctx, params) + mediaItem, err := setup.DB.CreateMediaItem(seedCtx(), params) require.NoError(t, err, "Should create ebook") mediaUUID, err := uuid.FromBytes(mediaItem.ID.Bytes[0:16]) @@ -740,7 +740,7 @@ func TestComicMetadataDisplay_AllFieldsTogether(t *testing.T) { // Create a media item with ALL metadata fields populated alternateInfoStr := `{"alternate_series":"Ultimate X-Men","alternate_number":1,"alternate_count":12}` var mediaItem database.MediaItems - mediaItem, err = setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + mediaItem, err = setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: parsedLibraryID, Valid: true}, Title: "Complete Metadata Comic", FilePath: "/test/complete.cbz", diff --git a/cmd/server/tests/comic_metadata_test.go b/cmd/server/tests/comic_metadata_test.go index dc6b5c5b..d217d305 100644 --- a/cmd/server/tests/comic_metadata_test.go +++ b/cmd/server/tests/comic_metadata_test.go @@ -25,7 +25,7 @@ func TestComicMetadataExtraction(t *testing.T) { require.NoError(t, err, "Should parse library UUID") // Insert test media item with full comic metadata - _, err = setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err = setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: parsedLibraryID, Valid: true}, Title: "Test Manga", FilePath: "/test/manga.cbz", @@ -64,7 +64,7 @@ func TestComicMetadataExtraction(t *testing.T) { parsedLibraryID, err := uuid.Parse(libraryID) require.NoError(t, err, "Should parse library UUID") - _, err = setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err = setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: parsedLibraryID, Valid: true}, Title: "Test Comic", FilePath: "/test/comic.cbz", @@ -94,7 +94,7 @@ func TestComicMetadataExtraction(t *testing.T) { parsedLibraryID, err := uuid.Parse(libraryID) require.NoError(t, err, "Should parse library UUID") - _, err = setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err = setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: parsedLibraryID, Valid: true}, Title: "Minimal Comic", FilePath: "/test/minimal.cbz", @@ -136,7 +136,7 @@ func TestReadingDirectionAPI(t *testing.T) { } for _, tc := range testCases { - _, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: parsedLibraryID, Valid: true}, Title: tc.title, FilePath: "/test/" + tc.title + ".cbz", @@ -205,7 +205,7 @@ func TestUniversalMetadataFields(t *testing.T) { require.NoError(t, err, "Should parse ebook library UUID") t.Run("Comic with universal fields", func(t *testing.T) { - _, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: parsedComicLibraryID, Valid: true}, Title: "Comic with Universal Metadata", FilePath: "/test/comic.cbz", @@ -231,7 +231,7 @@ func TestUniversalMetadataFields(t *testing.T) { }) t.Run("Ebook with universal fields", func(t *testing.T) { - _, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: parsedEbookLibraryID, Valid: true}, Title: "Ebook with Universal Metadata", FilePath: "/test/book.epub", @@ -270,7 +270,7 @@ func TestComicSpecificFields(t *testing.T) { t.Run("Alternate series info as JSONB", func(t *testing.T) { alternateInfo := `{"alternate_series":"Ultimate X-Men","alternate_number":1,"alternate_count":12}` - _, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: parsedLibraryID, Valid: true}, Title: "X-Men with Alternate Series", FilePath: "/test/xmen.cbz", diff --git a/cmd/server/tests/contract_pins_test.go b/cmd/server/tests/contract_pins_test.go index 17388157..8e5cddc4 100644 --- a/cmd/server/tests/contract_pins_test.go +++ b/cmd/server/tests/contract_pins_test.go @@ -120,7 +120,7 @@ func createContractFixture(t *testing.T, setup *TestServerSetup) contractFixture seriesName := fmt.Sprintf("Test Contract Series %d", time.Now().UnixNano()) itemTitle := fmt.Sprintf("Test Contract Item %d", time.Now().UnixNano()) - media, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + media, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: libUUID, Valid: true}, Title: itemTitle, Authors: []string{"Test Contract Author"}, diff --git a/cmd/server/tests/facets_integration_test.go b/cmd/server/tests/facets_integration_test.go index 735439e2..dbbabca6 100644 --- a/cmd/server/tests/facets_integration_test.go +++ b/cmd/server/tests/facets_integration_test.go @@ -89,7 +89,7 @@ func TestFacetRegistryParamsFilter(t *testing.T) { if mutate != nil { mutate(&p) } - _, err := setup.DB.CreateMediaItem(ctx, p) + _, err := setup.DB.CreateMediaItem(seedCtx(), p) require.NoError(t, err, "create %s", title) } @@ -194,7 +194,7 @@ func TestFacetsEndpoint(t *testing.T) { // Two items share the genre; one carries the language + narrator. for _, title := range []string{"Facet List One", "Facet List Two"} { - _, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: library.ID, Title: title, Authors: []string{"Facet Listing Author"}, @@ -206,7 +206,7 @@ func TestFacetsEndpoint(t *testing.T) { }) require.NoError(t, err) } - _, err = setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err = setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: library.ID, Title: "Facet List Three", Authors: []string{"Facet Listing Author"}, @@ -329,7 +329,7 @@ func facetFixtureBooks(t *testing.T, setup *TestServerSetup, libraryID pgtype.UU {prefix + " Gamma", "history", "Pinecrest"}, } for _, b := range books { - _, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: libraryID, Title: b.title, Authors: []string{prefix + " Author"}, @@ -426,7 +426,7 @@ func TestFacetCountsUnderFilters(t *testing.T) { facetFixtureBooks(t, setup, library.ID, "Counts") // A book in the HIDDEN library: distinctive values that must never // surface for the regular user. - _, err = setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err = setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: hiddenLib.ID, Title: "Counts Hidden Book", Authors: []string{"Counts Secret Author"}, @@ -559,7 +559,7 @@ func TestFieldValuesAllLibraries(t *testing.T) { id pgtype.UUID name string }{{visibleLib.ID, "Visible"}, {hiddenLib.ID, "Hidden"}} { - _, err = setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err = setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: lib.id, Title: fmt.Sprintf("Field Values %s Book", lib.name), Authors: []string{"Verdana Scribe"}, diff --git a/cmd/server/tests/filter_builder_test.go b/cmd/server/tests/filter_builder_test.go index 0979da53..4d34946d 100644 --- a/cmd/server/tests/filter_builder_test.go +++ b/cmd/server/tests/filter_builder_test.go @@ -156,7 +156,7 @@ func builderFixtureLib(t *testing.T, setup *TestServerSetup, userID uuid.UUID, n } books[i].TagsSearch = lowered } - _, err := setup.DB.CreateMediaItem(ctx, books[i]) + _, err := setup.DB.CreateMediaItem(seedCtx(), books[i]) require.NoError(t, err, "create %s", books[i].Title) } return libUUID, library.ID @@ -445,7 +445,7 @@ func TestFilterSuggestionsUnderRules(t *testing.T) { // Add a second distinct author+genre pair (trigram-disjoint). ctx := context.Background() - _, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: libUUID, Valid: true}, Title: "Sug Dune", Authors: []string{"Zephyrine Hale"}, @@ -632,7 +632,7 @@ func TestFilterFamilyLens(t *testing.T) { id pgtype.UUID title string }{{visibleID, "Lens Vis Book"}, {hiddenID, "Lens Hid Book"}} { - _, err = setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err = setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: lib.id, Title: lib.title, Authors: []string{"Lens Secret Author"}, FilePath: fmt.Sprintf("/tmp/builder/%s-%d.epub", lib.title, time.Now().UnixNano()), diff --git a/cmd/server/tests/ownership_and_admin_test.go b/cmd/server/tests/ownership_and_admin_test.go index 6e0cbfbb..2492bb64 100644 --- a/cmd/server/tests/ownership_and_admin_test.go +++ b/cmd/server/tests/ownership_and_admin_test.go @@ -85,7 +85,7 @@ func (s *OwnershipAndAdminTestSuite) createItemViaDB(withFolder bool) (itemID, l } } - item, err := db.CreateMediaItem(ctx, database.CreateMediaItemParams{ + item, err := db.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: library.ID, Title: "Ownership Suite Item", Authors: []string{"Suite Author"}, @@ -269,7 +269,7 @@ func (s *OwnershipAndAdminTestSuite) TestDelete_StorageDownRefused() { }) require.NoError(s.T(), err) - item, err := db.CreateMediaItem(ctx, database.CreateMediaItemParams{ + item, err := db.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: library.ID, Title: "Offline Storage Item", FilePath: "offline.epub", diff --git a/cmd/server/tests/phase13_test.go b/cmd/server/tests/phase13_test.go index a6bee305..dbf07b39 100644 --- a/cmd/server/tests/phase13_test.go +++ b/cmd/server/tests/phase13_test.go @@ -134,7 +134,7 @@ func TestManuallyEditedFilter(t *testing.T) { t.Cleanup(func() { deleteLibraries(t, setup, libID) }) createBook := func(path string) pgtype.UUID { - created, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + created, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: libID, Title: path, Authors: []string{}, diff --git a/cmd/server/tests/queue_test.go b/cmd/server/tests/queue_test.go index d674e9df..0a41f438 100644 --- a/cmd/server/tests/queue_test.go +++ b/cmd/server/tests/queue_test.go @@ -48,7 +48,7 @@ func createTestQueueItem(t *testing.T, db *database.Queries, userID uuid.UUID) ( require.NoError(t, err, "Should create library") // 4. Create media item - mediaItem, err := db.CreateMediaItem(ctx, database.CreateMediaItemParams{ + mediaItem, err := db.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: library.ID, Title: "Test Book for Queue", Authors: []string{"Test Author"}, diff --git a/cmd/server/tests/rls_visibility_test.go b/cmd/server/tests/rls_visibility_test.go index 5be5c790..19f3ecfc 100644 --- a/cmd/server/tests/rls_visibility_test.go +++ b/cmd/server/tests/rls_visibility_test.go @@ -23,9 +23,9 @@ package main import ( "context" + "encoding/json" "fmt" "net/http" - "regexp" "strings" "testing" @@ -39,8 +39,7 @@ import ( // uuidInBody finds candidate collection ids in a dashboard-sections // body so the View-All detail endpoint can be probed without coupling -// the test to the response shape. -var rlsUUIDPattern = regexp.MustCompile(`[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}`) +// the test to unrelated ids in the body. func TestRlsInvariant(t *testing.T) { setup := setupTestServer(t) @@ -77,7 +76,7 @@ func TestRlsInvariant(t *testing.T) { libV := createLib("RLS Test Visible Library " + controlMarker) createBook := func(libraryID pgtype.UUID, title string) { - _, cerr := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, cerr := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: libraryID, Title: title, Authors: []string{"RLS Invariant Author"}, @@ -199,16 +198,27 @@ func TestRlsInvariant(t *testing.T) { t.Run("restricted_user_collection_detail_view_all", func(t *testing.T) { // The mobile View All path: GET /api/collections/{id} for every // collection the dashboard references (system sections included). + // Structured decode — probe exactly the collection ids the + // sections payload declares (its own contract field), not + // whatever UUIDs happen to appear in the body. _, sectionsBody := rlsGet(regularToken, "/api/dashboard/sections?limit=500") + var sections struct { + Sections []struct { + CollectionID string `json:"collection_id"` + } `json:"sections"` + } + require.NoError(t, json.Unmarshal([]byte(sectionsBody), §ions), + "dashboard sections should decode; body: %s", sectionsBody) seen := map[string]bool{} - for _, id := range rlsUUIDPattern.FindAllString(sectionsBody, -1) { - if seen[id] { + for _, sec := range sections.Sections { + id := sec.CollectionID + if id == "" || seen[id] { continue } seen[id] = true code, body := rlsGet(regularToken, "/api/collections/"+id) if code != http.StatusOK { - continue // not a collection id (or not visible) — absence trivially holds + continue // not visible to this user — absence trivially holds } assert.NotContains(t, body, hiddenMarker, "LEAK: collection detail %s (View All surface) returned hidden-library content", id) diff --git a/cmd/server/tests/scan_state_test.go b/cmd/server/tests/scan_state_test.go index 8ecb6099..21008646 100644 --- a/cmd/server/tests/scan_state_test.go +++ b/cmd/server/tests/scan_state_test.go @@ -78,7 +78,7 @@ func TestScanStateBulkLoad(t *testing.T) { require.NoError(t, os.WriteFile(apiFile, []byte("api made book content"), 0644)) apiStat, err := os.Stat(apiFile) require.NoError(t, err) - created, err := setup.DB.CreateMediaItem(context.Background(), database.CreateMediaItemParams{ + created, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: libID, Title: "API Made Book", Authors: []string{"API Author"}, diff --git a/cmd/server/tests/statistics_test.go b/cmd/server/tests/statistics_test.go index 705c6177..2e08d9cd 100644 --- a/cmd/server/tests/statistics_test.go +++ b/cmd/server/tests/statistics_test.go @@ -253,7 +253,7 @@ func TestReadingStatsSummary(t *testing.T) { media, err = setup.DB.GetMediaItem(ctx, uuidToPGType(uuid.MustParse(reuseMediaID[0]))) require.NoError(t, err, "reuse seeded media item") } else { - media, err = setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + media, err = setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: [16]byte(libUUID), Valid: true}, Title: title, FilePath: fmt.Sprintf("/tmp/stats-%s-%d.epub", title, time.Now().UnixNano()), @@ -448,7 +448,7 @@ func TestReadingStatsWeekLibraryScope(t *testing.T) { libB := uuid.MustParse(createLibraryDirect(t, setup.DB, userID, fmt.Sprintf("Test Scope LibB %d", time.Now().UnixNano()), "ebooks")) seed := func(lib uuid.UUID, title string, daysAgo int, minutes int32) { - media, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + media, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: [16]byte(lib), Valid: true}, Title: title, FilePath: fmt.Sprintf("/tmp/%s-%d.epub", title, time.Now().UnixNano()), @@ -541,7 +541,7 @@ func TestLibraryUsageStats(t *testing.T) { if durationSeconds > 0 { audioMeta = []byte(fmt.Sprintf(`{"duration_seconds":%d,"chapters":[]}`, durationSeconds)) } - media, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + media, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: [16]byte(lib), Valid: true}, Title: title, Authors: []string{author}, @@ -697,7 +697,7 @@ func TestLibraryUsageStats_Visibility(t *testing.T) { fmt.Sprintf("Test Usage Hidden %d", time.Now().UnixNano()), "ebooks")) seed := func(lib uuid.UUID, title string, size int64) { - _, err := setup.DB.CreateMediaItem(ctx, database.CreateMediaItemParams{ + _, err := setup.DB.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: pgtype.UUID{Bytes: [16]byte(lib), Valid: true}, Title: title, FilePath: fmt.Sprintf("/tmp/usage-vis-%s-%d.bin", title, time.Now().UnixNano()), diff --git a/cmd/server/tests/sync_integration_test.go b/cmd/server/tests/sync_integration_test.go index efed65f8..0bab36b6 100644 --- a/cmd/server/tests/sync_integration_test.go +++ b/cmd/server/tests/sync_integration_test.go @@ -92,7 +92,7 @@ func createSyncTestDevice(t *testing.T, db *database.Queries, userID pgtype.UUID func createSyncTestMedia(t *testing.T, db *database.Queries, libraryID pgtype.UUID) pgtype.UUID { ctx := context.Background() - media, err := db.CreateMediaItem(ctx, database.CreateMediaItemParams{ + media, err := db.CreateMediaItem(seedCtx(), database.CreateMediaItemParams{ LibraryID: libraryID, Title: "Sync Test Book", Authors: []string{"Test Author"}, diff --git a/cmd/server/tests/test_helpers_test.go b/cmd/server/tests/test_helpers_test.go index f009ce53..7f451c21 100644 --- a/cmd/server/tests/test_helpers_test.go +++ b/cmd/server/tests/test_helpers_test.go @@ -318,6 +318,17 @@ func createRegularUserOnce(t *testing.T, db *database.Queries) UserTestData { } } +// seedCtx is the context harness-level media seeding runs under. The +// seed identity is a valid UUID with NO library_visibility rows, so +// RLS grants it full visibility — needed because sqlc's :one inserts +// use RETURNING, which requires SELECT visibility of the new row. +// Product code never uses this; requests carry real personas. +var seedIdentity = pgtype.UUID{Bytes: [16]byte(uuid.MustParse("00000000-0000-0000-0000-000000000002")), Valid: true} + +func seedCtx() context.Context { + return rls.WithUser(context.Background(), seedIdentity) +} + // uuidToPGType converts uuid.UUID to pgtype.UUID func uuidToPGType(u uuid.UUID) pgtype.UUID { return pgtype.UUID{Bytes: [16]byte(u), Valid: true} @@ -497,10 +508,15 @@ func setupTestServer(t *testing.T) *TestServerSetup { // ScopedDB: the test server must behave EXACTLY like the production // wiring (main.go) — every statement through Queries declares the - // acting user for RLS. Helper seeding that must bypass user scoping - // uses direct pool writes (permissive write policies) or - // rls.WithInternal for non-policy'd tables. - queries := database.New(database.NewScopedDB(dbPool)) + // acting user for RLS, including the search service's raw DBTX path. + // TestServerSetup.DB below is a SEPARATE raw-pool handle used ONLY + // for harness seeding/cleanup: write policies are permissive (no GUC + // read) and every seeded table except media_items is RLS-ungoverned, + // so seeding needs no identity; direct media SELECTs through it + // raise loudly, which is correct — tests read media through HTTP. + scopedDB := database.NewScopedDB(dbPool) + queries := database.New(scopedDB) + seedDB := database.New(dbPool) // Wire the tunable settings registry exactly like main.go: Load // populates the cache (missing rows fall back to compiled defaults) @@ -509,7 +525,7 @@ func setupTestServer(t *testing.T) *TestServerSetup { // initialized") — tests that write settings through the API (the // only sanctioned path) need this wired. settingsRegistry := database.NewSettingsRegistry(queries) - if err := settingsRegistry.Load(rls.WithInternal(context.Background())); err != nil { + if err := settingsRegistry.Load(context.Background()); err != nil { t.Logf("settings registry Load failed (defaults apply): %v", err) } @@ -558,7 +574,7 @@ func setupTestServer(t *testing.T) *TestServerSetup { statisticsService := services.NewStatisticsService(queries) statsHandler := handlers.NewStatsHandler(statisticsService) seriesHandler := handlers.NewSeriesHandler(queries) - mediaHandler := handlers.NewMediaHandler(queries, dbPool, libraryService, progressService, annotationService, worker) + mediaHandler := handlers.NewMediaHandler(queries, scopedDB, libraryService, progressService, annotationService, worker) matchingHandler := handlers.NewMatchingHandler(queries, connManager) // Create conversion service for OPDS @@ -762,7 +778,7 @@ func setupTestServer(t *testing.T) *TestServerSetup { // Create TestServerSetup struct with all resources setup := &TestServerSetup{ Server: ts, - DB: queries, + DB: seedDB, // harness seeding/cleanup handle (see comment above) DBPool: dbPool, Config: cfg, ConnManager: connManager, diff --git a/internal/database/schema.go b/internal/database/schema.go index 5cb25a01..a0729260 100644 --- a/internal/database/schema.go +++ b/internal/database/schema.go @@ -178,6 +178,16 @@ func verifyFunctions(ctx context.Context, db *pgxpool.Pool, expectedFunctions [] return nil } +// schemaMigrationIdentity is the synthetic identity the schema +// migration transaction declares for RLS (plan appendix A11). The +// schema's dedup DO block SELECTs/DELETEs media_items, and with the +// visibility policies live (FORCE applies to the owner) that SELECT +// needs app.user_id set even in this maintenance tx. The identity has +// NO library_visibility rows, so the policy grants it full visibility +// — appropriate for the server's own schema pass, and unreachable by +// requests (this tx only ever runs at startup, on the raw pool). +const schemaMigrationIdentity = "00000000-0000-0000-0000-000000000001" + // executeSchema runs the entire schema.sql in a single transaction func executeSchema(ctx context.Context, db *pgxpool.Pool) error { tx, err := db.Begin(ctx) @@ -186,6 +196,12 @@ func executeSchema(ctx context.Context, db *pgxpool.Pool) error { } defer tx.Rollback(ctx) + // Declare the migration identity BEFORE the file executes (A11): + // policy'd tables are queried by schema.sql itself on re-runs. + if _, err := tx.Exec(ctx, `SELECT set_config('app.user_id', $1, true)`, schemaMigrationIdentity); err != nil { + return fmt.Errorf("schema execution failed (declare migration identity): %w", err) + } + _, err = tx.Exec(ctx, SchemaFile) if err != nil { log.Printf("HINT: Run manually to debug: psql -h localhost -U postgres -d bookhoard -f database/schema/schema.sql") diff --git a/internal/database/scoped.go b/internal/database/scoped.go index 933d1aaa..d06496e0 100644 --- a/internal/database/scoped.go +++ b/internal/database/scoped.go @@ -3,14 +3,12 @@ // `app.user_id` GUC — the one thing the RLS policies // (database/schema/schema.sql) need to filter media rows per user. // -// Fail-loud, twice: a query whose context carries neither a user -// identity nor the internal marker is refused HERE, and the policies -// themselves raise on an unset/empty GUC. The internal marker -// (rls.WithInternal) declares "not a user" — it sets NO GUC, so -// policy'd tables (media_items) still raise at the engine; only -// RLS-ungoverned tables (system_settings, users counts) work, which -// user-less startup paths need. A code path that forgets the auth -// middleware bridge breaks visibly; it never silently sees all rows. +// Fail-loud lives in the ENGINE, not here: a context with no identity +// simply declares nothing, and any RLS-governed statement (media_items) +// then raises on the unset GUC — while ungoverned tables (auth lookups, +// settings, sessions) keep working, as they have no policy. That is the +// plan-C property: the database is the one door everything crosses; the +// decorator is only the identity declarer. // // SEMANTICS PRESERVED: every sqlc call was already a single // auto-committed statement; one transaction per call keeps per-call @@ -50,32 +48,27 @@ func NewScopedDB(pool *pgxpool.Pool) *ScopedDB { const zeroUUID = "00000000-0000-0000-0000-000000000000" -// begin opens the per-statement transaction. Identity semantics: -// - user context → declares app.user_id (LOCAL) — RLS filters rows -// - internal ctx → NO GUC declared. Grants nothing: policy'd tables -// still raise at the engine (FORCE applies to the owner); only -// RLS-ungoverned tables (system_settings, users, …) are workable — -// which is exactly what user-less startup paths need. -// - neither → refused HERE (fail-loud before SQL is even sent) +// begin opens the per-statement transaction. If the context carries a +// user identity, the GUC is declared (LOCAL); if not, NOTHING is +// declared and the ENGINE decides: RLS-governed tables (media_items) +// raise on the unset GUC — fail-loud at the database, the layer plan C +// exists to enforce at — while RLS-ungoverned tables (users, settings, +// sessions, auth lookups pre-bridge) simply work, as they have no +// policy to evaluate. The decorator therefore never refuses: refusing +// here would re-create a convention gate in code, one more door future +// paths could forget — the engine is the one door everything crosses. func (s *ScopedDB) begin(ctx context.Context) (pgx.Tx, error) { - if !rls.IsInternal(ctx) { - uid, ok := rls.FromContext(ctx) - if !ok || !uid.Valid || uid.String() == zeroUUID { - return nil, fmt.Errorf("rls: no authenticated user in context — query refused " + - "(route through the auth middleware bridge, or declare an internal " + - "identity via rls.WithUser; see the RLS plan §5–6)") - } - tx, err := s.Pool.Begin(ctx) - if err != nil { - return nil, err - } + tx, err := s.Pool.Begin(ctx) + if err != nil { + return nil, err + } + if uid, ok := rls.FromContext(ctx); ok && uid.Valid && uid.String() != zeroUUID { if err := rls.DeclareUser(ctx, tx, uid); err != nil { _ = tx.Rollback(ctx) return nil, fmt.Errorf("rls: declare user: %w", err) } - return tx, nil } - return s.Pool.Begin(ctx) + return tx, nil } func (s *ScopedDB) Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) { diff --git a/internal/rls/rls.go b/internal/rls/rls.go index 448cf4d3..d63a7bbd 100644 --- a/internal/rls/rls.go +++ b/internal/rls/rls.go @@ -21,20 +21,9 @@ import ( type ctxKey int -const ( - // CtxKeyUserID is the context key the auth middleware bridge sets and - // ScopedDB reads. Typed key — no string-key collisions. - CtxKeyUserID ctxKey = iota - // CtxKeyInternal marks a context as INTERNAL (startup, system - // jobs): the statement runs WITHOUT the RLS identity GUC. This - // grants NOTHING — the policies still apply to the table owner - // (FORCE) — so any query it runs against a POLICY'D table (today: - // media_items) still RAISES at the engine. It only unblocks - // statements on tables RLS does not govern (system_settings, - // users counts, sessions…), which a user-less startup path must - // be able to read. NEVER wrap a request context with this. - CtxKeyInternal -) +// CtxKeyUserID is the context key the auth middleware bridge sets and +// ScopedDB reads. Typed key — no string-key collisions. +const CtxKeyUserID ctxKey = iota // FromContext returns the authenticated end user's UUID. // ok=false means the caller forgot the middleware bridge — the @@ -55,20 +44,6 @@ func WithUser(ctx context.Context, uid pgtype.UUID) context.Context { return context.WithValue(ctx, CtxKeyUserID, uid) } -// WithInternal marks a context as internal (startup paths, system -// settings): statements run WITHOUT the RLS GUC. Grants nothing — -// policy'd tables still raise at the engine; only RLS-ungoverned -// tables become readable. See CtxKeyInternal. -func WithInternal(ctx context.Context) context.Context { - return context.WithValue(ctx, CtxKeyInternal, true) -} - -// IsInternal reports whether the context opted into internal semantics. -func IsInternal(ctx context.Context) bool { - v, _ := ctx.Value(CtxKeyInternal).(bool) - return v -} - // DeclareUser sets the RLS identity GUC inside an ALREADY-OPEN // transaction. The `true` argument is is_local: transaction-scoped, // evaporating at COMMIT/ROLLBACK — NEVER session-scoped on a pooled