forked from fa/breadcrumb-the-shire
harden(api-login): remove account-state leak and equalize failure timing
This commit is contained in:
@@ -66,12 +66,6 @@ paths:
|
||||
request_id: 550e8400-e29b-41d4-a716-446655440000
|
||||
error_code: invalid_credentials
|
||||
details: {}
|
||||
account_inactive:
|
||||
value:
|
||||
ok: false
|
||||
request_id: 550e8400-e29b-41d4-a716-446655440000
|
||||
error_code: account_inactive
|
||||
details: {}
|
||||
'403':
|
||||
$ref: '#/components/responses/Forbidden'
|
||||
'409':
|
||||
|
||||
@@ -20,6 +20,7 @@ $apiTokenEndpointService = app(ApiTokenEndpointService::class);
|
||||
// ---- Login-specific rate limits (stricter than the general API rate limit in ApiBootstrap) ----
|
||||
$rateLimiter = (app(SecurityServicesFactory::class))->createRateLimiterService();
|
||||
$ip = $requestRuntime->ip();
|
||||
$dummyPasswordHash = '$2y$12$cPhOFmglmOpMfs28m7KwMuM58w1W7STb2xtVDsj8.WZCmOCuEtN4a';
|
||||
|
||||
$ipResult = $rateLimiter->isBlocked('api.auth.login.ip', $ip);
|
||||
if (!($ipResult['allowed'] ?? true)) {
|
||||
@@ -50,30 +51,20 @@ if (!($emailResult['allowed'] ?? true)) {
|
||||
ApiResponse::tooManyRequests((int) ($emailResult['retry_after'] ?? 60));
|
||||
}
|
||||
|
||||
// ---- User lookup ----
|
||||
// ---- User lookup + credential verification ----
|
||||
$userReadRepository = (app(UserServicesFactory::class))->createUserReadRepository();
|
||||
$user = $userReadRepository->findByEmail($email);
|
||||
if (!$user) {
|
||||
$rateLimiter->registerFailure('api.auth.login.ip', $ip, 10, 600, 300);
|
||||
$rateLimiter->registerFailure('api.auth.login.email_ip', $emailKey, 5, 900, 900);
|
||||
ApiResponse::error('invalid_credentials', 401);
|
||||
}
|
||||
|
||||
// ---- Account checks ----
|
||||
if (!(int) ($user['active'] ?? 0)) {
|
||||
$rateLimiter->registerFailure('api.auth.login.ip', $ip, 10, 600, 300);
|
||||
$rateLimiter->registerFailure('api.auth.login.email_ip', $emailKey, 5, 900, 900);
|
||||
ApiResponse::error('account_inactive', 401);
|
||||
}
|
||||
// Use a fixed bcrypt hash when the account does not exist to keep verification timing closer.
|
||||
$userPasswordHash = $user ? (string) ($user['password'] ?? '') : $dummyPasswordHash;
|
||||
$passwordMatches = password_verify($password, $userPasswordHash);
|
||||
|
||||
if (empty($user['email_verified_at'])) {
|
||||
$rateLimiter->registerFailure('api.auth.login.ip', $ip, 10, 600, 300);
|
||||
$rateLimiter->registerFailure('api.auth.login.email_ip', $emailKey, 5, 900, 900);
|
||||
ApiResponse::error('invalid_credentials', 401);
|
||||
}
|
||||
|
||||
// ---- Password verification ----
|
||||
if (!password_verify($password, (string) ($user['password'] ?? ''))) {
|
||||
if (
|
||||
!$user
|
||||
|| !$passwordMatches
|
||||
|| !(int) ($user['active'] ?? 0)
|
||||
|| empty($user['email_verified_at'])
|
||||
) {
|
||||
$rateLimiter->registerFailure('api.auth.login.ip', $ip, 10, 600, 300);
|
||||
$rateLimiter->registerFailure('api.auth.login.email_ip', $emailKey, 5, 900, 900);
|
||||
ApiResponse::error('invalid_credentials', 401);
|
||||
|
||||
@@ -14,6 +14,15 @@ class AuthzApiBootstrapContractTest extends TestCase
|
||||
$this->assertStringContainsString('ApiBootstrap::init(false);', $content);
|
||||
}
|
||||
|
||||
public function testApiLoginUsesUniformCredentialErrorAndTimingEqualization(): void
|
||||
{
|
||||
$content = $this->readProjectFile('pages/api/v1/auth/login().php');
|
||||
$this->assertStringContainsString('$dummyPasswordHash =', $content);
|
||||
$this->assertStringContainsString('password_verify($password, $userPasswordHash)', $content);
|
||||
$this->assertStringContainsString("ApiResponse::error('invalid_credentials', 401);", $content);
|
||||
$this->assertStringNotContainsString("ApiResponse::error('account_inactive', 401);", $content);
|
||||
}
|
||||
|
||||
public function testApiBootstrapSupportsOptionalAuthRequirement(): void
|
||||
{
|
||||
$content = $this->readProjectFile('core/Http/ApiBootstrap.php');
|
||||
|
||||
Reference in New Issue
Block a user