fix(api): enforce user isolation on saved filters delete operation
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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 *;
|
||||
|
||||
@@ -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"})
|
||||
}
|
||||
|
||||
|
||||
@@ -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},
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user