Fix XSS in app launcher and remove use of unvalidated URL
The third parameter of "launchApp" is only used for the webstore app,
and used to append utm_source=chrome-ntp-icon to the app URL.
But the launchApp handler did not validate that the URL is safe.
To fix that issue, I specialize the parameter for launchApp: It now takes the
source string ("chrome-ntp-icon") instead of a URL without validation.
BUG=668665
TEST=Manually using test case from bug report. Also opened the app launcher and
verified that clicking on the Webstore icon still leads to the same place.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2527413002
Cr-Commit-Position: refs/heads/master@{#434939}
(cherry picked from commit 15120efa4b9394086d687086e443f47290b5170a)
Review URL: https://codereview.chromium.org/2542593002 .
Cr-Commit-Position: refs/branch-heads/2924@{#186}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}
diff --git a/chrome/browser/resources/ntp4/apps_page.js b/chrome/browser/resources/ntp4/apps_page.js
index e1f4a00..9e7b71e6 100644
--- a/chrome/browser/resources/ntp4/apps_page.js
+++ b/chrome/browser/resources/ntp4/apps_page.js
@@ -396,13 +396,8 @@
onClick_: function(e) {
if (/** @type {MouseEvent} */(e).button > 1) return;
- var url = !this.appData_.is_webstore ? '' :
- appendParam(this.appData_.url,
- 'utm_source',
- 'chrome-ntp-icon');
-
chrome.send('launchApp',
- [this.appId, APP_LAUNCH.NTP_APPS_MAXIMIZED, url,
+ [this.appId, APP_LAUNCH.NTP_APPS_MAXIMIZED, 'chrome-ntp-icon',
e.button, e.altKey, e.ctrlKey, e.metaKey, e.shiftKey]);
// Don't allow the click to trigger a link or anything
@@ -709,9 +704,9 @@
if (html) {
// It's important that we don't attach this node to the document
// because it might contain scripts.
- var node = this.ownerDocument.createElement('div');
- node.innerHTML = html;
- title = node.textContent;
+ var doc = document.implementation.createHTMLDocument();
+ doc.body.innerHTML = html;
+ title = doc.body.textContent;
}
// Make sure title is >=1 and <=45 characters for Chrome app limits.
diff --git a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc b/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
index a8c2df6..0e2b5be 100644
--- a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
+++ b/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
@@ -67,6 +67,7 @@
#include "extensions/common/extension.h"
#include "extensions/common/extension_icon_set.h"
#include "extensions/common/extension_set.h"
+#include "net/base/url_util.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/webui/web_ui_util.h"
#include "url/gurl.h"
@@ -482,9 +483,7 @@
CHECK(args->GetString(0, &extension_id));
double source = -1.0;
CHECK(args->GetDouble(1, &source));
- std::string url;
- if (args->GetSize() > 2)
- CHECK(args->GetString(2, &url));
+ GURL override_url;
extension_misc::AppLaunchBucket launch_bucket =
static_cast<extension_misc::AppLaunchBucket>(
@@ -511,6 +510,16 @@
extensions::RecordAppLaunchType(launch_bucket, extension->GetType());
} else {
extensions::RecordWebStoreLaunch();
+
+ if (args->GetSize() > 2) {
+ std::string source_value;
+ CHECK(args->GetString(2, &source_value));
+ if (!source_value.empty()) {
+ override_url = net::AppendQueryParameter(
+ extensions::AppLaunchInfo::GetFullLaunchURL(extension),
+ extension_urls::kWebstoreSourceField, source_value);
+ }
+ }
}
if (disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB ||
@@ -522,7 +531,7 @@
? extensions::LAUNCH_CONTAINER_WINDOW
: extensions::LAUNCH_CONTAINER_TAB,
disposition, extensions::SOURCE_NEW_TAB_PAGE);
- params.override_url = GURL(url);
+ params.override_url = override_url;
OpenApplication(params);
} else {
// To give a more "launchy" experience when using the NTP launcher, we close
@@ -538,7 +547,7 @@
old_contents ? WindowOpenDisposition::CURRENT_TAB
: WindowOpenDisposition::NEW_FOREGROUND_TAB,
extensions::SOURCE_NEW_TAB_PAGE);
- params.override_url = GURL(url);
+ params.override_url = override_url;
WebContents* new_contents = OpenApplication(params);
// This will also destroy the handler, so do not perform any actions after.
