Fix/android app crash on authorize

[nachoperez714]

Fixes #600, #672, #360

Description

Fixes app crash issue on authorize for Android

Steps to verify

Enable "Don't Keep Activities" setting in your android Device's Developer Setting,
Authenticate user using authorize method

[gksander]

@nachoperez714 Thank you for contributing this! I've got this prioritized, looking to review (and find someone else familiar to test and review) asap.

[gksander]

I'm still working on testing this out and replicating the original issue.

However, this line stands out to me:

editor.putString("clientSecret", clientSecret);

This would be storing the client secret in an unencrypted text file, and that seems like perhaps a security vulnerability. It's recommended that you don't use a client secret in your app unless you really have to, so this perhaps isn't that much worse than just having the client secret in your app bundle in the first place. However, I'd like to think a bit about if there's a way to avoid storing such a sensitive piece of information unencrypted (or if this is really that much of a concern if you're already in the position of having the client secret in your app in the first place).

[nachoperez714]

I appreciate that this could potentially be a security issue. Storing the value in the bundle, as you pointed out, is also insecure.

We could encrypted the value and then decrypt it later. However a vulnerability would still remain as the value would be accessible after use within active memory. This is because we are at the mercy of the JS engine garbage collection for when these objects stored in memory would be cleaned up once they are no longer in use. Per the MDN docs (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_Management) there is currently no way to explicitly trigger the garbage collection. Would it be possible to accept this pull request while we continue to investigate alternative methods to both fix the crash and improve security? At the moment this is a trade-off between a large number of our users experiencing crashes and a marginal security risk given the limitations of React Native.

[nachoperez714]

@gksander

Any new development on this?

[gksander]

Sorry for the delay on this @nachoperez714. In terms of whether or not it's appropriate to merge this, I'm going to defer to @kadikraman and @Jay-A-McBee since they have much more knowledge than I do on this topic.

[NullPainter2]

Hi guys, I don't have much experience with native Android, but docs says You can save data of Activity into a Bundle, and when it gets recreated You can load them from that Bundle https://developer.android.com/guide/components/activities/activity-lifecycle#oncreate ... ??? This seems to like a simpler solution, and it follows what Google intended.

[kadikraman]

Hya! The reason for the indecision is that SharedPreferences is not a secure place to store data - any values stored in shared preferences will readable in plan text for any application or user with superuser privileges on a rooted device. Would this be a security risk for an auth library?

[nachoperez714]

Have yoou considered implementing @NullPainter2's suggestion to improve this fix?

We are still getting crashes regarding this issue and would like to see it resolved.

[dolly22]

Hello I was not able to replicate this exact issue with just setting "Don't Keep Activities" in developer settings, but we are also experiencing the crash for some of our users.

I have read all comments in linked issues and to my understanding the problem here probably is instance variables of RNAppAuthModule are set to initial state (it seems to me this is only possible when new instance is created through constructor). To my knowledge in ReactNative native module instance is created only once when application starts up.

@nachoperez714 Ale you able to catch this situation in debugger? What is creating new instance of RNAppAuthModule or settings those instance variables to their default values? Are you able to consistently replicate this issue somehow?

As suggested in #360 when the original activity is destroyed it leads to specific kind of problems, but in such case application is able to correctly resolve javascript promise and continue executing javascript code.

The null reference issues mentioned in #600 and #672 seem to me more like whole application process was already stopped and new one started when custom browser tab redirects back to application through net.openid.appauth.RedirectUriReceiverActivity and net.openid.appauth.AuthorizationManagementActivity (from https://github.com/openid/AppAuth-Android). If this would be the case (and I was not able to replicate it in emulator) I think there would be no easy fix due to RNAppAuth design (the Promise instance is already lost).

[nachoperez714]

We weren't able to consistently reproduce the issue but we can still see it in AppDynamics impacting our users

[SebastianHenn]

Hey team, I just wanted to check in on progress here as this is still an issue facing a number of our users.

For me personally I can reproduce it consistently on a Samsung A13 with the following steps:

• Click on a firebase deep link to our app
• Install the app after being redirected to the playStore
• Launch the app for the first time
• Attempt to authorize, app crashes
• Subsequent launches of the app, either through deep link or regular do not cause the crash (in my instance, not sure if this is the case for our other users)

Interestingly the app also does not seem to crash if I install it regularly without having requested it via a deep link.

[eithe]

It has now gone another year without updates from the maintainers. I know I'm getting this for free, but If you don't intend to keep supporting this library (no error in that), please note that in the readme so people can avoid to start using it.

This is a major showstopper for using this library. We are currently seeing this on 2% of sessions (according to Google crash analytics) which means that Google now has flagged the app for being above the "bad behavior threshold" resulting in the following: "Your 28-day user-perceived crash rate exceeds the bad behavior threshold of 1.09%. Your app is likely to be less discoverable on Google Play on all devices."

I have tried the patch that @yberstad suggested in #564 (comment) as a workaround, but haven't put it into production yet.

[carbonrobot]

@eithe We merged a PR that can help with the crashes, let me know if you are still seeing high error counts.

We do not yet have a solution since the problem originates with the upstream React Native modules. The specifics are captured really well here in #773 and the upstream issue is facebook/react-native#30277 here.

[eithe]

Thanks! The patch I mentioned above (@yberstad in #564 (comment)) has worked for us, will update here once I try this updated version, and get that to production.

[eithe]

As much as I appreciate the update, unfortunately it has been decided above my pay grade to not ship our app to production without the mentioned patch so I won't be able to test if the error occurs with your updated version. The problem is that we haven't been able to reproduce the issue (we only saw the crashes started happening for our users in the Google Play console) so the only way to test is to release to production :/

[VariabileAleatoria]

I think this issue needs attention, it's been two years since this pull request and we still do not have a solution

[sanduluca]

Any updates to this ?

[eliaslecomte]

Hi @nachoperez714. Also having a look at this. Are you still working on this?

My first thought was, why not using onSaveInstanceState and onRestoreInstanceState for doing this? Is it because ActivityEventListener doesn't support these?

However, in the meantime, a promise is also added to the login flow:

if (promise != null) {
    promise.resolve(map);
}

We can't store promises in shared prefs or a bundle. Which means, in order to support this case (the app being destroyed when you're looking at the login webview), we would actually need another way to pass login-success from native lib to react. Which means a major refactor or adding an alternative flow for this case.

I could work on this, what does everyone in this thread think? Could I or someone else work on such refactor? Anyone at FormidableLabs that I could have a chat with? @robwalkerco ?

[carbonrobot]

Hi folks, please understand that the underlying issue in React Native itself is still not resolved and this affects many libraries. The only currently known workarounds are storing values in Shared Preferences. While that solution works great for libraries like React Native Image Crop Picker, it is not a secure method for an authentication library.

We rely on the underlying library AppAuth-Android for authentication, and any workarounds should be applied there, which will automatically flow to this library and other authentication libraries after release.