Skip to content

Commit fbe83a1

Browse files
ssddanbrownssddanbrown
committed
Drawings: Validated drawing upload content as image data
Specifically check the mime type of Base64 drawing data to ensure that it reads as image data rather than anything else which could be potentially dangerous. Thanks to Emanuele Cervelli (https://github.com/M4nu02) for reporting.
1 parent a889c18 commit fbe83a1

7 files changed

Lines changed: 76 additions & 19 deletions

File tree

‎app/Exceptions/Handler.php‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -169,14 +169,4 @@ protected function unauthenticated($request, AuthenticationException $exception)
169169

170170
return redirect()->guest('login');
171171
}
172-
173-
/**
174-
* Convert a validation exception into a JSON response.
175-
*
176-
* @param Request $request
177-
*/
178-
protected function invalidJson($request, ValidationException $exception): JsonResponse
179-
{
180-
return response()->json($exception->errors(), $exception->status);
181-
}
182172
}

‎app/Uploads/Base64UriMimeRule.php‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<?php
2+
3+
namespace BookStack\Uploads;
4+
5+
use BookStack\Util\WebSafeMimeSniffer;
6+
use Closure;
7+
use Illuminate\Contracts\Validation\ValidationRule;
8+
use Illuminate\Translation\PotentiallyTranslatedString;
9+
use InvalidArgumentException;
10+
11+
class Base64UriMimeRule implements ValidationRule
12+
{
13+
public function __construct(
14+
protected string $requiredMime
15+
) {
16+
if (empty($requiredMime)) {
17+
throw new InvalidArgumentException('A required mime type must be provided');
18+
}
19+
}
20+
21+
/**
22+
* Run the validation rule.
23+
*
24+
* @param Closure(string, ?string=): PotentiallyTranslatedString $fail
25+
*/
26+
public function validate(string $attribute, mixed $value, Closure $fail): void
27+
{
28+
$imageDataEncoded = explode(',', $value, 2)[1] ?? '';
29+
if (empty($imageDataEncoded)) {
30+
$fail('validation.base64_uri_mime')->translate(['mime' => $this->requiredMime]);
31+
return;
32+
}
33+
34+
$imageData = base64_decode($imageDataEncoded);
35+
if (empty($imageData)) {
36+
$fail('validation.base64_uri_mime')->translate(['mime' => $this->requiredMime]);
37+
return;
38+
}
39+
40+
$sniffer = new WebSafeMimeSniffer();
41+
$mime = $sniffer->sniff($imageData);
42+
43+
if ($mime !== $this->requiredMime) {
44+
$fail('validation.base64_uri_mime')->translate(['mime' => $this->requiredMime]);
45+
}
46+
}
47+
}

‎app/Uploads/Controllers/DrawioImageController.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use BookStack\Exceptions\ImageUploadException;
77
use BookStack\Http\Controller;
88
use BookStack\Permissions\Permission;
9+
use BookStack\Uploads\Base64UriMimeRule;
910
use BookStack\Uploads\ImageRepo;
1011
use BookStack\Uploads\ImageResizer;
1112
use BookStack\Util\OutOfMemoryHandler;
@@ -57,7 +58,7 @@ public function create(Request $request)
5758
{
5859
$this->checkPermission(Permission::ImageCreateAll);
5960
$validated = $this->validate($request, [
60-
'image' => ['required', 'string'],
61+
'image' => ['required', 'string', new Base64UriMimeRule('image/png')],
6162
'uploaded_to' => ['required', 'integer'],
6263
]);
6364

‎lang/en/validation.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
'alpha_num' => 'The :attribute may only contain letters and numbers.',
1717
'array' => 'The :attribute must be an array.',
1818
'backup_codes' => 'The provided code is not valid or has already been used.',
19+
'base64_uri_mime' => 'The :attribute must be a valid base64 URI containing data of :mime mime type.',
1920
'before' => 'The :attribute must be a date before :date.',
2021
'between' => [
2122
'numeric' => 'The :attribute must be between :min and :max.',

‎resources/js/services/events.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,10 @@ export class EventManager {
6767
* Notify of standard server-provided validation errors.
6868
*/
6969
showValidationErrors(responseErr: HttpError): void {
70-
if (responseErr.status === 422 && responseErr.data) {
71-
const message = Object.values(responseErr.data).flat().join('\n');
70+
if (responseErr.status === 422 && responseErr.data instanceof Object) {
71+
const data = responseErr.data;
72+
const errorValues = Object.values(data.errors ?? data);
73+
const message = errorValues.flat().join('\n');
7274
this.error(message);
7375
}
7476
}

‎resources/js/services/http.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -192,9 +192,8 @@ export class HttpManager {
192192
}
193193

194194
/**
195-
* Parse the response text for an error response to a user
196-
* presentable string. Handles a range of errors responses including
197-
* validation responses & server response text.
195+
* Parse the response text for an error response to a user-presentable string.
196+
* Handles a range of error responses, including validation responses and server response text.
198197
*/
199198
protected formatErrorResponseText(text: string): string {
200199
const data = text.startsWith('{') ? JSON.parse(text) : {message: text};
@@ -206,13 +205,13 @@ export class HttpManager {
206205
return data.message || data.error;
207206
}
208207

209-
const values = Object.values(data);
210-
const isValidation = values.every(val => {
208+
const errorValues = Object.values(data.errors || {});
209+
const isValidation = errorValues.length > 0 && errorValues.every(val => {
211210
return Array.isArray(val) && val.every(x => typeof x === 'string');
212211
});
213212

214213
if (isValidation) {
215-
return values.flat().join(' ');
214+
return errorValues.flat().join(' ');
216215
}
217216

218217
return text;

‎tests/Uploads/DrawioTest.php‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,23 @@ public function test_base64_upload_requires_edit_permission_to_page()
9696
$upload()->assertStatus(200);
9797
}
9898

99+
public function test_drawing_base64_upload_validates_data_as_image()
100+
{
101+
$page = $this->entities->page();
102+
$editor = $this->users->editor();
103+
$this->actingAs($editor);
104+
105+
$upload = $this->postJson('images/drawio', [
106+
'uploaded_to' => $page->id,
107+
'image' => 'image/png;base64,' . base64_encode('i like turtles'),
108+
]);
109+
110+
$upload->assertStatus(422);
111+
$upload->assertJsonValidationErrors([
112+
'image' => 'The image must be a valid base64 URI containing data of image/png mime type.',
113+
]);
114+
}
115+
99116
public function test_drawio_url_can_be_configured()
100117
{
101118
config()->set('services.drawio', 'http://cats.com?dog=tree');

0 commit comments

Comments
 (0)