From 0d34c00d2edbfc4ce548d4f357afdb3e446aadac Mon Sep 17 00:00:00 2001
From: Gani Georgiev <gani.georgiev@gmail.com>
Date: Thu, 14 Nov 2024 14:31:17 +0200
Subject: [PATCH] added randomized trottle for failed list filter requests

---
 apis/record_crud.go | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/apis/record_crud.go b/apis/record_crud.go
index 9fce86fc..48a14399 100644
--- a/apis/record_crud.go
+++ b/apis/record_crud.go
@@ -1,9 +1,12 @@
 package apis
 
 import (
+	cryptoRand "crypto/rand"
 	"fmt"
 	"log/slog"
+	"math/big"
 	"net/http"
+	"time"
 
 	"github.com/labstack/echo/v5"
 	"github.com/pocketbase/dbx"
@@ -71,7 +74,13 @@ func (api *recordApi) list(c echo.Context) error {
 
 	records := []*models.Record{}
 
-	result, err := searchProvider.ParseAndExec(c.QueryParams().Encode(), &records)
+	// note: in v0.23.0 this has been migrated as option check in the search.Provider
+	queryStr := c.QueryParams().Encode()
+	if len(queryStr) > 2048 {
+		return NewBadRequestError("query string is too large", nil)
+	}
+
+	result, err := searchProvider.ParseAndExec(queryStr, &records)
 	if err != nil {
 		return NewBadRequestError("", err)
 	}
@@ -91,10 +100,43 @@ func (api *recordApi) list(c echo.Context) error {
 			api.app.Logger().Debug("Failed to enrich list records", slog.String("error", err.Error()))
 		}
 
+		// note: in v0.23.0 this is combined with extra check for repeated attempts
+		//
+		// Add a randomized throttle in case of empty search filter attempts.
+		//
+		// This is just for extra precaution since security researches raised concern regarding the possibity of eventual
+		// timing attacks because the List API rule acts also as filter and executes in a single run with the client-side filters.
+		// This is by design and it is an accepted tradeoff between performance, usability and correctness.
+		//
+		// While technically the below doesn't fully guarantee protection against filter timing attacks, in practice combined with the network latency it makes them even less feasible.
+		// A properly configured rate limiter or individual fields Hidden checks are better suited if you are really concerned about eventual information disclosure by side-channel attacks.
+		//
+		// In all cases it doesn't really matter that much because it doesn't affect the builtin PocketBase security sensitive fields (e.g. password and tokenKey) since they
+		// are not client-side filterable and in the few places where they need to be compared against an external value, a constant time check is used.
+		if requestInfo.Admin == nil &&
+			(collection.ListRule != nil && *collection.ListRule != "") &&
+			(requestInfo.Query["filter"] != "") &&
+			len(e.Records) == 0 {
+			api.app.Logger().Debug("Randomized throttle because of failed filter search", "collectionId", collection.Id)
+			randomizedThrottle(100)
+		}
+
 		return e.HttpContext.JSON(http.StatusOK, e.Result)
 	})
 }
 
+func randomizedThrottle(softMax int64) {
+	var timeout int64
+	randRange, err := cryptoRand.Int(cryptoRand.Reader, big.NewInt(softMax))
+	if err == nil {
+		timeout = randRange.Int64()
+	} else {
+		timeout = softMax
+	}
+
+	time.Sleep(time.Duration(timeout) * time.Millisecond)
+}
+
 func (api *recordApi) view(c echo.Context) error {
 	collection, _ := c.Get(ContextCollectionKey).(*models.Collection)
 	if collection == nil {