Skip to content

feat(android): Allow loading of local file using "https://cdvfile/" #322

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

Closed
wants to merge 3 commits into from

Conversation

syonip
Copy link

@syonip syonip commented May 29, 2019

Platforms affected

Android

Motivation and Context

Fixes issue #295 for Android platform, without requiring to set the WebView to a less secure mixed content mode.

Description

Allows loading of local file using "https://cdvfile/", which the browser sees as secure, instead of "cdvfile://" which the browser sees as unsecure.

Testing

npm test

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

@janpio janpio changed the title (android) fix mixed content error (#295) fix(android) fix mixed content error Jul 4, 2019
@janpio janpio changed the title fix(android) fix mixed content error fix(android): fix mixed content error Jul 4, 2019
@janpio janpio closed this Jul 4, 2019
@janpio
Copy link
Member

janpio commented Jul 4, 2019

Rerun tests.

@janpio janpio reopened this Jul 4, 2019
@janpio janpio changed the title fix(android): fix mixed content error feat(android): Allow loading of local file using "https://cdvfile/" Jul 4, 2019
@janpio
Copy link
Member

janpio commented Jul 4, 2019

This change should probably be added to the documentation/README somehow so people can discover it.

@janpio
Copy link
Member

janpio commented Jul 4, 2019

Thanks.

Now on to testing this, what does "in a WebView that is loaded over https" mean exactly? Navigate the webview to a website that uses https and then load a cdvfile URL? Or via InAppBrowser?

@syonip
Copy link
Author

syonip commented Jul 4, 2019

The former.
This is for a use-case of the cordova WebView loading a remote https site, but some of the resources are loaded from local files using cdvfile.

@janpio
Copy link
Member

janpio commented Jul 4, 2019

Ok, repro:

  • cordova create ...
  • cordova plugin add cordova-plugin-file
  • Add to config.xml:
     <allow-navigation href="http://*/*" />
     <allow-navigation href="https://*/*" />
    
  • Add to index.html:
     <div>
       <p>
          <a href="http://cordova.betamo.de/repro/cordovaFileCdvFileHttps.html">Go to http site</a><br>
          <a href="https://cordova.betamo.de/repro/cordovaFileCdvFileHttps.html">Go to https site</a>
        </p>
     </div>
    
  • Create http://cordova.betamo.de/repro/cordovaFileCdvFileHttps.html and https://cordova.betamo.de/repro/cordovaFileCdvFileHttps.html with content:
     cdvfile://localhost/assets/www/img/logo.png<br>
     <img src="cdvfile://localhost/assets/www/img/logo.png" style="height:200px; width:200px; background-color:red;" />
     <hr>
     https://cdvfile/assets/www/img/<br>
     <img src="https://cdvfile/assets/www/img/logo.png" style="height:200px; width:200px; background-color:red;" />
    
    (assets/www/img/logo.png is part of default app)

Observations:

  1. http => loads cdvfile:// successfully, second fails
  2. https => cdvfile:// fails because of mixed content, second fails

Test PR:

  • cordova plugin rm cordova-plugin-file
  • cordova plugin add https://github.com/syonip/cordova-plugin-file

Observations:

  1. http => loads both
  2. https => cdvfile:// fails because of mixed content, second works this time!

So I would say this works as designed, correct?


What tripped me up a bit is that the localhost of the usual cdvfile:// URL gets lost - I first had https://cdvfile/localhost in my cordovaFileCdvFileHttps.html. Does it maybe make for a cleaner, more obvious API if we say cdvfile://loclahost just becomes https://cdvfile/loclahost @syonip?

@syonip
Copy link
Author

syonip commented Jul 4, 2019

Yeah I guess you're right. Shall I make the change to https://cdvfile/loclahost?

@janpio
Copy link
Member

janpio commented Jul 4, 2019

Without the typo I introduced, then yeah ;)

(Similar discussion in the iOS PR which currently does a totally different thing: #296 (comment) - we have to unify on one pattern there.)

@guylando
Copy link

guylando commented Jul 4, 2019

I dont think https://cdvfile/ is a good idea as it is a malformed non-standard url in web context. We are talking about loading a standard https website in a webview so no reason to force that website to use https://cdvfile/ I think. The idea is that if the remote website loads in web context then it loads resources from remote server but if it loads SAME URL from inside the app webview then the app will intercept and return local content. It should work for normal urls.

@guylando
Copy link

guylando commented Jul 4, 2019

Furthermore, if we force the site to load content from a different domain, then we can get into other issues of cross domain requests. If we allow the content to be loaded from same domain as remote https site (intercepted loading) then we avoid those problems.

@syonip
Copy link
Author

syonip commented Jul 5, 2019

Done.

@guylando
Copy link

guylando commented Jul 5, 2019

This change will make this unusable I believe because of cross domain requests

@gwynjudd
Copy link

It seems to be working for me from https site. Are there any issues outstanding with this?

@janpio
Copy link
Member

janpio commented Jul 26, 2019

Yes, the iOS side and how these two compare: #295 (comment)

@gwynjudd
Copy link

OK thanks, please let me know if you have any testing. Would love to get this working at my end (only problem seems to be iOS for me right now)

@vitto32
Copy link

vitto32 commented Feb 22, 2020

I do prefer the idea from the iOS PR (#296) as it prevents CORS issues.

@breautek
Copy link
Contributor

The former.
This is for a use-case of the cordova WebView loading a remote https site, but some of the resources are loaded from local files using cdvfile.

We can't really support this kind of configuration. It's against the iOS terms to load in local resources from a remote site. Doing so is a security risk because one could load in the cordova.js file and thus have access to native APIs.

For iOS this breaks Apple's terms, section 4.7 which reads:

Apps may contain or run code that is not embedded in the binary (e.g. HTML5-based games, bots, etc.), as long as code distribution isn’t the main purpose of the app, the code is not offered in a store or store-like interface, and provided that the software (1) is free or purchased using in-app purchase; (2) only uses capabilities available in a standard WebKit view (e.g. it must open and run natively in Safari without modifications or additional software); your app must use WebKit and JavaScript Core to run third-party software and should not attempt to extend or expose native platform APIs to third-party software; ...

https://developer.apple.com/app-store/review/guidelines/#third-party-software

Google has similar text, however not well explained:

An app distributed via Google Play may not modify, replace, or update itself using any method other than Google Play's update mechanism. Likewise, an app may not download executable code (e.g. dex, JAR, .so files) from a source other than Google Play. This restriction does not apply to code that runs in a virtual machine and has limited access to Android APIs (such as JavaScript in a webview or browser).

https://support.google.com/googleplay/android-developer/answer/9888379?hl=en&ref_topic=9877467

Do note that the Cordova webview is a webview that explicitly provides the means to access native Android APIs, therefore the mentioned restriction regarding JS in the webview does not apply to Cordova.

For these reasons we cannot accept a PR that allows local access to files from remotely loaded sources.

@breautek breautek closed this Dec 17, 2020
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.

6 participants