Do not install WebAPKs with web manifests with invalid URL components

chrome/browser/installable/installable_manager.cc

Index: chrome/browser/installable/installable_manager.cc
diff --git a/chrome/browser/installable/installable_manager.cc b/chrome/browser/installable/installable_manager.cc
index 7b4362cc60fb3e779b8aecfcc085a48fbe08c061..2449d78f32e65985f5b62a61be0f74cb03e16468 100644
--- a/chrome/browser/installable/installable_manager.cc
+++ b/chrome/browser/installable/installable_manager.cc
@@ -50,6 +50,29 @@ bool DoesManifestContainRequiredIcon(const content::Manifest& manifest) {
   return false;
 }
 
+bool UrlHasUsernameOrPassword(const GURL& url) {
+  return url.has_username() || url.has_password();
+}
+
+// Returns whether any of the URLs in |manifest| has a username or password.
+bool DoesManifestHaveUrlWithUsernameOrPassword(
+    const content::Manifest& manifest) {
+  for (const content::Manifest::Icon& icon : manifest.icons) {
+    if (UrlHasUsernameOrPassword(icon.src))
+      return true;
+  }
+
+  for (const content::Manifest::RelatedApplication& related_application :
+       manifest.related_applications) {
+    if (UrlHasUsernameOrPassword(related_application.url)) {
+      return true;
+    }
+  }
+
+  return UrlHasUsernameOrPassword(manifest.start_url) ||
+         UrlHasUsernameOrPassword(manifest.scope);
+}
+
 }  // namespace
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(InstallableManager);
@@ -94,6 +117,7 @@ InstallableManager::InstallableManager(content::WebContents* web_contents)
 InstallableManager::~InstallableManager() = default;
 
 bool InstallableManager::IsManifestValidForWebApp(
+    const GURL& manifest_url,
     const content::Manifest& manifest) {
   if (manifest.IsEmpty()) {
     installable_->error = MANIFEST_EMPTY;
@@ -111,6 +135,15 @@ bool InstallableManager::IsManifestValidForWebApp(
     return false;
   }
 
+  // WebAPK web manifests are stored on the Chrome WebAPK server. Classify
+  // web manifests with a user name or password as not-installable to avoid
+  // storing user names and passwords on the WebAPK server.
+  if (DoesManifestHaveUrlWithUsernameOrPassword(manifest) ||

[dominickn, 2016/08/17 22:54:43]

I think this check should belong in WebAPK-specific code, not in
InstallableManager. You can check the URL once InstallableManager gets back to
you. This restriction doesn't really make sense for other InstallableManager
clients like banners.

If you're thinking about efficiency, you could even split the InstallableManager
call into two: one to get the manifest first, check it's URL, and then a second
call to fetch everything else. App banners do this to support native apps (which
don't need a SW or app icon in the manifest).

[pkotwicz, 2016/08/18 01:11:41]

The reason I put this code here is that there are at least two places which will
need to do this check:
- ShortcutHelper::AddToLauncherWithSkBitmap() (from both banners and
add-to-homescreen-dialog)
- ManifestUpgradeDetectorFetcher in order to determine whether an updated Web
Manifest is still WebAPK-able

Should I create a static only class to determine whether an installable
WebManifest is WebAPK-able?
WebApkManifestValidator::IsInstallableWebManifestValidForWebAPK() perhaps?

For the sake of this milestone we are going to consider non-WebAPKable Web
Manifests as non-installable (because it is simpler to implement)
For instance, when a user has enabled WebAPKs via chrome://flags, the app-banner
will only show if the Web Manifest is WebAPK-able

[dominickn, 2016/08/18 01:19:16]

I think a separate WebApkManifestValidator is the right way to go, since right
now we know that WebAPKs have more stringent requirements than banners and add
to homescreen (though the base requirements are the same). You could probably
put it into ShortcutHelper for now to be honest.

That way, we can isolate WebAPK specific behaviour for now, and in the future
look to merge things into InstallableManager if we decide that banners and add
to homescreen should also have the more stringent WebAPK behaviour. Is that
reasonable?

+      UrlHasUsernameOrPassword(manifest_url)) {
+    installable_->error = URL_USERNAME_AND_PASSWORD_NOT_SUPPORTED;
+    return false;
+  }
+
   // TODO(dominickn,mlamouri): when Chrome supports "minimal-ui", it should be
   // accepted. If we accept it today, it would fallback to "browser" and make
   // this check moot. See https://crbug.com/604390.
@@ -321,7 +354,7 @@ void InstallableManager::CheckInstallable() {
   DCHECK(!installable_->fetched);
   DCHECK(!manifest().IsEmpty());
 
-  if (IsManifestValidForWebApp(manifest())) {
+  if (IsManifestValidForWebApp(manifest_url(), manifest())) {
     CheckServiceWorker();
   } else {
     installable_->installable = false;