[RFC] Working with cookies

[alexeyzimarev]

Separating the cookie discussion from #1786

How would the authenticator change the token though? Since the authenticator is called on a per request basis, unless the tokens are static it needs to do something to configure them.

It can be done in the authenticator itself as I've done in the sample https://restsharp.dev/usage.html#authenticator

I would suggest Encode and EncodeQuery become part of the Options and would not be changed for a client instance.

Agreed

I am not sure it's desirable to have a shared cookie container for a single RestClient instance it that thing is cached forever.

I am not sure either as I never used it. But it always been a part of the client (or request, don't remember) for some reason. Yes, I think it was on the request, but since it is used for the message handler instantiation and cannot be changed, I had to move it to the client. I guess the discussion if it was a good decision, maybe not.

If there would be a way to specify cookies per request using headers, the cookie container could stay as-is, but AddCookie in the client can be removed, or converted to AddDefaultCookie, and we can add AddCookie to the request, where it was previously.

[kendallb]

Separating the cookie discussion from #1786

How would the authenticator change the token though? Since the authenticator is called on a per request basis, unless the tokens are static it needs to do something to configure them.

It can be done in the authenticator itself as I've done in the sample https://restsharp.dev/usage.html#authenticator

I don't agree that is a viable option. The reason is that your Twitter example where you need to get and cache at runtime a token, based on a static clientId and clientSecret. Those are configured once and the authenticator then dynamically calls Twitter to get the value and then stores it for later use. So nothing in that scenario allows for that tokens to be dynamically managed on a per used basis which is needed for some API's in multi user environments like web apps.

BTW, this brings up something else I noticed. The function to get the GetAuthenticationParameter() ends up needing it's own RestClient internally to call Twitter the first time. The actual function to process authorization is passed the current RestClient and RestRequest, but those are NOT passed onto the GetAuthenticationParameter() function:

    public async ValueTask Authenticate(RestClient client, RestRequest request)
        => request.AddOrUpdateParameter(await GetAuthenticationParameter(Token).ConfigureAwait(false));

I think that function signature should be changed (or have another overload for it) that would be passed the RestClient and RestRequest. That way you can avoid having to create a short lived RestClient for the Twitter token that needs to be disposed of, but more importantly the authenticator class CAN inspect the RestRequest to get request specific stuff out of it.

Currently I am not sure where you would put the request specific stuff in the RestRequest property bag, but it could be retrieved from a header or something.

Or perhaps the correct solution is a combination of a) adding an overload for GetAuthenticationParameter() that accepts the RestClient and RestRequest, as well as hooking up an optional version of that in the RestRequest itself so the caller can construct something to be done at request time. That way the user has two ways to wire up an Authenticator. Wire up a static one that is the same across all API calls in the RestClient, or wire up a request specific one in RestRequest.

That was kind of the approach I was planning to take in the PR I am working on.

Although this is such an important issue IMHO to avoid folks trying to set this on a single instance RestClient at request time and having threading issues, that I almost feel breaking the API and moving it to the request is the correct approach. Otherwise you will end up with two problems with existing code being ported to v107:

1. They port to v107 and continue to use a RestClient per request, as EasyPost did as stuff like configuring the Authenticator in the client kinda indicates it's request specific.
2. They correctly change to using a shared instance of RestClient per API entry point, but override the Authenticator on a per request basis (as it's entirely possible to do that) and end up with really wonky runtime threading issues (or cross request authentication bugs).

I am not sure it's desirable to have a shared cookie container for a single RestClient instance it that thing is cached forever.

I am not sure either as I never used it. But it always been a part of the client (or request, don't remember) for some reason. Yes, I think it was on the request, but since it is used for the message handler instantiation and cannot be changed, I had to move it to the client. I guess the discussion if it was a good decision, maybe not.

Actually it was always on the client, which is another reason everyone (including myself) used a RestClient per request. I am in the process of re-writing the current code that uses that right now. In our case, we manage our own cross request CookieContainer and setup a new cookie container on every request with just what is needed. I think the correct place for the cookie container is in the RestRequest, not RestClient as it needs to be request specific and not shared between calls.

If there would be a way to specify cookies per request using headers, the cookie container could stay as-is, but AddCookie in the client can be removed, or converted to AddDefaultCookie, and we can add AddCookie to the request, where it was previously.

Actually I would argue that having a common cookie container shared across requests with different threads is extremely dangerous due to cookies leaking between requests. It only worked previously on the existing RestClient as everyone used a new one per request. Removing it from the client and moving it to the RestRequest is the correct solution and while a breaking change, a necessary one to avoid security problems with cookie leakage across requests.

I suspect most folks never use it for 99% of REST requests, and in the few cases they do use it I expect they would be asking the same questions I have, which is how do we avoid cross contamination between requests. In other words, anyone already moved to v107 that has not complained about this is most likely not using cookies.

[alexeyzimarev]

So nothing in that scenario allows for that tokens to be dynamically managed on a per used basis which is needed for some API's in multi user environments like web apps.

That's correct, but not all the apps out there are user-facing apps. The OAuth2 token I used is the app token for machine-to-machine integration, where the current solution works.

Actually it was always on the client, which is another reason everyone (including myself) used a RestClient per request.

That basically forced HttpWebRequest to create a new instance of HttpMessageHandler, causing hanging connections and breaking the connection pooling. So, it was broken from the moment Microsoft re-implemented HttpWebRequest using HttpClient.

Actually I would argue that having a common cookie container shared across requests with different threads is extremely dangerous due to cookies leaking between requests.

"It depends" :) It works totally fine for machine-to-machine integration. But yes, it will hurt when used in user-facing apps, where each user has its own creds. No doubt.

[kendallb]

Related to cookies as mentioned in the other RFC, Microsoft says do not use them with HttpClient:

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

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

[alexeyzimarev]

Correct. It explicitly mentions the pooled handler. RestSharp doesn't use pooled handlers, if it is used as a singleton, as part of some typed API client, it works fine as cookies aren't shared outside of that client. That implies that we are talking about a single-user app or machine-to-machine API calls.

[kendallb]

Right, a single instance client won't work with shared cookies though for a web app, and we use cookies for some of our usages for RestClient (not for our API calls, but when using it for accessing third party web sites for scraping etc as we need to perform a login). So for that, a shared cookie container won't work :(

[alexeyzimarev]

That's right. What I mean by this issue is not to remove the cooking container but put a big warning to the docs. Also, remove the AddCookie implementation that adds a cookie to the container (maybe) and bring back the cookie parameter. The issue with the old cookie parameter was that it added cookies to HttpWebRequest and I think it used a cookie container. I need to look at that .NET issue to better understand how to pass cookies as headers. Plus, we need to ensure to properly get cookies from the response. Currently, I query the container for the request domain, so it doesn't return other cookies, but, as you rightfully mentioned, cookies for multiple users might be overriding each other and it will be madness at some point.

[kendallb]

Right. Sending the cookies in the header is easy, but then as you mentioned you also need to extract the cookies from the response and put them back into the jar.

[alexeyzimarev]

Right now, the client always uses a cookie container. If it is gone, we need to find a way to get the cookies.

[kendallb]

Right the cookie container is in RestClient and is attached to the HttpClientHandler of HttpClient. And even using HttpClientFactory, as Microsoft says you should not rely on it for cookies as those handlers get reused across requests and they do nothing to ensure the cookie container is not reused. I don't think you can change the container either after HttpClient has been created (or really after the handler has been created).

I think the only viable way to do this correctly with a single instance RestClient is to have a CookeContainer attached to the request object (provided by the caller if needed), and not RestClient. Then on each request, we manually serialize the cookies into the headers (pretty easy), and then when we process the response we manually parse the cookies from the response headers. That way it's all per-request and no cross contamination of cookies.

I spent a lot of time researching this a long time ago when we tried to change to using HttpClient for our web scraping code, and eventually gave up and instead layered it on top of RestClient. LOL. So now we are back at square one. As far as I can tell from googling what others have done, everyone using HttpClient is manually serializing and de-deserializing cookies from the headers.

[kendallb]

Updated cookie handling is fixed by this PR and does nothing else:

#1801

[kendallb]

Grrr, run into a major snag with this and using Cookies with RestSharp. As mentioned it's not possible to use cookies with RestSharp as it is currently set up in v107 as the cookies will get shared across requests (super bad). So the code I wrote works really well, EXCEPT when a request is actually a redirect as the cookies need to get picked up from the redirect and put into the cookie jar. Since redirect handling is all handled by HttpClient, the cookies get lost as they are no longer in the response headers when we get the final response back. It works only if HttpClient is handling the cookies.

The solution would be to process redirects in RestSharp itself, so that cookies would work but that is going to be quite a bit of work to make it correct. And the catch is I cannot just turn it off for a single request, because that is all configured on the HttpClient handler. So it either needs to be on or off completely for each client.

Should I try to add code to handle redirects internally in RestSharp to get cookies working?

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

[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

[Bidthedog]

Is there a plan to move cookies back to the RestRequest object? I'm currently facing an issue where I am implementing a singleton RestClient, as per the docs, but I now need to set a per-request cookie that will be used by an external auth server that injects middleware into the HTTP server pipeline, which identifies the individual user based on the cookie that is set.

Obviously, adding the cookie to the client is not going to work as it's only created once, and there are no facilities to set cookies in the request. Is there a plan / timescale for making this change?

[kendallb]

Yes, I changed the cookie handling back to the request object with this PR which has been merged:

#1966

I am not sure if this code is live yet, but I assume it will be in the next release. In the interim if you want to play around with it you can try out my personal build that we are using. Some stuff in there is not yet accepted upstream, but all the cookie stuff is in there:

https://www.nuget.org/packages/AMain.RestSharp/107.4.4

The only caveat is that cookies will not follow redirects, but that is not something you can easily fix with the way HttpClient is structured.

[Bidthedog]

It's in 109 preview, I upgraded yesterday and it seems to be working OK. Thanks for the effort :)

[poojakatte]

So i had been using the addcookies to add sso cookie to automate an api flow for testing purpose and i just upgraded all the packages.. i did upgrade to the 109 preview version , now the question is how do i add the sso cookie to the request ??

[alexeyzimarev]

There's the AddCookie method in the RestRequest class.

For tests you might prefer using the ConfigureHttpMessageHandler option and provide a function to add the cooking container that contains your SSO cookies. That way, you only need to change the options for the client for the tests rather than changing the request code.

[alexeyzimarev]

I consider the cookie issue to be solved in v109, which is now available in preview. Big thanks to @kendallb for the contribution and all the discussions.

If anyone believes that something still needs to be improved, please comment here, otherwise I will close this issue.

[billybooth]

I agree with you that the v109 preview addresses this issue. I am anxiously awaiting the v109 release, since we desperately need cookie support for an OpenAPI generated client. We had it previously via IRestRequest and then lost it with RestRequest in the v108 line. In the generated library, we have the ability to manipulate the RestRequest before it's sent, and v109 slots into that workflow nicely.

[alexeyzimarev]

I will do my best to wrap it up asap. I think the client factory work would need to wait.

[fcastells]

I've spent a significant amount of time trying to get RestSharp to authenticate to an API which returns an authentication cookie. I've tried both 107 and 109 preview 2 and I haven't managed.

With 107, the login request goes through, but the cookie is not in client.CookieContainer.Cookies, although I can see it in Fiddler.

With 109, ExecutePostAsync response shows the error "System.Net.CookieException: The 'Domain'='' part of the cookie is invalid.
at System.Net.Cookie.VerifySetDefaults(CookieVariant variant, Uri uri, Boolean isLocalDomain, String localDomain, Boolean setDefault, Boolean shouldThrow)
at System.Net.CookieContainer.CookieCutter(Uri uri, String headerName, String setCookieHeader, Boolean isThrow)"

While looking at it, I noticed in Fiddler that in fact, the cookie comes back with domain=; path=/. As I cannot modify the API, it seems that I cannot use RestSharp to authenticate into this API? or is there something I can do from the RestSharp side?

Also, @alexeyzimarev I don't understand your advice above "For tests you might prefer using the ConfigureHttpMessageHandler option and provide a function to add the cooking container that contains your SSO cookies. ". Specifically, I don't understand what that function should do given the parameter it gets.

[kendallb]

If the cookie domain is blank it means it applies to the domain of the calling site, so that is pretty normal. I am not sure why it would not work with RestSharp, but it must be something with how the internal .NET API's are handling the request if it did it in 107 and also in 109. The difference in how things work in 109 is that we changed it to move the cookie handling out of the HttpClient and into the request handler because the client is reused and cookies should never be reused across requests. But internally its still using the .NET API's to process the cookies.

The one difference with prior versions (v106 and earlier) but I don't think is affecting this, is that the new cookie handling will not be passed across redirects. Generally this is not an issue for REST API's as redirects are not common but it can be a problem if you are doing web scraping. In our case we turn it off in RestSharp and then manually handle the redirects ourselves in the few instances we need it.

Have you tried consuming the API in v106 to see if that makes a difference? Curious if it worked before any of the changes to use HttpClient.

Also it would help if we knew what API it is, unless it's a private API?

[alexeyzimarev]

While looking at it, I noticed in Fiddler that in fact, the cookie comes back with domain=; path=/. As I cannot modify the API, it seems that I cannot use RestSharp to authenticate into this API? or is there something I can do from the RestSharp side?

It looks like that HttpClient is unable to parse cooking with invalid domain, I am not sure what can be done about it.

[kendallb]

I think it is very common for a cookie to have no domain passed back and then if the request was for www.example.com the cookie is set in the browser to www.example.com. So I think it's something else that is causing the cookies to not show up? We generally set the cookie domain if we want it shared across an entire domain (like example.com for the domain so then it works with www.example.com or store.example.com or whatever).

[fcastells]

All right.
I managed to find the source of the problem on the API side. It configures the authentication as follows:

        services.AddAuthentication(securityConfiguration.AuthenticationScheme)
				.AddCookie(securityConfiguration.AuthenticationScheme, o =>
				{
					o.Cookie.Domain = securityConfiguration.AuthenticationCookieDomain;
             [...]

and securityConfiguration.AuthenticationCookieDomain happens to be null. Interestingly, If I set that property to null, the cookie comes with "domain=;" but if I don't set the property at all, the cookie comes without the "domain" part and RestSharp doesn't complain.

[fcastells]

Apparently, the behaviour of 107 (ignoring the cookie) is not exclusive of RestSharp. See Cookies with empty domain are not stored, this is contrary to RFC and standard browser practice. That behaviour is slightly better than the 109 which ends up with an exception.

[kendallb]

Interesting. This must be something about how cookies are handled and parsed in .NET since all the parsing is done using the .NET framework functions.

[fcastells]

Yeah. Looking at the exception, it comes from System.Net.Primitives.

https://github.com/dotnet/runtime/blob/cbc8695ae0c8c2c2d1ac1fc4546d81e0967ef716/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs

Line 567 doesn't like empty strings.

So, yeah, I don't think anything can be done on RestSharp, except maybe don't fail the request if a cookie cannot be parsed as it was doing in 107. It wouldn't work for me, but I think it's a better behaviour, specially if you don't care for that cookie.

[kendallb]

Sure enough, it deliberately crashes out if it's blank, which IMHO is wrong?

                    // Syntax check for Domain charset plus empty string.
                    if (!DomainCharsTest(domain))
                    {
                        if (shouldThrow)
                        {
                            throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.DomainAttributeName, domain ?? "<null>"));
                        }
                        return false;
                    }

On the one hand I am not sure what is better? Capturing the exception and swallowing it and expecting the caller to inspect the error condition or throwing the exception so the caller knows something is wrong?

I will let @alexeyzimarev determine what is the best course of action here.

[kendallb]

One complete hack solution to make RestSharp 'work' with blank cookie domains would be to modify the cookie string from the response to fill in the domain for blank entries to match the domain of the caller, as then they would parse? I think that would be correct as per the spec, but then again it's odd so much code requires a non-blank domain?

[alexeyzimarev]

I think it happens here when the headers are extracted:

            if (responseMessage.Headers.TryGetValues(KnownHeaders.SetCookie, out var cookiesHeader)) {
                foreach (var header in cookiesHeader) {
                    cookieContainer.SetCookies(url, header);
                }
            }

If RestSharp swallows the exception it will still be possible to use response headers to get the cookie value.

[kendallb]

True. And if the cookies are all blank in the response, the developer is likely to wonder why and investigate anyway. Crashing is always not good in production :)

[alexeyzimarev]

Apparently, .NET team thinks otherwise :)

[fcastells]

Thanks for the quick responses on this issue. I submitted a PR with the fix, I hope it aligns with what you had in mind, otherwise, I can adjust it.

[kendallb]

Seems reasonable. I do wonder what if it makes sense to set the domain when it's missing but clearly it's common for this to happen in .NET.

[alexeyzimarev]

I assume we are done with this one :)