Skip to content
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

breaking: implement WebViewAssetLoader #1137

Merged
merged 15 commits into from
Apr 22, 2021

Conversation

NiklasMerz
Copy link
Member

@NiklasMerz NiklasMerz commented Nov 30, 2020

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

  • Set hostname
  • Finish hook integration like iOS
  • Docs

Closes #1135

ionic-team/cordova-plugin-ionic-webview#483

Testing

Unit Tests TODO!

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@jcesarmobile
Copy link
Member

avoids CORS problems

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

@NiklasMerz
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

NiklasMerz commented Dec 1, 2020

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
Copy link
Member

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
Copy link
Member

erisu commented Dec 1, 2020

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
Copy link
Member

dpogue commented Dec 1, 2020

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Contributor

breautek commented Dec 1, 2020

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
Copy link
Member

dpogue commented Dec 1, 2020

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
Copy link
Member Author

@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
Copy link

codecov-io commented Dec 3, 2020

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.

@NiklasMerz NiklasMerz added this to the 10.0.0 milestone Dec 3, 2020
@NiklasMerz NiklasMerz marked this pull request as ready for review December 7, 2020 09:26
@breautek breautek mentioned this pull request Dec 19, 2020
2 tasks
@NitzDKoder
Copy link

@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
Copy link
Contributor

breautek commented Mar 4, 2021

@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.

Copy link

@knight9999 knight9999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is ok for me. 😊

@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2021

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%) ⬆️

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.

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM 👍

@NiklasMerz NiklasMerz merged commit 5e7be8e into apache:master Apr 22, 2021
@dpogue
Copy link
Member

dpogue commented Apr 22, 2021

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
Copy link
Member Author

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
Copy link
Member

dpogue commented Apr 22, 2021

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
Copy link
Contributor

breautek commented Apr 22, 2021

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
Copy link
Member

erisu commented Apr 23, 2021

@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.

breautek pushed a commit to breautek/cordova-android that referenced this pull request Apr 24, 2021
Implement AndroidX WebViewAssetLoader with hook for plugins


Co-authored-by: エリス <[email protected]>
breautek pushed a commit to breautek/cordova-android that referenced this pull request Apr 25, 2021
Implement AndroidX WebViewAssetLoader with hook for plugins


Co-authored-by: エリス <[email protected]>
@NiklasMerz
Copy link
Member Author

wedgberto pushed a commit to wedgberto/cordova-android that referenced this pull request May 17, 2022
Implement AndroidX WebViewAssetLoader with hook for plugins


Co-authored-by: エリス <[email protected]>
@boedy
Copy link

boedy commented Jun 7, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement 'WebViewAssetLoader'
10 participants