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 😱 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 🙈

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.