fix(cli): replace usesCleartextTraffic with network-security-config - #8572
Conversation
| if (networkSecurityConfigValue) { | ||
| await ensureDir(networkSecurityConfigDir); | ||
| await writeFile( | ||
| networkSecurityConfigPath, | ||
| `<?xml version="1.0" encoding="utf-8"?> | ||
| <network-security-config> | ||
| <base-config cleartextTrafficPermitted="true" /> | ||
| </network-security-config> | ||
| `, | ||
| ); | ||
| } else if (await pathExists(networkSecurityConfigPath)) { | ||
| await remove(networkSecurityConfigPath); | ||
| } |
There was a problem hiding this comment.
Because se are not writing to the app's manifest directly (but maybe we might need to, in relation to my other comment), I think we're fine. However, I wonder if you tested this in an app that has their own network-security-config (maybe with clearTestTraffic=false, or with pinning configuration like SSL Pinning Plugin).
Wondering if those could clash and fail the build (which may not fail before with the deprecated way), and if we need to place additional guards to protect those cases.
Let me know if that makes sense.
There was a problem hiding this comment.
Tested with server.cleartext=true + app declaring its own network-security-config, in order to understand what could happen, and here are the findings:
-
Same filename (
network_security_config.xml): build passes, but the app's file wins, ourserver.cleartext=trueis silently ignored. Also tested with: configfalse+ appnetwork_security_configtruestill allows HTTP. -
Different filename: build fails with manifest merger conflict (different values for
android:networkSecurityConfigbetween library and app). The error itself suggests the standard Android fix: addtools:replace="android:networkSecurityConfig"to the app's<application>, once the user does that, build passes and the app's file wins.
Manifest merger failed : Attribute application@networkSecurityConfig value=(@xml/my_security_config) from AndroidManifest.xml:12:9-64 is also present at [:capacitor-cordova-android-plugins] AndroidManifest.xml:8:18-78 value=(@xml/network_security_config). Suggestion: add 'tools:replace="android:networkSecurityConfig"' to <application> element at AndroidManifest.xml:5:5-39:19 to override.
So when the app has its own NSC, our injection is either silently overridden or breaks the build.
There was a problem hiding this comment.
Hmm, that's why we may need to write the config directly to the app, but in a way that doesn't break the existing config (I'm fine with overriding the cleartext to true if the config requests but the app has it at false, which is technically what the CLI is meant to be doing, but not so much overwriting the entire networkSecurityConfig file).
But also that has implications in relation to the other PR comment.
| cleartext || config.app.extConfig.server?.cleartext || applicationXMLAttributes.includes(cleartextString) | ||
| ? 'android:networkSecurityConfig="@xml/network_security_config"' | ||
| : ''; | ||
| const networkSecurityConfigDir = join(config.android.cordovaPluginsDirAbs, 'src', 'main', 'res', 'xml'); |
There was a problem hiding this comment.
Now that I see it, I'm skeptical of this working in general for Capacitor 9.
Since capacitor 9 only comes with the cordovaPluginsDirAbs tied to the app project if the app has cordova plugins, we could run into situations where setting config.app.extConfig.server?.cleartext would have no effect because the app has no Cordova Plugins - which would lead me to indicate that perhaps this needs to be written somewhere else?
This is kind of separate from your PR, but I guess is highlighted by it.
Did you run into this when testing, or did you always test in apps with a Cordova Plugin?
There was a problem hiding this comment.
Yes, ran into it during testing. Started in an app without any Cordova plugins with server.cleartext=true set, and on the first sync noticed the mechanism wasn't triggered, writeCordovaAndroidManifest only runs with effect when there are Cordova plugins (via the if(enableCordova) path in update.ts). Without one, the library manifest stayed empty (no network-security-config, no attribute), and server.cleartext=true was a no-op. Ended up installing cordova-plugin-device so the mechanism would trigger and I could test in runtime.
From what I understand this is inherited from the pre-existing behavior, the old usesCleartextTraffic="true" injection had the same limitation on Cap 9, but this PR keeps it that way.
There was a problem hiding this comment.
Yes you are correct that this is not a fault with your PR specifically, but it should be fixed for Cap 9 imo.
Now, that doesn't mean it needs to be fixed in your PR necessarily, but maybe because of the other comment it could need further changes? We can align offline as well.
OS-pedrogustavobilro
left a comment
OS-pedrogustavobilro
left a comment
There was a problem hiding this comment.
Tested here and seems to work fine with a cordova plugin installed. As we aligned privately, the other changes in PR comments will be scoped to a separate task / PR.
Here is the separate task to address the other comments. |
2 participants
Description
Migrates
usesCleartextTraffic opt-in in the generatedcapacitor-cordova-android-pluginsmodule from the deprecatedandroid:usesCleartextTrafficattribute tonetwork-security-config.xmlwithcleartextTrafficPermitted="true". Any legacyusesCleartextTrafficattribute injected by Cordova plugins viaedit-configis filtered out, so the final manifest is free of the deprecated attribute. Theandroid:networkSecurityConfig` reference is set on the library module's manifest and propagated to the app's final manifest at build time.The three existing triggers still work:
server.cleartext=truein the Capacitor config, live-reload, and Cordova plugins declaringusesCleartextTraffic="true"viaedit-config. When none apply, thenetwork-security-config.xmlfile is removed.ref: RMET-5243
Change Type
Rationale / Problems Fixed
android:usesCleartextTrafficis deprecated in a future Android version and will eventually be removed.The recommended replacement is
network-security-configwithcleartextTrafficPermitted.Tests or Reproductions
Verified (inspecting the generated manifest +
network-security-config.xmlfile) and in runtime on a Samsung SM-S721B using the@capacitor/inappbrowserexample app pointed athttp://httpforever.comandhttps://outsystems.com:server.cleartext=true: HTTP loads, HTTPS loads.edit-configwithusesCleartextTraffic="true"(andserver.cleartext=false): HTTP loads, HTTPS loads. Legacy attribute filtered out of the final manifest.Platforms Affected