-
-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug] [Android] Capacitor 4.3+ causes duplicate cookie header #492
Comments
@silviogutierrez Thanks for your solution. |
Hi @silviogutierrez , Thanks. Kr |
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: ionic-team/capacitor#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. |
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:
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:
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 |
@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. 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. |
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: ionic-team/capacitor#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
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 outCookieHandler.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.
The text was updated successfully, but these errors were encountered: