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

Fix/android app crash on authorize #743

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nachoperez714
Copy link

@nachoperez714 nachoperez714 commented Jun 23, 2022

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

@gksander

Any new development on this?

@gksander
Copy link
Contributor

gksander commented Aug 8, 2022

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
Copy link

NullPainter2 commented Aug 9, 2022

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
Copy link
Contributor

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
Copy link
Author

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
Copy link

dolly22 commented Sep 15, 2022

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
Copy link
Author

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

image

@SebastianHenn
Copy link

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
Copy link

eithe commented Feb 2, 2024

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 carbonrobot added the needs-triage Waiting for a member of the team to confirm label May 2, 2024
@carbonrobot carbonrobot added the non-standard flow An implementation that breaks from the standard AppAuth lib's code -> code/token exchange flow label May 2, 2024
@carbonrobot
Copy link
Contributor

@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.

@carbonrobot carbonrobot added bug Confirmed bug on hold Awaiting upstream changes in React Native (but not blocked) and removed needs-triage Waiting for a member of the team to confirm labels May 2, 2024
@eithe
Copy link

eithe commented May 2, 2024

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
Copy link

eithe commented Aug 5, 2024

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
Copy link

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

@sanduluca
Copy link

Any updates to this ?

@eliaslecomte
Copy link
Contributor

eliaslecomte commented Sep 30, 2024

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug non-standard flow An implementation that breaks from the standard AppAuth lib's code -> code/token exchange flow on hold Awaiting upstream changes in React Native (but not blocked)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On some Android devices, the application crashes after passing the authentication procedure