Runtime: [Cookie] SocketsHttpHandler/CookieContainer does not accept cookies from Twitter, fails request

[Tratcher]

I tested ASP.NET's Twitter OAuth handler with the new SocketsHttpHandler (dotnet --version: 2.1.300-preview2-008251) and found that the CookieContainer does not accept cookies from Twitter and it fails the request rather than ignoring them.

System.Net.CookieException: An error occurred when parsing the Cookie header for Uri 'https://api.twitter.com/oauth/request_token'. ---> System.Net.CookieException: Cookie format error.
   at System.Net.CookieContainer.CookieCutter(Uri uri, String headerName, String setCookieHeader, Boolean isThrow)
   --- End of inner exception stack trace ---
   at System.Net.CookieContainer.CookieCutter(Uri uri, String headerName, String setCookieHeader, Boolean isThrow)
   at System.Net.CookieContainer.SetCookies(Uri uri, String cookieHeader)
   at System.Net.Http.CookieHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.AuthenticateAndRedirectHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.FinishSendAsyncBuffered(Task`1 sendTask, HttpRequestMessage request, CancellationTokenSource cts, Boolean disposeCts)
   at Microsoft.AspNetCore.Authentication.Twitter.TwitterHandler.ObtainRequestTokenAsync(String callBackUri, AuthenticationProperties properties) in d:\github\AspNet\Security\src\Microsoft.AspNetCore.Authentication.Twitter\TwitterHandler.cs:line 197
   at Microsoft.AspNetCore.Authentication.Twitter.TwitterHandler.HandleChallengeAsync(AuthenticationProperties properties) in d:\github\AspNet\Security\src\Microsoft.AspNetCore.Authentication.Twitter\TwitterHandler.cs:line 140
   at Microsoft.AspNetCore.Authentication.AuthenticationHandler`1.ChallengeAsync(AuthenticationProperties properties) in d:\github\AspNet\Security\src\Microsoft.AspNetCore.Authentication\AuthenticationHandler.cs:line 227
   at Microsoft.AspNetCore.Authentication.AuthenticationService.ChallengeAsync(HttpContext context, String scheme, AuthenticationProperties properties)

Reference app: https://github.com/aspnet/Security/blob/4e3e8bb109f3d1b3d95f7af4775a386d291c2b96/samples/SocialSample/Startup.cs#L111

The cookies in question are:

set-cookie: guest_id=v1%3A151966890829154034; Expires=Sat, 07 Mar 2020 00:28:24 UTC; Path=/; Domain=.twitter.com
set-cookie: personalization_id="v1_Xfcbmc7S2QFG3YSiebMfA=="; Expires=Sat, 07 Mar 2020 00:28:24 UTC; Path=/; Domain=.twitter.com

And it appears to be the UTC time zone that causes it to fail.

var cookieContainer = new CookieContainer();
cookieContainer.SetCookies(new Uri("http://example.com"), "key=value; Expires=Sat, 07 Mar 2020 00:28:24 UTC");

Where GMT works:

 cookieContainer.SetCookies(new Uri("http://example.com"), "key=value; Expires=Sat, 07 Mar 2020 00:28:24 GMT");

If SocketsHttpHandler becomes the default then this behavior will regress new and existing server apps using Twitter logins.

Fix: Unreadable cookie values must at a minimum be ignored. They should not fail the request. I recall this is the behavior of .NET HttpClient/HttpWebRequest, and CookieCutter already has an option for this.

Workaround, ignore cookies, they're optional for this flow:

.AddTwitter(o =>
{
   ...
   o.BackchannelHttpHandler = new HttpClientHandler() { UseCookies = false };
})

[davidsh]

Fix: Unreadable cookie values must at a minimum be ignored.

Yes, the handler (SocketsHttpHandler) should ignore unreadable cookies and continue with the request.

But we should also consider making a fix to Cookie parsing. Perhaps the use of the "UTC" timezone is a valid construct but our cookie parser just doesn't know how to handle that.

[davidsh]

cc: @stephentoub @geoffkizer @karelz

[geoffkizer]

The RFC seems to require GMT, but if UTC is used in practice we should support that as well (it's the same as GMT so should be easy).

[davidsh]

The RFC seems to require GMT

Which RFC? Looking at RFC6265, I don't see a lot of mentions about timezone formats except for this text:
https://tools.ietf.org/html/rfc6265#section-5.1.1

Let the parsed-cookie-date be the date whose day-of-month, month,
year, hour, minute, and second (in UTC) are the day-of-month-
value, the month-value, the year-value, the hour-value, the
minute-value, and the second-value, respectively. If no such
date exists, abort these steps and fail to parse the cookie-date.

It would be good to understand how common the use of the "UTC" letters are compared to "GMT" in date formats for response headers.

[davidsh]

And looking at this old RFC:
https://tools.ietf.org/html/rfc822#section-5

It seems that "UT" was a valid time zone indicator for "GMT".

zone        =  "UT"  / "GMT"                ; Universal Time
                                                 ; North American : UT
                 /  "EST" / "EDT"                ;  Eastern:  - 5/ - 4
                 /  "CST" / "CDT"                ;  Central:  - 6/ - 5
                 /  "MST" / "MDT"                ;  Mountain: - 7/ - 6
                 /  "PST" / "PDT"                ;  Pacific:  - 8/ - 7
                 /  1ALPHA                       ; Military: Z = UT;
                                                 ;  A:-1; (J not used)
                                                 ;  M:-12; N:+1; Y:+12
                 / ( ("+" / "-") 4DIGIT )        ; Local differential
                                                 ;  hours+min. (HHMM)

So, we probably need to be flexible in how we parse this. I think that browsers actually are better at this and will accept a few different formats for GMT time.

[geoffkizer]

I was looking here: https://tools.ietf.org/html/rfc6265#section-4.1.1

This refers to the old HTTP RFC here: https://tools.ietf.org/html/rfc2616#section-3.3.1

All HTTP date/time stamps MUST be represented in Greenwich Mean Time
(GMT), without exception.

[geoffkizer]

That said, I think we probably should accept "UTC"; I don't see any real downside.

[stephentoub]

I assume after https://github.com/dotnet/corefx/pull/27865 this is no longer "blocking-partner"?

[Tratcher]

Agreed.

[davidsh]

@Tratcher Does .NET Framework accept the Twitter cookie into the CookieContainer? Or did it ignore it?

[Tratcher]

No, .NET Framework doesn't accept these either, but at least it didn't throw.

[davidsh]

Moving to future since the behavior now matches .NET Framework and the request can be processed by ignoring the cookie. We still may enhance cookie parsing to handle variations in the date format that include 'UTC' timezone indicator.

[Tratcher]

Update: Twitter no longer returns UTC in their cookies, they changed back to GMT.

[caesar-chen]

Next steps:

1. Verify that major browsers accepting cookie with UTC time zone.
2. Wrote test cases: see those cookies with UTC time are NOT added to CookieContainer. (But those cookies are valid and should be accepted by browsers).
3. Make product fix (base on code path for how we handling GMT time zone).
4. Verify previously failing test cases are now passing.

[karelz]

Thanks @caesar1995 for the steps above. @davidsh I don't think you're working on it so I unassigned you. Feel free to change it back if I was wrong ... thanks!

[karelz]

@golf1052 wants to pick it up, assigning to you ...

[golf1052]

Tested in Firefox, Chrome, and Edge that setting a cookie with Expires UTC is accepted. Tried to test IE but the internet doesn't seem to know where cookies are stored now, the cookies location moved in the Creators Update.

[davidsh]

As per my comment, https://github.com/dotnet/corefx/pull/30104#discussion_r206337828, we should try and fix DateTime.TryParse() in the CoreClr to allow for "UTC" string in addition to "GMT" string.

[stephentoub]

we should try and fix DateTime.TryParse() in the CoreClr to allow for "UTC" string in addition to "GMT" string.

cc: @tarekgh

[tarekgh]

I don't think supporting UTC is a good idea. it is not part of any standard and even it is better to get who is using it to fix such usage. I am seeing Twitter already did that which make sense. you may look at following link for some similar discussion

https://stackoverflow.com/questions/25658897/is-utc-a-valid-timezone-name-for-rfc-1123-specification

The issue is if we start to support such that case, users will wonder why we don't support any other zone abbreviations too which will be sane to have DateTime parser go this route. I suggest this can be supported by other library doing more custom things but not really in the core.

[karelz]

Alright, we had a discussion on Networking team about what to do here.
One one side: All major browsers support UTC time. On the other side, it goes against RFC and sites who used it already reverted it.

We decided to NOT take the change now. If we see another server hitting the same problem, we will reopen the PR dotnet/corefx#30104 and let it go through.

@golf1052 thanks a lot for your contribution and your help to land at a solution. Sorry, that we don't take your change here, I hope you understand our reasoning to not support corner case things which are not RFC-compliant and don't have large uptake in the wild. Thanks!