Redirect Plugin - `max` option

[supun-io]

Description Looking at the docs/code, I don't see a way to define the number of maximum redirects to follow (similar to allow_redirects.max in Guzzle). This seems like a good security measure to have. There seems to be circulation redirects detection, but there could be a website redirecting from /1 -> /2 -> ... infinitely, causing essentially an infinite loop.

Example

new RedirectPlugin([
     'max' => 3,
]);

Additional context

I'm happy to work on the PR if the idea is accepted.

[dbu]

thanks for the report. you are right, we should allow to limit redirects.

i had a look at the code (expecting to point you to where we define the limit) but actually we don't have it :scream: we have a limit in the retry plugin but not in redirect plugin.

we do have a cyclic redirection detection in the plugin.

i would be happy if you can add the functionality. to be strictly backwards compatible, lets have a default value of null and treat that as no limit. in the next major version, we should use a conservative default, e.g. 10.

[supun-io]

After posting this, I found that the PluginClient has a max_restarts config, which worked well for my use case. It returns "Too many restarts in plugin client" error if there are more redirects than max_restarts. This has been documented as well.

Since this works, do you think we still need a max option in the RedirectPlugin?

~~Also, inside a plugin, we only have access to the current request and next/first callable. I'm not sure if we can get the current plugin context somehow, which is required to implement max within the RedirectPlugin.~~ (Just realized this can be done similar to the Retry plugin)

[dbu]

ah right, that was the reason why we did not already do this :see_no_evil:

imo thats good enough. other things can cause a restart, which makes that limit less exact than a limit on the retry plugin itself. but the limits are protection against accidents / malicous actors and people should not expect to regularly go to exactly the limit of redirects i'd say.

the doc mentions max in the intro - if you want to add a line after the Options caption that again explains that there is no need for a max redirects option because that is handled by the plugin client, it would help people who jump directly to the options.

additionally, adding a note to the RedirectPlugin constructor in the same vein would help people who look directly at the code.