Move Cookie handling out of HttpClient so we do not cross pollinate requests

[kendallb]

Description

Implements the following:

• Make RestClient.Options internal again and expose the BaseUrl and BaseHost as read only getters on the client.
• Move handling of Cookies out of HttpClient and into RestSharp, so they will not cross pollinate requests.
• Make the CookieContainer a property on the request, not the client.
• Add tests for cookie handling.

Purpose

This pull request is a:

• Breaking change (fix or feature that would cause existing functionality to not work as expected)

although the breakage is only against stuff that was not released other than in alpha anyway (making Options internal again).

Checklist

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

[alexeyzimarev]

We can create a cookie container on the first call of AddCookie

[kendallb]

Yeah good point. I will change it.

[alexeyzimarev]

If the Options.BaseUrl is nullable, this property should also be nullable.

[kendallb]

Hmm, I thought I did that. I know it's that way in my full tree, but perhaps I missed that part. Will fix.

[alexeyzimarev]

Same here

[kendallb]

Yep will fix this also.

[kendallb]

Rebased against dev. Still need to decide how to handle redirects. For now I have simply disabled redirects in the bits of code that caused problems and process it outselves (we didn't actually need to follow it anyway). But it's not a generic solution.

Honestly it might be good enough and to document to the user that cookies won't follow redirects. 99% of the time REST services do not use cookies at all, so this is really just to make sure the functionality actually works correctly for those who have used it.

[bednar]

Hi @kendallb,

For our purpose it would be convenient to give us the option to redirect the cookies to another domain. Our users uses cookies to authentication. But at the same time they have a proxy set up that performs the redirection to cloud service.

• fix: Fix authorization by username/password. influxdata/influxdb-client-csharp#302

Regards

[alexeyzimarev]

The Influx case will get worse if cookies aren't held in the client. I believe having a cookie container for the client should not be removed entirely, but allowed to be switched off.

[kendallb]

I completely disagree here and I am not sure you fully appreciate the problem? The cookie container is being shared across all requests, which is bad, really bad. Even Microsoft says in their documentation to not use the cookie container at all. The solution I built allows the cookies to be request specific which is what they need to be.

If the users wants them to persist between their own requests, they will install their own container. That is how we did it previously and how we still do it now with our own rest sharp client library.

[alexeyzimarev]

Could you point to the documentation where they say "don't use cookie container"? Why does it exist? I think you are referring to the HttpClientFactory docs where they say that the factory cannot be used if you need to use a custom cookie container.

Here's the case. I have a client for an external API. It uses authorisation cookies. After the initial authorisation, the cookies must be kept in the container and sent with any consequent request.

The case when cookies must not be shared between requests if the client is used for multiple tenants (for example).

These are two different use cases.

[kendallb]

Sure, it is here:

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-6.0

Cookies
The pooled HttpMessageHandler instances results in CookieContainer objects being shared. Unanticipated CookieContainer object sharing often results in incorrect code. For apps that require cookies, consider either:

• Disabling automatic cookie handling
• Avoiding IHttpClientFactory

So yes, this is in reference to HttpClientFactory, but the point of why they mention it is that if you are using a singleton instance of an HttpClient that is shared across requests from multiple threads, you cannot use the cookie container as you will get cookies being cross pollinated between requests.

The specific case of authentication cookies is a great example of where it is a major problem. If you can assume that your singleton HttpClient instance is ONLY going to be communicating with a single authenticated account on the endpoint it talks to, then sure, you could used a shared cookie container (although I would argue I do not believe it is actually thread safe so you can get threading problems but that is neither here nor there as maybe that is handled in HttpClient itself). But the reality is a lot of folks will be building code using RestSharp that must not share cookies across requests. If the code is a web site for instance where requests coming in might be for specific authenticated users, you cannot allow the authentication cookies to be used across multiple user requests.

We use it for exactly that purpose and for us, we have always maintained a separate cookie container that has the authentication cookies in it that persists long enough to do that is needed and then destroyed. We persist it across requests.

Now in the old days most folks used RestSharp as an instance per request (I know we did). And then you already had to do something similar if you wanted cookies to be maintained between requests so I think anyone using RestSharp to date is already familiar with that concept.

Finally you can only have one cookie container in use at a time really and if you do not use the one built into HttpClient (which is pretty useful given the above problems), the you need to roll your own which is what I ended up doing.

The biggest problem with doing it outside of the client is that it does not handle maintaining the cookies across automatic redirects so you have to turn off automatic redirects. The only way to resolve that is with a lot more code to set up your own HttpHandler because the redirects get handled way lower. Or what we did is just turn off the redirect following and manage that ourselves.

[alexeyzimarev]

I checked the CookigContainer code and it's thread-safe, they use locks in the Add method.

Don't get me wrong, I don't argue with the need to handle cookies per request. The only thing I am saying is that we should consider both use cases, and clearly separate them. I really like your PR where cookies are added per request as headers, I just don't want to remove the custom cookie container from the client entirely in case people really need to use it.

[kendallb]

Well the pros to leaving it in the RestClient itself is that folks who might want a simple way to persist cookies can have that, but the big con IMHO is that users of the library see that and then make the fatal mistake of thinking that either a) they should be using RestClient as instance per request like before (as Easy Post did and still has not fixed and ignored my PR!) or b) they end up using it without realizing the ramifications of the shared cookie container across requests.

b) is my biggest concern because it's a glaring security hole. a) is a concern because the more 'code smells' people run across trying to move to v107 and later the more they finally are forced to realize that they are doing it wrong and need to change their code.

It would certainly be possible to get the container from either the client or the request, but I think it's too dangerous to leave it in the client? Folks will ask why it was moved, and once they understand why I think they would be fine doing things differently.

[alexeyzimarev]

Yeah, I agree. I think your change to delete the new CookieContainer instance creation from the client is good. If the default container isn't set, the chance of hitting this issue is basically the same as it's with HttpClient.

[kendallb]

Also 99% of folks using RestSharp for REST services generally do not use cookies anyway so it won't affect that many folks, and for those who do get affected it probably matters to get it right.

[stale]

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.

[repo-ranger]

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

[stale]

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.

[repo-ranger]

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