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

fix(android): Content FS support in PathHandler #629

Merged
merged 4 commits into from
Oct 17, 2024

Conversation

breautek
Copy link
Contributor

@breautek breautek commented Jul 18, 2024

Platforms affected

Android

Motivation and Context

Improves support for the webview asset loader Path handling for easier DOM access,
especially for content:// urls.

Description

There are 3 main changes:

  1. Assets filesystem check is unsafe

targetFileSystem == "assets";

does not do what might have been expected, the condition will always yield false.
This is because in Java, comparing an object to a string literal using a == operator is a reference equality check. That is it's not testing if the values are equal, it's testing if they are the same object. Because a string literal is always making a new object to evaluate this line, it will always be false.

This was corrected to use .equals which does a value equality check.

  1. Support content filesystem.

The existing path handler supported many different filesystems, but content was missing. Adding support for the content filesystem required a few other changes.

Including:

  • Adding the mapping
  • Parsing path segments (which is a decoded form) and re-encoding them for content paths. This is important because the content path must be rebuilt exactly how the Android OS originally produced it, otherwise it will produce permission issues.
  • Replacing raw file API usage with CordovaResourceApi which intelligently checks for the uri type and calls the appropriate API to handle reading the resource.

Testing

Ran npm test.

Paramedic tests isn't running locally and I'm not sure why, so we'll see if it runs on the CI.

I also have a test project that I used to test this change specifically:

cameraTest.zip

Note: The camera plugin was tested with apache/cordova-plugin-camera#889 PR.

Note: The failing iOS change is not a regression of this PR, as can be observed it was failing on the 8.1 release. Not sure when that test started failing.

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

@breautek breautek marked this pull request as draft July 19, 2024 15:56
@lorenzodallavecchia
Copy link

Is there still something preventing the merge, apart from failing tests?
This PR is needed to comply with the new media permission guidelines (see apache/cordova-plugin-camera#866), and the October 31 deadline is approaching.

@breautek
Copy link
Contributor Author

Last call for review.

iOS test is failing but it has been failing for some time now, including on our 8.1.0 release therefore I have reasonable belief this PR will not make that situation worse.

Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

At a quick glance, this looks okay to me. Not going to "approve" it since I don't have all the background info and didn't look at it in detail, but no concerns as far as the code I see in the PR.

@lorenzodallavecchia
Copy link

I have been doing some manual tests on these changes.
All looks good except for one thing that we may have overlooked.

If an application is built with the AndroidInsecureFileModeEnabled option set to true in config.xml, the toURL method returns the content:// URL which is not accessible directly from WebView as @breautek found out (see apache/cordova-plugin-camera#889)).

Is there a way to have this working with AndroidInsecureFileModeEnabled?
In my case, I could just disable that flag, which I probably added as a temporary solution for a long-gone problem.

@breautek
Copy link
Contributor Author

If an application is built with the AndroidInsecureFileModeEnabled option set to true in config.xml, the toURL method returns the content:// URL which is not accessible directly from WebView as @breautek found out (see apache/cordova-plugin-camera#889 (comment)).

Is there a way to have this working with AndroidInsecureFileModeEnabled?
In my case, I could just disable that flag, which I probably added as a temporary solution for a long-gone problem.

I don't think so.

With AndroidInsecureFileModeEnabled enabled, the toURL passes through the underlying URL, which in this case is a content:// url. If the file entry has a native file path it will return a file:// url. So this is working as intended. Even if we resolve the content:// path to a file:// path (which is effectively what we are doing in our current releases), the file:// path is not usable on API 29 (due to the lack of FUSE support), and on API 30+ requires READ_EXTERNAL_STORAGE, or READ_MEDIA ON API 33+, which is the permissions we are trying to strip the requirement for.

AndroidInsecureFileModeEnabled was provided to buy developers time to migrate off of file-based protocol, since that can be an extensive change. That was introduced in Cordova-Android 10, which is when the WebViewAssetLoader implementation was introduced, in July 2021. If developers haven't done that migration yet in the 3 years, that's on them in my opinion.

I could add a note somewhere in the documentation for this caveat though.

lorenzodallavecchia added a commit to webratio/phonegap-plugin-file that referenced this pull request Oct 16, 2024
lorenzodallavecchia added a commit to webratio/phonegap-plugin-file that referenced this pull request Oct 16, 2024
This is a cherry-pick of a non-final implementation of PR
apache#629

WR-Issue: #17891
lorenzodallavecchia pushed a commit to webratio/phonegap-plugin-file that referenced this pull request Oct 16, 2024
This is a cherry-pick and back-port of a non-final implementation of PR
apache#629

WR-Issue: #17891
@breautek breautek merged commit 99ca439 into apache:master Oct 17, 2024
11 of 15 checks passed
@breautek breautek deleted the path-handler-improvements branch October 17, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants