search

apache / cordova-android

Apache Cordova Android
https://cordova.apache.org/
Apache License 2.0
3.59k stars 1.52k forks source link

breaking: implement WebViewAssetLoader #1137

Closed NiklasMerz closed 3 years ago

NiklasMerz commented 3 years ago

This is a breaking change since it requires the AndroidX dependency in cordova-android core

Platforms affected

Android

Motivation and Context

The Android Webview team suggests to use the WebViewAssetLoader to load local files. This PR is work in progress to show if this is possible and what needs to be considered.

This has some significant benefits over using the file: scheme. Having a 'proper' origin makes routing for frameworks like Angular work etc.

Description

TODO

Closes #1135

https://github.com/ionic-team/cordova-plugin-ionic-webview/issues/483

Testing

Unit Tests TODO!

Checklist

jcesarmobile commented 3 years ago

avoids CORS problems

Really? When you make server requests? Or for local requests only?

NiklasMerz commented 3 years ago

avoids CORS problems

Really? When you make server requests? Or for local requests only?

Maybe I confused this with iOS but in some cases requests from the file protocol to servers have CORS issues. This is just one of the benefits. What are the other problems the Ionic webview solves with the local server?

jcesarmobile commented 3 years ago

CORS is a WKWebView problem, doesn't matter if you use file or a custom protocol.

In Android in the other hand, using file will ignore server CORS, while using a custom protocol or http/https will require to allow the origin on the server because it's affected by CORS.

benefits of ionic-webview plugin were more about the routing, and at the beginning when it uses a local server on iOS, it allowed to use http://localhost as origin on both iOS and Android.

NiklasMerz commented 3 years ago

Thank you. I got confused with Android and iOS quirks. There are some web APIs that need a https origin to work right? Maybe that's another benefit.

My motivation to try that out, was that I need that origin for routing and I tried to build a proxy into that again like I did for iOS. The WebViewAssetLoader has similar capabilities like the WKURLSchemeHandler on iOS and I try to integrate this as similar as possible.

The 'proxy' is to solve CORS and cookie issues and can live in a fork or the Ionic or custom webview as well. The proxy plugin can be found here: https://github.com/GEDYSIntraWare/cordova-plugin-webview-proxy. This is just my personal requirement and it should not justify adding this breaking change to cordova-android, alone..

The hook for plugins to plug into the assetloaders path handlers is work in progress and I will change that again.

Besides that, are there reasons for and against implementing the WebViewAssetLoader into cordova-android?

jcesarmobile commented 3 years ago

I have not used file for a long time, but as far as I remember android considered file safe, so all APIs worked, but things might have changed. They also work on http://localhost (default on ionic webview)

I was under the impression that WebViewAssetLoader didn’t allow to “intercept” the requests, I thought you could only point to a folder in assets and it would load whatever is there, but using a proxy wouldn’t be possible, glad to be wrong.

I think we are going to need this or some other approach because when you target SDK 30 you can’t fetch file urls anymore https://bugs.chromium.org/p/chromium/issues/detail?id=1101250

So unless somebody proposes an alternative, this is probably the only way

erisu commented 3 years ago

Besides that, are there reasons for and against implementing the WebViewAssetLoader into cordova-android?

No reasons against implementing. In fact, it is a must implement feature.

Google has disabled allowing access to file by default. Before, in earlier APIs, it was allowed as default. We can re-enable file access with the setAllowFileAccess method, which I had already committed in master to fix future issues coming from API 30.

Enabling this setting allows malicious scripts loaded in a file:// context to launch cross-site scripting attacks, either accessing arbitrary local files including WebView cookies, app private data or even credentials used on arbitrary web sites.

Additionally, it is generally discouraged to load from file protocol, hence the reason they disabled this.

Setting the file access back to true was only a quick temporary fix. I was already planning to implement the WebViewAssetLoader, which is the replacement solution.

Some methods are already deprecated starting from API 30. For example the setAllowUniversalAccessFromFileURLs method was deprecated. This setting was not secure, and is also recommend to use androidx.webkit.WebViewAssetLoader to load file content securely, in its replacement.

dpogue commented 3 years ago

I think we do want to do this, to resolve the issues with file URLs and API 30 and security concerns, but if we're adding an AndroidX dependency then we should also just move to the AndroidX WebView instead of the system one. Adding an AndroidX dependency will also require a major version bump, and will break any existing plugins that use the Android Support Libraries (unless a tool like Jetifier is used).

I think a good way to prototype this would be to make a new pluggable webview plugin for the AndroidX webview, that can be installed in existing versions of cordova-android for testing, and then bring it in either as part of the next major or as a cordova-androidx platform (as we'd discussed in one of the contributor meetings).

jcesarmobile commented 3 years ago

I don't think AndroidX is as big of a problem as it sounds. The idea was to make it enabled by default in cordova-android 9, but was changed on last minute.

Plugins that don't use AndroidX is mainly because they are unmaintained, or because it's hard to use it when cordova-android doesn't enforce it's usage. A few of them are maintaining 2 versions at the same time, one with AndroidX and other with the old support libraries. So current status overloads maintainers that maintain the plugins for not breaking unmaintained plugins.

Anyway, with jetifier or dave alden's plugins the problem is solved, in Capacitor 2 we moved to AndroidX and there have not been any problems with the cordova plugin compatibility, we just tell users to use jetifier.

AndroidX apart, this is still a breaking change, since changing from file to https will mean data loss, so yeah, this should be targeted for next major.

NiklasMerz commented 3 years ago

I pushed some more breaking changes with APIs that got deprecated in API level 30. Lets test it out and see if we can get rid of them.

@jcesarmobile Maybe you could help me with routing for single page apps? I am not sure how to do that properly since the WebViewAssetLoader behaves a bit differently. So my Angular has a page /login. I open the app it starts at /index.html and everything is fine. If I reload the page it tries to load /login. How could we solve that?

breautek commented 3 years ago

So my Angular has a page /login. I open the app it starts at /index.html and everything is fine. If I reload the page it tries to load /login. How could we solve that?

I think I can pitch in here. I'm not sure if this is something that Cordova should try to solve... but for now, let's assume that Cordova should. In a "traditional" web server setup... you'd have a redirect rule so when virtual url routes are requested.. it always serves the index.html page (because it's the client that handles the routing). I'm not sure if WebAssetLoader can easily mimic that without making too many assumptions about the app, after all -- this is really an application detail.

On another note... In Cordova apps, you don't see the URL ever -- it should be perfectly fine to use hash-bashed routing strategies. Is there a strong reason why we should support virtual urls?

dpogue commented 3 years ago

I'm not sure if this is something that Cordova should try to solve...

Cordova definitely should not try to solve this. We don't want to be responsible for a URL rewriting layer on top of the filesystem. Hash-based routing is the only option for Cordova apps serving from local files.

NiklasMerz commented 3 years ago

@breautek @dpogue Thanks for your points. I agree that we should not have this rewrite in Cordova.

This means there would probably be a (third-party) plugin that hooks into the WebViewAssetHandler and does the rewrite for people that need need it.

codecov-io commented 3 years ago

Codecov Report

Merging #1137 (10ba677) into master (2a84d7c) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1137   +/-   ##
=======================================
  Coverage   71.98%   71.98%           
=======================================
  Files          21       21           
  Lines        1692     1692           
=======================================
  Hits         1218     1218           
  Misses        474      474

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2a84d7c...10ba677. Read the comment docs.

NitzDKoder commented 3 years ago

@erisu @dpogue @NiklasMerz @breautek Any time cordova is adopting to WebAssetLoader usage? Please confirm. My app uses file:// kind of image display and setAllowFileAccess(true) is temp fix for me.

https://bugs.chromium.org/p/chromium/issues/detail?id=1101250#c7

breautek commented 3 years ago

@erisu @dpogue @NiklasMerz @breautek Any time cordova is adopting to WebAssetLoader usage? Please confirm. My app uses file:// kind of image display and setAllowFileAccess(true) is temp fix for me.

https://bugs.chromium.org/p/chromium/issues/detail?id=1101250#c7

I think our current goals is to be completely on AndroidX for version 10, which will allow us to also incorporate WebAssetLoader.

NiklasMerz commented 3 years ago

@knight9999 Thanks for the review. I agree and applied your changes.

codecov-commenter commented 3 years ago

Codecov Report

Merging #1137 (5b8af79) into master (2a84d7c) will decrease coverage by 0.00%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1137      +/-   ##
==========================================
- Coverage   71.98%   71.97%   -0.01%     
==========================================
  Files          21       21              
  Lines        1692     1695       +3     
==========================================
+ Hits         1218     1220       +2     
- Misses        474      475       +1
Impacted Files Coverage Δ
bin/templates/cordova/lib/prepare.js 47.94% <0.00%> (+0.17%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2a84d7c...5b8af79. Read the comment docs.

dpogue commented 3 years ago

What does this mean for apps with existing data in localStorage or indexedDB? That data will be "lost" if the origin changes from file:/// to https://localhost, right?

NiklasMerz commented 3 years ago

Yes that's right. Switching the from file to http://localhost changes the origin and you cannot access the web storage anymore. This was already the case for the ioc platform with the changes around UIWebView to WKWebView, WKUrlSchemeHandler etc. We need to announce that with the release post and document it.

I don't know if many are still using file: because of the many limitations. Probably a lot are using Ionics WebView and with this change and the past changes in iOS it's not really needed anymore. I could prepare a blog post/doc how to migrate away from Ionics WebView.

dpogue commented 3 years ago

Anyone using default Cordova-Android will still be using file:///. On iOS we ended up keeping file:/// by default and only using the new scheme if it was declared in config.xml, but given the changes in API 31 that might not be feasible on Android.

We should consider if there's any way to automatically migrate data in this case. A lot of apps aren't in a position where they'll be able to upgrade if it means data loss.

breautek commented 3 years ago

I have a plugin that I used to migrate from crosswalk local storage containers to the normal webview storage container under an http://localhost origin

I'm sure it could be adapted for this specific migration.

Chrome has two different storage containers that is used, depending on the webview version (i think the switch happened around chrome 78 if i recall correctly).

So the plugin will definitely have to be adapted to look for a leveldb database under the old origin, and if missing then fallback to the sqllite database.

Chrome (unless if they have changed it since Chrome 80ish) will auto migrate sqllite databases to leveldb, so we don't need to worry about converting the database ourselves.

I have no idea if indexeddb follows the same pattern though.

https://github.com/totalpave/cordova-plugin-crosswalk-data-migration/tree/totalpave/master

erisu commented 3 years ago

@breautek we can maybe integrate that plugin into the cordova-android platform as a core feature and handle all the various types of data migration (cookies, indexdb, etc..) from file:// to http://localhost. And also introduce the migration configs, in config.xml, to handle the various migration paths over time. I recall mentioning this as an idea for the plugin in the past.

NiklasMerz commented 3 years ago

I have created an e-mail thread for this please respond there: https://lists.apache.org/thread.html/rdf5efb2f33b1ced770297326c4f9fb87a1123d93f5ca0b271b8797c7%40%3Cdev.cordova.apache.org%3E

boedy commented 1 year ago

I'm currently not able to upgrade to version 10 as I have not yet found a way to migrate IndexedDB storage. Copying the contents of the old directories doesn't work unfortunately: From:

/data/user/0/<app_id>/app_webview/Default/IndexedDB/file__0.indexeddb.leveldb
/data/user/0/<app_id>/app_webview/Default/IndexedDB/file__0.indexeddb.blob

to:

/data/user/0/<app_id>/app_webview/Default/IndexedDB/https_localhost_0.indexeddb.leveldb
/data/user/0/<app_id>/app_webview/Default/IndexedDB/https_localhost_0.indexeddb.blob

Haven't figured out yet why it doesn't work. Has anyone managed to successfully migrate indexedDB?