Do not install WebAPKs with web manifests with invalid URL components

chrome/browser/installable/installable_manager.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/installable/installable_manager.h"
 
 #include "base/bind.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/manifest/manifest_icon_downloader.h"
 #include "chrome/browser/manifest/manifest_icon_selector.h"
@@ skipping 32 matching lines @@
       if (size.width() >= kIconMinimumSizeInPx &&
           size.height() >= kIconMinimumSizeInPx) {
         return true;
       }
     }
   }
 
   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);
 
 struct InstallableManager::ManifestProperty {
   InstallableStatusCode error = NO_ERROR_DETECTED;
   GURL url;
   content::Manifest manifest;
   bool fetched = false;
 };
@@ skipping 24 matching lines @@
 InstallableManager::InstallableManager(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       manifest_(new ManifestProperty()),
       installable_(new InstallableProperty()),
       is_active_(false),
       weak_factory_(this) { }
 
 InstallableManager::~InstallableManager() = default;
 
 bool InstallableManager::IsManifestValidForWebApp(
+    const GURL& manifest_url,
     const content::Manifest& manifest) {
   if (manifest.IsEmpty()) {
     installable_->error = MANIFEST_EMPTY;
     return false;
   }
 
   if (!manifest.start_url.is_valid()) {
     installable_->error = START_URL_NOT_VALID;
     return false;
   }
 
   if ((manifest.name.is_null() || manifest.name.string().empty()) &&
       (manifest.short_name.is_null() || manifest.short_name.string().empty())) {
     installable_->error = MANIFEST_MISSING_NAME_OR_SHORT_NAME;
     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.
   if (manifest.display != blink::WebDisplayModeStandalone &&
       manifest.display != blink::WebDisplayModeFullscreen) {
     installable_->error = MANIFEST_DISPLAY_NOT_SUPPORTED;
     return false;
   }
 
   if (!DoesManifestContainRequiredIcon(manifest)) {
@@ skipping 190 matching lines @@
   manifest_->url = manifest_url;
   manifest_->manifest = manifest;
   manifest_->fetched = true;
   WorkOnTask();
 }
 
 void InstallableManager::CheckInstallable() {
   DCHECK(!installable_->fetched);
   DCHECK(!manifest().IsEmpty());
 
-  if (IsManifestValidForWebApp(manifest())) {
+  if (IsManifestValidForWebApp(manifest_url(), manifest())) {
     CheckServiceWorker();
   } else {
     installable_->installable = false;
     installable_->fetched = true;
     WorkOnTask();
   }
 }
 
 void InstallableManager::CheckServiceWorker() {
   DCHECK(!installable_->fetched);
@@ skipping 100 matching lines @@
 }
 
 bool InstallableManager::is_installable() const {
   return installable_->installable;
 }
 
 // static
 int InstallableManager::GetMinimumIconSizeInPx() {
   return kIconMinimumSizeInPx;
 }