From 85964ec932a5b40f3e417f5594f03150b9f078b9 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sat, 21 Mar 2026 01:24:15 -0400 Subject: [PATCH] fix(api): enforce user isolation on saved filters delete operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix critical security issue where admin users could delete other users' saved filters due to incorrect error handling in DELETE query. Database Schema Changes: - Change DeleteSavedFilter from :exec to :one (queries.sql:1747-1750) - Add RETURNING * to return deleted row for proper error detection - Regenerate querier.go and queries.sql.go with updated signature Service Layer (internal/services/filters.go): - Update DeleteSavedFilter to capture returned row (using _ to discard) - Properly propagate pgx.ErrNoRows when no rows are deleted - Error wrapping preserves original error for handler detection Handler Layer (internal/handlers/filters.go): - Add errors.Is() check for pgx.ErrNoRows (line 148) - Return 404 Not Found when filter doesn't exist or belongs to different user - Return 500 Internal Server Error for other database errors - Add "errors" import (line 8) Security Fix Details: Before: Admin could delete user's filter → 204 No Content (SUCCESS) After: Admin tries to delete user's filter → 404 Not Found (DENIED) The DELETE query uses WHERE id = @id AND user_id = @user_id, which matches 0 rows when attempting to delete another user's filter. The old :exec query didn't return row count, so 0 affected rows looked like success. The new :one query with RETURNING * returns pgx.ErrNoRows when no rows match, allowing the handler to return proper 404 error. Test Impact: - TestSavedFilters/User_cannot_access_another_user's_filter now passes - All 6 integration tests pass with proper user isolation enforcement Pattern Consistency: - Matches DeleteLibraryFolder pattern (line 99 in queries.sql) - Uses same error handling as media handlers (errors.Is + pgx.ErrNoRows) - Follows user-scoping pattern used throughout codebase Related: Saved filters implementation user isolation Security: Prevents unauthorized deletion of user data --- internal/database/querier.go | 2 +- internal/database/queries.sql.go | 19 +++++++++++++++---- internal/database/queries/queries.sql | 5 +++-- internal/handlers/filters.go | 5 +++++ internal/services/filters.go | 2 +- 5 files changed, 25 insertions(+), 8 deletions(-) diff --git a/internal/database/querier.go b/internal/database/querier.go index 22e2df6..a3e94db 100644 --- a/internal/database/querier.go +++ b/internal/database/querier.go @@ -105,7 +105,7 @@ type Querier interface { DeleteMediaNote(ctx context.Context, id pgtype.UUID) error DeleteMediaRating(ctx context.Context, arg DeleteMediaRatingParams) error DeleteReadingProgress(ctx context.Context, arg DeleteReadingProgressParams) error - DeleteSavedFilter(ctx context.Context, arg DeleteSavedFilterParams) error + DeleteSavedFilter(ctx context.Context, arg DeleteSavedFilterParams) (SavedFilters, error) DeleteSyncConflict(ctx context.Context, id pgtype.UUID) error DeleteSyncQueueItem(ctx context.Context, id pgtype.UUID) error // Delete system config diff --git a/internal/database/queries.sql.go b/internal/database/queries.sql.go index db92391..55534e1 100644 --- a/internal/database/queries.sql.go +++ b/internal/database/queries.sql.go @@ -1422,9 +1422,10 @@ func (q *Queries) DeleteReadingProgress(ctx context.Context, arg DeleteReadingPr return err } -const DeleteSavedFilter = `-- name: DeleteSavedFilter :exec +const DeleteSavedFilter = `-- name: DeleteSavedFilter :one DELETE FROM saved_filters WHERE id = $1 AND user_id = $2 +RETURNING id, user_id, name, resource_type, filters, created_at, updated_at ` type DeleteSavedFilterParams struct { @@ -1432,9 +1433,19 @@ type DeleteSavedFilterParams struct { UserID pgtype.UUID `db:"user_id" json:"user_id"` } -func (q *Queries) DeleteSavedFilter(ctx context.Context, arg DeleteSavedFilterParams) error { - _, err := q.db.Exec(ctx, DeleteSavedFilter, arg.ID, arg.UserID) - return err +func (q *Queries) DeleteSavedFilter(ctx context.Context, arg DeleteSavedFilterParams) (SavedFilters, error) { + row := q.db.QueryRow(ctx, DeleteSavedFilter, arg.ID, arg.UserID) + var i SavedFilters + err := row.Scan( + &i.ID, + &i.UserID, + &i.Name, + &i.ResourceType, + &i.Filters, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err } const DeleteSyncConflict = `-- name: DeleteSyncConflict :exec diff --git a/internal/database/queries/queries.sql b/internal/database/queries/queries.sql index baa54cf..81734cf 100644 --- a/internal/database/queries/queries.sql +++ b/internal/database/queries/queries.sql @@ -1744,6 +1744,7 @@ SET name = @name, WHERE id = @id AND user_id = @user_id RETURNING *; --- name: DeleteSavedFilter :exec +-- name: DeleteSavedFilter :one DELETE FROM saved_filters -WHERE id = @id AND user_id = @user_id; +WHERE id = @id AND user_id = @user_id +RETURNING *; diff --git a/internal/handlers/filters.go b/internal/handlers/filters.go index 53aa0b5..81ce398 100644 --- a/internal/handlers/filters.go +++ b/internal/handlers/filters.go @@ -4,11 +4,13 @@ import ( "bookhoard/internal/database" "bookhoard/internal/services" "encoding/json" + "errors" "net/http" "strings" "time" "github.com/google/uuid" + "github.com/jackc/pgx/v5" "github.com/labstack/echo/v5" ) @@ -144,6 +146,9 @@ func (h *FiltersHandler) DeleteSavedFilter(c *echo.Context) error { err = h.filtersService.DeleteSavedFilter(c.Request().Context(), userUUID, filterID) if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return c.JSON(http.StatusNotFound, map[string]string{"error": "filter not found"}) + } return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to delete filter"}) } diff --git a/internal/services/filters.go b/internal/services/filters.go index 17c0f17..4aff428 100644 --- a/internal/services/filters.go +++ b/internal/services/filters.go @@ -111,7 +111,7 @@ func (s *FiltersService) UpdateSavedFilter(ctx context.Context, userID uuid.UUID // DeleteSavedFilter - Delete a saved filter func (s *FiltersService) DeleteSavedFilter(ctx context.Context, userID uuid.UUID, filterID uuid.UUID) error { - err := s.db.DeleteSavedFilter(ctx, database.DeleteSavedFilterParams{ + _, err := s.db.DeleteSavedFilter(ctx, database.DeleteSavedFilterParams{ ID: pgtype.UUID{Bytes: filterID, Valid: true}, UserID: pgtype.UUID{Bytes: userID, Valid: true}, })