Skip to content
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

Question: Are the instructions for production AppTransportSecurity complete? #1058

Closed
RobinCsl opened this issue Jun 25, 2019 · 3 comments · Fixed by #1312
Closed

Question: Are the instructions for production AppTransportSecurity complete? #1058

RobinCsl opened this issue Jun 25, 2019 · 3 comments · Fixed by #1312

Comments

@RobinCsl
Copy link
Contributor

Hi 👋
I was just preparing an app for production and read in the docs that it was necessary to enable ATS for iOS by removing localhost from the list of domain exceptions. (cf. https://facebook.github.io/react-native/docs/running-on-device#1-enable-app-transport-security)

However, upon closer inspection of the info.plist file, I see that there is this other key in the NSAppTransportSecurity dictionary by default: NSAllowsArbitraryLoads, which is set to true by default.

I just created a new project to see what's going on (npx expo init testProject), and selecting the bare-minimum option, this is the part concerning ATS in the freshly generated info.plist:

<key>NSAppTransportSecurity</key>
<!--See http://ste.vn/2015/06/10/configuring-app-transport-security-ios-9-osx-10-11/ -->
<dict>
    <key>NSAllowsArbitraryLoads</key>
    <true/>
    <key>NSExceptionDomains</key>
	<dict>
		<key>localhost</key>
		<dict>
			<key>NSExceptionAllowsInsecureHTTPLoads</key>
			<true/>
		</dict>
	</dict>
</dict>

I tried to run the project before bringing any changes: yarn ios and it worked fine, meaning the JS bundle was correctly loaded and the screen displayed "Welcome to React Native!" as expected.

I then removed the part concerning localhost being an exception, i.e. my info.plist then looked like:

<key>NSAppTransportSecurity</key>
	<!--See http://ste.vn/2015/06/10/configuring-app-transport-security-ios-9-osx-10-11/ -->
<dict>
    <key>NSAllowsArbitraryLoads</key>
    <true/>
</dict>

and the JS bundle still loads, and no warnings are issued; that makes me think that the key NSExceptionDomains is not necessary since NSAllowsArbitraryLoads being true allows everything to be loaded anyway.

I then tried removing only the NSAllowsArbitraryLoads key, so that my info.plist looked like:

<key>NSAppTransportSecurity</key>
<!--See http://ste.vn/2015/06/10/configuring-app-transport-security-ios-9-osx-10-11/ -->
<dict>
	<key>NSExceptionDomains</key>
	<dict>
		<key>localhost</key>
		<dict>
			<key>NSExceptionAllowsInsecureHTTPLoads</key>
			<true/>
		</dict>
	</dict>
</dict>

and the JS bundle still loaded correctly.

Finally, setting the ATS in info.plist to an empty dictionary (or removing it altogether), like so:

<key>NSAppTransportSecurity</key>
<!--See http://ste.vn/2015/06/10/configuring-app-transport-security-ios-9-osx-10-11/ -->
<dict></dict>

produced an error: "No bundle URL present", which was expected.

TL;DR: Shall I add an extra sentence in the docs for ATS in production or am I missing something? Only following the part about the removal of localhost from domain exceptions does not seem enough since NSAllowsArbitraryLoads defaulting to true disables ATS for any domain.
Thanks a lot!

@Ashoat
Copy link
Contributor

Ashoat commented Sep 18, 2019

NSAllowsArbitraryLoads is new in the template Info.plist (as of facebook/react-native#19643). I do think it would be helpful to update the referenced documentation. I'm guessing nobody noticed it before.

@RobinCsl
Copy link
Contributor Author

@Ashoat Do you know who to tag to move that issue forward? I think I saw on Twitter that they are planning to revamp the security part of the docs, so maybe that's going to be taken care of too?

@Ashoat
Copy link
Contributor

Ashoat commented Sep 23, 2019

If you want to make this change it should be easy to put up a PR!

elicwhite pushed a commit that referenced this issue Oct 7, 2019
`NSAllowsArbitraryLoads` was introduced in facebook/react-native#19643 and defaults to `true`. That means that removing `localhost` from `NSExceptionDomains` is not enough anymore, as describe in #1058.

Kudos to @Ashoat who tracked down the origin of the issue: #1058 (comment)

Closes #1058
espipj pushed a commit to espipj/react-native-website that referenced this issue Feb 8, 2020
`NSAllowsArbitraryLoads` was introduced in facebook/react-native#19643 and defaults to `true`. That means that removing `localhost` from `NSExceptionDomains` is not enough anymore, as describe in facebook#1058.

Kudos to @Ashoat who tracked down the origin of the issue: facebook#1058 (comment)

Closes facebook#1058
JackWillie added a commit to JackWillie/react-native-website that referenced this issue Nov 27, 2022
`NSAllowsArbitraryLoads` was introduced in facebook/react-native#19643 and defaults to `true`. That means that removing `localhost` from `NSExceptionDomains` is not enough anymore, as describe in facebook/react-native-website#1058.

Kudos to @Ashoat who tracked down the origin of the issue: facebook/react-native-website#1058 (comment)

Closes facebook/react-native-website#1058
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants