Skip to content

Commit

Permalink
Block intents to self, and block fallback URLs in sandboxed main frames
Browse files Browse the repository at this point in the history 
Intents to self and main frame fallback URLs don't currently support
maintaining sandbox attributes. I've been wanting to block intents to
self for a while, and this seems like a good reason to finally do it -
we'll instead just load the intent's target URL in the browser (unless
the frame is sandboxed, in which case we just block the navigation).

This is guarded by a kill switch in case we find that important use
cases are relying on intenting to the current browser (which would be
very weird).

Bug: 1418061
Change-Id: Iaa836be3783122ba6d3fa286bfbf131b0b5cf494
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4294954
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1115894}
  • Loading branch information
Michael Thiessen authored and Chromium LUCI CQ committed Mar 10, 2023
1 parent b1ebf57 commit 8fd3520
Show file tree
Hide file tree
Showing 23 changed files with 298 additions and 64 deletions.
4 changes: 2 additions & 2 deletions android_webview/java/src/org/chromium/android_webview/AwContents.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -688,8 +688,8 @@ public WebResourceResponseInfo shouldInterceptRequest(
//
private class InterceptNavigationDelegateImpl extends InterceptNavigationDelegate {
@Override
public boolean shouldIgnoreNavigation(
NavigationHandle navigationHandle, GURL escapedUrl, boolean crossFrame) {
public boolean shouldIgnoreNavigation(NavigationHandle navigationHandle, GURL escapedUrl,
boolean crossFrame, boolean isSandboxedFrame) {
// The shouldOverrideUrlLoading call might have resulted in posting messages to the
// UI thread. Using sendMessage here (instead of calling onPageStarted directly)
// will allow those to run in order.
Expand Down
4 changes: 2 additions & 2 deletions ...ndroid/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,8 @@ public InterceptNavigationDelegateImpl() {
}

@Override
public boolean shouldIgnoreNavigation(
NavigationHandle navigationHandle, GURL escapedUrl, boolean crossFrame) {
public boolean shouldIgnoreNavigation(NavigationHandle navigationHandle, GURL escapedUrl,
boolean crossFrame, boolean isSandboxedFrame) {
// If either of the required params for the delegate are null, do not call the
// delegate and ignore the navigation.
if (mExternalNavHandler == null || navigationHandle == null) return true;
Expand Down
4 changes: 2 additions & 2 deletions chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,8 @@ public void onLoadUrl(Tab tab, LoadUrlParams params, int loadType) {

mCustomTabNavigationDelegate = new InterceptNavigationDelegate() {
@Override
public boolean shouldIgnoreNavigation(
NavigationHandle navigationHandle, GURL escapedUrl, boolean crossFrame) {
public boolean shouldIgnoreNavigation(NavigationHandle navigationHandle,
GURL escapedUrl, boolean crossFrame, boolean isSandboxedFrame) {
if (DomDistillerUrlUtils.isDistilledPage(navigationHandle.getUrl())
|| navigationHandle.isExternalProtocol()) {
return false;
Expand Down
27 changes: 16 additions & 11 deletions ...vatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
import org.chromium.chrome.test.util.FullscreenTestUtils;
import org.chromium.chrome.test.util.MenuUtils;
import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
import org.chromium.components.external_intents.ExternalNavigationHandler;
import org.chromium.content_public.browser.NavigationHandle;
import org.chromium.content_public.browser.WebContents;
import org.chromium.content_public.browser.test.util.TestThreadUtils;
Expand Down Expand Up @@ -329,6 +330,7 @@ public void testAcceptedPrivacy(@EnabledFeature int enabledFeature) throws Excep
@SmallTest
@Feature({"ContextualSearch"})
public void testRedirectedExternalNavigationWithUserGesture() throws Exception {
ExternalNavigationHandler.sAllowIntentsToSelfForTesting = true;
simulateResolveSearch("intelligence");
GURL initialUrl = new GURL("http://test.com");
final NavigationHandle navigationHandle = NavigationHandle.createForTesting(initialUrl,
Expand All @@ -339,17 +341,17 @@ public void testRedirectedExternalNavigationWithUserGesture() throws Exception {
InstrumentationRegistry.getInstrumentation().runOnMainSync(new Runnable() {
@Override
public void run() {
Assert.assertFalse(
mPanel.getOverlayPanelContent()
.getInterceptNavigationDelegateForTesting()
.shouldIgnoreNavigation(navigationHandle, initialUrl, false));
Assert.assertFalse(mPanel.getOverlayPanelContent()
.getInterceptNavigationDelegateForTesting()
.shouldIgnoreNavigation(
navigationHandle, initialUrl, false, false));
Assert.assertEquals(0, mActivityMonitor.getHits());

navigationHandle.didRedirect(redirectUrl, true);
Assert.assertTrue(
mPanel.getOverlayPanelContent()
.getInterceptNavigationDelegateForTesting()
.shouldIgnoreNavigation(navigationHandle, redirectUrl, false));
Assert.assertTrue(mPanel.getOverlayPanelContent()
.getInterceptNavigationDelegateForTesting()
.shouldIgnoreNavigation(
navigationHandle, redirectUrl, false, false));
Assert.assertEquals(1, mActivityMonitor.getHits());
}
});
Expand All @@ -363,6 +365,7 @@ public void run() {
@SmallTest
@Feature({"ContextualSearch"})
public void testExternalNavigationWithUserGesture() throws Exception {
ExternalNavigationHandler.sAllowIntentsToSelfForTesting = true;
testExternalNavigationImpl(true);
}

Expand All @@ -374,6 +377,7 @@ public void testExternalNavigationWithUserGesture() throws Exception {
@SmallTest
@Feature({"ContextualSearch"})
public void testExternalNavigationWithoutUserGesture() throws Exception {
ExternalNavigationHandler.sAllowIntentsToSelfForTesting = true;
testExternalNavigationImpl(false);
}

Expand All @@ -386,9 +390,10 @@ private void testExternalNavigationImpl(boolean hasGesture) throws Exception {
InstrumentationRegistry.getInstrumentation().runOnMainSync(new Runnable() {
@Override
public void run() {
Assert.assertTrue(mPanel.getOverlayPanelContent()
.getInterceptNavigationDelegateForTesting()
.shouldIgnoreNavigation(navigationHandle, url, false));
Assert.assertTrue(
mPanel.getOverlayPanelContent()
.getInterceptNavigationDelegateForTesting()
.shouldIgnoreNavigation(navigationHandle, url, false, false));
}
});
Assert.assertEquals(hasGesture ? 1 : 0, mActivityMonitor.getHits());
Expand Down
1 change: 1 addition & 0 deletions ...javatests/src/org/chromium/chrome/browser/customtabs/CustomTabExternalNavigationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ private void launchTwa(String twaPackageName, String url) throws TimeoutExceptio
@Test
@SmallTest
public void testExternalActivityStartedForDefaultUrl() {
ExternalNavigationHandler.sAllowIntentsToSelfForTesting = true;
final GURL testUrl = new GURL("customtab://customtabtest/intent");
RedirectHandler redirectHandler = RedirectHandler.create();
redirectHandler.updateNewUrlLoading(PageTransition.LINK, false, true, 0, 0, false, true);
Expand Down
182 changes: 158 additions & 24 deletions chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,8 @@ public class UrlOverridingTest {
BASE_PATH + "navigation_from_page_show.html";
private static final String SUBFRAME_NAVIGATION_PARENT =
BASE_PATH + "subframe_navigation_parent.html";
private static final String SUBFRAME_NAVIGATION_PARENT_SANDBOX =
BASE_PATH + "subframe_navigation_parent_sandbox.html";
private static final String SUBFRAME_NAVIGATION_CHILD =
BASE_PATH + "subframe_navigation_child.html";
private static final String NAVIGATION_FROM_RENAVIGATE_FRAME =
Expand Down Expand Up @@ -494,29 +496,20 @@ public void onNewTabCreated(Tab newTab, @TabCreationState int creationState) {
}
}

if (shouldFailNavigation) {
try {
failCallback.waitForCallback(0, 1, 20, TimeUnit.SECONDS);
} catch (TimeoutException ex) {
Assert.fail("Haven't received navigation failure of intents.");
return OverrideUrlLoadingResult.forNoOverride();
}
}

if (createsNewTab) {
try {
newTabCallback.waitForCallback(0, 1, 20, TimeUnit.SECONDS);
} catch (TimeoutException ex) {
Assert.fail("New Tab was not created.");
}
}

if (shouldLaunchExternalIntent) {
try {
destroyedCallback.waitForCallback(0, 1, 20, TimeUnit.SECONDS);
} catch (TimeoutException ex) {
Assert.fail("Intercepted new tab wasn't destroyed.");
return OverrideUrlLoadingResult.forNoOverride();
}
if (shouldFailNavigation) {
try {
failCallback.waitForCallback(0, 1, 20, TimeUnit.SECONDS);
} catch (TimeoutException ex) {
Assert.fail("Haven't received navigation failure of intents.");
return OverrideUrlLoadingResult.forNoOverride();
}
}

Expand Down Expand Up @@ -556,6 +549,15 @@ public void onNewTabCreated(Tab newTab, @TabCreationState int creationState) {
Criteria.checkThat(latestTab.getUrl().getSpec(), Matchers.is(expectedFinalUrl));
});

if (createsNewTab && shouldLaunchExternalIntent) {
try {
destroyedCallback.waitForCallback(0, 1, 20, TimeUnit.SECONDS);
} catch (TimeoutException ex) {
Assert.fail("Intercepted new tab wasn't destroyed.");
return OverrideUrlLoadingResult.forNoOverride();
}
}

CriteriaHelper.pollUiThread(() -> {
Criteria.checkThat(
mActivityMonitor.getHits(), Matchers.is(shouldLaunchExternalIntent ? 1 : 0));
Expand Down Expand Up @@ -598,7 +600,8 @@ private void assertMessagePresent() throws Exception {
Assert.assertNotNull(message.get(MessageBannerProperties.ICON));
}

private String getSubframeNavigationUrl(String subframeTargetUrl) {
private String getSubframeNavigationUrl(
String subframeTargetUrl, boolean openInNewTab, boolean sandbox) {
// The replace_text parameters for SUBFRAME_NAVIGATION_CHILD, which is loaded in
// the iframe in SUBFRAME_NAVIGATION_PARENT, have to go through the
// embedded test server twice and, as such, have to be base64-encoded twice.
Expand All @@ -609,11 +612,18 @@ private String getSubframeNavigationUrl(String subframeTargetUrl) {
byte[] base64SubframeUrl = Base64.encode(
ApiCompatibilityUtils.getBytesUtf8(subframeTargetUrl), Base64.URL_SAFE);

return mTestServer.getURL(SUBFRAME_NAVIGATION_PARENT
byte[] paramBlank = ApiCompatibilityUtils.getBytesUtf8("PARAM_BLANK");
byte[] valBlank = ApiCompatibilityUtils.getBytesUtf8("_blank");

String url = sandbox ? SUBFRAME_NAVIGATION_PARENT_SANDBOX : SUBFRAME_NAVIGATION_PARENT;

return mTestServer.getURL(url
+ "?replace_text=" + Base64.encodeToString(paramBase64Name, Base64.URL_SAFE) + ":"
+ Base64.encodeToString(base64ParamSubframeUrl, Base64.URL_SAFE)
+ "&replace_text=" + Base64.encodeToString(paramBase64Value, Base64.URL_SAFE) + ":"
+ Base64.encodeToString(base64SubframeUrl, Base64.URL_SAFE));
+ Base64.encodeToString(base64SubframeUrl, Base64.URL_SAFE)
+ "&replace_text=" + Base64.encodeToString(paramBlank, Base64.URL_SAFE) + ":"
+ (openInNewTab ? Base64.encodeToString(valBlank, Base64.URL_SAFE) : ""));
}

@Test
Expand Down Expand Up @@ -722,7 +732,7 @@ public void testNavigationWithFallbackURLInSubFrame_FallbackDisabled() {
String fallbackUrl = mTestServer.getURL(FALLBACK_LANDING_PATH);
String subframeUrl = "intent://test/#Intent;scheme=badscheme;S.browser_fallback_url="
+ fallbackUrl + ";end";
String originalUrl = getSubframeNavigationUrl(subframeUrl);
String originalUrl = getSubframeNavigationUrl(subframeUrl, false, false);

OverrideUrlLoadingResult result = loadUrlAndWaitForIntentUrl(originalUrl, true, false);

Expand All @@ -739,7 +749,7 @@ public void testNavigationWithFallbackURLInSubFrame_FallbackEnabled() throws Exc
String fallbackUrl = mTestServer.getURL(FALLBACK_LANDING_PATH);
String subframeUrl = "intent://test/#Intent;scheme=badscheme;S.browser_fallback_url="
+ fallbackUrl + ";end";
String originalUrl = getSubframeNavigationUrl(subframeUrl);
String originalUrl = getSubframeNavigationUrl(subframeUrl, false, false);

final Tab tab = mActivityTestRule.getActivity().getActivityTab();

Expand Down Expand Up @@ -1408,7 +1418,7 @@ public void testRedirectFromCCTSpeculation() throws Exception {
+ "https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.android.chrome"
+ ";end";

String originalUrl = getSubframeNavigationUrl(subframeTarget);
String originalUrl = getSubframeNavigationUrl(subframeTarget, false, false);

final Tab tab = mActivityTestRule.getActivity().getActivityTab();

Expand Down Expand Up @@ -1449,7 +1459,7 @@ void doTestIncognitoSubframeExternalNavigation(boolean acceptPrompt) throws Exce
String subframeUrl =
"intent://test/#Intent;scheme=externalappscheme;S.browser_fallback_url="
+ fallbackUrl + ";end";
String originalUrl = getSubframeNavigationUrl(subframeUrl);
String originalUrl = getSubframeNavigationUrl(subframeUrl, false, false);

final Tab tab = mActivityTestRule.getActivity().getActivityTab();

Expand Down Expand Up @@ -1523,7 +1533,7 @@ public void testWindowOpenRedirect() throws Exception {
}

@Test
@SmallTest
@LargeTest
@Features.EnableFeatures({ExternalIntentsFeatures.BLOCK_FRAME_RENAVIGATIONS_NAME})
public void testWindowRenavigation() throws Exception {
String finalUrl = mTestServer.getURL(HELLO_PAGE);
Expand All @@ -1534,4 +1544,128 @@ public void testWindowRenavigation() throws Exception {
Assert.assertEquals(OverrideUrlLoadingResultType.NO_OVERRIDE, result.getResultType());
Assert.assertNull(getCurrentExternalNavigationMessage());
}

@Test
@LargeTest
@Features.EnableFeatures({ExternalIntentsFeatures.BLOCK_SUBFRAME_INTENT_TO_SELF_NAME,
ExternalIntentsFeatures.BLOCK_INTENTS_TO_SELF_NAME})
public void
testIntentToSelf() {
String targetUrl = mTestServer.getURL(HELLO_PAGE);
// Strip off the https: from the URL.
String strippedTargetUrl = targetUrl.substring(6);
String link = "intent:" + strippedTargetUrl + "#Intent;scheme=https;package="
+ ContextUtils.getApplicationContext().getPackageName() + ";end";

byte[] paramName = ApiCompatibilityUtils.getBytesUtf8("PARAM_SUBFRAME_URL");
byte[] paramValue = ApiCompatibilityUtils.getBytesUtf8(link);

String url = mTestServer.getURL(SUBFRAME_NAVIGATION_CHILD
+ "?replace_text=" + Base64.encodeToString(paramName, Base64.URL_SAFE) + ":"
+ Base64.encodeToString(paramValue, Base64.URL_SAFE));

mActivityTestRule.startMainActivityOnBlankPage();
loadUrlAndWaitForIntentUrl(url, true, false, false, targetUrl, true);
}

@Test
@LargeTest
@Features.EnableFeatures({ExternalIntentsFeatures.BLOCK_SUBFRAME_INTENT_TO_SELF_NAME,
ExternalIntentsFeatures.BLOCK_INTENTS_TO_SELF_NAME})
public void
testIntentToSelfWithFallback() throws Exception {
mActivityTestRule.startMainActivityOnBlankPage();

String targetUrl = mTestServer.getURL(HELLO_PAGE);
// Strip off the https: from the URL.
String strippedTargetUrl = targetUrl.substring(6);
String subframeTarget = "intent:" + strippedTargetUrl + "#Intent;scheme=https;package="
+ ContextUtils.getApplicationContext().getPackageName() + ";S.browser_fallback_url="
+ "https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.android.chrome"
+ ";end";

String originalUrl = getSubframeNavigationUrl(subframeTarget, true, false);

final Tab tab = mActivityTestRule.getActivity().getActivityTab();

final CallbackHelper subframeRedirect = new CallbackHelper();
final AtomicInteger navCount = new AtomicInteger(0);
EmptyTabObserver observer = new EmptyTabObserver() {
@Override
public void onDidStartNavigationInPrimaryMainFrame(
Tab tab, NavigationHandle navigation) {
int count = navCount.getAndIncrement();
if (count == 0) {
Assert.assertEquals(originalUrl, navigation.getUrl().getSpec());
} else if (count == 1) {
Assert.assertEquals(subframeTarget, navigation.getUrl().getSpec());
} else if (count == 2) {
Assert.assertEquals(targetUrl, navigation.getUrl().getSpec());
} else {
Assert.fail();
}
}
};
TestThreadUtils.runOnUiThreadBlocking(() -> { tab.addObserver(observer); });

OverrideUrlLoadingResult result =
loadUrlAndWaitForIntentUrl(originalUrl, true, true, false, targetUrl, true);
Assert.assertEquals(
OverrideUrlLoadingResultType.OVERRIDE_WITH_NAVIGATE_TAB, result.getResultType());
}

// Ensures that for a sandboxed main frame, we block both intents to ourself, and fallback URLs
// that would escape the sandbox by clobbering the main frame.
@Test
@LargeTest
@Features.EnableFeatures({ExternalIntentsFeatures.BLOCK_SUBFRAME_INTENT_TO_SELF_NAME,
ExternalIntentsFeatures.BLOCK_INTENTS_TO_SELF_NAME})
public void
testIntentToSelfWithFallback_Sandboxed() throws Exception {
mActivityTestRule.startMainActivityOnBlankPage();

String targetUrl = mTestServer.getURL(HELLO_PAGE);
// Strip off the https: from the URL.
String strippedTargetUrl = targetUrl.substring(6);
String subframeTarget = "intent:" + strippedTargetUrl + "#Intent;scheme=https;package="
+ ContextUtils.getApplicationContext().getPackageName() + ";S.browser_fallback_url="
+ "https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.android.chrome"
+ ";end";

String originalUrl = getSubframeNavigationUrl(subframeTarget, true, true);

final Tab tab = mActivityTestRule.getActivity().getActivityTab();

final CallbackHelper subframeRedirect = new CallbackHelper();
final AtomicInteger navCount = new AtomicInteger(0);
EmptyTabObserver observer = new EmptyTabObserver() {
@Override
public void onDidStartNavigationInPrimaryMainFrame(
Tab tab, NavigationHandle navigation) {
int count = navCount.getAndIncrement();
if (count == 0) {
Assert.assertEquals(originalUrl, navigation.getUrl().getSpec());
} else if (count == 1) {
Assert.assertEquals(subframeTarget, navigation.getUrl().getSpec());
} else {
Assert.fail();
}
}
};
TestThreadUtils.runOnUiThreadBlocking(() -> { tab.addObserver(observer); });

OverrideUrlLoadingResult result =
loadUrlAndWaitForIntentUrl(originalUrl, true, true, false, null, true);
// Navigation to self is blocked, ExternalNavigationHandler asks to navigate to the
// fallback URL.
Assert.assertEquals(
OverrideUrlLoadingResultType.OVERRIDE_WITH_NAVIGATE_TAB, result.getResultType());
// Fallback URL is blocked by InterceptNavigationDelegateImpl, no URL is loading and the
// final URL is the subframe's target.
TestThreadUtils.runOnUiThreadBlocking(() -> {
Tab newTab = mActivityTestRule.getActivity().getActivityTab();
Assert.assertEquals(subframeTarget, newTab.getUrl().getSpec());
Assert.assertFalse(newTab.getWebContents().isLoading());
});
}
}

0 comments on commit 8fd3520

Please sign in to comment.