expo / config-plugins

Out-of-tree Expo config plugins for packages that haven't adopted the config plugin system yet.
433 stars 92 forks source link

chore(networkConfig): Fix android crash on empty subdomains config #46

Closed chitezh closed 2 years ago

chitezh commented 2 years ago

Background

The app crashes when detox plugin is configured with an empty subdomain in cases where it's desired to allow all traffic.

Cause

      [
        "@config-plugins/detox",
        {
          "subdomains": []
        }
      ]

The configuration above generates an invalid xml resulting in a Caused by: [android.security.net](http://android.security.net/).config.XmlConfigSource$ParserException: No domain elements in domain-config at: Binary XML file line #3 as it generates

<?xml version="1.0" encoding="utf-8"?>
<network-security-config>

        <domain-config cleartextTrafficPermitted="true">

        </domain-config>

</network-security-config>

Fix

This PR inserts a base-config instead of a domain-config when subdomains is "*" to allow all traffic.

<?xml version="1.0" encoding="utf-8"?>
<network-security-config>
    <base-config cleartextTrafficPermitted="true" />
</network-security-config>
chitezh commented 2 years ago

Thanks for the quick review!

@ide what syntax would you prefer for all domains? a wildcard "*"?

On the latter question, maybe some more context here. We are developing against an expo development build that is distributed to all devs. It works when connecting from an emulator since 10.0.2.2 is whitelisted but throws an error when installed on a physical device connected from n IPs. Thus, we are needing to open up the traffic to make this work for all.

ide commented 2 years ago

Yes, I think "*" would be appropriate. The type of the option would then be subdomains: string[] | '*'. This way an empty array intuitively means "no domains" in which case we'd avoid creating the separate XML file and modifying AndroidManifest.xml altogether.

chitezh commented 2 years ago

@ide please take a look when you can

EvanBacon commented 2 years ago

Published in @config-plugins/detox@1.2.0, thanks for the great contribution!

AronBe commented 2 years ago

Great change, thank you @chitezh. But I believe your changes are causing a compilation error due to added whitespace in front of <?xml version="1.0" encoding="utf-8"?>

error

[stderr] Execution failed for task ':app:mergeDebugResources'.
[stderr] > A failure occurred while executing com.android.build.gradle.internal.tasks.Workers$ActionFacade
[stderr]    > Android resource compilation failed
[stderr]      /home/expo/workingdir/build/android/app/src/main/res/xml/network_security_config.xml:2: AAPT: error: XML or text declaration not at start of entity.
[stderr]          
[stderr]      /home/expo/workingdir/build/android/app/src/main/res/xml/network_security_config.xml: AAPT: error: file failed to compile.

whitespace code https://github.com/expo/config-plugins/commit/88e0cd0ccfdcc337e9c03f318d781aa1097e148b#diff-a2f1a6c9c7be0fdb09dae9c6c899118f81f1721591c97c54ecc76fbb80cf4dcbR32-R37