Skip to content

Commit 6e7cc16

Browse files
committed
Preferences: Updated return redirect with better origin checks
As suggested by Alex Dan in their security report.
1 parent 6216c89 commit 6e7cc16

File tree

2 files changed

+34
-2
lines changed

2 files changed

+34
-2
lines changed

app/Http/Controller.php

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,14 +167,26 @@ protected function getImageValidationRules(): array
167167

168168
/**
169169
* Redirect to the URL provided in the request as a '_return' parameter.
170-
* Will check that the parameter leads to a URL under the root path of the system.
170+
* Will check that the parameter leads to a URL under the same origin as the application.
171171
*/
172172
protected function redirectToRequest(Request $request): RedirectResponse
173173
{
174174
$basePath = url('/');
175175
$returnUrl = $request->input('_return') ?? $basePath;
176176

177-
if (!str_starts_with($returnUrl, $basePath)) {
177+
// Only allow use of _return on requests where we expect CSRF to be active
178+
// to prevent it potentially being used as an open redirect
179+
$allowedMethods = ['POST', 'PUT', 'PATCH', 'DELETE'];
180+
if (!in_array($request->getMethod(), $allowedMethods)) {
181+
return redirect($basePath);
182+
}
183+
184+
$intendedUrl = parse_url($returnUrl);
185+
$baseUrl = parse_url($basePath);
186+
$isSameOrigin = ($intendedUrl['host'] ?? '') === ($baseUrl['host'] ?? '')
187+
&& ($intendedUrl['scheme'] ?? '') === ($baseUrl['scheme'] ?? '')
188+
&& ($intendedUrl['port'] ?? 0) === ($baseUrl['port'] ?? 0);
189+
if (!$isSameOrigin) {
178190
return redirect($basePath);
179191
}
180192

tests/User/UserPreferencesTest.php

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,26 @@ public function test_shelf_view_type_change()
153153
->assertElementNotExists('.content-wrap .entity-list-item');
154154
}
155155

156+
public function test_redirect_on_preference_change_checks_host()
157+
{
158+
$expectedByRedirect = [
159+
'http://localhost/beans' => 'http://localhost/beans',
160+
'https://localhost/beans' => 'http://localhost',
161+
'http://localhost:9090/beans' => 'http://localhost',
162+
'http://localhost.example.com/beans' => 'http://localhost',
163+
'http://localhost@example.com/beans' => 'http://localhost',
164+
];
165+
166+
$this->asEditor();
167+
foreach ($expectedByRedirect as $url => $expected) {
168+
$req = $this->patch("/preferences/change-view/bookshelf", [
169+
'view' => 'grid',
170+
'_return' => $url,
171+
]);
172+
$req->assertRedirect($expected);
173+
}
174+
}
175+
156176
public function test_update_code_language_favourite()
157177
{
158178
$editor = $this->users->editor();

0 commit comments

Comments
 (0)