daniel-de-wit / lighthouse-sanctum

Laravel Sanctum support for Laravel Lighthouse
MIT License
56 stars 9 forks source link

Concern about verification_url. #88

Open LiamKarlMitchell opened 1 year ago

LiamKarlMitchell commented 1 year ago

If the verification_url is sent during register (or resend email mutation) by a malicious user, couldn't they trick a user / admin into entering their login into a different website which may appear to look like real one?

Wouldn't it be better to have a server side generated url, or at least a way to verify and limit the input accordingly (so that app based urls or custom schemes could potentially be used) although I can't see how to generate 1 url for Desktop browser and 1 for a mobile (android/iOS).

Leaving the verification_url blank however is seeming to generate the url in a different way and not have a valid signature when submitting the Verify Email mutation with the details extracted from the link in the email received so that it does not let me validate with the mutation and signed url enabled in the config.

I think that a validation rule of some kind could be added to the graphql mutations schema for Register and Resend Verification mutations to mitigate this where the url value is confirmed to

1) Be set. 2) Match specific pattern/protocol/domain/path and have only the expected query params.

Lets say "some_pish_website.com" looks the part and is very similar to "real-domain.com" but the user or admin is perhaps not so technically adept to recognize this threat. They see an email that for all accounts uses the same email template and comes from the same sender as legitimate emails from the service they do recognize so they trust it and do not realize the problem.

mutation register($input: RegisterInput!) {
  register (input: $input) {
    status
    token
  }
}
{
  "input": {
    "name" :"Bob Admin",
    "email": "bob@real-domain.com",
    "password": "password1234",
    "password_confirmation": "password1234",
    "verification_url": {
        "url": "https://some_pish_website.com/verify-email/__ID__/__HASH__/?expires=__EXPIRES__&signature=__SIGNATURE__"
    }
  }
}
danieledelgiudice commented 1 year ago

I recently started using this library and I'm also concerned about the verification_url potentially being used maliciously. Can the project owner clarify if there are any plans to mitigate this? Thanks!

daniel-de-wit commented 1 year ago

Thank you for reaching out, I appreciate it.

blank verification_url & use_signed_email_verification_url

When using the default Laravel email verification notification, it will generate a signature that includes the url of the verification.verify route. Not just the ID, HASH and EXPIRES as you can see here:

Illuminate/Auth/Notifications/VerifyEmail.php

    /**
     * Get the verification URL for the given notifiable.
     *
     * @param  mixed  $notifiable
     * @return string
     */
    protected function verificationUrl($notifiable)
    {
        if (static::$createUrlCallback) {
            return call_user_func(static::$createUrlCallback, $notifiable);
        }

        return URL::temporarySignedRoute(
            'verification.verify',
            Carbon::now()->addMinutes(Config::get('auth.verification.expire', 60)),
            [
                'id' => $notifiable->getKey(),
                'hash' => sha1($notifiable->getEmailForVerification()),
            ]
        );
    }

This means a combination of blank verification_url & use_signed_email_verification_url will not work. I think you should choose to use Laravel default email verification with dedicated routes or require the verification_url to be set.

input RegisterInput {
    name: String!
    email: String! @rules(apply: ["bail", "email", "unique:users,email"])
    password: String! @rules(apply: ["confirmed"])
    password_confirmation: String!
    verification_url: VerificationUrlInput!      <----- Notice the !
}

I will update the README to inform users between these two options, or both.

Trusted Domains

To restrict the url's provided to register, resendEmailVerification and forgotPassword mutations I can implement something like a configurable list of trusted domains.

Something like:

config/lighthouse-sanctum.php

    /*
    |--------------------------------------------------------------------------
    | Trusted Domains
    |--------------------------------------------------------------------------
    | 
    | A list of domains that can be used when generating urls for email 
    | verification or password reset mutations.
    |
    */
    'trusted_domains' => [
         'localhost',
         'my-front-end.com',
     ],

Would that provide enough security for you?

danieledelgiudice commented 1 year ago

Looks great! The trusted domains solution is pretty much what we came up with in our own codebase (we used a custom validation rule but it would be nice having it be "natively" supported).

LiamKarlMitchell commented 1 year ago

Thanks @daniel-de-wit ,

I'm currently using Laravel default routes, was worried about the security issues that could arise if bad actors try to manipulate the verification or other URLs.

Locking the URL to a specific domain or configured string could also prevent redirect link scripts from being abused could be a good precaution, although developers should probably sign their url redirects and check where it initiated from to prevent those issues.

I understand that imposing such limits may make the package less flexible for non-default routes so it is good that this is configurable.

Although developers can customize mutations as needed, I agree that it is best to improve this in the package.

LiamKarlMitchell commented 1 year ago

Might it also be worth having a trusted_schemes which can default to http & https, or empty to allow all.

    /*
    |--------------------------------------------------------------------------
    | Trusted Schemes
    |--------------------------------------------------------------------------
    | 
    | A list of schemes that can be used when generating urls for email 
    | verification or password reset mutations.
    |
    */
    'trusted_schemes => [
      "https",
      "http"
    ],

For instance on mobile apps one might have configured something like this for deep linking or triggering resolution of the link with app logic which would send the relevant mutation.

// iOS
myapp:verify-email/__ID__/__HASH__/?expires=__EXPIRES__&signature=__SIGNATURE__
// Android
myapp://verify-email/__ID__/__HASH__/?expires=__EXPIRES__&signature=__SIGNATURE__

Web ones might have :// but Apple app urls has just a : and android has :// https://developer.apple.com/documentation/xcode/defining-a-custom-url-scheme-for-your-app https://developer.android.com/training/app-links/deep-linking

Of course developers should probably aim to use a Deep Linking service such as universal links or dynamic links there are a few solutions for this kind of thing.

I'm actually using the website https domain in prod but http in development at the moment and setting it up to be recognized in Android/Apple manifests as needed, but just thinking that others might use it for this purpose as well.