Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concern about verification_url. #88

Open
LiamKarlMitchell opened this issue Feb 8, 2023 · 5 comments
Open

Concern about verification_url. #88

LiamKarlMitchell opened this issue Feb 8, 2023 · 5 comments

Comments

@LiamKarlMitchell
Copy link

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": "[email protected]",
    "password": "password1234",
    "password_confirmation": "password1234",
    "verification_url": {
        "url": "https://some_pish_website.com/verify-email/__ID__/__HASH__/?expires=__EXPIRES__&signature=__SIGNATURE__"
    }
  }
}
@danieledelgiudice
Copy link

danieledelgiudice commented Mar 21, 2023

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
Copy link
Owner

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
Copy link

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
Copy link
Author

LiamKarlMitchell commented Mar 30, 2023

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
Copy link
Author

LiamKarlMitchell commented Mar 30, 2023

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.

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

No branches or pull requests

3 participants