Skip to content

Commit b87789d

Browse files
ssddanbrownssddanbrown
committed
Merge branch 'sec_jun26' into development
2 parents a213175 + caeea65 commit b87789d

17 files changed

Lines changed: 387 additions & 35 deletions

‎app/Access/Controllers/LoginController.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
use BookStack\Exceptions\LoginAttemptException;
99
use BookStack\Facades\Activity;
1010
use BookStack\Http\Controller;
11+
use BookStack\Util\UrlComparison;
1112
use Illuminate\Http\RedirectResponse;
1213
use Illuminate\Http\Request;
1314
use Illuminate\Validation\ValidationException;
@@ -186,7 +187,8 @@ protected function updateIntendedFromPrevious(): void
186187
{
187188
// Store the previous location for redirect after login
188189
$previous = url()->previous('');
189-
$isPreviousFromInstance = str_starts_with($previous, url('/'));
190+
$comparison = new UrlComparison($previous, url('/'));
191+
$isPreviousFromInstance = $comparison->originsMatch() && $comparison->pathsOverlap();
190192
if (!$previous || !setting('app-public') || !$isPreviousFromInstance) {
191193
return;
192194
}

‎app/Activity/Controllers/CommentController.php‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,7 @@ public function update(Request $request, int $commentId)
5959
'html' => ['required', 'string'],
6060
]);
6161

62-
$comment = $this->commentRepo->getById($commentId);
63-
$this->checkOwnablePermission(Permission::PageView, $comment->entity);
62+
$comment = $this->commentRepo->getVisibleById($commentId);
6463
$this->checkOwnablePermission(Permission::CommentUpdate, $comment);
6564

6665
$comment = $this->commentRepo->update($comment, $input['html']);
@@ -76,8 +75,7 @@ public function update(Request $request, int $commentId)
7675
*/
7776
public function archive(int $id)
7877
{
79-
$comment = $this->commentRepo->getById($id);
80-
$this->checkOwnablePermission(Permission::PageView, $comment->entity);
78+
$comment = $this->commentRepo->getVisibleById($id);
8179
if (!userCan(Permission::CommentUpdate, $comment) && !userCan(Permission::CommentDelete, $comment)) {
8280
$this->showPermissionError();
8381
}
@@ -96,8 +94,7 @@ public function archive(int $id)
9694
*/
9795
public function unarchive(int $id)
9896
{
99-
$comment = $this->commentRepo->getById($id);
100-
$this->checkOwnablePermission(Permission::PageView, $comment->entity);
97+
$comment = $this->commentRepo->getVisibleById($id);
10198
if (!userCan(Permission::CommentUpdate, $comment) && !userCan(Permission::CommentDelete, $comment)) {
10299
$this->showPermissionError();
103100
}
@@ -116,7 +113,7 @@ public function unarchive(int $id)
116113
*/
117114
public function destroy(int $id)
118115
{
119-
$comment = $this->commentRepo->getById($id);
116+
$comment = $this->commentRepo->getVisibleById($id);
120117
$this->checkOwnablePermission(Permission::CommentDelete, $comment);
121118

122119
$this->commentRepo->delete($comment);

‎app/App/Providers/ValidationRuleServiceProvider.php‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace BookStack\App\Providers;
44

55
use BookStack\Uploads\ImageService;
6+
use BookStack\Util\UrlFilter;
67
use Illuminate\Support\Facades\Validator;
78
use Illuminate\Support\ServiceProvider;
89

@@ -21,10 +22,8 @@ public function boot(): void
2122

2223
Validator::extend('safe_url', function ($attribute, $value, $parameters, $validator) {
2324
$cleanLinkName = strtolower(trim($value));
24-
$isJs = str_starts_with($cleanLinkName, 'javascript:');
25-
$isData = str_starts_with($cleanLinkName, 'data:');
26-
27-
return !$isJs && !$isData;
25+
$filter = new UrlFilter($cleanLinkName);
26+
return $filter->isAllowed();
2827
});
2928
}
3029
}

‎app/Console/Commands/InstallModuleCommand.php‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use BookStack\Theming\ThemeModuleException;
88
use BookStack\Theming\ThemeModuleManager;
99
use BookStack\Theming\ThemeModuleZip;
10+
use BookStack\Util\UrlComparison;
1011
use GuzzleHttp\Psr7\Request;
1112
use Illuminate\Console\Command;
1213
use Illuminate\Support\Str;
@@ -199,7 +200,6 @@ protected function downloadModuleFile(string $location): string|null
199200
{
200201
$httpRequests = app()->make(HttpRequestService::class);
201202
$client = $httpRequests->buildClient(30, ['stream' => true]);
202-
$originalUrl = parse_url($location);
203203
$currentLocation = $location;
204204
$maxRedirects = 3;
205205
$redirectCount = 0;
@@ -212,12 +212,11 @@ protected function downloadModuleFile(string $location): string|null
212212
if ($statusCode >= 300 && $statusCode < 400 && $redirectCount < $maxRedirects) {
213213
$redirectLocation = $resp->getHeaderLine('Location');
214214
if ($redirectLocation) {
215-
$redirectUrl = parse_url($redirectLocation);
216-
$redirectOriginMatches = ($originalUrl['host'] ?? '') === ($redirectUrl['host'] ?? '')
217-
&& ($originalUrl['scheme'] ?? '') === ($redirectUrl['scheme'] ?? '')
218-
&& ($originalUrl['port'] ?? '') === ($redirectUrl['port'] ?? '');
215+
$comparison = new UrlComparison($location, $redirectLocation);
216+
$redirectOriginMatches = $comparison->originsMatch();
219217

220218
if (!$redirectOriginMatches) {
219+
$redirectUrl = parse_url($redirectLocation);
221220
$redirectOrigin = ($redirectUrl['scheme'] ?? '') . '://' . ($redirectUrl['host'] ?? '') . (isset($redirectUrl['port']) ? ':' . $redirectUrl['port'] : '');
222221
$this->info("The download URL is redirecting to a different site: {$redirectOrigin}");
223222
$shouldContinue = $this->confirm("Do you trust downloading the module from this site?");

‎app/Uploads/Attachment.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use BookStack\Users\Models\HasCreatorAndUpdater;
1111
use BookStack\Users\Models\OwnableInterface;
1212
use BookStack\Users\Models\User;
13+
use BookStack\Util\UrlFilter;
1314
use Illuminate\Database\Eloquent\Builder;
1415
use Illuminate\Database\Eloquent\Factories\HasFactory;
1516
use Illuminate\Database\Eloquent\Relations\BelongsTo;
@@ -71,7 +72,7 @@ public function jointPermissions(): HasMany
7172
public function getUrl($openInline = false): string
7273
{
7374
if ($this->external && !str_starts_with($this->path, 'http')) {
74-
return $this->path;
75+
return (new UrlFilter($this->path))->clean();
7576
}
7677

7778
return url('/attachments/' . $this->id . ($openInline ? '?open=true' : ''));

‎app/Uploads/Controllers/AttachmentController.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use BookStack\Permissions\Permission;
1212
use BookStack\Uploads\Attachment;
1313
use BookStack\Uploads\AttachmentService;
14+
use BookStack\Util\UrlFilter;
1415
use Exception;
1516
use Illuminate\Contracts\Filesystem\FileNotFoundException;
1617
use Illuminate\Http\Request;
@@ -221,7 +222,8 @@ public function get(Request $request, string $attachmentId)
221222
}
222223

223224
if ($attachment->external) {
224-
return redirect($attachment->path);
225+
$url = (new UrlFilter($attachment->path))->clean();
226+
return redirect($url);
225227
}
226228

227229
$fileName = $attachment->getFileName();

‎app/Util/HtmlPurifier/ConfiguredHtmlPurifier.php‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use BookStack\App\AppVersion;
66
use BookStack\Util\HtmlPurifier\Filters\UriLimitFileProtocolToAnchors;
7+
use BookStack\Util\UrlFilter;
78
use HTMLPurifier;
89
use HTMLPurifier_Config;
910
use HTMLPurifier_DefinitionCache_Serializer;
@@ -84,16 +85,13 @@ protected function setConfig(HTMLPurifier_Config $config, string $cachePath): vo
8485
$config->set('Attr.ID.HTML5', true);
8586
$config->set('Output.FixInnerHTML', false);
8687
$config->set('URI.SafeIframeRegexp', '%^(http://|https://|//)%');
87-
$config->set('URI.AllowedSchemes', [
88-
'http' => true,
89-
'https' => true,
90-
'mailto' => true,
91-
'ftp' => true,
92-
'nntp' => true,
93-
'news' => true,
94-
'tel' => true,
95-
'file' => true,
96-
]);
88+
89+
$allowedSchemes = UrlFilter::getAllowedSchemes();
90+
$allowedSchemesSetting = [];
91+
foreach ($allowedSchemes as $scheme) {
92+
$allowedSchemesSetting[$scheme] = true;
93+
}
94+
$config->set('URI.AllowedSchemes', $allowedSchemesSetting);
9795

9896
// $config->set('Cache.DefinitionImpl', null); // Disable cache during testing
9997
}
@@ -156,6 +154,10 @@ protected function configureHtmlDefinition(HTMLPurifier_HTMLDefinition $definiti
156154

157155
// Allow mention-ids on links
158156
$definition->addAttribute('a', 'data-mention-user-id', 'Number');
157+
158+
// Set up custom handler for srcset to limit accepted types
159+
$definition->addAttribute('img', 'srcset', new SrcsetAttrDef());
160+
$definition->addAttribute('source', 'srcset', new SrcsetAttrDef());
159161
}
160162

161163
protected function configureUriDefinition(HTMLPurifier_URIDefinition $definition): void

‎app/Util/HtmlPurifier/Filters/UriLimitFileProtocolToAnchors.php‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,5 +51,3 @@ public function filter(&$uri, $config, $context)
5151
return false;
5252
}
5353
}
54-
55-
// vim: et sw=4 sts=4

‎app/Util/HtmlPurifier/SrcsetAttrDef.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<?php
2+
3+
namespace BookStack\Util\HtmlPurifier;
4+
5+
use HTMLPurifier_AttrDef;
6+
7+
/**
8+
* Custom attribute definition to filter out potentially dangerous
9+
* values from the srcset attribute.
10+
*/
11+
class SrcsetAttrDef extends HTMLPurifier_AttrDef
12+
{
13+
public function validate($string, $config, $context)
14+
{
15+
$lower = strtolower($string);
16+
$nonAllowed = ['javascript:', 'vbscript:', 'data:', 'file:'];
17+
18+
foreach ($nonAllowed as $nonAllowedString) {
19+
if (str_contains($lower, $nonAllowedString)) {
20+
return false;
21+
}
22+
}
23+
24+
return $string;
25+
}
26+
}

‎app/Util/UrlComparison.php‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<?php
2+
3+
namespace BookStack\Util;
4+
5+
class UrlComparison
6+
{
7+
public function __construct(
8+
protected readonly string $a,
9+
protected readonly string $b,
10+
) {
11+
}
12+
13+
/**
14+
* Check if the two URLs have the same origin.
15+
*/
16+
public function originsMatch(): bool
17+
{
18+
$aParts = parse_url($this->a);
19+
$bParts = parse_url($this->b);
20+
21+
return $aParts['host'] === $bParts['host']
22+
&& $aParts['scheme'] === $bParts['scheme']
23+
&& $aParts['port'] === $bParts['port'];
24+
}
25+
26+
/**
27+
* Check if there's some overlap between the two URLs' paths.
28+
*/
29+
public function pathsOverlap(): bool
30+
{
31+
$aPath = parse_url($this->a, PHP_URL_PATH) ?? '';
32+
$bPath = parse_url($this->b, PHP_URL_PATH) ?? '';
33+
34+
return str_starts_with($aPath, $bPath) || str_starts_with($bPath, $aPath);
35+
}
36+
}

0 commit comments

Comments
 (0)