alexrainman / ModernHttpClient

ModernHttpClient
MIT License
126 stars 27 forks source link

Android implementation not performing path-check on Cookies #43

Closed phdonnelly closed 4 years ago

phdonnelly commented 4 years ago

Hi,

I'm updating an old code base built using the original modernhttpclient library and ran into some strange behaviour when migrating to this updated library. The problem I'm experiencing only happens in the android branch, and manifests as cookies being sent that should not be. I did some debugging and I believe I've found the root cause and a fix/workaround. It looks like the cookie-handling code is inadvertently only using the hostname for cookie matching and not doing any path-matching; this is incorrect behaviour per RFC 6265 and is causing issues in my app.

The root cause appears to be that the native java CookieStore.Get function being leveraged to filter cookies does not do any path-matching, and expects this matching to be done at a higher level. Per the openjdk source:

         //
         // for cookie purpose, the effective uri should only be http://host
         // the path will be taken into account when path-match algorithm applied
         //

There is a corresponding higher-level Get function in the native java CookieManager code that does perform the required path matching (as well as sorting and some other checks), however this code only returns a list of cookie strings with none of the parameters needed to create the OkHttp3 cookie objects used in the code.

As a fix/workaround I have created a simple function to perform path-matching per the RFC, and call this function to check cookie paths before adding them to the cookie collection in the LoadForRequest function. Note that there is actually a native java pathMatches function that performs essentially the same function already present in CookieManager.java, but it is not accessible due to its protection level.

I'll create a pull request shortly with the code.

alexrainman commented 4 years ago

Awesome.

phdonnelly commented 4 years ago

https://github.com/alexrainman/ModernHttpClient/pull/44 created

alexrainman commented 4 years ago

Merged. Will be out in next release.