fsprojects / FSharp.Data

F# Data: Library for Data Access
https://fsprojects.github.io/FSharp.Data
Other
812 stars 287 forks source link

Http.Request fails with "part of the cookie is invalid" on VSTS API #1040

Open kberridge opened 7 years ago

kberridge commented 7 years ago

When I query the Visual Studio Team Services (VSTS) API:

Http.RequestString("https://organization.visualstudio.com/DefaultCollection/Project/_apis/wit/queries", headers = [ "Authorization", "Basic base64encodeduseraccesstoken"])

I get the following error:

Unhandled Exception: System.Net.CookieException: The 'Value'='{"PersistentSessionId":"3cd7392c-35f2-4fac-a31a-533d33d6ca6f","PendingAuthenticationSessionId":"0
0000000-0000-0000-0000-000000000000","CurrentAuthenticationSessionId":"00000000-0000-0000-0000-000000000000"}' part of the cookie is invalid.
   at System.Net.Cookie.VerifySetDefaults(CookieVariant variant, Uri uri, Boolean isLocalDomain, String localDomain, Boolean set_default, Boolean isThrow)
   at System.Net.CookieContainer.Add(Uri uri, Cookie cookie)
   at <StartupCode$FSharp-Data>.$Http.InnerRequest@930-5.Invoke(WebResponse _arg2) in C:\Git\FSharp.Data\src\Net\Http.fs:line 938
   at Microsoft.FSharp.Control.AsyncBuilderImpl.args@825-1.Invoke(a a)

I switched to WebClient.DownloadString() and that is working. I also tried it in Postman hoping I could attach some more information about what the server is returning but Postman says the server didn't return a cookie.

I also found this possibly related open issue #982, but that error was "The 'Name'='' part of the cookie is invalid." which seemed different enough so I thought opening a new issue was the right move.

ovatsus commented 6 years ago

Hi @kberridge, do you want to try to put a breakpoint and see what is failing and try to fix it?

ovatsus commented 6 years ago

I did a similar request to my organisation VSTS and couldn't repro the bug. If you can still repro it, please provide the cookie value that breaks and reopen this issue

Peter360 commented 6 years ago

The sample at http://fsharp.github.io/FSharp.Data/library/Http.html Cause this error. `` open System.Net open FSharp.Data let cc = CookieContainer() let msdnUrl className = let root = "http://msdn.microsoft.com" sprintf "%s/en-gb/library/%s.aspx" root className

// Send a request to switch the language Http.RequestString ( msdnUrl "system.datetime", query = ["cs-save-lang", "1"; "cs-lang","fsharp"], cookieContainer = cc) |> ignore

// Request the documentation again & search for F# let docInFSharp = Http.RequestString ( msdnUrl "system.web.httprequest", cookieContainer = cc ) docInFSharp.Contains "F# `` The exception System.Net.CookieException: The 'Value'='ms310241(n)/aa139615(n)/aa139616(n)/mt502185(VS.110,n)/mt507958(VS.110,n)/mt521147(VS.110,n)/' part of the cookie is invalid.

FSharp.Data 2.4.6

ovatsus commented 6 years ago

@Peter360, can you check if it's working with 3.0? There was a cookie related fix

Peter360 commented 6 years ago

Yes the problem persists.

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="Program.fs" />
  </ItemGroup>

  <ItemGroup>
    <PackageReference Include="FSharp.Core" Version="4.3.4" />
    <PackageReference Include="FSharp.Data" Version="3.0.0-beta3" />
  </ItemGroup>

</Project>

Unhandled Exception: System.Net.CookieException: The 'Value'='ms310241(n)/aa139615(n)/aa139616(n)/mt502185(VS.110,n)/mt507958(VS.110,n)/mt521147(VS.110,n)/' 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.Add(Uri uri, Cookie cookie) at <StartupCode$FSharp-Data>.$Http.InnerRequest@1436-5.Invoke(WebResponse _arg2) in C:\Git\FSharp.Data\src\Net\Http.fs:line 1444 at Microsoft.FSharp.Control.AsyncBuilderImpl.args@506-1.Invoke(a a) --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() at Microsoft.FSharp.Control.AsyncBuilderImpl.commit[a](AsyncImplResult1 res) at Microsoft.FSharp.Control.CancellationTokenOps.RunSynchronouslyInCurrentThread[a](CancellationToken token, FSharpAsync1 computation) at Microsoft.FSharp.Control.CancellationTokenOps.RunSynchronously[a](CancellationToken token, FSharpAsync1 computation, FSharpOption1 timeout) at Microsoft.FSharp.Control.FSharpAsync.RunSynchronously[T](FSharpAsync1 computation, FSharpOption1 timeout, FSharpOption`1 cancellationToken) at Program.main(String[] argv) in Program.fs:line 16

devcrafting commented 6 years ago

Hi,

I got same error with the following Value : {"echanges":1,"notifications":1,"messages":1} (on another website). Seems JSON is not well supported in Cookie value, perhaps it needs escaping ?

I created a unit test to highlight the issue (cf. commit link below). I am not sure about how to fix it...seems comma at least is an issue, we could escape, but then will it be usable in next requests... Not sure and can't test, since the site I'm testing works even without this cookie...

devcrafting commented 6 years ago

From here : https://github.com/Microsoft/referencesource/blob/c0e8bf3fd2d79d1e73286bc384ba057b7e7b601e/System/net/System/Net/cookie.cs

Reserved2Value =  new char[] {';', ',' };
....
if (m_value == null ||
                (!(m_value.Length > 2 && m_value[0] == '\"' && m_value[m_value.Length-1] == '\"') && m_value.IndexOfAny(Reserved2Value) != -1)) {
                if (isThrow) {
                    throw new CookieException(SR.GetString(SR.net_cookie_attribute, "Value", m_value == null? "<null>": m_value));
                }
                return false;
            }

So need to escape ; and , if Value does not start and end with "...very weird that we need to handle that (!?!)...

devcrafting commented 6 years ago

A bit embarassed because the 2 examples we have do not even exactly conform to RFC : http://www.rfc-editor.org/rfc/rfc6265.txt It should be URL encoded...but not sure the server will be ok with URL encoded values when sent back to it?!?

cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
 cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash
ovatsus commented 6 years ago

@devcrafting , what solution do you propose for this?

devcrafting commented 6 years ago

I would try to URL encode the cookie value. But as I said, I am a bit embarassed that servers of the examples we have do not conform to cookie RFC. And also, I don't have any real life test case since the server using the example I gave don't care about the cookie, so if I send it a value it does not like, I will not see it easily :/.

devcrafting commented 6 years ago

I tried Url encoding, it fails several tests, not sure it is a good idea, it would introduce breaking changes for those who use their CookieContainer.

A workaround would be to not use CookieContainer in my case. I used it only for convenience of not managing cookies from one request to another. Still there is an issue in this code since CookieContainer is used even if not passed as parameter :

match headers.TryFind HttpResponseHeaders.SetCookie with
            | Some cookieHeader ->
                CookieHandling.getAllCookiesFromHeader cookieHeader resp.ResponseUri
                |> Array.iter cookieContainer.Add
            | None -> ()

            let cookies = Map.ofList [ for cookie in cookieContainer.GetCookies uri |> Seq.cast<Cookie> -> cookie.Name, cookie.Value ]

I would say that if CookieContainer is not passed in arguments, we don't need to cookieContainer.Add response SetCookies to it. We would directly fill the cookies and then add them to CookieContainer if one was given in argument. What do you think ?

We could also try..with the CookieException with an additional argument to ignore this error ? (like IgnoreCookieException ? with false as default)

devcrafting commented 6 years ago

I proposed a PR to solve some issues and that could help some people. Perhaps @kberridge could have a look if the proposed solution could be enough?