Add verification of allowed hosts in login-link

[davidjr82]

Originally, this package was designed to be installed only in composer dev. The problem was that in that case it throws an exception when compiling the blade x-login-link component, so that recommendation was removed from the README.

This pull request proposes to add the allowed_hosts configuration with the default value localhost to avoid security issues with the default configuration, as explained in discussion #41.

The blade template has not been modified intentionally, to keep a clean installation of the package. If someone uses it and has a host other than localhost, they will see the NotAllowedInCurrentHost exception indicating the reason.

[freekmurze]

Thanks!

[Lockyc]

Is it worth whitelisting *.test as well?

[davidjr82]

It sounds good to me since it is the default in herd, so probably a PR will be well received.

[Shaz3e]

Also note you need to add manually if you have already published config file the domain should be added other than localhost in my case laravel.test so I have to add in order to work otherwise exception will be thrown.

'allowed_hosts' => ['laravel.test'],

[htulibacki]

It's a great feature – thanks for that! But I think the implementation introduces a breaking change. The login link won't work if someone forgets to add allowed hosts in the package config file or doesn't use the localhost domain. This can be confusing because after a package update, you might run into an exception, even though everything was fine just a moment ago.

[davidjr82]

Thanks @htulibacki! I wouldn't say this PR is a breaking change, I would say this PR is a necessary enhanced security layer in the package, which is a clear exception to the “no breaking changes in minor releases” rule.

If a site stopped working due to the installation/upgrade of this package, it means that the site was vulnerable in production environment if they didn't upgrade the Laravel framework package to the latest version. So, we pay the price of having a breaking change in local environments, but in return, we make production sites non-vulnerable through this package.

It is true that I could have made the PR have less impact on existing installations by adding “*.test” to the allowed_hosts, but the truth is that I didn't realize it. I'm sure a PR in that direction would be welcome.

Edit: add strikethrough

[liamja]

Hey, @davidjr82

I agree with @htulibacki that this is a breaking change - the behaviour broke a client site in staging and development environments (but not production, of course!!)

These environments are public-facing and have live registered domains pointed at them for clients to play and test with, so perhaps this use-case could be considered?

this PR is a necessary enhanced security layer in the package, which is a clear exception to the “no breaking changes in minor releases” rule.

I'd disagree here - I don't see this as being necessary to introduce in a minor release, unless there was a CVE?

[davidjr82]

Hi @liamja,

there is a CVE in the Laravel framework from November 12th: CVE-2024-52301.

This CVE is related to the Laravel framework and not to this package. But if you have this package installed and you don't update Laravel framework in your installation, your website is vulnerable to CVE-2024-52301 through this package.

My PR makes production sites safe to this vulnerability because it limits the login bypass on the localhost domain by default (here is where I could add the *.test domains by default, but I didn't realize).

In my opinion, since this package is intended only for development and not for production, it is worth to introduce the breaking change in the minor version to allow secure production sites (this package is always in composer require and not in composer require-dev because otherwise the blade compilation would fail).

But this is my opinion and my intention by publishing this PR was to make all sites using this package more secure. If @freekmurze feels it should go to a major version, he is always free to revert my commit and create a new major version. But then, all sites with the current major version will be vulnerable to being hacked in production environments, which is the most important issue to address here (in my opinion, of course!).

[jonneroelofs]

Hi,

Nice contribution. I definitely consider it a breaking change though. We now have to update +- 20 repo's where we have to suddenly publish the config file which will potentially make upgrading harder in the future.

Maybe make it opt-in instead of enabling it by default?

Cheers,

Jonne

[davidjr82]

Hi @jonneroelofs ,

the problem with making it opt-in is that then production sites are not protected by default. The key point to understand here is that any site with this package before the PR that did not update the laravel framework either are exposed to being hacked.

My suggestion is for someone to do another PR by adding to the default config file, for example, 127.0.0.1 and *.test (allowing to check the regex since right now it only compares full strings).

Even in your case, 20+ sites can mean 30 mins of update in local environment, because in production they didn't break. In exchange for that time, you get full production protection to the latest laravel CVE through this package. Isn't it worth it?

By the way, I agree that it is a breaking change, but it is (in my opinion again) good to go in the minor version due to the existence of the security bug.

[jonneroelofs]

@davidjr82 Thanks for your response.

Having now actually read the CVE is see your point!

Cheers

[Israel5]

I completely agree with @davidjr82.

Even if this PR introduces a breaking change, it's a necessary one. Unless you have very strong reasons (which I can't think of), always rule in favor of the most secure option.

From both a general and security perspective, it's better to have a breaking change than to risk a security vulnerability. It's better for users to update their local code than to have their production data compromised.

Perhaps people were not fully aware of the severity of CVE-2024-52301. To put it simply, with a PHP configuration turned on and a specially crafted query string, an attacker could change your APP_ENV from production to local and gain access to your application.

I don’t have numbers, but I suspect the majority of applications using this package have an admin user access in the x-login-link component, which could lead to other severe security issues. For example, if you have Laravel Telescope installed, it could lead to a complete compromise of your application and a catastrophic data leak.

ALWAYS rule in favor of security, even if it means extra work or a breaking change.

I also agree that a PR could be created to address *.test domains. This would be a good addition to the package and help mitigate the impact of this breaking change.

[htulibacki]

Yeah, my original post was just to point out that this update might cause some issues for existing apps, not to say it shouldn't have been introduced. This is a great feature, even without this vulnerability.

Keep up the awesome work, and thanks again!

[davidjr82]

I think we can all agree that a breaking change to a minor release is always annoying to say the least.

Complaints about this PR are understandable, but in this case, if you understand the significance of the CVE, it's understandable that it's a sacrifice that needs to be made.

That said, @htulibacki I always appreciate feedback from other developers because sometimes they show you things that you overlooked at first. So, thanks for all comments to this PR even when you don't agree with it!

I encourage others to contribute a PR that adds the *.test to the configuration defaults.

Thanks!

[liamja]

Makes perfect sense @davidjr82 !

In hindsight this will save a lot of people from getting hacked than it does cause a minor inconvenience! Thanks for jumping on it. Admittedly it's unusual to want to use this package on internet-facing sites anyway - so I'm most likely an outlier there.

For posterity, in case anyone else lands here needing to bypass the host check, here is the change I made in my projects.

Only do this if you've updated to the latest version of Laravel with the CVE patch! 😉

1. Create a new \App\Http\Controllers\LoginLinkController that extends the packages LoginLinkController and skips the host check:

<?php

namespace App\Http\Controllers;

class LoginLinkController extends \Spatie\LoginLink\Http\Controllers\LoginLinkController
{
    protected function ensureAllowedHost(\Spatie\LoginLink\Http\Requests\LoginLinkRequest $request): void
    {
        // Skip the host check.
    }
}

2. Update the config/login-link.php file to point to the new controller:

You may have to publish the config file first with php artisan vendor:publish --tag="login-link-config"

    /*
     * The package will register a route that points to this controller. To have fine
     * grained control over what happens when a login link is clicked, you can
     * override this class.
     */
    'login_link_controller' => \App\Http\Controllers\LoginLinkController::class,

[davidjr82]

As an idea, if anyone adds a new PR with the "*.test", a way to skip it would be also to simply put "*" in allowed_hosts.

Thanks @liamja !

[davidjr82]

Hi all,

the new version 1.5.3 is now backward compatible by default, check PR #45.

Please keep your applications updated and stay safe! :)