-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(cli): replace usesCleartextTraffic with network-security-config #8572
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -867,14 +867,30 @@ export async function writeCordovaAndroidManifest( | |
| }); | ||
| }); | ||
| const cleartextString = 'android:usesCleartextTraffic="true"'; | ||
| const cleartextValue = | ||
| (cleartext || config.app.extConfig.server?.cleartext) && !applicationXMLAttributes.includes(cleartextString) | ||
| ? cleartextString | ||
| const networkSecurityConfigValue = | ||
| 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'); | ||
| const networkSecurityConfigPath = join(networkSecurityConfigDir, 'network_security_config.xml'); | ||
| 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); | ||
| } | ||
|
Comment on lines
+876
to
+888
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tested with
So when the app has its own NSC, our injection is either silently overridden or breaks the build.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 But also that has implications in relation to the other PR comment. |
||
|
|
||
| let content = `<?xml version='1.0' encoding='utf-8'?> | ||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android" | ||
| xmlns:amazon="http://schemas.amazon.com/apk/res/android"> | ||
| <application ${applicationXMLAttributes.join('\n')} ${cleartextValue}> | ||
| <application ${applicationXMLAttributes.filter((attr) => !attr.includes('android:usesCleartextTraffic')).join('\n')} ${networkSecurityConfigValue}> | ||
| ${applicationXMLEntries.join('\n')} | ||
| </application> | ||
| ${rootXMLEntries.join('\n')} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I see it, I'm skeptical of this working in general for Capacitor 9.
Since capacitor 9 only comes with the
cordovaPluginsDirAbstied to the app project if the app has cordova plugins, we could run into situations where settingconfig.app.extConfig.server?.cleartextwould 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ran into it during testing. Started in an app without any Cordova plugins with
server.cleartext=trueset, and on the first sync noticed the mechanism wasn't triggered,writeCordovaAndroidManifestonly runs with effect when there are Cordova plugins (via theif(enableCordova)path inupdate.ts). Without one, the library manifest stayed empty (no network-security-config, no attribute), andserver.cleartext=truewas a no-op. Ended up installingcordova-plugin-deviceso 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.