Laragear / WebAuthn

Authenticate users with Passkeys: fingerprints, patterns and biometric data.
MIT License
305 stars 37 forks source link

[1.2.1] userHandle is null on webauthn.ts #51

Closed asivaneswaran closed 8 months ago

asivaneswaran commented 1 year ago

PHP & Platform

8.1.3 & MacOs

Database

MYSQL 5

Laravel version

Laravel 9

Have you done this?

Expectation

When you login with credentials, it should log the user in

Description

I get a 422 error on login

Reproduction

Setup back and frontend and try to login.

Stack trace & logs

I was able to pinpoint the issue. In the webauthn file, in the login method:

async login(request = {}, response = {}) {
        const optionsResponse = await this.#fetch(
            request,
            this.routes.loginOptions
        );
        const json = await optionsResponse.json();
        const publicKey = this.#parseIncomingServerOptions(json);
        const credentials = await navigator.credentials.get({ publicKey });
        console.log(credentials);
        const publicKeyCredential = this.#parseOutgoingCredentials(credentials);

        Object.assign(publicKeyCredential, response);

        return await this.#fetch(
            publicKeyCredential,
            this.routes.login,
            response
        ).then(WebAuthn.#handleResponse);
    }

THe credentials has a userHandle of null, so it fails the check in the backend eventually.

Edit: I checked that during creation, the userHandle is not even set by the navigator.

In the documentation, it does say it maybe be null:

For [navigator.credentials.create()](https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/create) calls made with a non-empty allowCredentials properties, the returned userHandle may be null.

https://developer.mozilla.org/en-US/docs/Web/API/AuthenticatorAssertionResponse/userHandle

DarkGhostHunter commented 1 year ago

Got it. You could make a PR as I'm hands full.

DarkGhostHunter commented 8 months ago

It always struck me that the userHandle was being validated, even if it was null.

Since when on "login" I never passed down the $user instance to the AssertionValidation, the CheckCredentialIsForUser pipe always validated the userHandle regardless of its presence.

The fix was really simple: just pass the $user, since we already know the user exists by the Credential ID (otherwise we wouldn't ever hit the assertion validation, because it wouldn't find the user).

This keeps the original functionality of the CheckCredentialIsForUser, per W3C standard: if we didn't identify the user previously, require the userHandle.

    /**
     * Validate a user against the given credentials.
     *
     * @param  \Illuminate\Contracts\Auth\Authenticatable|\Laragear\WebAuthn\Contracts\WebAuthnAuthenticatable  $user
     * @param  array  $credentials
     *
     * @return bool
     */
    public function validateCredentials($user, array $credentials): bool
    {
        if ($user instanceof WebAuthnAuthenticatable && $this->isSignedChallenge($credentials)) {
            return $this->validateWebAuthn($user); // Here I pass it
        }

        // If the fallback is enabled, we will validate the credential password.
        return $this->fallback && parent::validateCredentials($user, $credentials);
    }

    /**
     * Validate the WebAuthn assertion.
     *
     * @param  \Laragear\WebAuthn\Contracts\WebAuthnAuthenticatable  $user
     * @return bool
     */
    protected function validateWebAuthn(WebAuthnAuthenticatable $user): bool
    {
        try {
            // When we hit this method, we already have the user for the credential, so we will
            // pass it to the Assertion Validation data, thus avoiding fetching it again.
            $this->validator->send(new AssertionValidation(request(), user: $user))->thenReturn();
        } catch (AssertionException $e) {
            // If we're debugging, like under local development, push the error to the logger.
            if (config('app.debug')) {
                logger($e->getMessage());
            }

            return false;
        }

        return true;
    }