scheb / 2fa

Two-factor authentication for Symfony applications 🔐
MIT License
495 stars 72 forks source link

Check if the session is started #168

Closed trsteel88 closed 1 year ago

trsteel88 commented 1 year ago

For bug fixes:

The Symfony Framework currently detects usage on the session. If it is used, Symfony will output private headers. See https://github.com/symfony/symfony/blob/c79d4ab7e787eb27d83581ccb8e15a4e11e88df3/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php#L197

This PR updates the Form listener to check if the session isStarted() before calling getToken on the token storage. Doing so, means the session usage counter isn't incremented.

Alternatively, the check for getToken() should be fired after isAuthFormRequest().

We currently have a site where we're trying to cache the front-end. However, this listener is firing for the front-end firewall even though it isn't relevant.

trsteel88 commented 1 year ago

@scheb any change of this being merged?

scheb commented 1 year ago

I can see a weird edge-case:

So, the correct approach would be to reorder to conditions. Check isAuthForm first, then - and only then - check the token. So that on all pages, except the 2fa auth form page, the listener would not touch the session.

scheb commented 1 year ago

This is what I mean:

    public function onKernelRequest(RequestEvent $requestEvent): void
    {
        $request = $requestEvent->getRequest();
        if (!$request->hasSession()) {
            return;
        }

        if (!$this->twoFactorFirewallConfig->isAuthFormRequest($request)) {
            return;
        }

        $token = $this->tokenStorage->getToken();
        if (!($token instanceof TwoFactorTokenInterface)) {
            return;
        }

        $event = new TwoFactorAuthenticationEvent($request, $token);
        $this->eventDispatcher->dispatch($event, TwoFactorAuthenticationEvents::FORM);
    }
trsteel88 commented 1 year ago

Thanks @scheb. I have updated that.