From 6f02e00b7927855e1a3341f5dd198a0a8515982f Mon Sep 17 00:00:00 2001 From: tedchoc Date: Mon, 6 Jul 2015 14:27:31 -0700 Subject: [PATCH] Ensure we have file access before loading file URLs. BUG=506336 Review URL: https://codereview.chromium.org/1219623011 Cr-Commit-Position: refs/heads/master@{#337480} --- .../externalnav/ExternalNavigationDelegate.java | 17 +++++ .../ExternalNavigationDelegateImpl.java | 37 +++++++++-- .../externalnav/ExternalNavigationHandler.java | 20 ++++-- .../externalnav/ExternalNavigationParams.java | 38 +++++++---- .../org/chromium/chrome/browser/tab/ChromeTab.java | 4 +- .../externalnav/ExternalNavigationHandlerTest.java | 75 +++++++++++++++++++++- 6 files changed, 166 insertions(+), 25 deletions(-) diff --git a/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java b/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java index d9c5889648c9..848129af5c03 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java @@ -66,6 +66,23 @@ interface ExternalNavigationDelegate { boolean needsToCloseTab); /** + * @param tab The current tab. + * @return Whether we should block the navigation and request file access before proceeding. + */ + boolean shouldRequestFileAccess(Tab tab); + + /** + * Trigger a UI affordance that will ask the user to grant file access. After the access + * has been granted or denied, continue loading the specified file URL. + * + * @param intent The intent to continue loading the file URL. + * @param referrerUrl The HTTP referrer URL. + * @param tab The current tab. + * @param needsToCloseTab Whether this action should close the current tab. + */ + void startFileIntent(Intent intent, String referrerUrl, Tab tab, boolean needsToCloseTab); + + /** * Clobber the current tab and try not to pass an intent when it should be handled by Chrome * so that we can deliver HTTP referrer information safely. * diff --git a/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java index 6510ac488538..7ceb9711784d 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java @@ -33,6 +33,7 @@ import org.chromium.chrome.browser.util.IntentUtils; import org.chromium.content_public.browser.LoadUrlParams; import org.chromium.content_public.common.Referrer; import org.chromium.ui.base.PageTransition; +import org.chromium.ui.base.WindowAndroid; import java.util.List; @@ -214,20 +215,48 @@ public class ExternalNavigationDelegateImpl implements ExternalNavigationDelegat .setNegativeButton(R.string.cancel, new OnClickListener() { @Override public void onClick(DialogInterface dialog, int which) { - loadIntent(intent, referrerUrl, fallbackUrl, tab, needsToCloseTab); + loadIntent(intent, referrerUrl, fallbackUrl, tab, needsToCloseTab, true); } }) .setOnCancelListener(new OnCancelListener() { @Override public void onCancel(DialogInterface dialog) { - loadIntent(intent, referrerUrl, fallbackUrl, tab, needsToCloseTab); + loadIntent(intent, referrerUrl, fallbackUrl, tab, needsToCloseTab, true); } }) .show(); } + @Override + public boolean shouldRequestFileAccess(Tab tab) { + // If the tab is null, then do not attempt to prompt for access. + if (tab == null) return false; + + return !tab.getWindowAndroid().hasFileAccess(); + } + + @Override + public void startFileIntent(final Intent intent, final String referrerUrl, final Tab tab, + final boolean needsToCloseTab) { + tab.getWindowAndroid().requestFileAccess(new WindowAndroid.FileAccessCallback() { + @Override + public void onFileAccessResult(boolean granted) { + if (granted) { + loadIntent(intent, referrerUrl, null, tab, needsToCloseTab, tab.isIncognito()); + } else { + // TODO(tedchoc): Show an indication to the user that the navigation failed + // instead of silently dropping it on the floor. + if (needsToCloseTab) { + // If the access was not granted, then close the tab if necessary. + tab.getChromeWebContentsDelegateAndroid().closeContents(); + } + } + } + }); + } + private void loadIntent(Intent intent, String referrerUrl, String fallbackUrl, Tab tab, - boolean needsToCloseTab) { + boolean needsToCloseTab, boolean launchIncogntio) { boolean needsToStartIntent = false; if (tab == null || tab.isClosing() || !tab.isInitialized()) { needsToStartIntent = true; @@ -245,7 +274,7 @@ public class ExternalNavigationDelegateImpl implements ExternalNavigationDelegat if (needsToStartIntent) { intent = new Intent(Intent.ACTION_VIEW, Uri.parse(url)); intent.putExtra(Browser.EXTRA_APPLICATION_ID, getPackageName()); - intent.putExtra(IntentHandler.EXTRA_OPEN_NEW_INCOGNITO_TAB, true); + if (launchIncogntio) intent.putExtra(IntentHandler.EXTRA_OPEN_NEW_INCOGNITO_TAB, true); intent.addCategory(Intent.CATEGORY_BROWSABLE); intent.setPackage(getPackageName()); intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK); diff --git a/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java b/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java index 78a4c6315644..2811ad919f8c 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java @@ -48,8 +48,9 @@ public class ExternalNavigationHandler { OVERRIDE_WITH_EXTERNAL_INTENT, /* We should override the URL loading and clobber the current tab. */ OVERRIDE_WITH_CLOBBERING_TAB, - /* We should override the URL loading and handle an intent in incognito mode. */ - OVERRIDE_WITH_INCOGNITO_MODE, + /* We should override the URL loading. The desired action will be determined + * asynchronously (e.g. by requiring user confirmation). */ + OVERRIDE_WITH_ASYNC_ACTION, /* We shouldn't override the URL loading. */ NO_OVERRIDE, } @@ -152,6 +153,17 @@ public class ExternalNavigationHandler { return OverrideUrlLoadingResult.NO_OVERRIDE; } + // If accessing a file URL, ensure that the user has granted the necessary file access + // to Chrome. This check should happen for reloads, navigations, etc..., which is why + // it occurs before the subsequent blocks. + if (params.getUrl().startsWith("file:") + && mDelegate.shouldRequestFileAccess(params.getTab())) { + mDelegate.startFileIntent( + intent, params.getReferrerUrl(), params.getTab(), + params.shouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent()); + return OverrideUrlLoadingResult.OVERRIDE_WITH_ASYNC_ACTION; + } + // http://crbug/331571 : Do not override a navigation started from user typing. // http://crbug/424029 : Need to stay in Chrome for an intent heading explicitly to Chrome. if (params.getRedirectHandler() != null) { @@ -340,8 +352,8 @@ public class ExternalNavigationHandler { // to apps out side of Chrome. mDelegate.startIncognitoIntent(intent, params.getReferrerUrl(), hasBrowserFallbackUrl ? browserFallbackUrl : null, params.getTab(), - params.needsToCloseTabAfterIncognitoDialog()); - return OverrideUrlLoadingResult.OVERRIDE_WITH_INCOGNITO_MODE; + params.shouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent()); + return OverrideUrlLoadingResult.OVERRIDE_WITH_ASYNC_ACTION; } else { if (params.getRedirectHandler() != null && incomingIntentRedirect) { if (!params.getRedirectHandler().hasNewResolver(intent)) { diff --git a/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationParams.java b/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationParams.java index f67fea36fbda..16aa3eee1b92 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationParams.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationParams.java @@ -43,14 +43,17 @@ public class ExternalNavigationParams { /** Whether this navigation happens in main frame. */ private final boolean mIsMainFrame; - /** Whether closing tab is needed or not after incognito dialog is closed. */ - private final boolean mNeedsToCloseTabAfterIncognitoDialog; + /** + * Whether the current tab should be closed when an URL load was overridden and an + * intent launched. + */ + private final boolean mShouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent; private ExternalNavigationParams(String url, boolean isIncognito, String referrerUrl, int pageTransition, boolean isRedirect, boolean appMustBeInForeground, TabRedirectHandler redirectHandler, Tab tab, boolean openInNewTab, boolean isBackgroundTabNavigation, boolean isMainFrame, - boolean needsToCloseTabAfterIncognitoDialog) { + boolean shouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent) { mUrl = url; mIsIncognito = isIncognito; mPageTransition = pageTransition; @@ -62,7 +65,8 @@ public class ExternalNavigationParams { mOpenInNewTab = openInNewTab; mIsBackgroundTabNavigation = isBackgroundTabNavigation; mIsMainFrame = isMainFrame; - mNeedsToCloseTabAfterIncognitoDialog = needsToCloseTabAfterIncognitoDialog; + mShouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent = + shouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent; } /** @return The URL to potentially open externally. */ @@ -123,9 +127,12 @@ public class ExternalNavigationParams { return mIsMainFrame; } - /** @return Whether closing tab is needed or not after incognito dialog is closed. */ - public boolean needsToCloseTabAfterIncognitoDialog() { - return mNeedsToCloseTabAfterIncognitoDialog; + /** + * @return Whether the current tab should be closed when an URL load was overridden and an + * intent launched. + */ + public boolean shouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent() { + return mShouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent; } /** The builder for {@link ExternalNavigationParams} objects. */ @@ -162,8 +169,11 @@ public class ExternalNavigationParams { /** Whether this navigation happens in main frame. */ private boolean mIsMainFrame; - /** Whether closing tab is needed or not after incognito dialog is closed. */ - private boolean mNeedsToCloseTabAfterIncognitoDialog; + /** + * Whether the current tab should be closed when an URL load was overridden and an + * intent launched. + */ + private boolean mShouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent; public Builder(String url, boolean isIncognito) { mUrl = url; @@ -215,9 +225,11 @@ public class ExternalNavigationParams { return this; } - /** Sets whether closing tab is needed or not after incognito dialog is closed. */ - public Builder setNeedsToCloseTabAfterIncognitoDialog(boolean v) { - mNeedsToCloseTabAfterIncognitoDialog = v; + /** Sets whether the current tab should be closed when an URL load was overridden and an + * intent launched. + */ + public Builder setShouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent(boolean v) { + mShouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent = v; return this; } @@ -226,7 +238,7 @@ public class ExternalNavigationParams { return new ExternalNavigationParams(mUrl, mIsIncognito, mReferrerUrl, mPageTransition, mIsRedirect, mApplicationMustBeInForeground, mRedirectHandler, mTab, mOpenInNewTab, mIsBackgroundTabNavigation, - mIsMainFrame, mNeedsToCloseTabAfterIncognitoDialog); + mIsMainFrame, mShouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent); } } } diff --git a/chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java b/chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java index 4376bbee5eae..9b1fef45e581 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/tab/ChromeTab.java @@ -1258,7 +1258,7 @@ public class ChromeTab extends Tab { .setOpenInNewTab(shouldCloseTab) .setIsBackgroundTabNavigation(isHidden() && !isInitialTabLaunchInBackground) .setIsMainFrame(navigationParams.isMainFrame) - .setNeedsToCloseTabAfterIncognitoDialog(shouldCloseTab + .setShouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent(shouldCloseTab && navigationParams.isMainFrame) .build(); ExternalNavigationHandler.OverrideUrlLoadingResult result = @@ -1274,7 +1274,7 @@ public class ChromeTab extends Tab { case OVERRIDE_WITH_CLOBBERING_TAB: mShouldClearRedirectHistoryForTabClobbering = true; return true; - case OVERRIDE_WITH_INCOGNITO_MODE: + case OVERRIDE_WITH_ASYNC_ACTION: if (!shouldCloseTab && navigationParams.isMainFrame) { onOverrideUrlLoadingAndLaunchIntent(); } diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java index 589136b3910a..4ff4182ee4b9 100644 --- a/chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java +++ b/chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandlerTest.java @@ -4,6 +4,7 @@ package org.chromium.chrome.browser.externalnav; +import android.annotation.SuppressLint; import android.content.ComponentName; import android.content.Context; import android.content.Intent; @@ -39,7 +40,8 @@ public class ExternalNavigationHandlerTest extends InstrumentationTestCase { private static final int IGNORE = 0x0; private static final int START_INCOGNITO = 0x1; private static final int START_ACTIVITY = 0x2; - private static final int INTENT_SANITIZATION_EXCEPTION = 0x4; + private static final int START_FILE = 0x4; + private static final int INTENT_SANITIZATION_EXCEPTION = 0x8; private static final int NO_REDIRECT = 0x0; private static final int REDIRECT = 0x1; @@ -95,7 +97,7 @@ public class ExternalNavigationHandlerTest extends InstrumentationTestCase { true, false, null, - OverrideUrlLoadingResult.OVERRIDE_WITH_INCOGNITO_MODE, + OverrideUrlLoadingResult.OVERRIDE_WITH_ASYNC_ACTION, START_INCOGNITO); } @@ -1008,6 +1010,58 @@ public class ExternalNavigationHandlerTest extends InstrumentationTestCase { IGNORE); } + @SuppressLint("SdCardPath") + @SmallTest + public void testFileAccess() { + String fileUrl = "file:///sdcard/Downloads/test.html"; + + mDelegate.shouldRequestFileAccess = false; + // Verify no overrides if file access is allowed (under different load conditions). + check(fileUrl, + null, /* referrer */ + false, /* incognito */ + PageTransition.LINK, + NO_REDIRECT, + true, + false, + null, + OverrideUrlLoadingResult.NO_OVERRIDE, + IGNORE); + check(fileUrl, + null, /* referrer */ + false, /* incognito */ + PageTransition.RELOAD, + NO_REDIRECT, + true, + false, + null, + OverrideUrlLoadingResult.NO_OVERRIDE, + IGNORE); + check(fileUrl, + null, /* referrer */ + false, /* incognito */ + PageTransition.AUTO_TOPLEVEL, + NO_REDIRECT, + true, + false, + null, + OverrideUrlLoadingResult.NO_OVERRIDE, + IGNORE); + + mDelegate.shouldRequestFileAccess = true; + // Verify that the file intent action is triggered if file access is not allowed. + check(fileUrl, + null, /* referrer */ + false, /* incognito */ + PageTransition.AUTO_TOPLEVEL, + NO_REDIRECT, + true, + false, + null, + OverrideUrlLoadingResult.OVERRIDE_WITH_ASYNC_ACTION, + START_FILE); + } + private static class TestExternalNavigationDelegate implements ExternalNavigationDelegate { private Context mContext; @@ -1076,6 +1130,17 @@ public class ExternalNavigationHandlerTest extends InstrumentationTestCase { } @Override + public boolean shouldRequestFileAccess(Tab tab) { + return shouldRequestFileAccess; + } + + @Override + public void startFileIntent(Intent intent, String referrerUrl, Tab tab, + boolean needsToCloseTab) { + startFileIntentCalled = true; + } + + @Override public OverrideUrlLoadingResult clobberCurrentTab( String url, String referrerUrl, Tab tab) { mNewUrlAfterClobbering = url; @@ -1096,6 +1161,7 @@ public class ExternalNavigationHandlerTest extends InstrumentationTestCase { public void reset() { startActivityIntent = null; startIncognitoIntentCalled = false; + startFileIntentCalled = false; } public void setCanResolveActivity(boolean value) { @@ -1122,6 +1188,9 @@ public class ExternalNavigationHandlerTest extends InstrumentationTestCase { private String mNewUrlAfterClobbering; private String mReferrerUrlForClobbering; public boolean mIsChromeAppInForeground = true; + + public boolean shouldRequestFileAccess; + public boolean startFileIntentCalled; } private void checkIntentSanity(Intent intent, String name) { @@ -1143,6 +1212,7 @@ public class ExternalNavigationHandlerTest extends InstrumentationTestCase { int otherExpectation) { boolean expectStartIncognito = (otherExpectation & START_INCOGNITO) != 0; boolean expectStartActivity = (otherExpectation & START_ACTIVITY) != 0; + boolean expectStartFile = (otherExpectation & START_FILE) != 0; boolean expectSaneIntent = (otherExpectation & INTENT_SANITIZATION_EXCEPTION) == 0; mDelegate.reset(); @@ -1160,6 +1230,7 @@ public class ExternalNavigationHandlerTest extends InstrumentationTestCase { assertEquals(expectedOverrideResult, result); assertEquals(expectStartIncognito, mDelegate.startIncognitoIntentCalled); assertEquals(expectStartActivity, startActivityCalled); + assertEquals(expectStartFile, mDelegate.startFileIntentCalled); if (startActivityCalled && expectSaneIntent) { checkIntentSanity(mDelegate.startActivityIntent, "Intent"); -- 2.11.4.GIT