Skip to content

Commit beb2adc

Browse files
jasonvargajasonvarga
authored
[5.x] Harden password reset (#14296)
1 parent 27105a1 commit beb2adc

File tree

5 files changed

+284
-90
lines changed

5 files changed

+284
-90
lines changed

‎src/Auth/UserTags.php‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ public function forgotPasswordForm()
352352
$params['error_redirect'] = $this->parseRedirect($errorRedirect);
353353
}
354354

355-
if ($resetUrl = $this->params->get('reset_url')) {
355+
if ($resetUrl = $this->getPasswordResetUrl($this->params->get('reset_url'))) {
356356
$params['reset_url'] = $resetUrl;
357357
}
358358

@@ -374,6 +374,19 @@ public function forgotPasswordForm()
374374
return $html;
375375
}
376376

377+
private function getPasswordResetUrl(?string $url = null): ?string
378+
{
379+
if (! $url) {
380+
return null;
381+
}
382+
383+
if (! preg_match('#^https?://#', $url) && ! str_starts_with($url, '/')) {
384+
$url = '/'.$url;
385+
}
386+
387+
return encrypt($url);
388+
}
389+
377390
/**
378391
* Output a reset password form.
379392
*

‎src/Http/Controllers/ForgotPasswordController.php‎

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,11 @@
22

33
namespace Statamic\Http\Controllers;
44

5+
use Illuminate\Contracts\Encryption\DecryptException;
56
use Illuminate\Http\Request;
67
use Illuminate\Support\Facades\Password;
78
use Statamic\Auth\Passwords\PasswordReset;
89
use Statamic\Auth\SendsPasswordResetEmails;
9-
use Statamic\Exceptions\ValidationException;
1010
use Statamic\Facades\URL;
1111
use Statamic\Http\Middleware\RedirectIfAuthenticated;
1212

@@ -30,17 +30,40 @@ public function showLinkRequestForm()
3030

3131
public function sendResetLinkEmail(Request $request)
3232
{
33-
if ($url = $request->_reset_url) {
34-
throw_if(URL::isExternalToApplication($url), ValidationException::withMessages([
35-
'_reset_url' => trans('validation.url', ['attribute' => '_reset_url']),
36-
]));
37-
33+
if ($url = $this->getResetFormUrl($request)) {
3834
PasswordReset::resetFormUrl(URL::makeAbsolute($url));
3935
}
4036

4137
return $this->traitSendResetLinkEmail($request);
4238
}
4339

40+
private function getResetFormUrl(Request $request): ?string
41+
{
42+
if (! $url = $request->_reset_url) {
43+
return null;
44+
}
45+
46+
if (strlen($url) > 2048) {
47+
return null;
48+
}
49+
50+
try {
51+
$url = decrypt($url);
52+
} catch (DecryptException $e) {
53+
if (! str_starts_with($url, '/') || str_starts_with($url, '//')) {
54+
return null;
55+
}
56+
57+
if (preg_match('/[\x00-\x1F\x7F]/', $url)) {
58+
return null;
59+
}
60+
61+
return $url;
62+
}
63+
64+
return URL::isExternalToApplication($url) ? null : $url;
65+
}
66+
4467
public function broker()
4568
{
4669
$broker = config('statamic.users.passwords.'.PasswordReset::BROKER_RESETS);

‎tests/Auth/ForgotPasswordTest.php‎

Lines changed: 126 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use Illuminate\Support\Facades\Password;
66
use PHPUnit\Framework\Attributes\DataProvider;
77
use PHPUnit\Framework\Attributes\Test;
8+
use Statamic\Auth\Passwords\PasswordReset;
89
use Statamic\Facades\User;
910
use Tests\Facades\Concerns\ProvidesExternalUrls;
1011
use Tests\PreventSavingStacheItemsToDisk;
@@ -22,35 +23,146 @@ protected function resolveApplicationConfiguration($app)
2223
$app['config']->set('app.url', 'http://absolute-url-resolved-from-request.com');
2324
}
2425

25-
#[Test]
26-
#[DataProvider('externalUrlProvider')]
27-
public function it_validates_reset_url_when_sending_reset_link_email($url, $isExternal)
26+
public function setUp(): void
2827
{
28+
parent::setUp();
29+
30+
PasswordReset::resetFormUrl(null);
31+
2932
$this->setSites([
3033
'a' => ['name' => 'A', 'locale' => 'en_US', 'url' => 'http://this-site.com/'],
3134
'b' => ['name' => 'B', 'locale' => 'en_US', 'url' => 'http://subdomain.this-site.com/'],
3235
'c' => ['name' => 'C', 'locale' => 'fr_FR', 'url' => '/fr/'],
3336
]);
37+
}
3438

39+
#[Test]
40+
public function it_accepts_encrypted_reset_url_when_sending_reset_link_email()
41+
{
3542
$this->simulateSuccessfulPasswordResetEmail();
43+
$this->createUser();
3644

37-
User::make()
38-
->email('san@holo.com')
39-
->password('chewy')
40-
->save();
45+
$this->post('/!/auth/password/email', [
46+
'email' => 'san@holo.com',
47+
'_reset_url' => encrypt('http://this-site.com/some-path'),
48+
])->assertSessionHasNoErrors();
49+
$this->assertEquals('http://this-site.com/some-path?token=test-token', PasswordReset::url('test-token', 'resets'));
50+
}
4151

42-
$response = $this->post('/!/auth/password/email', [
52+
#[Test]
53+
public function it_accepts_unencrypted_relative_reset_url_when_sending_reset_link_email()
54+
{
55+
$this->simulateSuccessfulPasswordResetEmail();
56+
$this->createUser();
57+
58+
$this->post('/!/auth/password/email', [
59+
'email' => 'san@holo.com',
60+
'_reset_url' => '/some-path',
61+
])->assertSessionHasNoErrors();
62+
$this->assertEquals('http://absolute-url-resolved-from-request.com/some-path?token=test-token', PasswordReset::url('test-token', 'resets'));
63+
}
64+
65+
#[Test]
66+
#[DataProvider('externalResetUrlProvider')]
67+
public function it_rejects_unencrypted_external_reset_url_when_sending_reset_link_email($url)
68+
{
69+
$this->simulateSuccessfulPasswordResetEmail();
70+
$this->createUser();
71+
72+
$this->post('/!/auth/password/email', [
4373
'email' => 'san@holo.com',
4474
'_reset_url' => $url,
45-
]);
75+
])->assertSessionHasNoErrors(); // Allow the notification to be sent, but without the bad url.
76+
$this->assertEquals('http://absolute-url-resolved-from-request.com/!/auth/password/reset/test-token?', PasswordReset::url('test-token', 'resets'));
77+
}
78+
79+
#[Test]
80+
public function it_rejects_unencrypted_absolute_internal_reset_url_when_sending_reset_link_email()
81+
{
82+
$this->simulateSuccessfulPasswordResetEmail();
83+
$this->createUser();
84+
85+
$this->post('/!/auth/password/email', [
86+
'email' => 'san@holo.com',
87+
'_reset_url' => 'http://this-site.com/some-path',
88+
])->assertSessionHasNoErrors();
89+
$this->assertEquals('http://absolute-url-resolved-from-request.com/!/auth/password/reset/test-token?', PasswordReset::url('test-token', 'resets'));
90+
}
91+
92+
#[Test]
93+
public function it_rejects_unencrypted_relative_reset_url_with_control_characters_when_sending_reset_link_email()
94+
{
95+
$this->simulateSuccessfulPasswordResetEmail();
96+
$this->createUser();
97+
98+
$this->post('/!/auth/password/email', [
99+
'email' => 'san@holo.com',
100+
'_reset_url' => "/some-path\r\nLocation: https://evil.com",
101+
])->assertSessionHasNoErrors();
102+
$this->assertEquals('http://absolute-url-resolved-from-request.com/!/auth/password/reset/test-token?', PasswordReset::url('test-token', 'resets'));
103+
}
104+
105+
#[Test]
106+
public function it_rejects_reset_url_longer_than_2048_characters_when_sending_reset_link_email()
107+
{
108+
$this->simulateSuccessfulPasswordResetEmail();
109+
$this->createUser();
110+
111+
$this->post('/!/auth/password/email', [
112+
'email' => 'san@holo.com',
113+
'_reset_url' => '/'.str_repeat('a', 2048),
114+
])->assertSessionHasNoErrors();
115+
$this->assertEquals('http://absolute-url-resolved-from-request.com/!/auth/password/reset/test-token?', PasswordReset::url('test-token', 'resets'));
116+
}
46117

47-
if ($isExternal) {
48-
$response->assertSessionHasErrors(['_reset_url']);
118+
#[Test]
119+
public function it_rejects_unencrypted_string_reset_url_when_sending_reset_link_email()
120+
{
121+
// Unencrypted string that doesn't look like a URL is probably a tampered encrypted string.
122+
// It might be a relative url without a leading slash, but we won't treat it as that.
49123

50-
return;
51-
}
124+
$this->simulateSuccessfulPasswordResetEmail();
125+
$this->createUser();
52126

53-
$response->assertSessionHasNoErrors();
127+
$this->post('/!/auth/password/email', [
128+
'email' => 'san@holo.com',
129+
'_reset_url' => 'not-an-encrypted-string',
130+
])->assertSessionHasNoErrors(); // Allow the notification to be sent, but without the bad url.
131+
$this->assertEquals('http://absolute-url-resolved-from-request.com/!/auth/password/reset/test-token?', PasswordReset::url('test-token', 'resets'));
132+
}
133+
134+
#[Test]
135+
#[DataProvider('externalResetUrlProvider')]
136+
public function it_rejects_encrypted_external_reset_url_when_sending_reset_link_email($url)
137+
{
138+
// It's weird to point to an external URL, even if you encrypt it yourself.
139+
// This is an additional safeguard.
140+
141+
$this->simulateSuccessfulPasswordResetEmail();
142+
$this->createUser();
143+
144+
$this->post('/!/auth/password/email', [
145+
'email' => 'san@holo.com',
146+
'_reset_url' => encrypt($url),
147+
])->assertSessionHasNoErrors(); // Allow the notification to be sent, but without the bad url.
148+
$this->assertEquals('http://absolute-url-resolved-from-request.com/!/auth/password/reset/test-token?', PasswordReset::url('test-token', 'resets'));
149+
}
150+
151+
public static function externalResetUrlProvider()
152+
{
153+
$keyFn = function ($key) {
154+
return is_null($key) ? 'null' : $key;
155+
};
156+
157+
return collect(static::externalUrls())->mapWithKeys(fn ($url) => [$keyFn($url) => [$url]])->all();
158+
}
159+
160+
private function createUser(): void
161+
{
162+
User::make()
163+
->email('san@holo.com')
164+
->password('chewy')
165+
->save();
54166
}
55167

56168
#[Test]

0 commit comments

Comments
 (0)