Feature: Support async use of the beforeRedirect option

[olebor]

I would like the beforeRedirect method to support async use cases. A possible implementation could be that it will wait if a promise is returned in the function. I believe that would not break any current uses of it.

I need this for a scenario where I do server-side requests to endpoints specified by untrusted users. The plan is to validate the requests to ensure they don't point to anywhere I don't want them to (for example, inside the infrastructure/SSRF). I would need to validate all redirect steps (resolve DNS, etc.), not only the initial URL.

If this is a feature you think could be useful, I would be happy to take a shot at it.

[RubenVerborgh]

Thanks @olebor, that makes sense. For backward compatibility, we could activate async support if either a done callback is passed as an argument or a promise is returned by the function.

[olebor]

Cool!

To clarify: Since the lib itself calls the function, it would be the lib passing the done callback into it. We would then have to check if the function expects a fourth argument. Is this what you meant by activating async by a done callback?

Adding a fourth done parameter could potentially break some cases. For example: if a user for some reason has defined a beforeRedirect function with four expected arguments, things would work fine today. But if we were to add a function as the fourth argument and expect it to be called before proceeding, this case would stop functioning.

Am I missing something?

Personally, I think the promise pattern is the cleanest one.

[RubenVerborgh]

To clarify: Since the lib itself calls the function, it would be the lib passing the done callback into it. We would then have to check if the function expects a fourth argument. Is this what you meant by activating async by a done callback?

Yes; although probably 4 arguments wouldn't be great to have. But we can discuss/change that once we have concrete code.

For example: if a user for some reason has defined a beforeRedirect function with four expected arguments, things would work fine today.

Ah well they shouldn't, it wasn't in our interface 😅

Personally, I think the promise pattern is the cleanest one.

Yes, but follow-redirects has a long compatibility history, with even some earlier versions of Node that did not have promises yet. But feel free to take a first stab at it, and we can discuss it from there.