From ecf99fa0edbddc5c47eb94b9cb3b5f22c65890ee Mon Sep 17 00:00:00 2001 From: Thomas Kuschan Date: Thu, 8 Jun 2023 09:53:53 +0200 Subject: [PATCH 1/5] Add test showing the "HTTP 500 on not found" bug --- tests/Api/PagesApiTest.php | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/Api/PagesApiTest.php b/tests/Api/PagesApiTest.php index 12b38bc07..75cc2807f 100644 --- a/tests/Api/PagesApiTest.php +++ b/tests/Api/PagesApiTest.php @@ -159,6 +159,27 @@ class PagesApiTest extends TestCase $this->assertStringContainsString('testing', $html); } + public function test_read_endpoint_returns_not_found() + { + $this->actingAsApiEditor(); + // get an id that is not used + $id = Page::orderBy('id', 'desc')->first()->id + 1; + $this->assertNull(Page::find($id)); + + $resp = $this->getJson($this->baseEndpoint . "/$id"); + + $resp->assertNotFound(); + $this->assertNull($resp->json('id')); + $resp->assertJsonIsObject('error'); + $resp->assertJsonStructure([ + 'error' => [ + 'code', + 'message', + ], + ]); + $this->assertSame(404, $resp->json('error')['code']); + } + public function test_update_endpoint() { $this->actingAsApiEditor(); From 9ba7d1e6c58c3730c343ea7730372f2890dbcfcc Mon Sep 17 00:00:00 2001 From: Thomas Kuschan Date: Thu, 8 Jun 2023 10:50:12 +0200 Subject: [PATCH 2/5] Fix "HTTP 500 on not found" bug #4290 --- app/Exceptions/Handler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index f2672cf57..00122c15a 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -79,7 +79,7 @@ class Handler extends ExceptionHandler */ protected function renderApiException(Throwable $e): JsonResponse { - $code = 500; + $code = $e->getCode() === 0 ? 500 : $e->getCode(); $headers = []; if ($e instanceof HttpException) { From 321a4594219a39e9d10b601168e139c1b926e373 Mon Sep 17 00:00:00 2001 From: Thomas Kuschan Date: Tue, 13 Jun 2023 18:40:37 +0200 Subject: [PATCH 3/5] Refactor exception handling by using interface --- app/Exceptions/Handler.php | 6 ++--- app/Exceptions/PrettyException.php | 36 ++++++++++++++++++++++++++++-- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index 00122c15a..02da3d5de 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -9,7 +9,7 @@ use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Validation\ValidationException; -use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; use Throwable; class Handler extends ExceptionHandler @@ -79,10 +79,10 @@ class Handler extends ExceptionHandler */ protected function renderApiException(Throwable $e): JsonResponse { - $code = $e->getCode() === 0 ? 500 : $e->getCode(); + $code = 500; $headers = []; - if ($e instanceof HttpException) { + if ($e instanceof HttpExceptionInterface) { $code = $e->getStatusCode(); $headers = $e->getHeaders(); } diff --git a/app/Exceptions/PrettyException.php b/app/Exceptions/PrettyException.php index f446442d0..d0aca5922 100644 --- a/app/Exceptions/PrettyException.php +++ b/app/Exceptions/PrettyException.php @@ -4,8 +4,9 @@ namespace BookStack\Exceptions; use Exception; use Illuminate\Contracts\Support\Responsable; +use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; -class PrettyException extends Exception implements Responsable +class PrettyException extends Exception implements Responsable, HttpExceptionInterface { /** * @var ?string @@ -17,6 +18,11 @@ class PrettyException extends Exception implements Responsable */ protected $details = null; + /** + * @var array + */ + protected $headers = []; + /** * Render a response for when this exception occurs. * @@ -24,7 +30,7 @@ class PrettyException extends Exception implements Responsable */ public function toResponse($request) { - $code = ($this->getCode() === 0) ? 500 : $this->getCode(); + $code = $this->getStatusCode(); return response()->view('errors.' . $code, [ 'message' => $this->getMessage(), @@ -46,4 +52,30 @@ class PrettyException extends Exception implements Responsable return $this; } + + /** + * Get the desired HTTP status code for this exception. + */ + public function getStatusCode(): int + { + return ($this->getCode() === 0) ? 500 : $this->getCode(); + } + + /** + * Get the desired HTTP headers for this exception. + * @return array + */ + public function getHeaders(): array + { + return $this->headers; + } + + /** + * Set the desired HTTP headers for this exception. + * @param array $headers + */ + public function setHeaders(array $headers): void + { + $this->headers = $headers; + } } From 34d8268b2b20d62781b40274686b7492c890294a Mon Sep 17 00:00:00 2001 From: Thomas Kuschan Date: Wed, 14 Jun 2023 11:07:13 +0200 Subject: [PATCH 4/5] Refactor notify exception to clean up api exception handling --- app/Exceptions/Handler.php | 4 --- app/Exceptions/NotifyException.php | 40 +++++++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php index 02da3d5de..36bdf845d 100644 --- a/app/Exceptions/Handler.php +++ b/app/Exceptions/Handler.php @@ -103,10 +103,6 @@ class Handler extends ExceptionHandler $code = $e->status; } - if (method_exists($e, 'getStatus')) { - $code = $e->getStatus(); - } - $responseData['error']['code'] = $code; return new JsonResponse($responseData, $code, $headers); diff --git a/app/Exceptions/NotifyException.php b/app/Exceptions/NotifyException.php index 307916db7..67ef27a75 100644 --- a/app/Exceptions/NotifyException.php +++ b/app/Exceptions/NotifyException.php @@ -4,29 +4,61 @@ namespace BookStack\Exceptions; use Exception; use Illuminate\Contracts\Support\Responsable; +use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; -class NotifyException extends Exception implements Responsable +class NotifyException extends Exception implements Responsable, HttpExceptionInterface { public $message; public $redirectLocation; protected $status; + /** + * @var array + */ + protected array $headers = []; + public function __construct(string $message, string $redirectLocation = '/', int $status = 500) { $this->message = $message; $this->redirectLocation = $redirectLocation; $this->status = $status; + + if ($status >= 300 && $status < 400) { + // add redirect header only when a matching HTTP status is given + $this->headers = ['location' => $redirectLocation]; + } + parent::__construct(); } /** - * Get the desired status code for this exception. + * Get the desired HTTP status code for this exception. + * + * {@inheritdoc} */ - public function getStatus(): int + public function getStatusCode(): int { return $this->status; } + /** + * Get the desired HTTP headers for this exception. + * + * {@inheritdoc} + */ + public function getHeaders(): array + { + return $this->headers; + } + + /** + * @param array $headers + */ + public function setHeaders(array $headers): void + { + $this->headers = $headers; + } + /** * Send the response for this type of exception. * @@ -38,7 +70,7 @@ class NotifyException extends Exception implements Responsable // Front-end JSON handling. API-side handling managed via handler. if ($request->wantsJson()) { - return response()->json(['error' => $message], 403); + return response()->json(['error' => $message], $this->getStatusCode()); } if (!empty($message)) { From e72cf61f7eb2245b2af59c0862e39ad55bb7a459 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 15 Jun 2023 17:07:40 +0100 Subject: [PATCH 5/5] Exceptions: Added some types, simplified some classes During review of #4291 --- app/Exceptions/NotifyException.php | 28 +++------------------------- app/Exceptions/PrettyException.php | 28 +++------------------------- 2 files changed, 6 insertions(+), 50 deletions(-) diff --git a/app/Exceptions/NotifyException.php b/app/Exceptions/NotifyException.php index 67ef27a75..b62b8fde6 100644 --- a/app/Exceptions/NotifyException.php +++ b/app/Exceptions/NotifyException.php @@ -9,13 +9,8 @@ use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; class NotifyException extends Exception implements Responsable, HttpExceptionInterface { public $message; - public $redirectLocation; - protected $status; - - /** - * @var array - */ - protected array $headers = []; + public string $redirectLocation; + protected int $status; public function __construct(string $message, string $redirectLocation = '/', int $status = 500) { @@ -23,18 +18,11 @@ class NotifyException extends Exception implements Responsable, HttpExceptionInt $this->redirectLocation = $redirectLocation; $this->status = $status; - if ($status >= 300 && $status < 400) { - // add redirect header only when a matching HTTP status is given - $this->headers = ['location' => $redirectLocation]; - } - parent::__construct(); } /** * Get the desired HTTP status code for this exception. - * - * {@inheritdoc} */ public function getStatusCode(): int { @@ -43,20 +31,10 @@ class NotifyException extends Exception implements Responsable, HttpExceptionInt /** * Get the desired HTTP headers for this exception. - * - * {@inheritdoc} */ public function getHeaders(): array { - return $this->headers; - } - - /** - * @param array $headers - */ - public function setHeaders(array $headers): void - { - $this->headers = $headers; + return []; } /** diff --git a/app/Exceptions/PrettyException.php b/app/Exceptions/PrettyException.php index d0aca5922..606085231 100644 --- a/app/Exceptions/PrettyException.php +++ b/app/Exceptions/PrettyException.php @@ -8,20 +8,8 @@ use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; class PrettyException extends Exception implements Responsable, HttpExceptionInterface { - /** - * @var ?string - */ - protected $subtitle = null; - - /** - * @var ?string - */ - protected $details = null; - - /** - * @var array - */ - protected $headers = []; + protected ?string $subtitle = null; + protected ?string $details = null; /** * Render a response for when this exception occurs. @@ -63,19 +51,9 @@ class PrettyException extends Exception implements Responsable, HttpExceptionInt /** * Get the desired HTTP headers for this exception. - * @return array */ public function getHeaders(): array { - return $this->headers; - } - - /** - * Set the desired HTTP headers for this exception. - * @param array $headers - */ - public function setHeaders(array $headers): void - { - $this->headers = $headers; + return []; } }