mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
denkfabrik-li
c72adadc44
POST /confirm-password verified the account's password and counted nothing. Forty wrong guesses, forty identical refusals, no lockout, no Retry-After, no log line. routes/auth.php opens by requiring the opposite: **Every `throttle:` below names its own bucket, and must.** and every other route in the file has one. POST login is the deliberate exception, and the file says why -- LoginRequest limits it per email *and* IP, which is a stronger boundary than a per-IP count. confirm-password had neither of those things. It is the wrong door to leave unlatched. Re-proving the password is what stands between a stolen session and disabling two-factor, regenerating recovery codes, or minting an API token -- credentials that outlive the session, which is the reason routes/settings.php gives for putting those routes behind it. An attacker who already holds the session can sit on this endpoint until the password falls out of it, and then has the password for everything else too. Two more with the same shape, in routes/settings.php: - PUT /settings/password -- update() validates `current_password`. - DELETE /settings/profile -- destroy() validates `current_password`. Both were equally uncounted, and both answer the same question in the same way, so an attacker refused at one door simply used the next. Fixing one of three would have been cosmetic. All three get named buckets at 6/1, matching the credential-facing routes already in auth.php. Named rather than bare: a bare `throttle:` keys on sha1(domain|ip) or sha1(user_id) with no route in it, which is how six share links once locked a visitor out of the two-factor challenge. Not changed: POST /logout has no bucket either and does not need one -- it checks no credential and reveals nothing by being repeated. PATCH /settings/profile likewise. Tests: three that fail against the unthrottled routes, and two that pin what the buckets must not do -- exhausting one must not spend another's, and one account's guesses must not lock a different account out.
93 lines
3.4 KiB
PHP
93 lines
3.4 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
use App\Models\User;
|
|
|
|
/**
|
|
* Every endpoint that checks the account's own password has a bucket.
|
|
*
|
|
* routes/auth.php says so at the top -- "Every `throttle:` below names its
|
|
* own bucket, and must" -- and POST login is the documented exception,
|
|
* because LoginRequest limits it per email *and* IP, which is stronger.
|
|
* confirm-password had neither, and it is the door that stands in front of
|
|
* disabling two-factor, regenerating recovery codes and minting an API
|
|
* token.
|
|
*/
|
|
beforeEach(function () {
|
|
$this->user = User::factory()->create();
|
|
});
|
|
|
|
test('confirming a password stops accepting guesses', function () {
|
|
foreach (range(1, 6) as $attempt) {
|
|
$this->actingAs($this->user)
|
|
->post('/confirm-password', ['password' => "wrong-{$attempt}"])
|
|
->assertSessionHasErrors('password');
|
|
}
|
|
|
|
$this->actingAs($this->user)
|
|
->post('/confirm-password', ['password' => 'wrong-7'])
|
|
->assertStatus(429)
|
|
->assertHeader('Retry-After');
|
|
});
|
|
|
|
test('changing a password stops accepting guesses at the current one', function () {
|
|
foreach (range(1, 6) as $attempt) {
|
|
$this->actingAs($this->user)->put('/settings/password', [
|
|
'current_password' => "wrong-{$attempt}",
|
|
'password' => 'a-new-password-1',
|
|
'password_confirmation' => 'a-new-password-1',
|
|
])->assertSessionHasErrors('current_password');
|
|
}
|
|
|
|
$this->actingAs($this->user)->put('/settings/password', [
|
|
'current_password' => 'wrong-7',
|
|
'password' => 'a-new-password-1',
|
|
'password_confirmation' => 'a-new-password-1',
|
|
])->assertStatus(429);
|
|
});
|
|
|
|
test('deleting an account stops accepting guesses at the password', function () {
|
|
foreach (range(1, 6) as $attempt) {
|
|
$this->actingAs($this->user)
|
|
->delete('/settings/profile', ['password' => "wrong-{$attempt}"])
|
|
->assertSessionHasErrors('password');
|
|
}
|
|
|
|
$this->actingAs($this->user)
|
|
->delete('/settings/profile', ['password' => 'wrong-7'])
|
|
->assertStatus(429);
|
|
|
|
expect(User::query()->whereKey($this->user->id)->exists())->toBeTrue();
|
|
});
|
|
|
|
test('the three buckets are separate, and separate from the rest', function () {
|
|
// Named buckets, so exhausting one does not spend another's budget --
|
|
// the failure the note at the top of routes/auth.php describes, where
|
|
// opening six share links locked the visitor out of the two-factor
|
|
// challenge.
|
|
foreach (range(1, 7) as $attempt) {
|
|
$this->actingAs($this->user)->post('/confirm-password', ['password' => "wrong-{$attempt}"]);
|
|
}
|
|
|
|
$this->actingAs($this->user)->put('/settings/password', [
|
|
'current_password' => 'wrong-again',
|
|
'password' => 'a-new-password-1',
|
|
'password_confirmation' => 'a-new-password-1',
|
|
])->assertSessionHasErrors('current_password');
|
|
});
|
|
|
|
test('somebody who knows the password is not locked out by somebody who does not', function () {
|
|
// The limiter is keyed on the account, not on the installation: a
|
|
// second account's guesses must not cost this one its own attempts.
|
|
$other = User::factory()->create();
|
|
|
|
foreach (range(1, 7) as $attempt) {
|
|
$this->actingAs($other)->post('/confirm-password', ['password' => "wrong-{$attempt}"]);
|
|
}
|
|
|
|
$this->actingAs($this->user)
|
|
->post('/confirm-password', ['password' => 'password'])
|
|
->assertSessionHasNoErrors();
|
|
});
|