-
Notifications
You must be signed in to change notification settings - Fork 394
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
Two different apps using this plugin conflict, start showing each other #165
Comments
reverting to 1.2.1 fixed it. But looks like an issue with the 2.x branch. |
We (our company) have also experienced this with our apps that updated to 2.x. We had version ^2.0.2 in the config.xml so it probably used the newest instead (last week). Edit: experienced this only on ios. |
This happened to me also. I got a support request from a completely confused user about this (I would be too, if I hadn't stumbled on this issue report as well). I am tempted to revert to 1.2.1 but am worried about down-migration issues it might have from 2.x to 1.x, as 2.x is already in production. |
@masimplo makes a good point; whereas I saw this from two of my own apps, I strongly suspect it would apply to any two apps packaged using 2.x on any person's device. That's quite a nasty flaw and one that probably needs fixing with some degree of urgency. FWIW I was able to revert to 1.2.1 and I didn't see any side effects of that at all. |
We are aware of this issue and investigating. Thanks for hanging in there. |
Quick update. We've got this patched in a test branch, but two things to keep in mind. First, make sure you choose a unique port using the |
Just released v2.2.0 which includes the changes from #186 and should fix this issue. Closing the issue now but feel free to open if you run into problems. Thanks for the report! |
Just to be clear, what happens in 2.2.0+ if two apps are using the same port? It doesn't seem fixed to me if there's a random chance that my app won't run whatsoever because another app happens to have chosen the same port as me. Will there be some sort of global registry of port numbers for users of this plugin (joke!) |
There should be an option for this plugin to not use the server etc, and just load from file:// URLs. I believe this embedded server was added to workaround the cross-domain restrictions caused by WKWebView - but if an app does not run into those cross domain issues and does not want to open local port etc, it should be allowed as an option. Maybe it should also be the default option. |
I don’t disagree. We also need to consider HTML5 routing |
I am completely baffled that this is even possible. Besides the obvious confusion for the users when a random app opens in another app, my main concern is security. If I understand this correctly, it completely breaks the sandboxing of apps provided by the OS. It means other apps could access your browser storage, SQLite-storage, as well as all the files stored in the app directories under certain conditions. I hope this is addressed as soon as possible so apps are again 100% isolated from each other. I don't know if local authentication would fully solve this, or if it's better to go back to |
It does NOT break sand boxing. No data is shared, just the socket is open for a brief time where another app could ask your server to serve a file, which will fail as your server will only serve files if a secure key is passed that only your app knows about. Again, No state or data is accessible. We are looking into a better way forward however as running a server in general is not ideal. |
It DOES break sand boxing! By definition you are able to execute code and extract data inside the context of another app. You won't share localstorage or whatever is there in the javascript/webview world, however sand boxing is broken because you can read another app's data. Imagine a plugin writes in his native sandboxed FS a secret file and now because of a bug/misconfiguration of the webserver you can simply fetch that file. This is an issue and has an huge security impact making it unusable for every application that needs to deal with sensitive data. Even with authentication of each and every request sandboxing is broken the second you open a socket. For iOS this might be only for a short amount of time (and only under certain circumstances), but on Android sockets break sand boxing entirely. |
@dcale the android side of this plugin does NOT use a socket. It is a "fake" webserver that just handles requests from the webview. Sandboxing is not broken. |
ok, that's good to know and I'm a bit relieved. So sandboxing is not broken on Android. What about iOS though? I've seen that if you perform app switching of two hybrid apps using the same port, it will fail on the second app because it cannot bind to the port. Luckily no content is served, but it still means that the port space is not sand boxed and is global. |
Hi all, we are working on a better solution right now, which will involve another hotfix and then removing the webserver for a longer-term solution |
Do we have progress on this aspect @mlynch ? |
This issue seems to still exist on 2.2.3. In my scenario I have 3 apps for dev/staging/production, all of them have different App Ids. Any of them open fine on iOS 12, unless one of the other apps is also open in which case the App fails to load and simply shows a screen 'Error, unable to load app.' If I change WKPort to be 'unique' port numbers then the issue go away, but how do I know a different App (made by a different developer) on the users device isn't using the same 'unique' port number? Other issue with changing the WKPort number from 8080 is that I'll have to update several APIs to accept different origins (which doesn't really help with security, as I still have to allow 8080 for backwards compatibility of App users who haven't updated yet). |
2.2.3 locks down any security risk from this. We are working right now on removing the webserver completely. Will have more info when that is ready but it's a major priority. |
Glad to hear this issue is a priority. The workaround of changing WKPort to something 'unique' per app (e.g not having multiple apps on 8080) stops the issues I'm having, but due to the same-origin policy causes any data I have in IndexedDB to be lost on app update, which isn't ideal for me. At the risk of getting off topic, is there a simple way to persist this data across WKPort change? |
To fix this issue in our app, we forked the plugin and completely removed the web server from it. It worked well in our case and only required minor changes. For anybody interested, the fork is available here https://github.com/airgap-it/cordova-plugin-airgap-webview/ until ionic updates their plugin. |
Great @AndreasGassmann But will Ionic push/deploy work with your forked & updated plugin? |
No, we tried to keep it to the bare minimum, so we deleted those functions. Our fork works well for us and we are using it in production, but our goal is to migrate back to this plugin once the web server is removed. So if you rely on push/deploy, it's probably easier to stick with this plugin until the new version is out. |
@mlynch If the fix you are working on involves removing the web server, does that mean LocalStorage and IndexedDB will no longer be accessible from within the ionic-webview? If it is still available, will LocalStorage/IndexedDB data from 2.2.3 persist when upgrading? |
We're not sure yet, it seems we might be able to automatically migrate those over with some APIs in WKWebView. Still researching that... |
@mlynch any update on the issue so far. thanks |
any update on the issue so far. |
Yes work is complete and we are finishing testing to make sure it won’t break deploys as well. I hope we will have a release next week |
@mlynch I am a bit confused, the CHANGELOG mentions that this issue is fixed in 2.2.0 (2018-10-04), i.e. 2 months ago. But you said only 4 days ago:
So I do not quite understand, is CHANGELOG incorrect or it is actually done and you were referring to something else when you said you are finishing testing? Can you please clarify what exactly did 2.2.0 fix and what is still remaining to be done? Is it still a requirement that any 2 Ionic apps that use this plugin must use different (randomly chosen?) port numbers using WKPort parameter? We have multiple Ionic apps and are about to upgrade cordova-plugin-ionic-webview from 1.2.1 to latest and I want to fully understand the consequences (the README file is a bit sparse on the migration details). |
@tigrannajaryan I think v2.2.0 is related to this comment #165 (comment):
I think the more recent comment you referenced ( |
2.3.0 was released yesterday and it works exactly the same way as 2.2.5, but it has a new preference available If you set that preference to true, the web server won't be started for iOS 11+ and it will use For this it will use For iOS 10 it will continue using the web server because Sadly going from Now we are working on the 3.0 release where the web server goes away entirely and uses |
@jcesarmobile So does this mean we don't need to specify UNIQUE port number anymore? Would leave Thanks for the quick fix btw!! |
If you use |
I see multiple concerns with the plan to use ionic://
Here is a feature request I opened on another cordova plugin, which recommends to use custom URL schemes which then created conflicts between multiple apps. Maybe this wont effect your plan to use WKURLSchemeHandler, but just something to be aware of:
Somehow I feel that the easiest option would work best for some apps (like ours), just loading from file://. At least an option, even if its not default. Something like:
|
Another question, if we just want to use file://, can we use https://github.com/apache/cordova-plugin-wkwebview-engine instead? Are there any features or improvements/fixes etc in this plugin other than that it does not use file:// ? |
Wouldn't another solution be to have the webserver check which ports are already occupied and then select a free one before launching the app? Or is this not possible? |
Definitely possible, and I think that is the correct solution! |
It’s possible to randomly choose a port and if already in use choose another one. However this is just another hack and not a long term solution. The main problem is that as soon as you use sockets on ios and android the sandbox is broken. You cannot have an entire framework that breaks the sandbox by design, because it renders the framework useless for every serious application that has a minimum of security requirements. The only viable solution which is not a hack is to remove everything that opens and communicates with local sockets. |
Correct. Also keep in mind sockets were never used on Android
On Mon, Dec 17, 2018 at 9:47 AM dcale ***@***.***> wrote:
It’s possible to randomly choose a port and if already in use choose
another one. However this is just another hack and not a long term
solution. The main problem is that as soon as you use sockets on ios and
android the sandbox is broken. You cannot have an entire framework that
breaks the sandbox by design, because it renders the framework useless for
every serious application that has a minimum of security requirements. The
only viable solution which is not a hack is to remove everything that opens
and communicates with local sockets.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#165 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAArzkBfzH7_pnuAokI2-Ia62ezoORQoks5u57x-gaJpZM4WdD2I>
.
--
Max Lynch
CEO/Co-founder, Ionic
max@ionicframework.com
|
I agree that anything really important should be saved somewhere other than localStorage/IndexedDB but all users losing this data on the same update isn't ideal (likewise changing the WKPort to something unique seems to cause anything there to be lost). I'm going to try changing to ionic:// scheme, but by the time a user is on that version it's too late to migrate any data from IndexedDB to somewhere more persistent as it's already gone! The best approach I can think of is do an app update staying on 8080... any users who update to that version I can migrate their data elsewhere, then in a few weeks time I can do an update to ionic:// and hope most users had run the release still on 8080 that did the migration. Feels pretty wrong but I can't think of any other options...
Maybe not a big deal for most people, but this would make using localStorage/IndexedDB, etc, pretty useless if the App can't even persist this data on relaunching, as when the port number changes this data is lost. |
I agree that a better solution is not to use local sockets, but not agree local sockets solution breaks the sandbox of iOS. The socket, which is just a service provided by the iOS operating system, is used for communication between processes of the same phone or nodes within that network. Since the ionic web server is just a internal one used by ionic web view, ionic web view just need to keep the access to the web server locally, then everything will be fine, no security issues. On the other hand, if using sockets breaks the sandbox, then all the apps that using sockets, like HTTP server, FTP server, even TCP/UDP clients, such as HTTP client, FTP client, all of them break the sandbox pattern. |
I see. Well in this case, that's no solution indeed...thanks for explaining! |
adding |
It should be fixed in 2.3.1 and also in master. Which plugin version are you using? how does your url looks like? |
I am using 2.3.1. |
We have migrated our production apps from cordova-plugin-ionic-webview to using both cordova-plugin-wkwebview-engine and cordova-plugin-wkwebview-file-xhr. The in-development apps are still using this plugin. Solves the CORS issues, and does not need to open ports or custom schema. Will return to this plugin when dust settles around these issues. |
That url looks good to me. If using Angular, check that it's not adding unsafe: to the url when you add it to the app, that will cause it to not work. You should use DomSanitizer and SafeResourceUrl or configure Angular to allow |
sanitizing the url did the trick. Thanks! |
steps to replicate:
then you see "app1" on the screen
The text was updated successfully, but these errors were encountered: