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

withoutAuthenticationRoutes with default login: /login creates redirect loop #6786

Closed
m-lotze opened this issue Mar 18, 2025 · 22 comments
Closed

Comments

@m-lotze
Copy link

m-lotze commented Mar 18, 2025

  • Laravel Version: 12.3.0
  • Nova Version: 5.4.1

Description:

Not sure when this was introduced, but in our app a default custom /login route now creates a redirect loop. We had to rename the route.

In 5.3.1 we had this route registration in NovaServiceProvider and it worked with a custom /login route:

protected function routes(): void
{
    Nova::routes()
        ->withPasswordResetRoutes()
        ->register();
}

After upgrading to 5.4.1 it needs to be this:

protected function routes(): void
{
    Nova::routes()
        ->withoutAuthenticationRoutes(Login: '/custom-login')
        ->withPasswordResetRoutes()
        ->register();
}

Using /login instead of /custom-login creates a redirect loop, because of line 244 in PendingRouteRegistration.php:

$router->redirect('/login', $this->loginPath)->name('nova.pages.login');
@crynobone
Copy link
Member

crynobone commented Mar 18, 2025

Unable to reproduce the issue, please provide full reproducing repository based on fresh installation as suggested in the bug report template (or you can refer to https://github.com/nova-issues for example)


Relevant files/information:

  1. config/nova.php content.
  2. app/Providers/NovaServiceProvider.php content.
  3. Output of php artisan route:list

@crynobone crynobone added the needs more info More information is required label Mar 18, 2025
@crynobone
Copy link
Member

I would believe this can be solved if your login route is named as login (the default logic for Laravel 11 and above).

@m-lotze
Copy link
Author

m-lotze commented Mar 18, 2025

It is named login. It does not make sense to register a redirect from /login to /login in PendingRouteRegistration.php.

@crynobone
Copy link
Member

Unable to reproduce the issue, please provide full reproducing repository based on fresh installation as suggested in the bug report template (or you can refer to https://github.com/nova-issues for example)

@Harrald
Copy link

Harrald commented Mar 19, 2025

I have the same issue. We have build a package that is in use for all our nova 4/5 applications to provide Google single sign on.
From 5.4.0 -> 5.4.1 this is broken.

5.4.0:

php artisan route:list --path=login

  GET|HEAD   login ......................... nova.pages.login › Laravel\Nova › AuthenticatedSessionController@create
  POST       login ................................ nova.login › Laravel\Nova › AuthenticatedSessionController@store
  GET|HEAD   login/google ................. nova.login.google › All4sports\LaravelNovaGoogleSso › RedirectController

                                                                                                  Showing [3] routes

5.4.1

 php artisan route:list --path=login

  GET|HEAD       product-base.test/login ...... nova.pages.login › All4sports\LaravelNovaGoogleSso › LoginController
  GET|HEAD       product-base.test/login/google nova.login.google › All4sports\LaravelNovaGoogleSso › RedirectContr…

                                                                                                  Showing [2] routes

so far i was able to narrow it down to PendingRouteRegistration.php. if i revert this file to what it was in 5.4.0 the problem is no longer there.

@crynobone
Copy link
Member

Don't use nova.pages.login for custom login page, you should use login

@m-lotze
Copy link
Author

m-lotze commented Mar 19, 2025

I do not understand, what is so hard to get this problem. You are adding a redirect from /login to /login in PendingRouteRegistration.php. This does not make any sense. There should be a check like some lines below.

if ($this->withAuthentication === false && ! empty($this->loginPath)) {
          Nova::router(middleware: $this->authenticationMiddlewares)
              ->group(function (Router $router) {
                  // dont do this if $this->loginPath === '/login'
                  $router->redirect('/login', $this->loginPath)->name('nova.pages.login');
              });
      } else {
          if (
              $this->withDefaultAuthentication === true
              && ! Route::has('login')
              && Nova::url('/login') !== '/login' // add the same check above
          ) {
              Route::redirect('/login', Nova::url('/login'))->name('login');
          }

          Nova::router(middleware: $this->authenticationMiddlewares)
              ->group(static function (Router $router) use ($limiter) {
                  $router->get('/login', [AuthenticatedSessionController::class, 'create'])->name('nova.pages.login');
                  $router->post('/login', [AuthenticatedSessionController::class, 'store'])
                      ->middleware(array_filter([$limiter ? 'throttle:'.$limiter : null]))
                      ->name('nova.login');
              });

          Nova::router()
              ->group(static function (Router $router) {
                  $router->post('/logout', [AuthenticatedSessionController::class, 'destroy'])->name('nova.logout');
              });
      }

@crynobone
Copy link
Member

I do not understand, what is so hard to get this problem.

There are at least 8 variations on how one can set up authentication, which is why a full reproduction repository is important.

if ($this->withAuthentication === false && ! empty($this->loginPath)) {

This is valid as of now, my suggestion was.

-GET|HEAD       product-base.test/login/google nova.login.google › All4sports\LaravelNovaGoogleSso › RedirectContr…
+GET|HEAD       product-base.test/login/google login › All4sports\LaravelNovaGoogleSso › RedirectContr…

@crynobone
Copy link
Member

Now, if you can't do that. A full reproduction repository is required for us to determine if there any additional variant we need to cover.

@Harrald
Copy link

Harrald commented Mar 19, 2025

the algorithm inside bootstrapAuthenticationRoutes has changed.

It was first like this:

if ($this->withAuthentication === true) {
}
elseif (! empty($this->loginPath)) {
}

And now it is this:

if ($this->withAuthentication === false && ! empty($this->loginPath)) {
} else {
}

Where in the first scenario i would never enter the if or the else branch when $this->withAuthentication of $this->loginPath were false.
In the new scenario i will enter the else branch.

And that would seem like an api breaking change

@m-lotze
Copy link
Author

m-lotze commented Mar 19, 2025

This is a paid product. Should be enough help, to point to the exact line in your code, where the error occurs.

And maybe this should be fixed in Laravel also. A redirect to the same path always creates a loop and therefore should not be added.

@crynobone
Copy link
Member

This is a paid product. Should be enough help, to point to the exact line in your code, where the error occurs.

This may solve your issue but would only lead to others having regression bugs. The only way to solve the issue is by showing the full use case.

@Harrald
Copy link

Harrald commented Mar 20, 2025

I was in the process of creating a test case, but then I could not reproduce the issue!

What I found out is that it is only a problem if the ServiceProvider that registers the route is auto-registered (it comes from a package).

So, a quick fix for now can be to suppress auto-registering for the package and manually register the ServiceProvider.

I will now create a test case with two public projects to illustrate the full problem.

@stale stale bot removed the stale label Mar 20, 2025
@Harrald
Copy link

Harrald commented Mar 20, 2025

Here is the test case: https://github.com/all4sports/nova-issue-6786

Three commits
The readme explains how to reproduce :)

@crynobone
Copy link
Member

crynobone commented Mar 20, 2025

@Harrald

Based on your reproduction repository.

  1. Update to Laravel Nova 5.4.2
  2. Remove Nova::routes()->withoutAuthenticationRoutes(login: false) from All4sports\GoogleSso\Providers\ServiceProvider.
  3. Update App\Providers\NovaServiceProvider::routes() method
    /**
     * Register the Nova routes.
     */
    protected function routes(): void
    {
        Nova::routes()
            ->withoutAuthenticationRoutes(login: false, logout: false)
            ->register();
    }

@Harrald
Copy link

Harrald commented Mar 20, 2025

Thanks @crynobone this fixes my problems :)

@crynobone
Copy link
Member

Hey there,

We're closing this issue because it's inactive, already solved, old, or not relevant anymore. Feel free to open up a new issue if you're still experiencing this problem.

@m-lotze
Copy link
Author

m-lotze commented Mar 25, 2025

It still creates a redirect loop from /login to /login with the default withoutAuthenticationRoutes(login: '/login', logout: '/logout'). You have to use withoutAuthenticationRoutes(login: false, logout: false) to prevent this.

@crynobone crynobone removed needs more info More information is required stale labels Mar 26, 2025
@crynobone
Copy link
Member

using login: '/login' indicates you wanting Nova to make a redirect, which cause the issue. Setting it to false is the correct solution here.

@m-lotze
Copy link
Author

m-lotze commented Mar 26, 2025

https://nova.laravel.com/docs/v5/customization/authentication

Not clear from the docs that false is needed. And it is a breaking change btw, because it worked before with withoutAuthenticationRoutes().

@crynobone
Copy link
Member

If anything, we will add an exception if developer try to set the same URL since it's not suppose to work.

@m-lotze
Copy link
Author

m-lotze commented Mar 26, 2025

Yeah, make it as complicated as possible, instead of changing the defaults to false.

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