zulip / zulip-mobile

Zulip mobile apps for Android and iOS.
https://zulip.com/apps/
Apache License 2.0
1.29k stars 651 forks source link

Resource filename collision between rn-fetch-blob and react-native-image-picker #5062

Open gnprice opened 3 years ago

gnprice commented 3 years ago

This is an issue I just noticed while looking at our built manifest in Android Studio with @chrisbobbe. It's an incompatibility between two libraries we use -- rn-fetch-blob and react-native-image-picker -- due to some poor API design by Android upstream, then some pretty roundabout documentation of that API. Then both of those libraries choosing a rather generic filename, but I don't really blame them because it's not at all obvious that these names end up living in a common namespace and having the opportunity to collide.


The issue is that each of these libraries has a <provider> element in its Android manifest fragment, referring to different subclasses of androidx.core.content.FileProvider; and then an essential piece of that element is where it specifies an android.support.FILE_PROVIDER_PATHS argument to the provider; and those look like this:

        <provider
            android:name="com.RNFetchBlob.Utils.FileProvider"
            android:authorities="${applicationId}.provider"
            android:exported="false"
            android:grantUriPermissions="true">
            <meta-data
                android:name="android.support.FILE_PROVIDER_PATHS"
                android:resource="@xml/provider_paths" />
        </provider>

      <provider
            android:name="com.imagepicker.FileProvider"
            android:authorities="${applicationId}.provider"
            android:exported="false"
            android:grantUriPermissions="true">
            <meta-data
                android:name="android.support.FILE_PROVIDER_PATHS"
                android:resource="@xml/provider_paths" />
        </provider>

One provides a value for that argument as an XML resource file.

And then the problem is that they've both picked the same name for that resource file: @xml/provider_paths, which corresponds to a filename res/xml/provider_paths.xml. So they both end up passing the same arguments to that FileProvider -- whatever the contents of that XML resource file end up being.

I don't know (haven't yet looked) whether one or the other file wins, or whether they get merged. But the files that the two libraries have by that name have different contents:

$ head -20 node_modules/{rn-fetch-blob,react-native-image-picker}/android/src/main/res/xml/*
==> node_modules/rn-fetch-blob/android/src/main/res/xml/provider_paths.xml <==
<?xml version="1.0" encoding="utf-8"?>
<paths xmlns:android="http://schemas.android.com/apk/res/android">
    <external-path
        name="external_files"
        path="." />
    <files-path
        name="files-path"
        path="." /> <!-- Used to access into application data files -->
    <cache-path
        name="cache-path"
        path="." /> <!-- Used to access files in cache directory -->
</paths>

==> node_modules/react-native-image-picker/android/src/main/res/xml/provider_paths.xml <==
<?xml version="1.0" encoding="utf-8"?>
<paths xmlns:android="http://schemas.android.com/apk/res/android">
  <files-path name="shared" path="."/>
  <external-path name="shared" path="."/>
  <external-files-path name="shared" path="."/>
  <root-path name="root" path="."/>
</paths>

So whatever the contents end up being, it can't be the intended contents for both of them.


I expect the user-visible symptom of this issue will be that when one or both of those libraries is used to share some content (like a downloaded image) from Zulip to another app, it won't work.

Specifically, what'll happen is that the library generates some content URL. The FILE_PROVIDER_PATHS argument is meant to configure how those content URLs get translated into actual filesystem paths. The content URL that gets generated will be one that according to that library's intended provider_paths.xml file would translate to the filename it intended, the one where the intended file actually is… but then because the wrong provider_paths.xml gets used, the URL will actually be invalid, not translating to any filename at all. (In principle it could translate to some wrong filename; but I believe that because these two configurations happen to not define any of the same names, it'll always just fail instead.) When the receiving app tries to read from that content URL, it'll get an error because the URL is invalid according to the provider's actual configuration, the actual contents of xml/provider_paths.xml.

This is potentially the cause of #2122 and/or #3796. -> Nope, on actually reading those issue descriptions, they're not related to this: they're about the fact that we don't try to share the image on a certain UI flow where you might think we would. We just "share" the message text from around where the image was.

gnprice commented 3 years ago

I just went and tried to share a file from Zulip, using the only flow that comes to mind where we offer to let you do that… and it worked just fine. So it may be that this is currently having no user-visible impact at all. I'll leave this issue open, though: it seems like at least a latent bug, and if in the future we run into symptoms that this issue could explain, then having it here may be helpful.

Specifically, the UI flow I tried was "Share" from the lightbox. What I did was:


Oh, but now on staring at our code here and thinking a bit more about how this API can work, I think I understand why this doesn't completely break things. It's still a latent bug, but I'm no longer so puzzled why it doesn't have visible symptoms.

What's happening is that when you do that share-from-lightbox, it goes through our ShareFileAndroid.kt, and that says this:

        val fileProviderAuthority = reactApplicationContext.packageName + ".provider"
        val sharedFileUri: Uri = FileProvider.getUriForFile(
            reactApplicationContext, fileProviderAuthority, File(path))

And then that getUriForFile method translates the file path (in that File object we just constructed with File(path)) into a URL.

And, crucially, it uses the same configuration (the same value of android.support.FILE_PROVIDER_PATHS) to turn the file path into a URL, as it'll use to turn the URL into a file path when the receiving app tries to use it.

So even though that configuration is the wrong configuration -- or anyway not one that was intended -- it's the same configuration at both ends. So as long as the given configuration has some way to encode the path as a URL, it'll successfully get encoded and then decoded right back.

And both of these xml/provider_paths.xml files have pretty broad paths that expose wide swaths of files, like:

    <files-path
        name="files-path"
        path="." /> <!-- Used to access into application data files -->

  <files-path name="shared" path="."/>

So the configuration it ends up with does have some way to encode the path as a URL, and it encodes it that way, and then the URL can be decoded back.

gnprice commented 3 years ago

One other anomaly I forgot to mention yesterday, which is actually what started me looking at this:

In Android Studio, if you look at our manifest file in the "Merged Manifest" view, there are four different <provider> elements for four different versions of FileProvider from different libraries, including the two mentioned above… but then if you look at where Android Studio is saying the elements come from, while the <provider> elements themselves are shown as coming from those four different libraries, in each case the <meta-data> element inside it is shown as coming from react-native-image-picker.

That is, the element that looks like this:

            <meta-data
                android:name="android.support.FILE_PROVIDER_PATHS"
                android:resource="@xml/provider_paths" />

inside the <provider> that comes from rn-fetch-blob is shown as coming from react-native-image-picker, just like the identical <meta-data> element that's inside the <provider> that comes from react-native-image-picker. But then so is the different-looking element

            <meta-data
                android:name="android.support.FILE_PROVIDER_PATHS"
                android:resource="@xml/file_provider_paths" />

that's inside the provider that comes from (IIRC) react-native-webview. Ditto a <meta-data> with @xml/file_system_provider_paths, in the provider that comes from expo-something. All four of those <meta-data> elements are shown as coming from react-native-image-picker, even though their parent <provider> elements are shown as coming from four different libraries.

This indication is seen in both places the UI here says where an element comes from: in the background colors for these elements in the "Merged Manifest" pane, and also in text in the "Merging Log" section in the pane next to that.

I don't have an explanation for why it's saying that about the two other <meta-data> elements. It seems like it probably has to involve some kind of bug in this IDE feature -- those elements do have the names file_provider_paths and file_system_provider_paths in them, and those names do appear in the respective places those <provider> elements come from but don't appear in react-native-image-picker, so it seems like they can't actually be coming from there.

Anyway. This all still doesn't appear to have any user-visible symptoms, and I don't think it's super likely to break -- and if it does someday break, this isn't a critical piece of functionality so it's OK (compared to our existing, known bugs) if it breaks and then we fix it. So having recorded what I know, I plan to stop thinking about this issue until we discover a problem it turns out to cause, which might be never.