search

capacitor-community / privacy-screen

⚡️ Capacitor plugin that protects your app from displaying a screenshot in Recents screen/App Switcher.
MIT License
88 stars 23 forks source link

Fix iOS crash #88 #93

Closed 7-plus-t closed 5 months ago

7-plus-t commented 5 months ago

Fixes #88

Ensure the privacyViewController is never presented on itself.

The order of the didBecomeActiveNotification and willResignActiveNotification events is not guaranteed by the OS. Therefore, it is possible that in rare occasions the order of events is unexpected, e.g.: willResignActiveNotification, willResignActiveNotification, didBecomeActiveNotification. This then caused the view to be presented on itself, which crashed the app.

The proper solution would be to use the corresponding synchronous callbacks from UIApplicationDelegate, where the order is guaranteed. As we are in the context of a plugin, we do not have a reasonable way to hook into those callbacks.

The trade-off with this fix is, that worst case the privacy cover is not shown (instead of the app crashing)

Pull request checklist

Please check if your PR fulfills the following requirements:

tafelnl commented 5 months ago

Nice work!

The trade-off with this fix is, that worst case the privacy cover is not shown (instead of the app crashing)

However, I would argue this is a hard no-go. A developer/user will expect this plugin to protect them from leaking sensitive content. So - IMHO - it would be better to crash the app then to prevent a (probably rare) crash and have the possibility of sensitive content leaking

robingenz commented 5 months ago

Nice work!

The trade-off with this fix is, that worst case the privacy cover is not shown (instead of the app crashing)

However, I would argue this is a hard no-go. A developer/user will expect this plugin to protect them from leaking sensitive content. So - IMHO - it would be better to crash the app then to prevent a (probably rare) crash and have the possibility of sensitive content leaking

Yes, that's what I think too. The app should crash before sensitive data is leaked. I'm therefore closing this PR. Maybe we can find a better fix.

@7-plus-t Thanks for your contribution, though!

7-plus-t commented 5 months ago

Thanks for the quick review.

I guess it's a matter of the kind of app using the plugin to decide what the preferred behaviour is. A crash is certainly not a good user experience either.

Looking forward to your ideas on how to fix the issue.