silkimen / cordova-plugin-advanced-http

Cordova / Phonegap plugin for communicating with HTTP servers. Allows for SSL pinning!
MIT License
391 stars 313 forks source link

[Bug] [Android] Capacitor 4.3+ causes duplicate cookie header #492

Open silviogutierrez opened 1 year ago

silviogutierrez commented 1 year ago

Describe the bug I fully understand this plugin is built for Cordova, not Capacitor. But the latter is wildly popular and passed Cordova in usage a while ago. A lot of people — myself included — love this plugin. And consider it a must-have for Capacitor applications.

Capacitor 4.3 introduced a "native" fetch that automatically patches everything and bypasses CORS. But it has a litany of bugs, see here: https://github.com/ionic-team/capacitor/issues/6177

Moreover, the bugs have spilled over into this plugin. The cookie header is arbitrarily sent twice. Some backends "correct" this, but some don't, causing issues. Note: duplicate Cookie headers are not allowed in anything but HTTP/2, so a lot of backends are not expecting it. Instead they merge it with commas, and cookies are to be separated with semicolons, not commas.

I can't quite pinpoint it, but I can reproduce it. Something in their own CookieManager is broken. I know this sounds like a Capacitor problem, but the this area seems to be low priority for them and it's spilling over into other plugins.

System info

💊   Capacitor Doctor  💊

Latest Dependencies:

  @capacitor/cli: 4.6.1
  @capacitor/core: 4.6.1
  @capacitor/android: 4.6.1
  @capacitor/ios: 4.6.1

Installed Dependencies:

  @capacitor/cli: 4.6.1
  @capacitor/core: 4.6.1
  @capacitor/android: 4.6.1
  @capacitor/ios: 4.6.1

[success] iOS looking great! 👌
[success] Android looking great! 👌

    "cordova-plugin-advanced-http": "^3.3.1",
    "cordova-plugin-file": "^7.0.0",

Minimum viable code to reproduce https://github.com/silviogutierrez/cordova-plugin-advanced-http-duplicate-cookie

I have both a capacitor and Cordova version. The Cordova version is in myApp, build it as you would any Cordova project, and run the project. You'll see the header count is 1 and the test passes.

Now do the same with capacitor. Notice the failing test, and look at the server logs.

Everything is self contained.

More notes I figured the "native" capacitor plugin was messing things up, and I was right. If you edit the file node_modules/@capacitor/android/capacitor/src/main/java/com/getcapacitor/plugin/CapacitorCookies.java and comment out CookieHandler.setDefault(cookieManager);, things go back to working.

So clearly, this cookie handler is messing things up in this plugin as well. I don't know how, maybe it's obvious to others.

Proposed solution

Is there a way to isolate this plugin to use its own instance of a cookie manager in HttpUrlConnection? That way it remains "safe" from other cookie managers?

Thanks again for all your work on this plugin. I've used it reliably for years and bypassing CORS is a huge benefit.

Mani-VD commented 1 year ago

@silviogutierrez Thanks for your solution. After commenting out "node_modules/@capacitor/android/capacitor/src/main/java/com/getcapacitor/plugin/CapacitorCookies.java" . My site is connecting and also its 3X faster. But it must be resolved by capacitor team..

TiBz0u commented 1 year ago

Hi @silviogutierrez , Q1/ Do you still encounter the problem with Capacitor 5? Q2/ Do you encounter the problem even without enabling the Capacitor Cookies ? It's disabled by default See documentation See code

Thanks.

Kr

silviogutierrez commented 1 year ago

Honestly, the HTTP implementation in Capacitor is so buggy (affecting even outside code, like your awesome plugin), that I just gave up. Just to give you an idea of the bugs: https://github.com/ionic-team/capacitor/issues/6177 . This isn't exhaustive, there's more out there.

So I went in and patched everything using https://www.npmjs.com/package/patch-package and moved on.

When I upgrade to 5, I'll give a try. Thanks for all your work on this plugin. It's a lifesaver.

silkimen commented 11 months ago

Hi @silviogutierrez,

thank you for detailed analysis and sorry for very late answer! I just started to dig into this topic and I'm not an Capacitor user. Maybe you can help me better understanding this problem and finding a viable solution which does not necessitate patching Capacitor internals.

Summarising your findings:

  1. Cookie header is sent twice when using Capacitor
  2. Capacitor uses a native plugin implementation for handling Cookies
  3. HTTP implementation in Capacitor is broken and therefore you prefer to use my plugin

This plugin does NOT manage the cookies in the native code, but instead uses tough-cookie within the webview. When I started implementing this plugin back in 2016 (it all started as a fork of Wymsee's HTTP plugin) I also tried to implement cookie handling in the native layer, writing Objective C and Java code. But back then there were so many odd behaviours and inconsistencies in the native APIs, so I decided to stick to tough-cookie instead. I think that most of those native API problems are solved in the meantime, so it seems reasonable that Capacitor is using them for their HTTP stack.

I think the duplicate cookie header problem happens when you use this plugin and Capacitor's HTTP stack, because both handle their own cookie store and do not sync. They don't "know" that there is another cookie handling mechanism. So I see two options to fix this problem:

  1. Disable cookie handling in Capacitor (I don't know if this is possible - not being a Capacitor user myself)
  2. Disable cookie handling in this plugin (not possible currently - but if needed please file a feature request, won't be too much work to implement)

But will option 2 work out without introducing all the other problems into this plugin you were experiencing in Capacitor HTTP? I guess, we'd need to try.

@TiBz0u you referred to the documentation of Capacitor cookie which lets you enable/disable patching document.cookie, but does it also enable/disable native cookie storage?

silviogutierrez commented 11 months ago

@silkimen : interesting, I didn't realize you didn't touch native cookies at all. I don't know too much about the cookie system at the native layer, but I think Capacitor still latches on to your code. Ultimately, you may be using tough cookie but I think you use the native platform's HTTP functions. And those rely on a singleton / CookieManager that Capacitor manipulates. But only if you enable the native cookie management functionality.

See here: https://github.com/ionic-team/capacitor/blob/main/android/capacitor/src/main/java/com/getcapacitor/plugin/CapacitorCookies.java#L32

So you can see how it just injects itself into your requests. Java will just send them with these cookies now. And intercept responses and save them.

I honestly just gave up on the whole thing. But I think an actionable take away: warn people to disable all Cookie/Http features in Capacitor if using this plugin. That's what I had to do.

I don't dare touch 5.0 for now, so I can't report findings on that. Thanks again for all your work.