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

Password Reset Email does not take into account a subdomain, if it's being used #27045

Closed
danmatthews opened this issue Jan 3, 2019 · 9 comments
Labels

Comments

@danmatthews
Copy link

danmatthews commented Jan 3, 2019

  • Laravel Version: 5.7.9
  • PHP Version: 7.2.11
  • Database Driver & Version: Mysql, 5.7.20

Description:

When Auth::routes() is registered from within a Route::group that uses the subdomain option, the reset email still only links to the root domain. This isn't ideal for those using subdomains to provide people with their own unique accounts.

I believe this is related to commit cef1055 where the APP_URL is added to ensure hostname security. Unfortunately, this breaks dynamic generation that url() would typically handle if the hostname wasn't included as part of the argument string.

Steps To Reproduce:

Create a route group for wildcard subdomains, then register the auth routes within that.

Route::domain('{subdomain}.'.env('APP_DOMAIN'))->group(function() {
    Auth::routes();
});

I also have a middleware registered that adds the domain as a URL param:

$subdomain = $request->route()->parameter('subdomain');
if ($subdomain) {
  if ($subdomain != 'admin') {
    // Sends a 404 if the organisation doesn't exist.
    Organisation::where('url', $request->route()->parameter('subdomain'))->firstOrFail();
  }
}

URL::defaults([
     'subdomain' => isset($subdomain) ? $subdomain : null,
]);

Then visit myrealsubdomain.myapp.com/password/reset and submit a password reset request - the URL on the action button will contain the value of APP_URL, without the subdomain.

@danmatthews
Copy link
Author

For the record, i've worked around it by setting APP_URL at runtime in sendResetEmailLink

public function sendResetLinkEmail(Request $request)
    {
        $this->validateEmail($request);
        config(['app.url' => url('/')]);
        // We will send the password reset link to this user. Once we have attempted
        // to send the link, we will examine the response then see the message we
        // need to show to the user. Finally, we'll send out a proper response.
        $response = $this->broker()->sendResetLink(
            $request->only('email')
        );

        return $response == Password::RESET_LINK_SENT
            ? $this->sendResetLinkResponse($request, $response)
            : $this->sendResetLinkFailedResponse($request, $response);
    }

@danmatthews
Copy link
Author

Also worth noting: When resetting the password, the $token argument can become out-of-order thanks to the fact that the subdomain is a request parameter, so the actual reset password page will show an invalid token error, as what it's validating is the subdomain parameter instead.

@driesvints driesvints added the bug label Jan 3, 2019
@laurencei
Copy link
Contributor

laurencei commented Jan 4, 2019

@danmatthews - I think I saw a PR for that for 5.8 - I think it is fixed there

edit: here is the PR: #26872

@danmatthews
Copy link
Author

@laurencei beautiful, thanks all.

@driesvints
Copy link
Member

@laurencei thanks for pointing that out.

@danmatthews
Copy link
Author

@driesvints @laurencei just actually checked the PR - looks like it solves the token issue, but not the subdomain issue?

@driesvints
Copy link
Member

Ah sorry, might have been a bit too fast.

@driesvints driesvints reopened this Jan 4, 2019
@danmatthews
Copy link
Author

@driesvints no problem - i was myself.

@GrahamCampbell GrahamCampbell changed the title Password Reset Email does not take into account a subdomain, if it's being used. Password Reset Email does not take into account a subdomain, if it's being used Apr 18, 2020
@driesvints
Copy link
Member

Just tried this and I believe this has been resolved with this fix: #32345

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants