[11.x] Calling throttleApi() as-is throws an exception for authenticated users

[Propaganistas]

Laravel Version

11

PHP Version

8.3.4

Description

Laravel 11 dropped the default api rate limiter.
But when calling throttleApi() when configuring middleware in bootstrap/app.php, the default limiter is set to api.

framework/src/Illuminate/Foundation/Configuration/Middleware.php

Line 678 in 29acd21

public function throttleApi($limiter = 'api', $redis = false)

If you didn't define the limiter yourself, the throttle middleware will try to wire up a limiter. If the route is accessed by an authenticated user, it'll encounter a string (api), so it thinks it's a user attribute and an exception gets thrown which is not pointing to throttling at all (has already been reported in #50818).

framework/src/Illuminate/Routing/Middleware/ThrottleRequests.php

Lines 185 to 187 in 29acd21

	if (! is_numeric($maxAttempts) && $request->user()) {
	$maxAttempts = $request->user()->{$maxAttempts};
	}

So in this fairly trivial use case the application blows up because of a default argument.

Shouldn't the max attempts resolver check for existence of a property/attribute on the user before trying to return it?
Or perhaps a more descriptive exception can be thrown?
Or re-add the default api limiter?
Or make an explicit note in the docs about this?

Happy to create a PR if a direction gets chosen.

Steps To Reproduce

Spin up a new 11.x application, configure throttling without any arguments, and call an api route when authenticated.

[driesvints]

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

[Propaganistas]

@driesvints Here you go: https://github.com/Propaganistas/laravel-bugreport-50931

While composing the repository I realized that the combination of an undefined named limiter, an authenticated user and Model::preventAccessingMissingAttributes() is causing the mayhem.

[driesvints]

Thanks @Propaganistas. Do you know what the value of $maxAttempts is here?

framework/src/Illuminate/Routing/Middleware/ThrottleRequests.php

Line 186 in 29acd21

$maxAttempts = $request->user()->{$maxAttempts};

[Propaganistas]

@driesvints

its value is api over there; it gets passed all the way down.

If Model::preventAccessingMissingAttributes() is set, that line will throw an exception because it tries to access an inexisting $user->api attribute.

If missing attributes are allowed, that line resolves to null, which is ultimately cast to an int in the subsequent lines so resolveMaxAttempts() will return 0. Might be undesired as well...?

[driesvints]

What's odd to me is that this feature just doesn't seem documented at all. What's extra odd is indeed the quirk that you mention that it defaults to zero when the result is null. I would expect it to be 1 or the default of 60.

I think one thing we absolutely need to fix is to check if the attribute exists on line 185 and if not, skip the if contents. But the return (int) $maxAttempts; should be a LogicException being thrown I think because there's no way for the limiter to get the correct value at that point. But at the same time it could be there for a perfect valid reason that I cannot see at this point.

[driesvints]

Just FYI, this is how I think this method should look like:

    /**
     * Resolve the number of attempts if the user is authenticated or not.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  int|string  $maxAttempts
     * @return int
     */
    protected function resolveMaxAttempts($request, $maxAttempts)
    {
        if (str_contains($maxAttempts, '|')) {
            $maxAttempts = explode('|', $maxAttempts, 2)[$request->user() ? 1 : 0];
        }

        if (! is_numeric($maxAttempts)) {
            if ($request->user() && $request->user()->hasAttribute($maxAttempts)) {
                $maxAttempts = $request->user()->{$maxAttempts};
            }

            new LogicException("The max attempts rate limiter key '$maxAttempts' is not defined on the user model.");
        }

        return (int) $maxAttempts;
    }

[taylorotwell]

PR already open for this: #50908