Skip to content

Commit 2183fc7

Browse files
ssddanbrownssddanbrown
committed
API: Limited exception details shown
Updated exception handler to reduce the amount of detail shown to prevent potentially sensitive details (like internal paths) being shown in the error message. Added an interface for specifically marking exceptions whos messages we may want to show. Thanks to Tanner Marks for reporting.
1 parent 1732f15 commit 2183fc7

8 files changed

Lines changed: 128 additions & 11 deletions

‎app/Exceptions/ApiAuthException.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
66

7-
class ApiAuthException extends \Exception implements HttpExceptionInterface
7+
class ApiAuthException extends \Exception implements HttpExceptionInterface, ShowsApiExceptionMessage
88
{
99
protected int $status;
1010

@@ -23,4 +23,9 @@ public function getHeaders(): array
2323
{
2424
return [];
2525
}
26+
27+
public function getMessageForApi(): string
28+
{
29+
return $this->getMessage();
30+
}
2631
}

‎app/Exceptions/Handler.php‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -126,16 +126,25 @@ protected function renderApiException(Throwable $e): JsonResponse
126126
$headers = $e->getHeaders();
127127
}
128128

129-
if ($e instanceof ModelNotFoundException) {
130-
$code = 404;
131-
}
132-
133129
$responseData = [
134130
'error' => [
135-
'message' => $e->getMessage(),
131+
'message' => 'An error occurred',
136132
],
137133
];
138134

135+
if ($e instanceof ModelNotFoundException) {
136+
$responseData['error']['message'] = 'The requested resource could not be found.';
137+
$code = 404;
138+
}
139+
140+
if ($e instanceof ShowsApiExceptionMessage) {
141+
$responseData['error']['message'] = $e->getMessageForApi();
142+
}
143+
144+
if (app()->hasDebugModeEnabled()) {
145+
$responseData['error']['message'] = $e->getMessage();
146+
}
147+
139148
if ($e instanceof ValidationException) {
140149
$responseData['error']['message'] = 'The given data was invalid.';
141150
$responseData['error']['validation'] = $e->errors();

‎app/Exceptions/NotifyException.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,13 @@
88

99
/**
1010
* An exception that is thrown to notify the user of something which went wrong.
11-
* Typically these should be translated messages since they will be shown to the end user
12-
* via a pop up notification error message in the UI.
11+
* Typically, these should be translated messages since they will be shown to the end user
12+
* via a pop-up notification error message in the UI.
1313
*
1414
* This exception is not intended to be used for internal system/application errors,
1515
* and therefore will not be logged by the exception handler.
1616
*/
17-
class NotifyException extends Exception implements Responsable, HttpExceptionInterface
17+
class NotifyException extends Exception implements Responsable, HttpExceptionInterface, ShowsApiExceptionMessage
1818
{
1919
public function __construct(
2020
string $message,
@@ -61,4 +61,9 @@ public function toResponse($request)
6161

6262
return redirect($this->redirectLocation);
6363
}
64+
65+
public function getMessageForApi(): string
66+
{
67+
return $this->getMessage();
68+
}
6469
}

‎app/Exceptions/PermissionsException.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@
44

55
use Exception;
66

7-
class PermissionsException extends Exception
7+
class PermissionsException extends Exception implements ShowsApiExceptionMessage
88
{
9+
public function getMessageForApi(): string
10+
{
11+
return $this->getMessage();
12+
}
913
}

‎app/Exceptions/PrettyException.php‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
use Illuminate\Contracts\Support\Responsable;
77
use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
88

9-
class PrettyException extends Exception implements Responsable, HttpExceptionInterface
9+
class PrettyException extends Exception implements Responsable, HttpExceptionInterface, ShowsApiExceptionMessage
1010
{
1111
protected ?string $subtitle = null;
1212
protected ?string $details = null;
@@ -56,4 +56,16 @@ public function getHeaders(): array
5656
{
5757
return [];
5858
}
59+
60+
public function getMessageForApi(): string
61+
{
62+
$message = $this->getMessage() . '.';
63+
if ($this->subtitle) {
64+
$message .= " {$this->subtitle}.";
65+
}
66+
if ($this->details) {
67+
$message .= " {$this->details}.";
68+
}
69+
return $message;
70+
}
5971
}

‎app/Exceptions/ShowsApiExceptionMessage.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php
2+
3+
namespace BookStack\Exceptions;
4+
5+
interface ShowsApiExceptionMessage
6+
{
7+
/**
8+
* Get the message to be shown to the API user for this exception.
9+
* This is used for non-debug scenarios, so should not contain sensitive information.
10+
* The original exception message will be used in debug scenarios.
11+
*/
12+
public function getMessageForApi(): string;
13+
}

‎tests/Api/ApiErrorTest.php‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
<?php
2+
3+
namespace Tests\Api;
4+
5+
use BookStack\App\SystemApiController;
6+
use BookStack\Exceptions\PrettyException;
7+
use Tests\TestCase;
8+
9+
class ApiErrorTest extends TestCase
10+
{
11+
use TestsApi;
12+
13+
public function test_exception_detail_only_shown_in_debug_mode()
14+
{
15+
$mockController = $this->partialMock(SystemApiController::class);
16+
$mockController->shouldReceive('read')->andThrow(\InvalidArgumentException::class, 'Potentially sensitive data', 500);
17+
18+
$resp = $this->actingAsApiEditor()->get('/api/system');
19+
$resp->assertStatus(500);
20+
$resp->assertDontSee('Potentially sensitive data', false);
21+
$resp->assertJsonPath('error.message', 'An error occurred');
22+
23+
config(['app.debug' => true]);
24+
25+
$resp = $this->actingAsApiEditor()->get('/api/system');
26+
$resp->assertStatus(500);
27+
$resp->assertJsonPath('error.message', 'Potentially sensitive data');
28+
}
29+
30+
public function test_exception_message_when_model_not_found()
31+
{
32+
$resp = $this->actingAsApiEditor()->get('/api/books/123456789');
33+
$resp->assertStatus(404);
34+
$resp->assertSee('The requested resource could not be found.', false);
35+
}
36+
37+
public function test_pretty_exception_messages_are_provided_in_non_debug_mode()
38+
{
39+
$mockController = $this->partialMock(SystemApiController::class);
40+
$exception = new PrettyException('Mr Error is here!');
41+
$exception->setSubtitle('Oh no!');
42+
$exception->setDetails('Something has really gone wrong');
43+
$mockController->shouldReceive('read')->andThrow($exception);
44+
45+
$resp = $this->actingAsApiEditor()->get('/api/system');
46+
$resp->assertStatus(500);
47+
$resp->assertJson([
48+
'error' => [
49+
'message' => 'Mr Error is here!. Oh no!. Something has really gone wrong.'
50+
]
51+
]);
52+
}
53+
}

‎tests/Api/ImageGalleryApiTest.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -454,4 +454,20 @@ public function test_delete_endpoint_requires_image_delete_permission()
454454
$resp = $this->deleteJson($this->baseEndpoint . "/{$image->id}");
455455
$resp->assertStatus(204);
456456
}
457+
458+
public function test_delete_works_on_orphaned_image()
459+
{
460+
$this->actingAsApiAdmin();
461+
$imagePage = $this->entities->page();
462+
$data = $this->files->uploadGalleryImageToPage($this, $imagePage);
463+
464+
$image = Image::query()->findOrFail($data['response']->id);
465+
466+
$this->entities->destroy($imagePage);
467+
468+
$resp = $this->deleteJson($this->baseEndpoint . "/{$image->id}");
469+
470+
$resp->assertStatus(204);
471+
$this->assertDatabaseMissing('images', ['id' => $image->id]);
472+
}
457473
}

0 commit comments

Comments
 (0)