Skip to content

Commit

Permalink
🛃 Prevent CCT launch to JavaScript URLs.
Browse files Browse the repository at this point in the history
Because Custom Tabs don't have a fallback UI like ChromeTabbedActivity
does (the New Tab Page), it would be awkward to check whether a given
URL is valid after launching the Custom Tab. So we prevent the Custom
Tab being launched if the URL is invalid.

Bug: 1240065
Change-Id: If4e5ce0ff6522f06a400f70746cfc08808dabcbc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3790983
Commit-Queue: Peter Conn <peconn@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033921}
  • Loading branch information
PEConn authored and Chromium LUCI CQ committed Aug 11, 2022
1 parent 5a3919f commit a006fac
Show file tree
Hide file tree
Showing 6 changed files with 134 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -929,6 +929,18 @@ public static long getTimestampFromIntent(Intent intent) {
* @return true if the intent should be ignored.
*/
public boolean shouldIgnoreIntent(Intent intent, boolean startedActivity) {
return shouldIgnoreIntent(intent, startedActivity, /*isCustomTab=*/false);
}

/**
* Returns true if the app should ignore a given intent.
*
* @param intent Intent to check.
* @param startedActivity True if the Activity was not running prior to receiving the Intent.
* @param isCustomTab True if the Intent will end up in a Custom Tab.
* @return true if the intent should be ignored.
*/
public boolean shouldIgnoreIntent(Intent intent, boolean startedActivity, boolean isCustomTab) {
// Although not documented to, many/most methods that retrieve values from an Intent may
// throw. Because we can't control what packages might send to us, we should catch any
// Throwable and then fail closed (safe). This is ugly, but resolves top crashers in the
Expand All @@ -944,18 +956,8 @@ public boolean shouldIgnoreIntent(Intent intent, boolean startedActivity) {
boolean isInternal = notSecureIsIntentChromeOrFirstParty(intent);
boolean isFromChrome = wasIntentSenderChrome(intent);

// "Open new incognito tab" is currently limited to Chrome.
//
// The pending incognito URL check is to handle the case where the user is shown an
// Android intent picker while in incognito and they select the current Chrome instance
// from the list. In this case, we do not apply our Chrome token as the user has the
// option to select apps outside of our control, so we rely on this in memory check
// instead.
if (!isFromChrome
&& IntentUtils.safeGetBooleanExtra(
intent, EXTRA_OPEN_NEW_INCOGNITO_TAB, false)
&& (getPendingIncognitoUrl() == null
|| !getPendingIncognitoUrl().equals(intent.getDataString()))) {
if (IntentUtils.safeGetBooleanExtra(intent, EXTRA_OPEN_NEW_INCOGNITO_TAB, false)
&& !isAllowedIncognitoIntent(isFromChrome, isCustomTab, intent)) {
return true;
}

Expand Down Expand Up @@ -999,6 +1001,22 @@ public boolean shouldIgnoreIntent(Intent intent, boolean startedActivity) {
}
}

private static boolean isAllowedIncognitoIntent(
boolean isChrome, boolean isCustomTab, Intent intent) {
// "Open new incognito tab" is currently limited to Chrome for the Chrome app. It can be
// launched by external apps if it's a Custom Tab, although there are additional checks in
// IncognitoCustomTabIntentDataProvider#isValidIncognitoIntent.
if (isChrome || isCustomTab) return true;

// The pending incognito URL check is to handle the case where the user is shown an
// Android intent picker while in incognito and they select the current Chrome instance
// from the list. In this case, we do not apply our Chrome token as the user has the
// option to select apps outside of our control, so we rely on this in memory check
// instead.
String pendingUrl = getPendingIncognitoUrl();
return pendingUrl != null && pendingUrl.equals(intent.getDataString());
}

private static boolean intentHasUnsafeInternalScheme(String scheme, String url, Intent intent) {
if (scheme != null
&& (intent.hasCategory(Intent.CATEGORY_BROWSABLE)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,11 @@ public class LaunchIntentDispatcher implements IntentHandler.IntentHandlerDelega
public static @Action int dispatchToCustomTabActivity(Activity currentActivity, Intent intent) {
LaunchIntentDispatcher dispatcher = new LaunchIntentDispatcher(currentActivity, intent);
if (!isCustomTabIntent(dispatcher.mIntent)) return Action.CONTINUE;
dispatcher.launchCustomTabActivity();
return Action.FINISH_ACTIVITY;
if (dispatcher.launchCustomTabActivity(new IntentHandler(currentActivity, dispatcher))) {
return Action.FINISH_ACTIVITY;
} else {
return Action.CONTINUE;
}
}

private LaunchIntentDispatcher(Activity activity, Intent intent) {
Expand Down Expand Up @@ -187,7 +190,7 @@ private LaunchIntentDispatcher(Activity activity, Intent intent) {

// Check if we should launch a Custom Tab.
if (isCustomTabIntent) {
launchCustomTabActivity();
launchCustomTabActivity(intentHandler);
return Action.FINISH_ACTIVITY;
}

Expand Down Expand Up @@ -347,17 +350,24 @@ private static SessionDataHolder getSessionDataHolder() {

/**
* Handles launching a {@link CustomTabActivity}, which will sit on top of a client's activity
* in the same task.
* in the same task. Returns whether an Activity was launched (or brought to the foreground).
*/
private void launchCustomTabActivity() {
private boolean launchCustomTabActivity(IntentHandler intentHandler) {
CustomTabsConnection.getInstance().onHandledIntent(
CustomTabsSessionToken.getSessionTokenFromIntent(mIntent), mIntent);

boolean startedActivity = false;
boolean isCustomTab = true;
if (intentHandler.shouldIgnoreIntent(mIntent, startedActivity, isCustomTab)) {
return false;
}

if (!clearTopIntentsForCustomTabsEnabled(mIntent)) {
// The old way of delivering intents relies on calling the activity directly via a
// static reference. It doesn't allow using CLEAR_TOP, and also doesn't work when an
// intent brings the task to foreground. The condition above is a temporary safety net.
boolean handled = getSessionDataHolder().handleIntent(mIntent);
if (handled) return;
if (handled) return true;
}
maybePrefetchDnsInBackground();

Expand All @@ -372,10 +382,11 @@ private void launchCustomTabActivity() {
// Samsung devices, see https://crbug.com/796548.
try (StrictModeContext ignored = StrictModeContext.allowDiskWrites()) {
if (TwaSplashController.handleIntent(mActivity, launchIntent)) {
return;
return true;
}

mActivity.startActivity(launchIntent, null);
return true;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1267,7 +1267,9 @@ public void onNewIntentWithNative(Intent intent) {

super.onNewIntentWithNative(intent);
getLaunchCauseMetrics().onReceivedIntent();
if (mIntentHandler.shouldIgnoreIntent(intent, /*startedActivity=*/false)) return;
if (mIntentHandler.shouldIgnoreIntent(intent, /*startedActivity=*/false, isCustomTab())) {
return;
}

// We send this intent so that we can enter WebVr presentation mode if needed. This
// call doesn't consume the intent because it also has the url that we need to load.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,9 @@ protected BaseCustomTabActivityComponent createComponent(
ModuleFactoryOverrides.getOverrideFor(BaseCustomTabActivityModule.Factory.class);

// mIntentHandler comes from the base class.
IntentIgnoringCriterion intentIgnoringCriterion =
(intent) -> mIntentHandler.shouldIgnoreIntent(intent, /*startedActivity=*/true);
IntentIgnoringCriterion intentIgnoringCriterion = (intent)
-> mIntentHandler.shouldIgnoreIntent(
intent, /*startedActivity=*/true, isCustomTab());

BaseCustomTabActivityModule baseCustomTabsModule = overridenBaseCustomTabFactory != null
? overridenBaseCustomTabFactory.create(mIntentDataProvider,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,45 @@ public void testShouldIgnoreIntentTranslateStartsActivity() {
assertTrue(mIntentHandler.shouldIgnoreIntent(intent, /*startedActivity=*/true));
}

/**
* Test that IntentHandler#shouldIgnoreIntent() returns true for Incognito non-Custom Tab
* Intents.
*/
@Test
@SmallTest
@Feature({"Android-AppBase"})
public void testShouldIgnoreIncognitoIntent() {
Intent intent = new Intent(GOOGLE_URL);
intent.putExtra(IntentHandler.EXTRA_OPEN_NEW_INCOGNITO_TAB, true);
assertTrue(mIntentHandler.shouldIgnoreIntent(intent, /*startedActivity=*/true));
}

/**
* Test that IntentHandler#shouldIgnoreIntent() returns false for Incognito non-Custom Tab
* Intents if they come from Chrome.
*/
@Test
@SmallTest
@Feature({"Android-AppBase"})
public void testShouldIgnoreIncognitoIntent_trusted() {
Context context = InstrumentationRegistry.getTargetContext();
Intent intent = IntentHandler.createTrustedOpenNewTabIntent(context, true);
assertFalse(mIntentHandler.shouldIgnoreIntent(intent, /*startedActivity=*/true));
}

/**
* Test that IntentHandler#shouldIgnoreIntent() returns false for Incognito Custom Tab Intents.
*/
@Test
@SmallTest
@Feature({"Android-AppBase"})
public void testShouldIgnoreIncognitoIntent_customTab() {
Intent intent = new Intent(GOOGLE_URL);
intent.putExtra(IntentHandler.EXTRA_OPEN_NEW_INCOGNITO_TAB, true);
assertFalse(mIntentHandler.shouldIgnoreIntent(
intent, /*startedActivity=*/true, /*isCustomTab=*/true));
}

@Test
@SmallTest
public void testIgnoreUnauthenticatedBringToFront() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;

import static org.chromium.base.test.util.Restriction.RESTRICTION_TYPE_NON_LOW_END_DEVICE;
import static org.chromium.chrome.browser.customtabs.CustomTabsTestUtils.createTestBitmap;
Expand Down Expand Up @@ -65,6 +68,7 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mockito;

import org.chromium.base.ActivityState;
import org.chromium.base.ApplicationStatus;
Expand All @@ -91,6 +95,7 @@
import org.chromium.chrome.browser.ChromeApplicationImpl;
import org.chromium.chrome.browser.ChromeTabbedActivity;
import org.chromium.chrome.browser.IntentHandler;
import org.chromium.chrome.browser.LaunchIntentDispatcher;
import org.chromium.chrome.browser.TabsOpenedFromExternalAppTest;
import org.chromium.chrome.browser.WarmupManager;
import org.chromium.chrome.browser.browserservices.SessionDataHolder;
Expand Down Expand Up @@ -2063,4 +2068,40 @@ private void assertLastLaunchedClientAppRecorded(String histogramSuffix, String
umaRecorded ? 1 : 0,
RecordHistogram.getHistogramTotalCountForTesting(histogramName));
}

@Test
@SmallTest
public void doesNotLaunchJavaScriptUrls_dispatchToCustomTabActivity() {
Context context = InstrumentationRegistry.getTargetContext();
String javaScriptUrl = "javascript: alert('Hello');";

TestThreadUtils.runOnUiThreadBlocking(() -> {
Intent intent =
CustomTabsIntentTestUtils.createMinimalCustomTabIntent(context, javaScriptUrl);

Activity activity = Mockito.mock(Activity.class);
@LaunchIntentDispatcher.Action
int result = LaunchIntentDispatcher.dispatchToCustomTabActivity(activity, intent);
assertEquals(LaunchIntentDispatcher.Action.CONTINUE, result);
verify(activity, never()).startActivity(any(), any());
});
}

@Test
@SmallTest
public void doesNotLaunchJavaScriptUrls_dispatch() {
Context context = InstrumentationRegistry.getTargetContext();
String javaScriptUrl = "javascript: alert('Hello');";

TestThreadUtils.runOnUiThreadBlocking(() -> {
Intent intent =
CustomTabsIntentTestUtils.createMinimalCustomTabIntent(context, javaScriptUrl);

Activity activity = Mockito.mock(Activity.class);
@LaunchIntentDispatcher.Action
int result = LaunchIntentDispatcher.dispatch(activity, intent);
assertEquals(LaunchIntentDispatcher.Action.FINISH_ACTIVITY, result);
verify(activity, never()).startActivity(any(), any());
});
}
}

0 comments on commit a006fac

Please sign in to comment.