Added configurable API throttling, Handled API errors standardly

diff --git a/.env.example.complete b/.env.example.complete
index 716d614a33074cbe3add5f7e36082fb7459a577c..e44644f08f5aea122bbf787fa3e4f2ad42d58b3d 100644 (file)
--- a/.env.example.complete
+++ b/.env.example.complete
@@ -260,4 +260,7 @@ ALLOW_ROBOTS=null
 
 # The default and maximum item-counts for listing API requests.
 API_DEFAULT_ITEM_COUNT=100
-API_MAX_ITEM_COUNT=500
\ No newline at end of file
+API_MAX_ITEM_COUNT=500
+
+# The number of API requests that can be made per minute by a single user.
+API_REQUESTS_PER_MIN=180
\ No newline at end of file

diff --git a/app/Config/api.php b/app/Config/api.php
index dd54b29060f3fd90f4054fbdd1da2be7812919eb..6afea2dc89d293b7fcf16d4129e3241301072c9a 100644 (file)
--- a/app/Config/api.php
+++ b/app/Config/api.php
@@ -17,4 +17,7 @@ return [
     // The maximum number of items that can be returned in a listing API request.
     'max_item_count' => env('API_MAX_ITEM_COUNT', 500),
 
+    // The number of API requests that can be made per minute by a single user.
+    'requests_per_minute' => env('API_REQUESTS_PER_MIN', 180)
+
 ];

diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php
index 70a53497599d36216ae82bcdd787ab1901247043..284d731d7194a434b3cdc69dea3e5a9d1ad3efd6 100644 (file)
--- a/app/Exceptions/Handler.php
+++ b/app/Exceptions/Handler.php
@@ -7,6 +7,9 @@ use Illuminate\Auth\Access\AuthorizationException;
 use Illuminate\Auth\AuthenticationException;
 use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
+use Illuminate\Http\JsonResponse;
+use Illuminate\Http\Request;
+use Illuminate\Http\Response;
 use Illuminate\Validation\ValidationException;
 use Symfony\Component\HttpKernel\Exception\HttpException;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
@@ -47,6 +50,10 @@ class Handler extends ExceptionHandler
      */
     public function render($request, Exception $e)
     {
+        if ($this->isApiRequest($request)) {
+            return $this->renderApiException($e);
+        }
+
         // Handle notify exceptions which will redirect to the
         // specified location then show a notification message.
         if ($this->isExceptionType($e, NotifyException::class)) {
@@ -70,6 +77,41 @@ class Handler extends ExceptionHandler
         return parent::render($request, $e);
     }
 
+    /**
+     * Check if the given request is an API request.
+     */
+    protected function isApiRequest(Request $request): bool
+    {
+        return strpos($request->path(), 'api/') === 0;
+    }
+
+    /**
+     * Render an exception when the API is in use.
+     */
+    protected function renderApiException(Exception $e): JsonResponse
+    {
+        $code = $e->getCode() === 0 ? 500 : $e->getCode();
+        $headers = [];
+        if ($e instanceof HttpException) {
+            $code = $e->getStatusCode();
+            $headers = $e->getHeaders();
+        }
+
+        $responseData = [
+            'error' => [
+                'message' => $e->getMessage(),
+            ]
+        ];
+
+        if ($e instanceof ValidationException) {
+            $responseData['error']['validation'] = $e->errors();
+            $code = $e->status;
+        }
+
+        $responseData['error']['code'] = $code;
+        return new JsonResponse($responseData, $code, $headers);
+    }
+
     /**
      * Check the exception chain to compare against the original exception type.
      * @param Exception $e

diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php
index 978583a7fef0b04ad91c59cceb6c39a82049d087..c2016281a2908415a00f2b7e5e5a624903889b3e 100644 (file)
--- a/app/Http/Kernel.php
+++ b/app/Http/Kernel.php
@@ -32,7 +32,7 @@ class Kernel extends HttpKernel
             \BookStack\Http\Middleware\GlobalViewData::class,
         ],
         'api' => [
-            'throttle:60,1',
+            \BookStack\Http\Middleware\ThrottleApiRequests::class,
             \BookStack\Http\Middleware\EncryptCookies::class,
             \BookStack\Http\Middleware\StartSessionIfCookieExists::class,
             \BookStack\Http\Middleware\ApiAuthenticate::class,

diff --git a/app/Http/Middleware/ThrottleApiRequests.php b/app/Http/Middleware/ThrottleApiRequests.php
new file mode 100644 (file)
index 0000000..d08840c
--- /dev/null
+++ b/app/Http/Middleware/ThrottleApiRequests.php
@@ -0,0 +1,18 @@
+<?php
+
+namespace BookStack\Http\Middleware;
+
+use Illuminate\Routing\Middleware\ThrottleRequests as Middleware;
+
+class ThrottleApiRequests extends Middleware
+{
+
+    /**
+     * Resolve the number of attempts if the user is authenticated or not.
+     */
+    protected function resolveMaxAttempts($request, $maxAttempts)
+    {
+        return (int) config('api.requests_per_minute');
+    }
+
+}
\ No newline at end of file

diff --git a/phpunit.xml b/phpunit.xml
index 546829247c6980b470e82eecf0dd0a523ef4efb2..85538c446a2438d7df122dd570b5141c64d1dbec 100644 (file)
--- a/phpunit.xml
+++ b/phpunit.xml
@@ -50,5 +50,6 @@
         <server name="APP_URL" value="http://bookstack.dev"/>
         <server name="DEBUGBAR_ENABLED" value="false"/>
         <server name="SAML2_ENABLED" value="false"/>
+        <server name="API_REQUESTS_PER_MIN" value="180"/>
     </php>
 </phpunit>

diff --git a/tests/Api/ApiAuthTest.php b/tests/Api/ApiAuthTest.php
index b6b6b72ac795090d6c603a8791cb6a147f045d9f..2ab81814bcde21b1453ac8ec0a7692a7ded8d892 100644 (file)
--- a/tests/Api/ApiAuthTest.php
+++ b/tests/Api/ApiAuthTest.php
@@ -120,4 +120,29 @@ class ApiAuthTest extends TestCase
         $resp->assertJson($this->errorResponse("The email address for the account in use needs to be confirmed", 401));
     }
 
+    public function test_rate_limit_headers_active_on_requests()
+    {
+        $resp = $this->actingAsApiEditor()->get($this->endpoint);
+        $resp->assertHeader('x-ratelimit-limit', 180);
+        $resp->assertHeader('x-ratelimit-remaining', 179);
+        $resp = $this->actingAsApiEditor()->get($this->endpoint);
+        $resp->assertHeader('x-ratelimit-remaining', 178);
+    }
+
+    public function test_rate_limit_hit_gives_json_error()
+    {
+        config()->set(['api.requests_per_minute' => 1]);
+        $resp = $this->actingAsApiEditor()->get($this->endpoint);
+        $resp->assertStatus(200);
+
+        $resp = $this->actingAsApiEditor()->get($this->endpoint);
+        $resp->assertStatus(429);
+        $resp->assertHeader('x-ratelimit-remaining', 0);
+        $resp->assertHeader('retry-after');
+        $resp->assertJson([
+            'error' => [
+                'code' => 429,
+            ]
+        ]);
+    }
 }
\ No newline at end of file

diff --git a/tests/Api/ApiConfigTest.php b/tests/Api/ApiConfigTest.php
index d9367741f55d02672d7a2424ac5040d9956657c3..1b3da2f34f8d19c8c0df42ceccea045dbea208ac 100644 (file)
--- a/tests/Api/ApiConfigTest.php
+++ b/tests/Api/ApiConfigTest.php
@@ -44,4 +44,15 @@ class ApiConfigTest extends TestCase
         $resp->assertJsonCount(2, 'data');
     }
 
+    public function test_requests_per_min_alters_rate_limit()
+    {
+        $resp = $this->actingAsApiEditor()->get($this->endpoint);
+        $resp->assertHeader('x-ratelimit-limit', 180);
+
+        config()->set(['api.requests_per_minute' => 10]);
+
+        $resp = $this->actingAsApiEditor()->get($this->endpoint);
+        $resp->assertHeader('x-ratelimit-limit', 10);
+    }
+
 }
\ No newline at end of file

diff --git a/tests/Api/BooksApiTest.php b/tests/Api/BooksApiTest.php
index 813e343603a47eebf6400433a53895744fcc6315..a40e4c93b6c43aab535dee805b78a7c91b6517a3 100644 (file)
--- a/tests/Api/BooksApiTest.php
+++ b/tests/Api/BooksApiTest.php
@@ -38,6 +38,26 @@ class BooksApiTest extends TestCase
         $this->assertActivityExists('book_create', $newItem);
     }
 
+    public function test_book_name_needed_to_create()
+    {
+        $this->actingAsApiEditor();
+        $details = [
+            'description' => 'A book created via the API',
+        ];
+
+        $resp = $this->postJson($this->baseEndpoint, $details);
+        $resp->assertStatus(422);
+        $resp->assertJson([
+            "error" => [
+                "message" => "The given data was invalid.",
+                "validation" => [
+                    "name" => ["The name field is required."]
+                ],
+                "code" => 422,
+            ],
+        ]);
+    }
+
     public function test_read_endpoint()
     {
         $this->actingAsApiEditor();