Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2392763002:
Sanitize unparcable intents (Closed)
DescriptionSanitize unparcable intents
ChromeLauncherActivity did not have sanitation of incoming intents, and thus
if for example an intent with a bad Parcel was sent, Chrome crashed.
This change strips away all extras and makes sure Chrome properly opens the
intended website instead.
BUG=652460, 412527
Committed: https://crrev.com/4bca3b37801c502a164536b804879c00aba7d304
Cr-Commit-Position: refs/heads/master@{#427697}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed Ted's comments #Patch Set 3 : Reverted one more change that was made obsolete by diff 2 #
Total comments: 7
Patch Set 4 : Log exception, use repalceExtras and some refactors for testing #
Total comments: 1
Patch Set 5 : Instrumentation tests and functional revert in LauncherActivity #
Total comments: 6
Patch Set 6 : Removed unit test and expanded instrumentation test #Patch Set 7 : Fixed merge conflict #Patch Set 8 : Fix lint and findbugs #
Messages
Total messages: 47 (12 generated)
kraush@amazon.com changed reviewers: + tedchoc@chromium.org
Hi Ted, Can you take a look at this change to guard Chrome against intents with bad extras? Thanks! :) Holger
tedchoc@chromium.org changed reviewers: + mariakhomenko@chromium.org
overall this seems like a good change to me. but Maria is our resident intent expert, so I'll defer to them on that https://codereview.chromium.org/2392763002/diff/1/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:335: "//third_party/android_tools:android_support_v7_appcompat_java", why is this needed? I don't see any new imports that require this. https://codereview.chromium.org/2392763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:93: if (ContextUtils.getApplicationContext() == null) { why do we need to add this? IMO, the test should be the one that needs to special case handle this and not the production code. https://codereview.chromium.org/2392763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:143: setIntent(IntentHandler.addTimestampToIntent(getIntent())); while this seems reasonable to do, it is a bit of an odd side effect of addTimestampToIntent. I wonder if it would be better to expose a method a generic sanitizeIntent in IntentUtils that just forces an unparcelling through one of the various methods. If that fails, then return a sanitized intent w/ just the action as you were doing in the put method. https://codereview.chromium.org/2392763002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/TestableChromeApplication.java (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/TestableChromeApplication.java:9: * which interacts with its (in tests nonexistant) TracingController. This actually seems like a bug in ContentApplication. If we were to add a mTracingController != null check in ContentApplication#onTermiate, would that solve this problem as well? https://codereview.chromium.org/2392763002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java:69: final AtomicReference<MultiWindowUtils> multiWindowUtils = out of curiosity, why do we need to mock these? In particular, you have a TestableChromeApplication instance that exposes methods to create MultiWindowUtils and InstantAppsHandler instances. Seems like we should be able to either mock the application or have the application return mock versions of those as needed than doing this here.
https://codereview.chromium.org/2392763002/diff/1/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:335: "//third_party/android_tools:android_support_v7_appcompat_java", On 2016/10/04 20:19:23, Ted C wrote: > why is this needed? I don't see any new imports that require this. You're absolutely right, sorry! (While developing this I introduced a dependency to it at some point, but import has since been removed) I'll remove the gn entry. https://codereview.chromium.org/2392763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:93: if (ContextUtils.getApplicationContext() == null) { On 2016/10/04 20:19:23, Ted C wrote: > why do we need to add this? IMO, the test should be the one that needs to > special case handle this and not the production code. This was required because the test framework spawns an application (Based on @Config) --> I'll instead base TestableChromeApplication on BaseChromiumApplication, that way this change won't be required. https://codereview.chromium.org/2392763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java:143: setIntent(IntentHandler.addTimestampToIntent(getIntent())); On 2016/10/04 20:19:23, Ted C wrote: > while this seems reasonable to do, it is a bit of an odd side effect of > addTimestampToIntent. > > I wonder if it would be better to expose a method a generic sanitizeIntent in > IntentUtils that just forces an unparcelling through one of the various methods. > If that fails, then return a sanitized intent w/ just the action as you were > doing in the put method. Will do. https://codereview.chromium.org/2392763002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/TestableChromeApplication.java (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/TestableChromeApplication.java:9: * which interacts with its (in tests nonexistant) TracingController. On 2016/10/04 20:19:23, Ted C wrote: > This actually seems like a bug in ContentApplication. > > If we were to add a mTracingController != null check in > ContentApplication#onTermiate, would that solve this problem as well? Using BaseChromiumApplication actually fixes this and we don't need to override this method any more. When I originally wrote this there was a hard dependency on ChromeApplication, but that seems to be gone. I'll remove this override (and probably the class as a whole - not sure we need it any more) https://codereview.chromium.org/2392763002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java (right): https://codereview.chromium.org/2392763002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java:69: final AtomicReference<MultiWindowUtils> multiWindowUtils = On 2016/10/04 20:19:23, Ted C wrote: > out of curiosity, why do we need to mock these? > > In particular, you have a TestableChromeApplication instance that exposes > methods to create MultiWindowUtils and InstantAppsHandler instances. Seems like > we should be able to either mock the application or have the application return > mock versions of those as needed than doing this here. Without this change, this line will fail: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... java.lang.ClassCastException: android.app.Application cannot be cast to org.chromium.chrome.browser.ChromeApplication at org.chromium.chrome.browser.multiwindow.MultiWindowUtils.getInstance(MultiWindowUtils.java:60) at org.chromium.chrome.browser.document.ChromeLauncherActivity.doOnCreate(ChromeLauncherActivity.java:154) at org.chromium.chrome.browser.document.ChromeLauncherActivityTest.testChromeLauncherActivitySanitizesBadIntents(ChromeLauncherActivityTest.java:58) I don't think we can really avoid the ClassCastException - the JUnit framework will always return a regular application from what I can tell (even with the application = @Config tag)
Addressed Ted's comments. This also reduced the number of files changed from 9 to 4, so that's awesome! :)
Description was changed from ========== Sanitize unparcable intents ChromeLauncherActivity did not have sanitation of incoming intents, and thus if for example an intent with a bad Parcel was sent, Chrome crashed. This change strips away all extras and makes sure Chrome properly opens the intended website instead. BUG=652460 ========== to ========== Sanitize unparcable intents ChromeLauncherActivity did not have sanitation of incoming intents, and thus if for example an intent with a bad Parcel was sent, Chrome crashed. This change strips away all extras and makes sure Chrome properly opens the intended website instead. BUG=652460,412527 ==========
https://codereview.chromium.org/2392763002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java (right): https://codereview.chromium.org/2392763002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java:352: Log.e(TAG, "Invalid incoming intent."); add e at the third argument to log, so that we know what the exception was. https://codereview.chromium.org/2392763002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java:355: incomingIntent.getData()); I imagine none of the intent properties other than the extras would trigger the exception. What do you think about calling incomingIntent.replaceExtras(null) instead? That way we get to keep cateogry, filters, package, flags, etc. https://codereview.chromium.org/2392763002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java (right): https://codereview.chromium.org/2392763002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java:35: public class ChromeLauncherActivityTest { I really appreciate that you've put in the work to write this unit test -- that's really good to have. I do think we would want to have an instrumentation test for this, however, because the throwing of BadParcelableException depends on Android implementation. With an instrumentation test we could catch issues if Android implementation no longer matches the mocked behaviour (e.g. they do unparcel in getAction()). My proposal would be to create an intent for ChromeLauncherActivity in the test that can't be parsed (e.g. we could make it over size limit) and then start the intent and verify onCreate successfully doesn't throw.
https://codereview.chromium.org/2392763002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java (right): https://codereview.chromium.org/2392763002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java:352: Log.e(TAG, "Invalid incoming intent."); On 2016/10/06 18:16:41, Maria wrote: > add e at the third argument to log, so that we know what the exception was. Sounds good, will add! https://codereview.chromium.org/2392763002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/util/IntentUtils.java:355: incomingIntent.getData()); On 2016/10/06 18:16:41, Maria wrote: > I imagine none of the intent properties other than the extras would trigger the > exception. > > What do you think about calling incomingIntent.replaceExtras(null) instead? That > way we get to keep cateogry, filters, package, flags, etc. That's a great idea! Just checked the implementation of replaceExtras and it seems safe to use. I'll use it instead of creating a new intent. https://codereview.chromium.org/2392763002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java (right): https://codereview.chromium.org/2392763002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java:35: public class ChromeLauncherActivityTest { On 2016/10/06 18:16:41, Maria wrote: > I really appreciate that you've put in the work to write this unit test -- > that's really good to have. > > I do think we would want to have an instrumentation test for this, however, > because the throwing of BadParcelableException depends on Android > implementation. With an instrumentation test we could catch issues if Android > implementation no longer matches the mocked behaviour (e.g. they do unparcel in > getAction()). > > My proposal would be to create an intent for ChromeLauncherActivity in the test > that can't be parsed (e.g. we could make it over size limit) and then start the > intent and verify onCreate successfully doesn't throw. Sounds like a good idea, thanks Maria! Do you have any hints on how to create an intent that would not work? I actually started out writing this as an instrumentation test, but gave up after the following three attempts: a) extending Intent and overriding some methods to cause errors --> Android just rewrapped this into a new pristine Intent class before sending it to ChromeApplication b) Creating an anonymous Parcelable in the test and adding it to the intent. This did not crash either (it looked like instrumentation and ChromeApplication shared the same classpath) c) Creating a 10MB extra and adding it to the intent. The extra seemed to be removed when arriving in ChromeApplication. Do you have any suggestions on how to create such a bad intent for testing? (Preferably one causing a BadParcelableException) Thanks!
https://codereview.chromium.org/2392763002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java (right): https://codereview.chromium.org/2392763002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java:35: public class ChromeLauncherActivityTest { On 2016/10/06 18:44:04, kraush wrote: > On 2016/10/06 18:16:41, Maria wrote: > > I really appreciate that you've put in the work to write this unit test -- > > that's really good to have. > > > > I do think we would want to have an instrumentation test for this, however, > > because the throwing of BadParcelableException depends on Android > > implementation. With an instrumentation test we could catch issues if Android > > implementation no longer matches the mocked behaviour (e.g. they do unparcel > in > > getAction()). > > > > My proposal would be to create an intent for ChromeLauncherActivity in the > test > > that can't be parsed (e.g. we could make it over size limit) and then start > the > > intent and verify onCreate successfully doesn't throw. > > Sounds like a good idea, thanks Maria! > Do you have any hints on how to create an intent that would not work? > I actually started out writing this as an instrumentation test, but gave up > after the following three attempts: > a) extending Intent and overriding some methods to cause errors > --> Android just rewrapped this into a new pristine Intent class before sending > it to ChromeApplication > b) Creating an anonymous Parcelable in the test and adding it to the intent. > This did not crash either (it looked like instrumentation and ChromeApplication > shared the same classpath) > c) Creating a 10MB extra and adding it to the intent. The extra seemed to be > removed when arriving in ChromeApplication. > > Do you have any suggestions on how to create such a bad intent for testing? > (Preferably one causing a BadParcelableException) > > Thanks! I have one idea, but not sure whether it would work. How about creating a bundle of extras with Chrome-specific classes and then explicitly overriding the class-loader to be the system classloader, instead of the application specific class-loader. The class loader is a constructor parameter to Bundle and ClassLoader.getSystemClassLoader() should give us one. But not sure if it'll return application specific one anyway.
On 2016/10/10 23:25:53, Maria wrote: > > I have one idea, but not sure whether it would work. How about creating a bundle > of extras with Chrome-specific classes and then explicitly overriding the > class-loader to be the system classloader, instead of the application specific > class-loader. > > The class loader is a constructor parameter to Bundle and > ClassLoader.getSystemClassLoader() should give us one. But not sure if it'll > return application specific one anyway. That's a great idea, thanks Maria! I'll try injecting a class with some custom class loader. I'll let you know how it goes. Unless told otherwise I'll also leave the junit test in place though. Let me know if you'd rather have me remove the junit test and only keep an instrumentation test though.
On 2016/10/10 23:39:50, kraush wrote: > On 2016/10/10 23:25:53, Maria wrote: > > > > I have one idea, but not sure whether it would work. How about creating a > bundle > > of extras with Chrome-specific classes and then explicitly overriding the > > class-loader to be the system classloader, instead of the application specific > > class-loader. > > > > The class loader is a constructor parameter to Bundle and > > ClassLoader.getSystemClassLoader() should give us one. But not sure if it'll > > return application specific one anyway. > > That's a great idea, thanks Maria! > I'll try injecting a class with some custom class loader. > I'll let you know how it goes. > > Unless told otherwise I'll also leave the junit test in place though. Let me > know > if you'd rather have me remove the junit test and only keep an instrumentation > test though. I think keeping the unit test is good.
On 2016/10/11 04:03:55, Maria wrote:
> On 2016/10/10 23:39:50, kraush wrote:
> > On 2016/10/10 23:25:53, Maria wrote:
> > >
> > > I have one idea, but not sure whether it would work. How about creating a
> > bundle
> > > of extras with Chrome-specific classes and then explicitly overriding the
> > > class-loader to be the system classloader, instead of the application
> specific
> > > class-loader.
> > >
> > > The class loader is a constructor parameter to Bundle and
> > > ClassLoader.getSystemClassLoader() should give us one. But not sure if
it'll
> > > return application specific one anyway.
> >
> > That's a great idea, thanks Maria!
> > I'll try injecting a class with some custom class loader.
> > I'll let you know how it goes.
> >
> > Unless told otherwise I'll also leave the junit test in place though. Let me
> > know
> > if you'd rather have me remove the junit test and only keep an
instrumentation
> > test though.
>
> I think keeping the unit test is good.
I tried creating a Bundle using a custom class loader, but that didn't work:
@SmallTest
public void testDoesNotCrashOnInvalidIntent() throws Exception {
final Intent intent = new Intent(Intent.ACTION_VIEW);
intent.putExtra("Dummy", new Bundle(new InvalidClassLoader()));
startMainActivityFromIntent(intent, "about:blank");
}
private class InvalidClassLoader extends ClassLoader {
@Override
public Class loadClass(String name) throws ClassNotFoundException {
throw new ClassNotFoundException();
}
}
I'll try and see if I can override the application class loader next, though I
would assume that would make all of Chromium crash due to missing classes?
Will investigate some more.
I tried the following things: - Specifically loading a class with a custom class loader, then putting it into the Bundle -> Android will automatically also load the class in the parent class loader and Chrome will query the parent first, despite using setClassLoader on Bundle. - Trying to inject a class loader that will just fail when trying to load classes. As above, ChromePublic does not seem to use that class loader. - Extending various classes along the chain to trigger exception --> Most (Parcel, Bundle, ArrayMap) are final --> Parcelable is not, but Parcel refuses to even start unparcelling anything I create. (Not crashing, just early returning) - Using reflection to trigger exceptions. The best I could do is a NullPointer during Unmarshalling, but that's not what we are looking for, nor is it a nice solution (as you mentioned, implementations might change in the future) The only other solution I could think of is moving this whole test into android.os, which should allow us to mock unparcel and make sure the three excluded methods don't trigger an unparcel themselves. Abusing package visibility like that does sound kind of ugly though :( Thoughts on how to proceed?
On 2016/10/12 17:52:55, kraush wrote: > I tried the following things: > > - Specifically loading a class with a custom class loader, then putting it into > the Bundle -> Android will automatically also load the class in the parent class > loader and Chrome will query the parent first, despite using setClassLoader on > Bundle. > - Trying to inject a class loader that will just fail when trying to load > classes. As above, ChromePublic does not seem to use that class loader. > - Extending various classes along the chain to trigger exception > --> Most (Parcel, Bundle, ArrayMap) are final > --> Parcelable is not, but Parcel refuses to even start unparcelling anything I > create. (Not crashing, just early returning) > - Using reflection to trigger exceptions. The best I could do is a NullPointer > during Unmarshalling, but that's not what we are looking for, nor is it a nice > solution (as you mentioned, implementations might change in the future) > > The only other solution I could think of is moving this whole test into > android.os, which should allow us to mock unparcel and make sure the three > excluded methods don't trigger an unparcel themselves. > Abusing package visibility like that does sound kind of ugly though :( > > Thoughts on how to proceed? Thanks for all the effort -- wow, this turned out to be more difficult than I hoped it would be. I suggest we give up and just keep the unit test for this.
On 2016/10/12 18:03:47, Maria wrote: > On 2016/10/12 17:52:55, kraush wrote: > > I tried the following things: > > > > - Specifically loading a class with a custom class loader, then putting it > into > > the Bundle -> Android will automatically also load the class in the parent > class > > loader and Chrome will query the parent first, despite using setClassLoader on > > Bundle. > > - Trying to inject a class loader that will just fail when trying to load > > classes. As above, ChromePublic does not seem to use that class loader. > > - Extending various classes along the chain to trigger exception > > --> Most (Parcel, Bundle, ArrayMap) are final > > --> Parcelable is not, but Parcel refuses to even start unparcelling anything > I > > create. (Not crashing, just early returning) > > - Using reflection to trigger exceptions. The best I could do is a NullPointer > > during Unmarshalling, but that's not what we are looking for, nor is it a nice > > solution (as you mentioned, implementations might change in the future) > > > > The only other solution I could think of is moving this whole test into > > android.os, which should allow us to mock unparcel and make sure the three > > excluded methods don't trigger an unparcel themselves. > > Abusing package visibility like that does sound kind of ugly though :( > > > > Thoughts on how to proceed? > > Thanks for all the effort -- wow, this turned out to be more difficult than I > hoped it would be. I suggest we give up and just keep the unit test for this. So when you address my other comments, I will approve it.
Hi Maria, I've addressed your comments. I had two do a few additional changes to accomodate for testing: - Added some extra logic in the test to no longer throw an exception after replaceExtras - Changed "Intent newIntent = new Intent(getIntent());" to "final Intent newIntent = getIntent();" --> The reason was that ShadowIntent breaks with a null copy constructor, but I also don't see any benefit of this call. This is the de-facto end of ChromeLauncherActivity, so it seems unexpected that we wouldn't want to alter its Intent. If you think I shouldn't change that let me know and I'll stub out launchTabbedMode in the test instead.
https://codereview.chromium.org/2392763002/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java (right): https://codereview.chromium.org/2392763002/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java:80: final Intent badIntent = mock(Intent.class, new BadParcelableExceptionAnswer()); I am concerned that by mocking out the intent here, there will be a lot of maintenance going forward for the test. This is brought by the fact that you had to switch the implementation not to copy the intent. While that change doesn't have any correctness issues, the fact that someone will have to fix this test if they need to copy an intent within ChromeLauncherActivity in the future is a concern. Do you think you could use a real intent object with mockito spy in order to throw BadParcelableException when extras are accessed -- but keep the rest of the implementation intact?
On 2016/10/14 23:37:32, Maria (OOO Oct 17-Oct 21) wrote: > https://codereview.chromium.org/2392763002/diff/60001/chrome/android/junit/sr... > File > chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java > (right): > > https://codereview.chromium.org/2392763002/diff/60001/chrome/android/junit/sr... > chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java:80: > final Intent badIntent = mock(Intent.class, new BadParcelableExceptionAnswer()); > I am concerned that by mocking out the intent here, there will be a lot of > maintenance going forward for the test. This is brought by the fact that you had > to switch the implementation not to copy the intent. While that change doesn't > have any correctness issues, the fact that someone will have to fix this test if > they need to copy an intent within ChromeLauncherActivity in the future is a > concern. > > Do you think you could use a real intent object with mockito spy in order to > throw BadParcelableException when extras are accessed -- but keep the rest of > the implementation intact? The problem with that approach is that there are no default returns for spy objects, so we would have to change the return type of every single method that unmarshalls the bundle. Since the list of which methods trigger the unmarshal might change in the future (as you mentioned in a prior comment), I'm that would work well. How about leaving the new Intent(getIntent()) and mocking out launchTabbedMode() instead? It shouldn't be relevant to this test case (since all the sanitizing takes place before it), In case someone wants to add additional setIntents in the future, they would be very likely to do so in the launch* methods, so I assume this shouldn't be any problem in that case either. What are your thoughts - would that be an acceptable approach? (Reverting the setIntent change and instead mocking out launchTabbedMode)
On 2016/10/17 16:24:29, kraush wrote: > On 2016/10/14 23:37:32, Maria (OOO Oct 17-Oct 21) wrote: > > > https://codereview.chromium.org/2392763002/diff/60001/chrome/android/junit/sr... > > File > > > chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java > > (right): > > > > > https://codereview.chromium.org/2392763002/diff/60001/chrome/android/junit/sr... > > > chrome/android/junit/src/org/chromium/chrome/browser/document/ChromeLauncherActivityTest.java:80: > > final Intent badIntent = mock(Intent.class, new > BadParcelableExceptionAnswer()); > > I am concerned that by mocking out the intent here, there will be a lot of > > maintenance going forward for the test. This is brought by the fact that you > had > > to switch the implementation not to copy the intent. While that change doesn't > > have any correctness issues, the fact that someone will have to fix this test > if > > they need to copy an intent within ChromeLauncherActivity in the future is a > > concern. > > > > Do you think you could use a real intent object with mockito spy in order to > > throw BadParcelableException when extras are accessed -- but keep the rest of > > the implementation intact? > > The problem with that approach is that there are no default returns for spy > objects, so we would have to change the return type of every single method that > unmarshalls the bundle. > Since the list of which methods trigger the unmarshal might change in the future > (as you mentioned in a prior comment), I'm that would work well. > How about leaving the new Intent(getIntent()) and mocking out launchTabbedMode() > instead? > It shouldn't be relevant to this test case (since all the sanitizing takes place > before it), > In case someone wants to add additional setIntents in the future, they would be > very likely to do so in the launch* methods, so I assume this shouldn't be any > problem in that case either. > > What are your thoughts - would that be an acceptable approach? (Reverting the > setIntent change and instead mocking out launchTabbedMode) Hmm...very good points. I honestly don't have a great option here. I think the "best and worst" solution is to actually find a way to generate a valid invalid bundle. I doubt that serializing to a byte stream and fiddling w/ bits is actually going to work. And the only option that I can think of is writing a new java file that is compiled into its own jar and not included in our default jar, then we write a new classloader that loads the file and generates a bundle with a class file that the default loader wouldn't understand. This would be such a pain to write for this test though, so I am not going to strongly recommend we go down this path. Again, I'll show my ignorance about class loaders, but could we build a class loader that doesn't understand any of the chromium code? Then we could put some chromium class into the bundle and set our classloader into the intent and that could fail? I'm thinking of something like: public static class TestParcelable implements Parcelable { // ... stuff here ... } public static class IntentTestClassLoader extends ClassLoader{ public IntentTestClassLoader(ClassLoader parent) { super(parent); } public Class loadClass(String name) throws ClassNotFoundException { if ("org.chromium.stuff.and.things.TestParcelable".equals(name)) { throw new ClassNotFoundException(); } return super.loadClass(name); } } But again, no idea of that is feasible and whether we could use that in combination of https://developer.android.com/reference/android/content/Intent.html#setExtras... to get the desired effect w/o having to fake it as we've explored thus far. If we can't honestly get to a place we are happy with, we might want to consider just testing the IntentUtils method w/ a spy class that throws exceptions for all get*/put*Extra methods and then ensure we get the right complete intent as a result.
On 2016/10/17 19:57:06, Ted C wrote:
>
> Hmm...very good points. I honestly don't have a great option here.
>
> I think the "best and worst" solution is to actually find a way to generate
> a valid invalid bundle. I doubt that serializing to a byte stream and
fiddling
> w/ bits is actually going to work. And the only option that I can think of
> is writing a new java file that is compiled into its own jar and not included
> in our default jar, then we write a new classloader that loads the file and
> generates a bundle with a class file that the default loader wouldn't
> understand.
>
> This would be such a pain to write for this test though, so I am not going
> to strongly recommend we go down this path.
>
> Again, I'll show my ignorance about class loaders, but could we build a
> class loader that doesn't understand any of the chromium code? Then we could
> put some chromium class into the bundle and set our classloader into the
> intent and that could fail?
>
> I'm thinking of something like:
> public static class TestParcelable implements Parcelable {
> // ... stuff here ...
> }
>
> public static class IntentTestClassLoader extends ClassLoader{
>
> public IntentTestClassLoader(ClassLoader parent) {
> super(parent);
> }
>
> public Class loadClass(String name) throws ClassNotFoundException {
> if ("org.chromium.stuff.and.things.TestParcelable".equals(name)) {
> throw new ClassNotFoundException();
> }
> return super.loadClass(name);
> }
> }
>
> But again, no idea of that is feasible and whether we could use that in
> combination of
>
https://developer.android.com/reference/android/content/Intent.html#setExtras...
> to get the desired effect w/o having to fake it as we've explored thus
> far.
>
> If we can't honestly get to a place we are happy with, we might want to
> consider just testing the IntentUtils method w/ a spy class that throws
> exceptions for all get*/put*Extra methods and then ensure we get the
> right complete intent as a result.
I don't think setExtrasClassLoader with an incorrect classloader will work :(
Since the ClassLoader will already have the class loaded from the test, it will
not try loading it again using the custom class loader.
For testing, I just changed LauncherActivityTest by adding a parcelable to the
intent and using setExtrasClassLoader to set a custom class loader that logs
every time findClass or loadClass is invoked.
Unfortunately neither of those methods was invoked when launching the activity
:(
I'll try if I can enforce it to try reloading the class a little more.
In case I can't get that to work and I write the IntentUtils unit test, would
you want me to keep the initial ChromeLauncherActivity unit test (after
reverting the functional changes and fixing the test in a better way), or should
I delete it in favor of the IntentUtils test?
On 2016/10/19 16:17:00, kraush wrote:
> On 2016/10/17 19:57:06, Ted C wrote:
> >
> > Hmm...very good points. I honestly don't have a great option here.
> >
> > I think the "best and worst" solution is to actually find a way to generate
> > a valid invalid bundle. I doubt that serializing to a byte stream and
> fiddling
> > w/ bits is actually going to work. And the only option that I can think of
> > is writing a new java file that is compiled into its own jar and not
included
> > in our default jar, then we write a new classloader that loads the file and
> > generates a bundle with a class file that the default loader wouldn't
> > understand.
> >
> > This would be such a pain to write for this test though, so I am not going
> > to strongly recommend we go down this path.
> >
> > Again, I'll show my ignorance about class loaders, but could we build a
> > class loader that doesn't understand any of the chromium code? Then we
could
> > put some chromium class into the bundle and set our classloader into the
> > intent and that could fail?
> >
> > I'm thinking of something like:
> > public static class TestParcelable implements Parcelable {
> > // ... stuff here ...
> > }
> >
> > public static class IntentTestClassLoader extends ClassLoader{
> >
> > public IntentTestClassLoader(ClassLoader parent) {
> > super(parent);
> > }
> >
> > public Class loadClass(String name) throws ClassNotFoundException {
> > if ("org.chromium.stuff.and.things.TestParcelable".equals(name)) {
> > throw new ClassNotFoundException();
> > }
> > return super.loadClass(name);
> > }
> > }
> >
> > But again, no idea of that is feasible and whether we could use that in
> > combination of
> >
>
https://developer.android.com/reference/android/content/Intent.html#setExtras...
> > to get the desired effect w/o having to fake it as we've explored thus
> > far.
> >
> > If we can't honestly get to a place we are happy with, we might want to
> > consider just testing the IntentUtils method w/ a spy class that throws
> > exceptions for all get*/put*Extra methods and then ensure we get the
> > right complete intent as a result.
>
> I don't think setExtrasClassLoader with an incorrect classloader will work :(
> Since the ClassLoader will already have the class loaded from the test, it
will
> not try loading it again using the custom class loader.
> For testing, I just changed LauncherActivityTest by adding a parcelable to the
> intent and using setExtrasClassLoader to set a custom class loader that logs
> every time findClass or loadClass is invoked.
> Unfortunately neither of those methods was invoked when launching the activity
> :(
> I'll try if I can enforce it to try reloading the class a little more.
>
> In case I can't get that to work and I write the IntentUtils unit test, would
> you want me to keep the initial ChromeLauncherActivity unit test (after
> reverting the functional changes and fixing the test in a better way), or
should
> I delete it in favor of the IntentUtils test?
I think I found an instrumentation test solution that triggers a
BadParcelableException! :)
(No custom class loader required. In my previous tests I didn't use
writeToParcel and readFromParcel correctly)
I'll clean it up a little and post a new diff.
Hi Ted, Hi Maria, Can you take a look at this new revision? - The functional change in launchTabbedActivity was reverted in favor of a test change - I kept the JUnit test around, since it's useful to verify other fields like action and data are not touched even after running through doOnCreate. - I've added an instrumentation test to verify it does not crash. Please find the test results below. Thanks! :) Holger ChromeLauncherActivityTest* Without functional changes C 29.544s Main ******************************************************************************** C 29.544s Main [==========] 2 tests ran. C 29.544s Main [ PASSED ] 1 test. C 29.544s Main [ FAILED ] 1 test, listed below: C 29.544s Main [ FAILED ] org.chromium.chrome.browser.document.LauncherActivityTest#testDoesNotCrashWithBadParcel C 29.544s Main C 29.544s Main 1 FAILED TEST C 29.544s Main ******************************************************************************** With functional changes C 25.980s Main ******************************************************************************** C 25.980s Main [==========] 2 tests ran. C 25.980s Main [ PASSED ] 2 tests. C 25.980s Main ********************************************************************************
Nice! I am very glad to see that you found a way to get bad parcelable working in instrumentation. IMO, we should remove the unit test altogether -- sorry, but I am concerned about the maintenance burden as we add new code (things will have to be mocked out/overriden/etc). The one thing the unit test does that instrumentation doesn't at the moment is check that the intent action/data didn't get mutated. We can just add this in the instrumentation test. After you launch the intent, you can do ApplicationStatus.getRunningActivities() (there should be just one) and inspect its intent. https://codereview.chromium.org/2392763002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/document/LauncherActivityTest.java (right): https://codereview.chromium.org/2392763002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/document/LauncherActivityTest.java:58: // Force unparcelling withing ChromeLauncherActivity nit: withing -> within https://codereview.chromium.org/2392763002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/document/LauncherActivityTest.java:60: intent.readFromParcel(parcel); Not sure I understand what this does -- why do we need to read from the parcel here? https://codereview.chromium.org/2392763002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/document/LauncherActivityTest.java:89: private class InvalidParcelable implements Parcelable { this should be private static class
I'll remove the unit test and address the comments. New revision incoming soon. https://codereview.chromium.org/2392763002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/document/LauncherActivityTest.java (right): https://codereview.chromium.org/2392763002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/document/LauncherActivityTest.java:58: // Force unparcelling withing ChromeLauncherActivity On 2016/10/25 17:59:23, Maria wrote: > nit: withing -> within Whoops! Will fix. https://codereview.chromium.org/2392763002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/document/LauncherActivityTest.java:60: intent.readFromParcel(parcel); On 2016/10/25 17:59:23, Maria wrote: > Not sure I understand what this does -- why do we need to read from the parcel > here? Unless we write to and then read from the parcel again, Bundles won't have to unmarshal their contents. This forces the unmarshalling the next time we try to read an extra from this intent. I'll make the comment more elaborate. https://codereview.chromium.org/2392763002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/document/LauncherActivityTest.java:89: private class InvalidParcelable implements Parcelable { On 2016/10/25 17:59:23, Maria wrote: > this should be private static class Will fix.
Hi Maria, Can you take a look at this new revision? I've removed the unit test and expanded the test to make sure a) ChromeActivity gets launched. b) Data and action are kept intact. I've also added check a) to the already existing test, as it didn't check that Chrome actually launched (at the end of its runtime it was still within ChromeLauncherActivity before) Also reran the tests with and without the functional change (and verified that without the changes, the new test still fails due to a BadParcelableException in this latest revision) Let me know if anything else needs changing. Thanks, Holger
lgtm
The CQ bit was checked by kraush@amazon.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Fixed a trivial merge conflict. No need to re-review.
The CQ bit was checked by kraush@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2392763002/#ps120001 (title: "Fixed merge conflict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/10/25 21:14:16, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) Argh, lint caught that this parcelable will throw a BadParcelableException! :) I'll add a lint suppression. Not sure why I didn't get this locally though.
On 2016/10/25 21:15:51, kraush wrote: > On 2016/10/25 21:14:16, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > > Argh, lint caught that this parcelable will throw a BadParcelableException! :) > I'll add a lint suppression. > > Not sure why I didn't get this locally though. Getting it locally as well now. Seems some new lint checks were turned into errors since I posted this CL. I'll make sure the build finishes, then post a new diff.
Ran a build with lint and fidbugs - both passed with this revision.
The CQ bit was checked by kraush@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2392763002/#ps140001 (title: "Fix lint and findbugs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Sanitize unparcable intents ChromeLauncherActivity did not have sanitation of incoming intents, and thus if for example an intent with a bad Parcel was sent, Chrome crashed. This change strips away all extras and makes sure Chrome properly opens the intended website instead. BUG=652460,412527 ========== to ========== Sanitize unparcable intents ChromeLauncherActivity did not have sanitation of incoming intents, and thus if for example an intent with a bad Parcel was sent, Chrome crashed. This change strips away all extras and makes sure Chrome properly opens the intended website instead. BUG=652460,412527 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Sanitize unparcable intents ChromeLauncherActivity did not have sanitation of incoming intents, and thus if for example an intent with a bad Parcel was sent, Chrome crashed. This change strips away all extras and makes sure Chrome properly opens the intended website instead. BUG=652460,412527 ========== to ========== Sanitize unparcable intents ChromeLauncherActivity did not have sanitation of incoming intents, and thus if for example an intent with a bad Parcel was sent, Chrome crashed. This change strips away all extras and makes sure Chrome properly opens the intended website instead. BUG=652460,412527 Committed: https://crrev.com/4bca3b37801c502a164536b804879c00aba7d304 Cr-Commit-Position: refs/heads/master@{#427697} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4bca3b37801c502a164536b804879c00aba7d304 Cr-Commit-Position: refs/heads/master@{#427697} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
