webapp/bookstack
1
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2025-07-28 17:02:04 +03:00
Code Activity

Prevented possible XSS via link attachments

Browse Source 
This filters out potentially malicious javascript: or data: uri's coming
through to be attached to attachments.
Added tests to cover.

Thanks to Yassine ABOUKIR (@yassineaboukir on twitter) for reporting this
vulnerability.
This commit is contained in:
Dan Brown
2020-10-31 15:01:52 +00:00
parent 18bcafaee4
commit 349162ea13
5 changed files with 80 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
app/Http/Controllers/AttachmentController.php
View File

@ -110,7 +110,7 @@ class AttachmentController extends Controller
try {
$this->validate($request, [
'attachment_edit_name' => 'required|string|min:1|max:255',
'attachment_edit_url' => 'string|min:1|max:255'
'attachment_edit_url' => 'string|min:1|max:255|safe_url'
]);
} catch (ValidationException $exception) {
return response()->view('attachments.manager-edit-form', array_merge($request->only(['attachment_edit_name', 'attachment_edit_url']), [
@ -145,7 +145,7 @@ class AttachmentController extends Controller
$this->validate($request, [
'attachment_link_uploaded_to' => 'required|integer|exists:pages,id',
'attachment_link_name' => 'required|string|min:1|max:255',
'attachment_link_url' => 'required|string|min:1|max:255'
'attachment_link_url' => 'required|string|min:1|max:255|safe_url'
]);
} catch (ValidationException $exception) {
return response()->view('attachments.manager-link-form', array_merge($request->only(['attachment_link_name', 'attachment_link_url']), [
@ -161,7 +161,7 @@ class AttachmentController extends Controller
$attachmentName = $request->get('attachment_link_name');
$link = $request->get('attachment_link_url');
$attachment = $this->attachmentService->saveNewFromLink($attachmentName, $link, $pageId);
$attachment = $this->attachmentService->saveNewFromLink($attachmentName, $link, intval($pageId));
return view('attachments.manager-link-form', [
'pageId' => $pageId,

7
app/Providers/AppServiceProvider.php
View File

@ -43,6 +43,13 @@ class AppServiceProvider extends ServiceProvider
return substr_count($uploadName, '.') < 2;
});
Validator::extend('safe_url', function ($attribute, $value, $parameters, $validator) {
$cleanLinkName = strtolower(trim($value));
$isJs = strpos($cleanLinkName, 'javascript:') === 0;
$isData = strpos($cleanLinkName, 'data:') === 0;
return !$isJs && !$isData;
});
// Custom blade view directives
Blade::directive('icon', function ($expression) {
return "<?php echo icon($expression); ?>";

13
app/Uploads/AttachmentService.php
View File

@ -88,12 +88,8 @@ class AttachmentService extends UploadService
/**
* Save a new File attachment from a given link and name.
* @param string $name
* @param string $link
* @param int $page_id
* @return Attachment
*/
public function saveNewFromLink($name, $link, $page_id)
public function saveNewFromLink(string $name, string $link, int $page_id): Attachment
{
$largestExistingOrder = Attachment::where('uploaded_to', '=', $page_id)->max('order');
return Attachment::forceCreate([
@ -123,13 +119,11 @@ class AttachmentService extends UploadService
/**
* Update the details of a file.
* @param Attachment $attachment
* @param $requestData
* @return Attachment
*/
public function updateFile(Attachment $attachment, $requestData)
public function updateFile(Attachment $attachment, array $requestData): Attachment
{
$attachment->name = $requestData['name'];
if (isset($requestData['link']) && trim($requestData['link']) !== '') {
$attachment->path = $requestData['link'];
if (!$attachment->external) {
@ -137,6 +131,7 @@ class AttachmentService extends UploadService
$attachment->external = true;
}
}
$attachment->save();
return $attachment;
}