Skip to content

fix: return early from the multi-factor timebox when no provider is registered - #20348

Draft
shaheenfawzy wants to merge 1 commit into
filamentphp:5.xfrom
shaheenfawzy:fix/multi-factor-timebox-padding
Draft

fix: return early from the multi-factor timebox when no provider is registered#20348
shaheenfawzy wants to merge 1 commit into
filamentphp:5.xfrom
shaheenfawzy:fix/multi-factor-timebox-padding

Conversation

@shaheenfawzy

Copy link
Copy Markdown

Description

Login::authenticate() runs its multi-factor lookup inside a Timebox so an attacker cannot tell from response time whether an account has a challenge pending:

$needsMultiFactorChallenge = $timebox->call(function (Timebox $timebox) use (...): bool {
    // ...
    return false;
}, $timeboxDuration);

Nothing in that closure ever calls $timebox->returnEarly(), so Timebox::call() sleeps out the remainder of auth.timebox_duration on every path, including the one where the panel has no multi-factor authentication providers registered at all. On that panel no challenge can ever be presented, so there is nothing to conceal, and the default 200ms is added to every successful sign-in for nothing.

This returns early only in that case:

if (Filament::getMultiFactorAuthenticationProviders() === []) {
    $timebox->returnEarly();
}

The providers come from the panel's own configuration rather than from the user being authenticated, so the branch reveals nothing about the account, and it stops applying the moment a provider is registered. Panels that use multi-factor authentication keep the full padding on every path.

Measured on a production application: 200ms off every sign-in. Reproduction: https://github.com/shaheenfawzy/filament-login-double-hash

Visual changes

None.

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

tests/src/Panels/Auth/LoginTest.php gains two tests, both using the Sleep::fake() pattern the file already uses: a sign-in on the admin panel records no padding, and a sign-in on the app-authentication panel still does. The first fails on 5.x without this change, recording [199.989] instead of []; the second passes either way and is there to prove the fix does not disable the timebox where it matters.

…nged

The second `Timebox` in `authenticate()` conceals whether a multi-factor
challenge is pending, by padding every sign-in to `auth.timebox_duration`.

It never calls `returnEarly()`, so a panel that registers no multi-factor
authentication providers pays that padding on every successful sign-in for a
challenge that can never be presented. With the default 200ms duration, that is
200ms added to every login on the majority of panels.

`Filament::getMultiFactorAuthenticationProviders()` is read from the panel's own
configuration, so the check costs nothing and stops being true the moment a
provider is registered. Panels that do register one are unaffected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant