Skip to content

Conversation

@tvalle
Copy link
Contributor

@tvalle tvalle commented Feb 13, 2021

Task/Issue URL: #953
Tech Design URL:
CC:

Description:
This issue happened because every time an intent.getX (X being string, bool, etc) was called, internally it would eventually call unparcel() on BaseBundle, and if there is a class that was passed somewhere on the intent that duckduckgo can't unserialize (either by Serializable or Parcelable) it would throw an exception and close the app. One thing to note is that I tracked all current calls of where the app received intents that were not under the app control, and it seems that this point is in BrowserActivity in onNewIntent. So before any call could be made to any intent getSomething call I called sanitize, which is the preferred design that duckduckgo team mentioned on the original issue URL.

I think it seems safe but may require further tests since the app is quite big and there may be corner cases, one design issue though is that every new addition to the app that handle intents would have to be checked (say if for some reason another Activity that handles outside intents would be added, someone will need to remember to add sanitize() in there). If you guys want extra control and futureproofing then maybe creating an extension method that gets extras and uses generics. So instead of using getBoolean, getString, getShort we could create a getSafe with generics and inside the method handle each case. This seems safer but it would require to refactor the entire app, but it could be enforced on future development to only use this extension method. Idk what's the best solution, it would be interesting to hear what you guys think about it.

Steps to test this PR:

  1. Create another application that sends a bundle with a serializable or parcelable class. On the original Issue URL there's a code snippet that shows this Android: Protect app from crashing due to unsafe input from 3rd parties #953 (comment)
  2. Send it to duckduckgo.

Internal references:

Software Engineering Expectations
Technical Design Template

@cmonfortep
Copy link
Contributor

Thanks, I will try to take a look as soon as we have some time. Thanks for the contribution.

@cmonfortep cmonfortep self-assigned this Feb 18, 2021
@CDRussell CDRussell assigned CDRussell and unassigned cmonfortep Apr 7, 2021
Copy link
Member

@CDRussell CDRussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks for the contribution! I think we also want to sanitize in onCreate(), so i'll merge this and add that in as a follow up PR #1163 .

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 this pull request may close these issues.

3 participants