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

[android] apply Network Security Config file (fixes #22375) (part 2 of #23105) #23135

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@Salakar
Copy link
Contributor

Salakar commented Jan 24, 2019

This is a follow-up PR for #23105 - as mentioned on discord.


This PR applies the network security config for the RN template project only. New RN projects started with the updated template will be able to connect to the packager on builds built with Android API 28 & above.

See https://developer.android.com/training/articles/security-config#CleartextTrafficPermitted for more information about this newly required config, specifically:

image

Changelog:

[ANDROID] [Template] add Network Security Config file to allow access to packager via cleartext requests in Android API 28 and above. (fixes #22375)

Test Plan:

Before change:
image

After change:
image

@pull-bot

This comment has been minimized.

Copy link

pull-bot commented Jan 24, 2019

Warnings
⚠️

📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

⚠️

📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

⚠️

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

Generated by 🚫 dangerJS

@hey99xx

This comment has been minimized.

Copy link

hey99xx commented Jan 24, 2019

@Salakar thanks for making progress and also mentioning me.

Now what you do is use android:networkSecurityConfig="@xml/rn_network_security_config" in ReactAndroid/src/debug/AndroidManifest.xml. This is not necessarily wrong, but I don't think it will work as you intended.

This is because when you install react-native from NPM, it comes with only one AAR file, the one named react-native-0.56.1.aar somewhere inside in node_modules/react-native. It is possible to release two separate AARs for debug and release but it's usually cumbersome for consumers and it sometimes creates issues with transitive dependencies.

I'm not going to suggest editing template/android/app/src/main/AndroidManifest.xml either, because that applies to both release and debug APKs and we wouldn't want to expose localhost in production.

Instead you can move

  • ReactAndroid/src/debug/AndroidManifest.xml into template/android/app/src/debug/AndroidManifest.xml and
  • ReactAndroid/src/main/res/devsupport/xml/network_security_config.xml into template/android/app/src/debug/res/xml/network_security_config.xml

Basically we do the same thing described in https://stackoverflow.com/a/46321866 Note that this solution obviously does not work for brownfield apps that wasn't started from the helloworld template in this repo, so they would have to manually add those 2 files. I think that's acceptable, but I'm not authority here :)

@Salakar

This comment has been minimized.

Copy link
Contributor Author

Salakar commented Jan 24, 2019

Ah damn, @hey99xx you're right again 🙈 this change would only apply when using RN from source like I was locally, completely forgot it gets distributed as an AAR - which this change won't apply to the bundled AAR that gets publish alongside on NPM >.>

I can apply it to the template but like you said this only works for apps that began with this template after the change.

I may have an alternative though; I can move it to the main manifest and wrap the security config in a <debug-overrides> block: https://developer.android.com/training/articles/security-config#debug-overrides - so it'll only apply to debug still

@hey99xx

This comment has been minimized.

Copy link

hey99xx commented Jan 24, 2019

I can move it to the main manifest and wrap the security config in a <debug-overrides> block

You can try, but as far as I recall it won't work, I've tried it myself in the past 🙃. <debug-overrides> can only be used to override <trust-anchors> block inside. I wish it worked according to the common intuition like you have.

Maybe you can ask in the Discord channel for alternative solutions (such as releasing debug and release AARs, but I feel it's too much trouble for this issue). Unfortunately I don't know a better way. As long as the solution is mentioned in Android tab at https://facebook.github.io/react-native/docs/integration-with-existing-apps I personally think it's ok.

@Salakar

This comment has been minimized.

Copy link
Contributor Author

Salakar commented Jan 24, 2019

😭 right, ok. I will ask about the AARs but I also feel it's far too much trouble.

I think this may just be a case of applying this to the template project & tests project only and then pair with some documentation for non-template based apps.

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Jan 29, 2019

@Salakar do you have any update on this PR?

@Salakar

This comment has been minimized.

Copy link
Contributor Author

Salakar commented Jan 30, 2019

@cpojer sorry for the delay getting back to you, 🙈I've been been held up with prior OSS commitments this week - I've not forgotten I owe you a writeup for this as per Discord.

Will get this done by Friday latest 👍

@Salakar

This comment has been minimized.

Copy link
Contributor Author

Salakar commented Feb 1, 2019

Hey @cpojer;

I've updated this to apply only to the template project. As discussed with @hey99xx above, this can't be applied to RN itself as RN is distributed as a single aar and distributing multiple aars (e.g. debug/release variants) is probably a bit OTT for this issue and could potentially cause more problems than it's worth.

I've also updated the PR description to reflect the above + added testing screenshots.

I will also follow up with a separate docs PR to https://facebook.github.io/react-native/docs/integration-with-existing-apps - to document this for existing apps that aren't using the template.

@dulmandakh

This comment has been minimized.

Copy link
Collaborator

dulmandakh commented Feb 1, 2019

how about having single config file, but use debug-overrides in it as described in https://developer.android.com/training/articles/security-config. so it'll be empty for non-debug builds.

@Salakar

This comment has been minimized.

Copy link
Contributor Author

Salakar commented Feb 1, 2019

@dulmandakh thanks for the suggestion, see the comments above, specifically;

I can move it to the main manifest and wrap the security config in a <debug-overrides> block

You can try, but as far as I recall it won't work, I've tried it myself in the past 🙃. can only be used to override <trust-anchors> block inside. I wish it worked according to the common intuition like you have.

@cpojer

cpojer approved these changes Feb 1, 2019

Copy link
Contributor

cpojer left a comment

Thanks for making this change, and yeah it seems like we need to make sure this is applied to new templates going forward.

@facebook-github-bot
Copy link

facebook-github-bot left a comment

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot

This comment has been minimized.

Copy link
Collaborator

react-native-bot commented Feb 1, 2019

@Salakar merged commit 84572c4 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 1, 2019

matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019

apply Network Security Config file (fixes facebook#22375) (part 2 of f…
…acebook#23105) (facebook#23135)

Summary:
This is a follow-up PR for facebook#23105 - as mentioned on discord.

 ---

This PR applies the network security config for the RN template project only. New RN projects started with the updated template will be able to connect to the packager on builds built with Android API 28 & above.

See https://developer.android.com/training/articles/security-config#CleartextTrafficPermitted for more information about this newly required config, specifically:

![image](https://user-images.githubusercontent.com/5347038/52124287-b3de2580-2620-11e9-958d-bc2da15c6f01.png)

Changelog:
----------
[ANDROID] [Template] add Network Security Config file to allow access to packager via cleartext requests in Android API 28 and above. (fixes facebook#22375)
Pull Request resolved: facebook#23135

Differential Revision: D13917058

Pulled By: cpojer

fbshipit-source-id: 0e66f2cde712c1285d217e3625b73028c3770b65

JunielKatarn added a commit to JunielKatarn/react-native that referenced this pull request Feb 11, 2019

apply Network Security Config file (fixes facebook#22375) (part 2 of f…
…acebook#23105) (facebook#23135)

Summary:
This is a follow-up PR for facebook#23105 - as mentioned on discord.

 ---

This PR applies the network security config for the RN template project only. New RN projects started with the updated template will be able to connect to the packager on builds built with Android API 28 & above.

See https://developer.android.com/training/articles/security-config#CleartextTrafficPermitted for more information about this newly required config, specifically:

![image](https://user-images.githubusercontent.com/5347038/52124287-b3de2580-2620-11e9-958d-bc2da15c6f01.png)

Changelog:
----------
[ANDROID] [Template] add Network Security Config file to allow access to packager via cleartext requests in Android API 28 and above. (fixes facebook#22375)
Pull Request resolved: facebook#23135

Differential Revision: D13917058

Pulled By: cpojer

fbshipit-source-id: 0e66f2cde712c1285d217e3625b73028c3770b65
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.