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

Improvements in processing redirects with cookie containers. #2119

Draft
wants to merge 24 commits into
base: dev
Choose a base branch
from

Conversation

rassilon
Copy link

@rassilon rassilon commented Jul 25, 2023

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:

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@rassilon
Copy link
Author

@dotnet-policy-service agree

@rassilon rassilon marked this pull request as draft July 25, 2023 16:14
This was referenced Jul 25, 2023
@rassilon rassilon mentioned this pull request Aug 17, 2023
@rassilon
Copy link
Author

rassilon commented Sep 7, 2023

@alexeyzimarev , I'm getting a build error for HttpUtility.ParseQueryString for net472, but heck if I can understand why...

@rassilon rassilon changed the base branch from follow-redirects to dev September 7, 2023 05:38
@rassilon
Copy link
Author

rassilon commented Sep 7, 2023

@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
Copy link
Author

rassilon commented Nov 2, 2023

@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?

@alexeyzimarev
Copy link
Member

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 OnRetry and OnRedirect, but with the current API I think you're right, just call it when it's all done.

@rassilon
Copy link
Author

@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.
@rassilon
Copy link
Author

@alexeyzimarev, I've added ForwardQuery, ForwardFragmnet, MaxRedirect, and FollowRedirectsToInsecure related tests.
This leaves ForwardBody (support/test), ForceForwardBody (test), AllowRedirectMethodStatusCodeToAlterVerb (test), and custom redirection status code additions for redirect detection (support and 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.

@rassilon
Copy link
Author

All of the current test explorer tests pass, btw.

@alexeyzimarev
Copy link
Member

Thank you, I will have a look this week.

@rassilon
Copy link
Author

rassilon commented Mar 13, 2024

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.
@rassilon
Copy link
Author

@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.

@rassilon
Copy link
Author

Bah, I just had this thought:
Should we ALWAYS regenerate the Cookie header after a redirect for security reasons?
And if so, should we even have ForceCookie? I'm leaning towards no.

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.

@alexeyzimarev
Copy link
Member

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.
I didn't look at HttpClient code to see how they handle redirects. Instead, I checked Refit and I thought we could/should do the same thing, or similar. I am wondering if you are making it too good? :)

@rassilon
Copy link
Author

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?

@rassilon
Copy link
Author

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...

@alexeyzimarev
Copy link
Member

Maybe create a Discord server?

@rassilon
Copy link
Author

Maybe create a Discord server?

@alexeyzimarev , https://discord.com/invite/MrGRGUug (expires in 7 days)

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

Successfully merging this pull request may close these issues.

2 participants