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

[RFC] Implement follow redirects #2059

Open
alexeyzimarev opened this issue Apr 7, 2023 · 9 comments · May be fixed by #2058
Open

[RFC] Implement follow redirects #2059

alexeyzimarev opened this issue Apr 7, 2023 · 9 comments · May be fixed by #2058

Comments

@alexeyzimarev
Copy link
Member

The issue of redirect follow not working because of the missing cookies (#2045) triggered the following thought.

The default redirect follow by the message handler has these limitations:

  • Cookie headers get populated to the consequent request only if the cookie container is used. When we removed the cookie container from the client in v109, this scenario stopped working (described in the issue referenced above)
  • Redirect won't be followed when changing the scheme
  • Max redirect count isn't supported in WASM
  • Need to test if SeeOther redirect is supported, it might not be
  • Need to test if headers are passed to the redirect

I did an experiment here #2058 to check feasibility, and it seems to be working fine. Everything described above can be implemented, some of the things need to be guarded with opt-in options (secure to insecure redirect support, forward auth header, maybe something else).

Any thoughts and suggestions welcome.

@alexeyzimarev alexeyzimarev linked a pull request Apr 7, 2023 that will close this issue
3 tasks
@stale
Copy link

stale bot commented May 22, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label May 22, 2023
@repo-ranger
Copy link

repo-ranger bot commented May 22, 2023

⚠️ This issue has been marked wontfix and will be closed in 3 days

@kendallb
Copy link
Contributor

Seems like a good idea to me.

@rassilon
Copy link

Based on my comments to #2045 , yes we definitely need to unbreak this. :( I really don't want to use selenium for some API tests when I can get away without using selenium if the cookies come back somewhere I can get my grubby hands on.

@rassilon
Copy link

Ok, I think i've got a handle on why the PR isn't working for me. i'll clean up a fork for you to take a look at. It was what was required for me to get past where I was stuck. (But i'll do that when after some shut-eye..)

@rassilon
Copy link

Okie, I created #2119 with my improvements. It got me past this scenario:

  • request.CookeContainer supplied from caller
  • request.Method == Post
  • no json, just query string parameters

After the changes in #2119 the redirect is successfully made with a GET of the destination location due to a 302 being returned from my product.

As you can see, though, many questions remain in #2119 about what exactly to do in some of the scenarios that the HttpClient RedirectHandler class wants to do.

@alexeyzimarev
Copy link
Member Author

Basically, what I planned to do:

  • Add a redirect settings class, add a redirect settings to the client options and request options (override)
    • FollowRedirects (default the same as today)
    • FollowRedirectsToInsecure (false)
    • ForwardHeaders (true)
    • ForwardAuthorization (false)
    • ForwardCookies (true)
    • ForwardBody (true)
    • ForwardQuery (true)
    • MaxRedirects (don't know)
    • RedirectStatusCodes (what's in my PR)
  • Disable redirects for HttpClient, obviously
  • Extract a function to make the actual call
  • Add a function to populate request data from previous request and redirect response
  • Put it all in a loop

@rassilon
Copy link

Sounds good. We should probably add a ForwardFragment option (true) as well for completeness.
Any thoughts on whether we should expose to the client the ability to prevent the forced verb from post to get? (like Flurl does via the redirect event). Although it looks like Flurl does the force to get thing a little differently than the .net framework at present..

That's also a fun number of tests based on all of those settings. :)

@rassilon
Copy link

.net appears to force to get with a 300/301/302 when the verb is POST and 303 when the requested verb isn't GET or HEAD. While Flurl only does 301/302 at the moment when the verb is POST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants