From 9cd6356892c4b166a4e396cb664022cb95ac1160 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Fri, 9 Oct 2026 22:33:30 -0400 Subject: [PATCH] =?UTF-8?q?feat(server):=20ghost-session=20guard=20?= =?UTF-8?q?=E2=80=94=20JWTs=20for=20deleted=20users=20stop=20authenticatin?= =?UTF-8?q?g?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JWT middleware validated signature + expiry and fabricated the request user purely from token claims, so a signature-valid token for a DELETED user kept full API access until token expiry — observed live on hardware (a phone session survived the test suite's user-table recreation and kept serving its true empty sets; open handoff bug 1). An ensureUserExistsMiddleware already existed but was only wired on the frontend SSR group (frontend.go) — every /api group used bare createJWTMiddleware. The existence check now lives INSIDE createJWTMiddleware's SuccessHandler so every protected group (SSR and API alike) gets it: - After claim validation, GetUser by the token's user_id; a missing row clears the token cookie and answers 401 user_not_found for API calls (the app's TokenAuthenticator then re-logins with stored credentials) or redirects to /login?session=invalid for page requests — same shape-aware split the error handler already uses. - Cost: one primary-key lookup per authenticated request (~0.2ms local); instant revocation, no TTL semantics. The redundant SSR-only middleware is deleted (its redirect behavior is preserved inside the guard). The guard exposed two processing-issues tests that were unknowingly RELYING on ghost semantics: they nested a second setupTestServer whose bootstrap deleted the first setup's regular user, then asserted 403 from the dead token. De-nested: a createLibraryDirect helper inserts the library under the live setup's user, no second bootstrap. New TestDeletedUserTokenRejected pins the contract: token works while the user exists -> 401 user_not_found after deletion. Full suite: 219 groups, zero failures. --- cmd/server/tests/processing_issues_test.go | 27 +++------ cmd/server/tests/progress_service_test.go | 63 +++++++++++++++++++ cmd/server/tests/test_helpers_test.go | 20 +++++++ internal/router/frontend.go | 2 +- internal/router/router.go | 70 ++++++++++------------ 5 files changed, 125 insertions(+), 57 deletions(-) diff --git a/cmd/server/tests/processing_issues_test.go b/cmd/server/tests/processing_issues_test.go index 5ad26b73..5f1939e8 100644 --- a/cmd/server/tests/processing_issues_test.go +++ b/cmd/server/tests/processing_issues_test.go @@ -16,8 +16,7 @@ func TestProcessingIssuesListAuthentication(t *testing.T) { setup := setupTestServer(t) // Create a test library - deviceSetup := setupDeviceTest(t) - libraryID := deviceSetup.CreateLibrary(t, "Test Auth Library", "manga") + libraryID := createLibraryDirect(t, setup.DB, getTestUserID(t, setup.DB), "Test Auth Library", "manga") t.Run("GET /api/libraries/:id/issues/list - No authentication", func(t *testing.T) { req := httptest.NewRequest("GET", "/api/libraries/"+libraryID+"/issues/list", nil) @@ -55,8 +54,7 @@ func TestProcessingIssuesListResponseStructure(t *testing.T) { setup := setupTestServer(t) // Create a test library - deviceSetup := setupDeviceTest(t) - libraryID := deviceSetup.CreateLibrary(t, "Test Structure Library", "comics") + libraryID := createLibraryDirect(t, setup.DB, getTestUserID(t, setup.DB), "Test Structure Library", "comics") t.Run("Returns JSON array for empty library", func(t *testing.T) { token := setup.Token @@ -163,8 +161,7 @@ func TestProcessingIssueStatsAuthentication(t *testing.T) { setup := setupTestServer(t) // Create a test library - deviceSetup := setupDeviceTest(t) - libraryID := deviceSetup.CreateLibrary(t, "Test Stats Auth Library", "manga") + libraryID := createLibraryDirect(t, setup.DB, getTestUserID(t, setup.DB), "Test Stats Auth Library", "manga") t.Run("GET /api/libraries/:id/issues/stats - No authentication", func(t *testing.T) { req := httptest.NewRequest("GET", "/api/libraries/"+libraryID+"/issues/stats", nil) @@ -202,8 +199,7 @@ func TestProcessingIssueStatsResponseStructure(t *testing.T) { setup := setupTestServer(t) // Create a test library - deviceSetup := setupDeviceTest(t) - libraryID := deviceSetup.CreateLibrary(t, "Test Stats Structure Library", "comics") + libraryID := createLibraryDirect(t, setup.DB, getTestUserID(t, setup.DB), "Test Stats Structure Library", "comics") t.Run("Returns correct structure for empty library", func(t *testing.T) { token := setup.Token @@ -343,11 +339,9 @@ func TestProcessingIssuesCrossLibraryIsolation(t *testing.T) { setup := setupTestServer(t) // Create two different libraries with different types - deviceSetup1 := setupDeviceTest(t) - library1ID := deviceSetup1.CreateLibrary(t, "Isolation Test Library 1", "manga") + library1ID := createLibraryDirect(t, setup.DB, getTestUserID(t, setup.DB), "Isolation Test Library 1", "manga") - deviceSetup2 := setupDeviceTest(t) - library2ID := deviceSetup2.CreateLibrary(t, "Isolation Test Library 2", "comics") + library2ID := createLibraryDirect(t, setup.DB, getTestUserID(t, setup.DB), "Isolation Test Library 2", "comics") token := setup.Token @@ -431,8 +425,7 @@ func TestProcessingIssuesDifferentLibraryTypes(t *testing.T) { for _, lt := range libraryTypes { t.Run(lt.name+" - list endpoint works", func(t *testing.T) { - deviceSetup := setupDeviceTest(t) - libraryID := deviceSetup.CreateLibrary(t, "Test "+lt.name, lt.libraryType) + libraryID := createLibraryDirect(t, setup.DB, getTestUserID(t, setup.DB), "Test "+lt.name, lt.libraryType) req := httptest.NewRequest("GET", "/api/libraries/"+libraryID+"/issues/list", nil) req.Header.Set("Authorization", "Bearer "+token) @@ -448,8 +441,7 @@ func TestProcessingIssuesDifferentLibraryTypes(t *testing.T) { }) t.Run(lt.name+" - stats endpoint works", func(t *testing.T) { - deviceSetup := setupDeviceTest(t) - libraryID := deviceSetup.CreateLibrary(t, "Test "+lt.name, lt.libraryType) + libraryID := createLibraryDirect(t, setup.DB, getTestUserID(t, setup.DB), "Test "+lt.name, lt.libraryType) req := httptest.NewRequest("GET", "/api/libraries/"+libraryID+"/issues/stats", nil) req.Header.Set("Authorization", "Bearer "+token) @@ -474,8 +466,7 @@ func TestProcessingIssuesInvalidTokens(t *testing.T) { setup := setupTestServer(t) // Create a test library - deviceSetup := setupDeviceTest(t) - libraryID := deviceSetup.CreateLibrary(t, "Test Token Library", "ebooks") + libraryID := createLibraryDirect(t, setup.DB, getTestUserID(t, setup.DB), "Test Token Library", "ebooks") t.Run("List endpoint rejects invalid token", func(t *testing.T) { req := httptest.NewRequest("GET", "/api/libraries/"+libraryID+"/issues/list", nil) diff --git a/cmd/server/tests/progress_service_test.go b/cmd/server/tests/progress_service_test.go index 11a49f71..84d3048c 100644 --- a/cmd/server/tests/progress_service_test.go +++ b/cmd/server/tests/progress_service_test.go @@ -657,6 +657,69 @@ func TestProgressWeb_EdgeCases(t *testing.T) { }) } +// TestDeletedUserTokenRejected pins the ghost-session guard: a +// signature-valid JWT for a user that has been DELETED must stop +// authenticating (401 user_not_found), not serve a phantom session. +// Before the guard, deleted users kept full API access until token +// expiry (observed live on hardware 2026-10-09). +func TestDeletedUserTokenRejected(t *testing.T) { + setup := setupTestServer(t) + ctx := context.Background() + + // Create a dedicated user, then log in as it to get a fresh token. + createResp := doReq(t, "POST", setup.Server.URL+"/api/auth/register", map[string]interface{}{ + "email": "ghost-test@tests.bookhoard.internal", + "username": "ghosttest", + "password": "Ghost@Pass123!", + }, setup.Token) + defer createResp.Body.Close() + if createResp.StatusCode != http.StatusCreated { + t.Skipf("registration unavailable (status %d)", createResp.StatusCode) + } + + loginResp := doReq(t, "POST", setup.Server.URL+"/api/auth/login", map[string]interface{}{ + "login": "ghost-test@tests.bookhoard.internal", + "password": "Ghost@Pass123!", + }, "") + defer loginResp.Body.Close() + require.Equal(t, http.StatusOK, loginResp.StatusCode) + var loginBody struct { + AccessToken string `json:"access_token"` + } + require.NoError(t, json.NewDecoder(loginResp.Body).Decode(&loginBody)) + require.NotEmpty(t, loginBody.AccessToken) + + ghostToken := loginBody.AccessToken + + // Sanity: the token works while the user exists (user-scoped endpoint + // a freshly-registered regular user can access). + okResp := doReq(t, "GET", setup.Server.URL+"/api/collections", nil, ghostToken) + defer okResp.Body.Close() + require.Equal(t, http.StatusOK, okResp.StatusCode, "token must work while the user exists") + + // Delete the user directly via the DB for determinism (same query the + // suite's own cleanup uses). + ghostID := "" + users, err := setup.DB.ListUsers(ctx) + require.NoError(t, err) + for _, u := range users { + if u.Email == "ghost-test@tests.bookhoard.internal" { + ghostID = uuid.UUID(u.ID.Bytes).String() + require.NoError(t, setup.DB.DeleteUser(ctx, u.ID)) + } + } + require.NotEmpty(t, ghostID, "ghost user row must exist before deletion") + + // The same token must now be rejected with 401 user_not_found. + resp := doReq(t, "GET", setup.Server.URL+"/api/collections", nil, ghostToken) + defer resp.Body.Close() + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "ghost token must be rejected") + + var errBody map[string]string + require.NoError(t, json.NewDecoder(resp.Body).Decode(&errBody)) + assert.Equal(t, "user_not_found", errBody["error"]) +} + // TestProgressWeb_LocatorCoherence pins the locator coherence rule: a // percentage-only write that DECLARES no locator (ClearLocator) must clear // any inherited stale anchor instead of re-pairing the new percentage with diff --git a/cmd/server/tests/test_helpers_test.go b/cmd/server/tests/test_helpers_test.go index b04cdc8d..2848cc15 100644 --- a/cmd/server/tests/test_helpers_test.go +++ b/cmd/server/tests/test_helpers_test.go @@ -794,6 +794,26 @@ func getTestUserID(t *testing.T, db *database.Queries) uuid.UUID { return userUUID } +// createLibraryDirect inserts a library owned by the given user WITHOUT a +// nested test-server setup: a second setupTestServer bootstraps away the +// first setup's users, and the ghost-session guard rightly rejects the +// outer test's tokens from then on. +func createLibraryDirect(t *testing.T, db *database.Queries, ownerID uuid.UUID, name, libraryType string) string { + ctx := context.Background() + libraryTypeRow, err := db.GetLibraryTypeByName(ctx, libraryType) + require.NoError(t, err, "Should find library type") + library, err := db.CreateLibrary(ctx, database.CreateLibraryParams{ + Name: name, + Description: pgtype.Text{String: "Test library description", Valid: true}, + LibraryTypeID: libraryTypeRow.ID, + CreatedByAdminID: pgtype.UUID{Bytes: [16]byte(ownerID), Valid: true}, + }) + require.NoError(t, err, "Should create library") + libraryUUID, err := uuid.FromBytes(library.ID.Bytes[0:16]) + require.NoError(t, err, "Should parse library ID") + return libraryUUID.String() +} + func getRegularUserID(t *testing.T, db *database.Queries) uuid.UUID { ctx := context.Background() user, err := db.GetUserByEmail(ctx, "testregularuser@tests.bookhoard.internal") diff --git a/internal/router/frontend.go b/internal/router/frontend.go index e7894b16..709aae9c 100644 --- a/internal/router/frontend.go +++ b/internal/router/frontend.go @@ -120,7 +120,7 @@ func registerFrontendRoutes(cfg *Config) { // ============================================================================ // Protected frontend routes (no /api prefix) - frontendProtected := e.Group("", jwtMiddleware, ensureUserExistsMiddleware(cfg)) + frontendProtected := e.Group("", jwtMiddleware) // Series browse page frontendProtected.GET("/series", func(c *echo.Context) error { diff --git a/internal/router/router.go b/internal/router/router.go index b894a95a..f2d9c31f 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -69,7 +69,7 @@ type Config struct { LoginTracker *ratelimit.LoginAttemptTracker ScannerHandler *handlers.Handler JobsHandler *handlers.JobsHandler - SidecarHandler *handlers.SidecarHandler + SidecarHandler *handlers.SidecarHandler ReaderHandler *handlers.ReaderHandler LibraryService *services.LibraryService } @@ -113,6 +113,31 @@ func createJWTMiddleware(cfg *Config) echo.MiddlewareFunc { return echo.NewHTTPError(http.StatusBadRequest, "invalid user ID in token") } + // Ghost-session guard: a signature-valid token for a user that + // no longer exists must not authenticate (deleted users kept + // full access until token expiry before this check). A + // primary-key lookup per request — instant revocation, no TTL + // semantics; the guard also covers the SSR groups that used to + // chain ensureUserExistsMiddleware. + if !userExists(cfg, userUUID) { + log.Printf("Ghost session rejected: user %s no longer exists", userIDStr) + c.SetCookie(&http.Cookie{ + Name: "token", + Value: "", + Expires: time.Now().Add(-24 * time.Hour), + Path: "/", + HttpOnly: true, + SameSite: http.SameSiteLaxMode, + }) + if wantsHTML(c.Request().Header) { + return c.Redirect(http.StatusFound, "/login?session=invalid") + } + return c.JSON(http.StatusUnauthorized, map[string]string{ + "error": "user_not_found", + "message": "This account no longer exists. Please log in again.", + }) + } + c.Set("user", database.Users{ ID: pgtype.UUID{Bytes: userUUID, Valid: true}, Email: claims["user_email"].(string), @@ -138,43 +163,12 @@ func createJWTMiddleware(cfg *Config) echo.MiddlewareFunc { }) } -// ensureUserExistsMiddleware checks if the authenticated user still exists in the database -func ensureUserExistsMiddleware(cfg *Config) echo.MiddlewareFunc { - return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c *echo.Context) error { - userIDStr, ok := c.Get("user_id").(string) - if !ok { - return next(c) - } - - userUUID, err := uuid.Parse(userIDStr) - if err != nil { - log.Printf("Invalid UUID in user existence check: %v", err) - return next(c) - } - - // Check if user exists in database - _, err = cfg.Queries.GetUser(c.Request().Context(), uuidToPGType(userUUID)) - if err != nil { - log.Printf("User not found in database: %s", userIDStr) - - // Clear invalid cookie - c.SetCookie(&http.Cookie{ - Name: "token", - Value: "", - Expires: time.Now().Add(-24 * time.Hour), - Path: "/", - HttpOnly: true, - SameSite: http.SameSiteLaxMode, - }) - - // Redirect to login with session=invalid - return c.Redirect(http.StatusFound, "/login?session=invalid") - } - - return next(c) - } - } +// userExists checks the users table for the token's claimed user. Runs on +// every authenticated request: a primary-key lookup, cheap at this scale, +// and it makes deletion revocable the moment it happens. +func userExists(cfg *Config, id uuid.UUID) bool { + _, err := cfg.Queries.GetUser(context.Background(), uuidToPGType(id)) + return err == nil } // wantsHTML determines if the request expects HTML response