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

App Rejected from App Store [Resolved: because of Amap and JSPatch, not React Native] #13011

Closed
sriraman opened this issue Mar 18, 2017 · 62 comments
Closed

Comments

@sriraman
Copy link

@sriraman sriraman commented Mar 18, 2017

Our App got rejected by App Store on March 17 with the following message from Apple

Mar 17, 2017 at 8:00 PM
From Apple

2. 5 Performance: Software Requirements (iOS)

Thank you for submitting your app.

Upon further review, we found your app out of compliance with the following guideline(s):

Performance - 2.5.2

Your app, extension, and/or linked framework appears to contain code designed explicitly with the capability to change your app’s behavior or functionality after App Review approval, which is not in compliance with App Store Review Guideline 2.5.2 and section 3.3.2 of the Apple Developer Program License Agreement.

This code, combined with a remote resource, can facilitate significant changes to your app’s behavior compared to when it was initially reviewed for the App Store. While you may not be using this functionality currently, it has the potential to load private frameworks, private methods, and enable future feature changes. This includes any code which passes arbitrary parameters to dynamic methods such as dlopen(), dlsym(), respondsToSelector:, performSelector:, method_exchangeImplementations(), and running remote scripts in order to change app behavior and/or call SPI, based on the contents of the downloaded script. Even if the remote resource is not intentionally malicious, it could easily be hijacked via a Man In The Middle (MiTM) attack, which can pose a serious security vulnerability to users of your app.

Next Steps

Perform an in-depth review of your app and remove any code, frameworks, or SDKs that fall in line with the functionality described above and resubmit your app’s binary for review.

Best regards,
App Store Review

List of iOS packages used by us

  • Reachability
  • GoogleSignin
  • Alamofire
  • Crashlytics
  • Fabric
  • NVActivityIndicator
  • React Native SQLite Storage
  • RNTableView
  • CodePush
  • RNDeviceInfo
  • react-native-mixpanel
  • OneSignal
  • react-native-camera
  • react-native-keep-awake

We were suspecting the CodePush. So, We re-submitted the App without CodePush. Still, Apple rejected by saying the same message.

They have mentioned that We shouldn't use dlopen(), dlsym() etc., So, We did the search of whole Repo for those functions. We found those functions in React Native itself. We found dlsym in RCTUtils.m ( https://github.com/facebook/react-native/blob/master/React/Base/RCTUtils.m#L535 ) etc.,

Thanks,

@brunolemos
Copy link
Contributor

@brunolemos brunolemos commented Mar 18, 2017

Is there any chance that you accidentally tried to publish a development build instead of a production one? Also, did you remove codepush completely (yarn remove) or you just removed from the main file so maybe it was left on some parts like package.json and the final bundle?

If not, there's a small possibility RCTUtils.m#L51-L52 is the case indeed.

@sriraman sriraman changed the title App Rejected without CodePush also App Rejected from App Store Mar 18, 2017
@nihgwu
Copy link
Contributor

@nihgwu nihgwu commented Mar 18, 2017

@brunolemos I can confirm that CodePush is OK ( react-native-mixpanel is safe too)

Please check Crashlytics and Fabric, most of the reject cases are caused by those crash analysis tools

@lyahdav
Copy link
Contributor

@lyahdav lyahdav commented Mar 18, 2017

@brunolemos are you sure the code you linked could be an issue? I'm not sure if that would be considered "code which passes arbitrary parameters to dynamic methods" since the name of the library to load is hard coded.

@brunolemos
Copy link
Contributor

@brunolemos brunolemos commented Mar 18, 2017

@lyahdav no, but it's the code that contains the dlsym function mentioned by Apple

@ide
Copy link
Contributor

@ide ide commented Mar 18, 2017

Apple's analysis tools may see JSC and dlopen()/dlsym() invoked the same binary, leading to a false positive. I recommend commenting out those dlopen() and dlsym() calls so that they are completely gone from the binary and resubmit.

@ide
Copy link
Contributor

@ide ide commented Mar 18, 2017

I also want to clarify that React Native's calls to those functions may not have raised that alert. Other libraries may be invoking those methods, too.

@lyahdav
Copy link
Contributor

@lyahdav lyahdav commented Mar 18, 2017

@ide if he commented out those dlopen() and dlsym() calls won't it break React Native? Or are you just suggesting that as a process of elimination?

@nihgwu if it's Crashlytics or Fabric why would the many others apps using those libraries be accepted?

@brunolemos
Copy link
Contributor

@brunolemos brunolemos commented Mar 18, 2017

I have made some investigations. I searched for the methods mentioned by Apple on all repositories listed in this issue and these are the ones that use them:

dlopen

dlsym

respondsToSelector

performSelector

method_exchangeImplementations

PS1: I couldn't check Fabric neither Crashlytics because they aren't open source, I believe.

PS2: This search only catches the direct source code, but some of their dependencies might be using these methods.

So basically react-native uses ALL methods mentioned by Apple.
The 2nd red flag goes to react-native-onesignal, which uses 3 of the 5 methods.

PS3: These methods can be used, but as the Apple message said, you can't pass arbitrary parameters to them.

Try removing one of these packages and retrying.
I would start with the react-native-table-view and then react-native-onesignal.

I hope that helps.

@ide
Copy link
Contributor

@ide ide commented Mar 19, 2017

@lyahdav I don't think removing those calls would break React Native in production. Trying to come up with a minimal repro.

All of these methods are quite common in typical iOS programming, especially the latter three. Also historically Apple's automatic scanners sometimes have had false positives (ex: thinking a private API was called when there definitely were no calls to it).

For what it's worth, today the App Store accepted our RN app we submitted a couple days ago. We use RN and Crashlytics (not all of Fabric though).

@natdm
Copy link

@natdm natdm commented Mar 19, 2017

@ide , and no codePush?

@hilkeheremans
Copy link
Contributor

@hilkeheremans hilkeheremans commented Mar 19, 2017

Just wanted to add here that our app also got approved twice this past week. Our app uses, amongst others, code push, bugsnag and onesignal.

It would be good if we could get in touch with someone who knows more about the internals of Apple's review process. For starters, we cannot know for certain that Apple tests all app submissions on all fronts. It's quite possible they limit to a standard subset of checks along with a few more randomly chosen checks. This would speed up the overall review process (remember they sped up the review process? This might be how they do it). That might also explain why not all apps with similar dependencies are being rejected.

@booboothefool
Copy link

@booboothefool booboothefool commented Mar 19, 2017

Just to contribute, I've been getting approved with CodePush + OneSignal as well.

@st0ffern
Copy link

@st0ffern st0ffern commented Mar 20, 2017

@sriraman are all your endpoints HTTPS?

Even if the remote resource is not intentionally malicious, it could easily be hijacked via a Man In The Middle (MiTM) attack, which can pose a serious security vulnerability to users of your app

MiTM will usually not be a issue when using HTTPS as Apple in most cases require.

@alloy
Copy link
Collaborator

@alloy alloy commented Mar 20, 2017

The dlopen, dlsym, respondsToSelector:, performSelector:, and method_exchangeImplementations APIs by themselves are public and may be used, the issue is specifically with “code which passes arbitrary parameters to dynamic methods”. E.g. the dlsym case that @brunolemos linked to refers to functions with static strings, so should be fine.

My suspicion would be CodePush, which is specifically meant to deploy updates after the initial review process. Maybe try resubmitting without that and see if it goes through? The fact that others are able to deploy with it could just be explained by them randomly picking out apps for more granular checks.

@alloy
Copy link
Collaborator

@alloy alloy commented Mar 20, 2017

FYI There have recently been rejections for people using Rollout (haven’t reread to see if other solutions were affected too). https://news.ycombinator.com/item?id=13817557

@sriraman
Copy link
Author

@sriraman sriraman commented Mar 20, 2017

@alloy We already submitted the app without CodePush. Still, they rejected the app. :(

@sriraman
Copy link
Author

@sriraman sriraman commented Mar 20, 2017

@alloy Can you check RCTUtils L535-L538 also.

@ide
Copy link
Contributor

@ide ide commented Mar 20, 2017

As far as we know, CodePush (for RN and Cordova, in this context the two are equivalent) itself is well within the Apple guidelines. CodePush and RN do not directly or indirectly pass arbitrary parameters to dynamic methods like dlopen and dlsym. As an app developer you shouldn't drastically change the advertised or intended purpose of your app (don't change your chat messenger into a catalog of obscene content), but from a technical perspective CodePush follows the guidelines. And as @sriraman posted, removing CodePush from a rejected app did not lead to the app's getting accepted -- there are no data points indicating that CodePush is a problem so far.

If you intentionally went out of your way, you could write a RN module or Cordova plugin or a plain Obj-C class for a plain UIKit app (e.g. Rollout, NativeScript) that does invoke those dynamic methods from JavaScript, but neither RN, Cordova, nor UIKit offer or encourage this functionality.

As mentioned above you could enable ATS, which enforces HTTPS for all connections of your app. Using HTTPS would greatly reduce the opportunity for MITM attacks -- an attacker would need to steal your server's private TLS keys or have physical access to the unlocked iOS device.

@sriraman Those calls you linked to also have static arguments. You could try commenting them out anyway to see if your app gets approved, but many people are reporting that their apps with RN and CodePush are getting approved.

@alloy
Copy link
Collaborator

@alloy alloy commented Mar 20, 2017

I haven't done an appeal in a long time, can you ask them for more details about what exactly is causing the rejection?

@sriraman
Copy link
Author

@sriraman sriraman commented Mar 20, 2017

We asked for the specific reason. They sent us the following message

Specifically, this was rejected because it contains a library which takes javascript and converts it directly to native code. It would be appropriate to remove all libraries and frameworks which perform such functionality before resubmitting for review.

What should we do next?

@satya164
Copy link
Collaborator

@satya164 satya164 commented Mar 20, 2017

which takes javascript and converts it directly to native code

hmm, which library does that?

@sriraman
Copy link
Author

@sriraman sriraman commented Mar 20, 2017

I have already listed the list of Library we use. No library is doing that. But, Apple App store saying that as the reason. We were suspecting CodePush, Mixpanel, RNTableView, etc., and Resubmitted without using it. Still, They are saying like this.

Can anyone guide us what to do next?

@brunolemos
Copy link
Contributor

@brunolemos brunolemos commented Mar 20, 2017

@sriraman so you have already removed react-native-table-view and the others?
Then I would just ask Apple specifically which lib is the problem.

@axemclion
Copy link
Contributor

@axemclion axemclion commented Mar 20, 2017

@sriraman So basically, are you now submitting a plain simple react native app without any dependencies and getting this error ?

@sriraman
Copy link
Author

@sriraman sriraman commented Mar 20, 2017

@axemclion No. We just removed CodePush, Mixpanel, Fabric and React Native Tableview. We didn't send them plain React Native app.

@brunolemos Yes. We removed React native tableview when we were submitting the app this time.

@brunolemos
Copy link
Contributor

@brunolemos brunolemos commented Mar 20, 2017

@sriraman I think you should do the following:

  1. Submit an appeal to get the app approved
  2. Ask Apple about exactly which library they are referring to
  3. Ask Apple their position about React Native
  4. Open the source code for a couple people that can help looking for problems, preferably including someone in the RN core team

The remote possibility of react native being the rejection cause is very prejudicial to React Native image and community, we must find the real root cause ASAP and end this.

@bgdavidx
Copy link

@bgdavidx bgdavidx commented Mar 20, 2017

We were able to get an app using CodePush into the app store. I don't believe it would be CodePush causing the problem.

@alloy
Copy link
Collaborator

@alloy alloy commented Mar 20, 2017

That linked ticket has good excerpts on the rules and why something like CodePush shouldn’t be the problem ^.

I agree with @brunolemos in that you should try to get Apple to answer specific questions through an appeal. Thanks for hanging in there for the rest of us 🙏

@brunolemos
Copy link
Contributor

@brunolemos brunolemos commented Mar 21, 2017

@sriraman React Native 0.30 is really old. Too much has changed in the last 8 months, maybe even code related to the methods mentioned by Apple. It's hard to give support for that, you should really upgrade it.

@clarle
Copy link

@clarle clarle commented Mar 22, 2017

I suspect it's due to react-native-one-signal.

The selector helpers here allow swizzling out any arbitrary methods. If these selector helpers are accessible from JavaScript in any way, then it's very likely that this is causing the issue, since you would be able to swap out native functions pretty easily.

@clarle
Copy link

@clarle clarle commented Mar 22, 2017

Some more details after diving into how the OneSignal SDK works:

  • It's swizzling out a lot of selectors on UIApplicationDelegate, including nearly all of the lifecycle methods on it.

  • There's an initWithLaunchOptions method on RCTOneSignal that isn't exposed using RCT_EXPORT_METHOD, but one of the callback options is handled by one of the swizzled selectors.

  • I can't think of a way just yet using the public React Native APIs or public React Native OneSignal APIs that would allow arbitrary code execution, but if RCT_EXPORT_MODULE exposes the hidden methods on the module to JSC in some other way, then it could be possible.

It seems pretty likely that Apple's static analyzer was able to catch something there.

I'm probably not going to dive much deeper into this, but I would recommend removing OneSignal first and then submitting again. I'd definitely be interested in their response after that.

@hilkeheremans
Copy link
Contributor

@hilkeheremans hilkeheremans commented Mar 22, 2017

The two app reviews we passed last week - one last monday, one just this sunday - both had react-native-onesignal in them.

I'm not saying react-native-onesignal is not the problem since we cannot be certain Apple fully analyzes all submissions, but if they do full checks all the time, it is definitely not the issue here.

In short, I would recommend you still follow @clarle's suggestion to resubmit without onesignal -- let us know how it turns out so we can then collectively jump on a PR in react-native-onesignal :-)

@hilkeheremans
Copy link
Contributor

@hilkeheremans hilkeheremans commented Mar 22, 2017

To get back to an earlier question I asked -- the OP is stating that they are still using RN v0.30. Question for the RN connaisseurs: as @brunolemos implies, is there any chance there is some naughty code in there that was removed in later versions?

@geoc3
Copy link

@geoc3 geoc3 commented Mar 27, 2017

Hey @sriraman just checking to see if you ever found out what was causing Apple to reject your app. Thanks!

@sriraman
Copy link
Author

@sriraman sriraman commented Mar 27, 2017

Apple rejected our app again with the same message. They didn't give any proper reason for the rejection after reviewing the app for past 7 days. Now, We are starting the appeal process to App Review Board. I'll update the same thread once we get the response from the App Review Board

@kapone89
Copy link

@kapone89 kapone89 commented Mar 30, 2017

I think Apple will eventually reject apps that use CodePush even if they are allowed now. This is because it's just out of compliance with the guidelines to update apps or their parts without App Store. And it's completely not related to system methods that are used or not.

And because RN allows running external js code itself, there is a non-zero probability that one day Apple will reject all apps that use RN.

I think RN should prevent loading & running external code to protect from that scenario. Once Apple rejects RN, it won't be easy to convince them that after last changes it's not possible anymore or it will take ages. RN should be proactive and protect from that now. Not wait for the disaster.

@ptomasroos
Copy link
Contributor

@ptomasroos ptomasroos commented Mar 30, 2017

@kapone89 That means web browsers should also be banned it runs external js code? Seems legit. Worst comment I've read in a while.

@kapone89
Copy link

@kapone89 kapone89 commented Mar 30, 2017

@ptomasroos You are completely right, but you only think about it from a developer perspective. But look at it from a user perspective, security, and Apple guidelines. RN allows writing apps that have some features and then reloads it's code and becomes completely different App. It's out of compliance with the Apple guidelines. It's not safe for users. Apple can't control that kind of apps. That's all.

All iOS browsers are based on built-in WebView. Firefox and others are just UIs for WebView from Apple. Apple want's to have everything under control. Apple can kill RN on iOS with one decision and one entry in its guidelines. Because it's Apple and they can.

IMHO, it's better to make CodeShip incompatible with latest RN on iOS because of some security mechanisms in RN framework than waiting until Apple kill RN on iOS.

UPDATE: Of course it's open source and developers, as well as CodeShip can make it possible again to run external js code on iOS but in core RN it should not be possible and be 100% compatible with Apple guidelines.

UPDATE (2): Apple is just killing https://rollout.io/ project after 3 years of its existence. I suggest to not be so confident about RN future.

@hilkeheremans
Copy link
Contributor

@hilkeheremans hilkeheremans commented Mar 30, 2017

Could we refrain from this type of discussion in this thread? The purpose of this thread is first and foremost to help solve the issue @sriraman has reported.

At this time, there is still nothing that clearly points towards Apple rejecting, as a whole, mechanisms such as Code Push or RN, so let's not speculate on that too much, at least not until we get a clear response from Apple. Most of us are aware of the Apple guidelines already, and as you probably know, both RN and Code Push fall under some very relevant exceptions in there.

@kapone89
Copy link

@kapone89 kapone89 commented Mar 30, 2017

@hilkeheremans Ok, I agree that it's maybe the wrong place for that discussion.

If the RN team is talking with Apple about its the RN compatibility with the Apple guidelines, is there a place/issue/whatever where we can track the current status of that topic? Is the RN team proactive in this or just wait until Apple starts blocking apps or something? Are there any guidelines for RN developers that were discussed with Apple?

Because I have a feeling that most of the statements I can find are based on suppositions, personal thoughts, and assumption that "if they haven't block RN yet, then it's fine, we will worry about it later".

@axemclion
Copy link
Contributor

@axemclion axemclion commented Mar 30, 2017

@kapone89 We are working with @sriraman and rest of the folks to see how we can resolve this issue. At this point, we have not heard any reports of CodePush apps being banned. Note that Ios Developer License section 3.3.2 explicitly states that it is ok to update code running in JavaScriptCore or Cordova, and React Native apps use JSC.

Also note that if the purpose of the app is changed, that would be a violation. Our team at Microsoft is proactively monitoring the situation and will be updating this thread as we get updates.

@kapone89
Copy link

@kapone89 kapone89 commented Mar 30, 2017

@axemclion Could you send me the link that developer license because I couldn't find it.

There is nothing about it here:
https://developer.apple.com/app-store/review/guidelines/

Just
"(vii) They must use the Mac App Store to distribute updates; other update mechanisms are not allowed."

@kelset
Copy link
Collaborator

@kelset kelset commented Mar 30, 2017

@kapone89 you need to be logged in to view it, but you can find it here: https://developer.apple.com/terms/

The only exceptions to the
foregoing are scripts and code downloaded and run by Apple's built-in WebKit framework or
JavascriptCore, provided that such scripts and code do not change the primary purpose of the
Application by providing features or functionality that are inconsistent with the intended and
advertised purpose of the Application as submitted to the App Store.

@hilkeheremans
Copy link
Contributor

@hilkeheremans hilkeheremans commented Mar 30, 2017

@kapone89 If you had carefully read through this thread above, you would find that both your last question and the gist of your initial remarks were already replied to. So might I suggest you re-read this entire thread?

@kapone89
Copy link

@kapone89 kapone89 commented Mar 30, 2017

@hilkeheremans @kelset Ok, I just thought that they removed the entry about JSC.

After reading this and following issues on the Github:

It looks like CodePush is "OK" because it doesn't change the native code of the app. BUT it is changing the code of the app. Just the code is not native. It's JS executed in JSC.

If Apple doesn't want apps to upgrade or patch itself without App Store and review process they eventually will take a look at apps that are (almost) entirely written in JS and executed by JSC.

When I'm reading how Rollout was implemented (https://rollout.io/blog/under-the-hood-2016-update/) and how it uses JSC to patch methods, I'm not so optimistic about this 3.3.2 point and JSC.

Rollout team was quite confident about their technology:

With Rollout’s SDK in your app, you can instantly push code level updates, such as bug fixes or diagnostic code, to native iOS apps in production. Yes really, and totally legit by Apple’s guidelines

I think allowing to run any external js code by RN (using JSC) is asking for troubles and waiting for a ban.

I recommend you to read this post (by Rollout):
https://rollout.io/blog/open-letter-to-apple-secure-javascript-injection-ios/
I don't think it's a perfect solution but I think that they are in troubles now and they started understanding what's the logic behind Apple guidelines and how to fulfill them somehow and save their project.

@satya164
Copy link
Collaborator

@satya164 satya164 commented Mar 30, 2017

If Apple doesn't want apps to upgrade or patch itself without App Store and review process they eventually will take a look at apps that are (almost) entirely written in JS and executed by JSC.

They have been allowing webview apps to do that from beginning and React Native is not any different. You cannot update native code, which means the API exposed to javaScript can't change, like a web view based app. So it doesn't pose any more security threats than a normal web view based app.

@kapone89
Copy link

@kapone89 kapone89 commented Mar 30, 2017

@satya164 If it's all about security only, then maybe you are right. But I think there is also a business logic behind it etc. and it's more complicated.

And running web app inside a webview is a little bit different than rendering native view in the RN using JSC.

@ide
Copy link
Contributor

@ide ide commented Mar 30, 2017

As @hilkeheremans posted earlier, please have speculative discussions somewhere else.

@sriraman
Copy link
Author

@sriraman sriraman commented Apr 4, 2017

Hi,

Finally, We found out the root cause of the problem. It is not rejected because of the React Native or CodePush. We had our Manufacturer's SDK in our iOS App. They were using Amap ( Alibaba Map ) which was using the JSPatch. Since everything was in the compiled code, We were not able to figure out.

Thank you @brentvatne, @axemclion and the whole community for helping us to find the cause.

@sriraman sriraman closed this Apr 4, 2017
@hilkeheremans
Copy link
Contributor

@hilkeheremans hilkeheremans commented Apr 4, 2017

@sriraman Really glad that you found the root cause and [hopefully] managed to work around it!

@ide ide changed the title App Rejected from App Store App Rejected from App Store [Resolved: because of Amap and JSPatch, not React Native] Apr 4, 2017
@makeitnew
Copy link

@makeitnew makeitnew commented Oct 27, 2017

@sriraman Did Apple help pinpoint the root cause of the issue above or did you figure it out for yourself?

Apple recently rejected our RN app for violations of 2.5.2. It's unlikely that the rejection is related to RN. However, I'm wondering if the appeals process was helpful in determining the issue.

Thanks!

@sriraman
Copy link
Author

@sriraman sriraman commented Oct 27, 2017

@makeitnew No. They didn't point it out anything. They were just pointing out 2.5.2 again and again. We found the JSPatch issue before the appeal. So, We don't know how helpful the appeal will be.

You can check all the libraries which you are included to find the suspicious one and send a build to Apple after removing the suspicious libraries.

@makeitnew
Copy link

@makeitnew makeitnew commented Oct 27, 2017

Thank you @sriraman. This is very helpful.

@facebook facebook locked as resolved and limited conversation to collaborators May 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.