Improvements in processing redirects with cookie containers. #2119
Conversation
|
@dotnet-policy-service agree |
rassilon
force-pushed
the
follow-redirects
branch
from
307f85c
to
df851af
Compare
|
@alexeyzimarev , I'm getting a build error for HttpUtility.ParseQueryString for net472, but heck if I can understand why... |
|
@alexeyzimarev , ok, I've started this in the general direction you mentioned. Still more changes to come. Let me know what you think so far. Should we have a Options.RedirectOptions property that determines the behavior of forcing the method to GET for instance? |
rassilon
force-pushed
the
follow-redirects
branch
from
d99384a
to
a4ce540
Compare
|
@alexeyzimarev , I've updated the branch for the interceptor changes. However, should After Request be called once for the last request in the redirect chain or repeatedly for each redirect? |
|
Yeah, I think so. Interceptors don't care about how the request is done. It might change in the future if we decide to add |
…uce warnings.. RequestContent: Don't use obsoleted property. RestClientOptions*: use new RedirectOptions class instead for FollowRedirects.
…r newer VS versions.
rassilon
force-pushed
the
follow-redirects
branch
from
42a8613
to
a69b48e
Compare
|
@alexeyzimarev , I added ForwardHeaders/ForwardAuthorization/ForwardCookies tests... Should ForwardAuthorization support the Authenticator's parameter name instead of just KnownHeaders.Authorization? |
Add secure test server untrusted certificate. Out of paranoia, don't let SimpleServer use either the secure or insecure ports from TestServer. Add tests both positive and negative for RedirectOptions.FollowRedirectsToInsecure Remember to override options.RemoteCertificateValidationCallback if you get a certificate validation error in new tests.
|
@alexeyzimarev, I've added ForwardQuery, ForwardFragmnet, MaxRedirect, and FollowRedirectsToInsecure related tests. However, I wonder if the ForceForwardBody support is the best way to achieve that goal... Please take a look and see if you have any feedback. |
|
All of the current test explorer tests pass, btw. |
|
Thank you, I will have a look this week. |
|
No problem. I managed to get Visual Studio .Net code coverage analysis working after getting nowhere with Coverlet and Fine Code Coverage. Hopefully, I'll be adding the rest of the test cases (indicated by coverage and otherwise) before the week's end. I see why the secure server failed on the GitHub check; I didn't notice I had to export the private key into the certificate when running the tests locally for some reason. That fix will be in the next set of test case additions. |
Add changes to allow the AlterVerb... RedirectOptions to work properly. RequestContent.cs: Add support for omitting the body (due to the HTTP Verb/Method changing under redirect processing) TestServer.cs: * Add certificate password for SSL test server. * Add HTTP verb changing related routes * Add dump-request route so that the method and hearders get dumped to response content to help enable authoring RedirectOption tests. * Minor tweaks to silence nullability warnings. RedirectTests.cs: Use new (Not)ContainCookieWithNameAndValue extension methods in tests, and use .And to cleanup the repetitive assertion code. RedirectOptionsTest.cs: * Add missing StatusCode assertions * Use new (Not)ContainCookieWithNameAndValue extension methods in the tests and .And. to cleanup the assertions. * For coverage reasons add initial request cookies to Can_RedirectForwardHeadersFalseWithCookie_DropHeaders. * New tests: Options.RedirectOptions.FollowRedirects = false Options.FollowRedirects = False (back compat) AllowForcedRedirectVerbChange = false with 302 AllowForcedRedirectVerbChange = false with 303 Change verb with defaults after 302 Change verb with defaults after 303 Don't chanve verb with defaults after 307 Options.CookieContainer contains expected results after a redirection. Change verb with defaults after 303, but with ForceForwardBody so that the request body is forwarded on the new verb. Additionally, due to having VS 17.9.3 updated xunit/ms test sdk nuget pkgs that makes some of the dependabot PRs obsolete.
|
@alexeyzimarev, that's the last set of changes I will make before we discuss ForwardBody vs ForceForwardBody. If we should only have, say, ForceForwardBody since RedirectMethod status code (303) returns true from the helper method to determine if we should force GET instead of the current method. I don't anticipate making any other changes before you get a chance to give me some feedback. The code coverage looks excellent. I noticed that sometimes some of the non-"TestServer.cs" related tests fail due to trying to dispose of an already disposed server. I don't know if this is related to the xunit nuget pkg update. I also don't know if moving to IAsyncLifetime like TestServer.cs would fix them. |
|
Bah, I just had this thought: For non-RestSharp users, if a supplied HttpClientHandler sets a CookieContainer, the HttpClient code does this as near as I can unravel from the source code. (That code makes my head hurt....) The basic idea is that the System.Net.Http inner handler of the RedirectHandler passes doAuth = false to SendAsync overloads of various sorts. However, RestSharp doesn't supply the CookieContainer to a HttpClient HttpClientHandler, so we have to do it ourselves. |
| @@ -60,7 +66,7 @@ public class RestClientOptions { | |||
| ? ResponseStatus.Completed | |||
| : ResponseStatus.Error; | |||
|
|
|||
| /// <summary> | |||
| /// <summary>s | |||
There was a problem hiding this comment.
I guess this s needs to go?
|
I am not sure. I thought we should collect the response headers and pass them over to the next call. It might, indeed, create security concerns if the response contains things like authentication cookie. |
|
Hah. I think removing ForceCookie, regenerating the cookies for every redirected request (for cross-domain security reasons), and preventing the verb change without supporting ForwardBody or ForceForwardBody nonsense would be good enough. There's a reason the CookieContainer has you supply the requesting Uri for the GetCookie method after all. Hrm.. I looked at Refit briefly (https://github.com/reactiveui/refit/) but didn't see the bits that did header propagation. Did I look in the wrong place? |
|
We could spin up a slack restsharp instance and have a conference call to discuss in a little more detail if you want or something similar... |
|
Maybe create a Discord server? |
@alexeyzimarev , https://discord.com/invite/MrGRGUug (expires in 7 days) |
Description
Improvements for #2058. This is just a starting point for these improvements, they do as you pointed out need some more tests. I have an outline for Can_Perform_RedirectingPost_With_Receive_Cookies Redirect test, I just need to make it RestSharp only as opposed to calling into the product I'm testing atm.
Purpose
This pull request is a:
Checklist