Lacros: Preserve queries in URLs for Ash

This makes URLs such as os://version?show-variations-cmd work again as
expected.

Bug: chromium:1445145
Change-Id: I4c9c605744b39ab14d2aefae410851164309ba0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4938495
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216752}

chrome/browser/ash/crosapi/url_handler_ash.cc

@@ -75,26 +75,16 @@ absl::optional<ash::SystemWebAppType> GetSystemAppForURL(Profile* profile,
   return swa_manager ? swa_manager->GetSystemAppForURL(url) : absl::nullopt;
 }
 
-void OpenUrlInternalContinue(Profile* profile,
-                             GURL target_url,
-                             const GURL& original_url) {
+void OpenUrlInternalContinue(Profile* profile, const GURL& url) {
   DCHECK(ash::SystemWebAppManager::Get(profile)->IsAppEnabled(
       ash::SystemWebAppType::OS_URL_HANDLER));
 
   ash::SystemWebAppType swa_type =
-      GetSystemAppForURL(profile, target_url)
+      GetSystemAppForURL(profile, url)
           .value_or(ash::SystemWebAppType::OS_URL_HANDLER);
 
-  // This is a hack for chrome://camera-app URLs with queries, as sent by
-  // assistant.
-  // TODO(crbug.com/1445145): Figure out how to get rid of this.
-  if (swa_type == ash::SystemWebAppType::CAMERA &&
-      !gurl_os_handler_utils::HasOsScheme(original_url)) {
-    target_url = original_url;
-  }
-
   ash::SystemAppLaunchParams launch_params;
-  launch_params.url = target_url;
+  launch_params.url = url;
   int64_t display_id =
       display::Screen::GetScreen()->GetDisplayForNewWindows().id();
   ash::LaunchSystemWebAppAsync(profile, swa_type, launch_params,
@@ -104,7 +94,9 @@ void OpenUrlInternalContinue(Profile* profile,
 }  // namespace
 
 // TODO(neis): Find a way to unify this code with the one in os_url_handler.cc.
-bool UrlHandlerAsh::OpenUrlInternal(const GURL& url) {
+bool UrlHandlerAsh::OpenUrlInternal(GURL url) {
+  url = gurl_os_handler_utils::SanitizeAshUrl(url);
+
   Profile* profile = Profile::FromBrowserContext(
       ash::BrowserContextHelper::Get()->GetBrowserContextByUser(
           user_manager::UserManager::Get()->GetPrimaryUser()));
@@ -117,16 +109,14 @@ bool UrlHandlerAsh::OpenUrlInternal(const GURL& url) {
     return false;
   }
 
-  const GURL target_url = gurl_os_handler_utils::SanitizeAshUrl(url);
-  if (!ChromeWebUIControllerFactory::GetInstance()->CanHandleUrl(target_url)) {
+  if (!ChromeWebUIControllerFactory::GetInstance()->CanHandleUrl(url)) {
     LOG(ERROR) << "Invalid URL passed to UrlHandlerAsh::OpenUrl: " << url;
     return false;
   }
 
   // Wait for all SWAs to be registered before continuing.
   ash::SystemWebAppManager::Get(profile)->on_apps_synchronized().Post(
-      FROM_HERE,
-      base::BindOnce(&OpenUrlInternalContinue, profile, target_url, url));
+      FROM_HERE, base::BindOnce(&OpenUrlInternalContinue, profile, url));
   return true;
 }
 

chrome/browser/ash/crosapi/url_handler_ash.h

@@ -30,7 +30,7 @@ class UrlHandlerAsh : public mojom::UrlHandler {
   // Returns |false| when the URL was invalid and will not get processed and
   // |true| when the URL will get processed (synchronous or asynchronously).
   // For testing only.
-  bool OpenUrlInternal(const GURL& url);
+  bool OpenUrlInternal(GURL url);
 
  private:
   mojo::ReceiverSet<mojom::UrlHandler> receivers_;

chromeos/crosapi/cpp/gurl_os_handler_unittest.cc

@@ -45,11 +45,11 @@ TEST(GurlOsHandlerUtilsTest, SanitizeAshUrl) {
   // Valid examples.
   EXPECT_EQ(SanitizeAshUrl(GURL("chrome://version")), GURL("chrome://version"));
   EXPECT_EQ(SanitizeAshUrl(GURL("chrome://abc/def/ghi?jkl")),
-            GURL("chrome://abc/def/ghi"));  // Query removed.
+            GURL("chrome://abc/def/ghi?jkl"));  // Query preserved.
   EXPECT_EQ(SanitizeAshUrl(GURL("chrome://abc:123/def/ghi?jkl#mno")),
-            GURL("chrome://abc/def/ghi"));  // Port and ref too.
+            GURL("chrome://abc/def/ghi?jkl"));  // Port and ref removed.
   EXPECT_EQ(SanitizeAshUrl(GURL("https://abc:123/def/ghi?jkl#mno")),
-            GURL("https://abc/def/ghi"));  // Here too.
+            GURL("https://abc/def/ghi?jkl"));  // Here too.
 }
 
 TEST(GurlOsHandlerUtilsTest, IsAshUrlInList) {

chromeos/crosapi/cpp/gurl_os_handler_utils.cc

@@ -22,24 +22,32 @@ const char kChromeUIOSSettingsHost[] = "os-settings";
 // The start of the host portion of a GURL which starts with the os scheme.
 const size_t kHostStart = sizeof(kOsUrlPrefix) - 1;
 
-GURL NormalizeAshUrl(GURL gurl) {
-  if (!gurl.is_valid() || !gurl.has_host()) {
+GURL NormalizeAshUrl(GURL url,
+                     bool keep_path = false,
+                     bool keep_query = false) {
+  if (!url.is_valid() || !url.has_host()) {
     return GURL();
   }
 
-  if (!gurl.has_ref() && !gurl.has_username() && !gurl.has_password() &&
-      !gurl.has_query() && !gurl.has_port() && !gurl.has_path()) {
-    return gurl;
+  // Shortcut.
+  if (!url.has_ref() && !url.has_username() && !url.has_password() &&
+      (keep_query || !url.has_query()) && !url.has_port() &&
+      (keep_path || !url.has_path())) {
+    return url;
   }
 
   GURL::Replacements replacements;
   replacements.ClearPassword();
-  replacements.ClearPath();
+  if (!keep_path) {
+    replacements.ClearPath();
+  }
   replacements.ClearPort();
-  replacements.ClearQuery();
+  if (!keep_query) {
+    replacements.ClearQuery();
+  }
   replacements.ClearRef();
   replacements.ClearUsername();
-  return gurl.ReplaceComponents(replacements);
+  return url.ReplaceComponents(replacements);
 }
 
 }  // namespace
@@ -48,25 +56,8 @@ namespace crosapi {
 
 namespace gurl_os_handler_utils {
 
-// Like NormalizeAshUrl but keep path.
-GURL SanitizeAshUrl(const GURL& gurl) {
-  if (!gurl.is_valid() || !gurl.has_host()) {
-    return GURL();
-  }
-
-  // Shortcut.
-  if (!gurl.has_ref() && !gurl.has_username() && !gurl.has_password() &&
-      !gurl.has_query() && !gurl.has_port()) {
-    return gurl;
-  }
-
-  GURL::Replacements replacements;
-  replacements.ClearRef();
-  replacements.ClearUsername();
-  replacements.ClearPassword();
-  replacements.ClearQuery();
-  replacements.ClearPort();
-  return gurl.ReplaceComponents(replacements);
+GURL SanitizeAshUrl(const GURL& url) {
+  return NormalizeAshUrl(url, /*keep_path*/ true, /*keep_query*/ true);
 }
 
 GURL GetAshUrlFromLacrosUrl(GURL url) {
@@ -86,17 +77,13 @@ GURL GetAshUrlFromLacrosUrl(GURL url) {
   return url;
 }
 
-bool IsAshUrlInList(const GURL& test_url, const std::vector<GURL>& list) {
-  GURL normalized_test_url = NormalizeAshUrl(test_url);
-  const bool normalized_match =
-      std::any_of(list.begin(), list.end(), [&](const GURL& elem) {
-        DCHECK(elem.is_valid());
-        DCHECK_EQ(elem, SanitizeAshUrl(elem));
-        return elem == normalized_test_url;
-      });
-  return normalized_match ||
-         std::find(list.begin(), list.end(), SanitizeAshUrl(test_url)) !=
-             std::end(list);
+bool IsAshUrlInList(const GURL& url, const std::vector<GURL>& list) {
+  return std::any_of(list.begin(), list.end(), [&](const GURL& elem) {
+    DCHECK(elem.is_valid());
+    DCHECK_EQ(elem, NormalizeAshUrl(elem, /*keep_path*/ true));
+    return elem == NormalizeAshUrl(url, /*keep_path*/ false) ||
+           elem == NormalizeAshUrl(url, /*keep_path*/ true);
+  });
 }
 
 bool HasOsScheme(const GURL& url) {

chromeos/crosapi/cpp/gurl_os_handler_utils.h

@@ -34,7 +34,7 @@ COMPONENT_EXPORT(CROSAPI)
 GURL GetAshUrlFromLacrosUrl(GURL url);
 
 // Sanitize an Ash URL by stripping away certain parts.
-// Example: chrome://os-settings/network?query => chrome://os-settings/network
+// Example: chrome://os-settings/network#ref => chrome://os-settings/network
 COMPONENT_EXPORT(CROSAPI)
 GURL SanitizeAshUrl(const GURL& url);