Fixed JS api url sanitation. (#25644)

* Fixed JS api url sanitation.
brave · Sep 23, 2024 · 826d977 · 826d977
1 parent 41f1560
commit 826d977
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 52 deletions.
diff --git a/browser/brave_content_browser_client.cc b/browser/brave_content_browser_client.cc
@@ -1347,11 +1347,11 @@ blink::UserAgentMetadata BraveContentBrowserClient::GetUserAgentMetadata() {
   return metadata;
 }
 
-GURL BraveContentBrowserClient::SanitizeURL(
+std::optional<GURL> BraveContentBrowserClient::SanitizeURL(
     content::RenderFrameHost* render_frame_host,
     const GURL& url) {
   if (!base::FeatureList::IsEnabled(features::kBraveCopyCleanLinkFromJs)) {
-    return url;
+    return std::nullopt;
   }
   CHECK(render_frame_host);
   CHECK(render_frame_host->GetBrowserContext());
@@ -1361,9 +1361,14 @@ GURL BraveContentBrowserClient::SanitizeURL(
   CHECK(url_sanitizer_service);
   if (!url_sanitizer_service->CheckJsPermission(
           render_frame_host->GetLastCommittedURL())) {
-    return url;
+    return std::nullopt;
+  }
+  const GURL& sanitized_url = url_sanitizer_service->SanitizeURL(url);
+  if (sanitized_url == url) {
+    // No actual replacements were made.
+    return std::nullopt;
   }
-  return url_sanitizer_service->SanitizeURL(url);
+  return sanitized_url;
 }
 
 bool BraveContentBrowserClient::AllowSignedExchange(

diff --git a/browser/brave_content_browser_client.h b/browser/brave_content_browser_client.h
@@ -171,8 +171,8 @@ class BraveContentBrowserClient : public ChromeContentBrowserClient {
                            blink::web_pref::WebPreferences* prefs) override;
   blink::UserAgentMetadata GetUserAgentMetadata() override;
 
-  GURL SanitizeURL(content::RenderFrameHost* render_frame_host,
-                   const GURL& url) override;
+  std::optional<GURL> SanitizeURL(content::RenderFrameHost* render_frame_host,
+                                  const GURL& url) override;
 
   bool AllowSignedExchange(content::BrowserContext* context) override;
 

diff --git a/browser/url_sanitizer/url_sanitizer_browsertest.cc b/browser/url_sanitizer/url_sanitizer_browsertest.cc
@@ -148,7 +148,8 @@ class URLSanitizerTestBase : public InProcessBrowserTest {
     const GURL url("https://www.YoUtUbE.com/url_sanitizer/js_api.html");
     ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url));
 
-    auto check = [this](const std::string& name, bool should_sanitize) {
+    auto check = [this](const std::string& name, bool should_sanitize,
+                        const std::string& expected_text) {
       {
         ui::ScopedClipboardWriter clear_clipboard(
             ui::ClipboardBuffer::kCopyPaste);
@@ -161,48 +162,57 @@ class URLSanitizerTestBase : public InProcessBrowserTest {
 
       constexpr const char kClickButton[] =
           R"js(
-        (function() {
-          const button = document.getElementById('$1');
-          button.click();
-        })();
-    )js";
+            (function() {
+              const button = document.getElementById('$1');
+              button.click();
+            })();
+        )js";
       const auto script =
           base::ReplaceStringPlaceholders(kClickButton, {name}, nullptr);
       web_contents->Focus();
       ASSERT_TRUE(content::ExecJs(web_contents, script));
 
       const std::string text_from_clipboard = WaitClipboard();
 
-      EXPECT_TRUE(base::StartsWith(text_from_clipboard, "https://youtu.be/"))
+      EXPECT_EQ(expected_text, text_from_clipboard)
           << name << " " << should_sanitize << " " << text_from_clipboard;
-
-      if (should_sanitize) {
-        EXPECT_EQ(std::string::npos, text_from_clipboard.find("si="))
-            << name << " " << should_sanitize << " " << text_from_clipboard;
-      } else {
-        EXPECT_NE(std::string::npos, text_from_clipboard.find("si="))
-            << name << " " << should_sanitize << " " << text_from_clipboard;
-      }
     };
 
     const bool should_sanitize =
         base::FeatureList::IsEnabled(features::kBraveCopyCleanLinkFromJs);
 
-    check("test_1", should_sanitize);
-    check("test_2", should_sanitize);
-    check("test_3", should_sanitize);
-    check("test_4", should_sanitize);
-    check("test_5", should_sanitize);
+    const std::string sanitized = "https://youtu.be/B";
+    const std::string unsanitized = "hTtPs://Youtu.Be/B?si=oLb865I64uJlLRJX";
+    const std::string& expected = (should_sanitize) ? sanitized : unsanitized;
+
+    check("test_1", should_sanitize, expected);
+    check("test_2", should_sanitize, expected);
+    check("test_3", should_sanitize, expected);
+    check("test_4", should_sanitize, expected);
+    check("test_5", should_sanitize, expected);
+    // We cannot distinguish the context, so even if a password similar to the
+    // URL is copied, we will sanitize it.
+    check("test_sanitizable_password", should_sanitize, expected);
+    // Not sanitazable password should be copied as is.
+    check("test_not_sanitizable_password_1", should_sanitize, "Pa$$w0rd");
+    check("test_not_sanitizable_password_2", should_sanitize, "A:^C,D");
+    check("test_not_sanitizable_password_3", should_sanitize,
+          "Ftp://Example.Com/?si=12345");
 
     const GURL no_permission_url(
         "https://no_permission.com/url_sanitizer/js_api.html");
     ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), no_permission_url));
 
-    check("test_1", false);
-    check("test_2", false);
-    check("test_3", false);
-    check("test_4", false);
-    check("test_5", false);
+    check("test_1", false, unsanitized);
+    check("test_2", false, unsanitized);
+    check("test_3", false, unsanitized);
+    check("test_4", false, unsanitized);
+    check("test_5", false, unsanitized);
+    check("test_sanitizable_password", false, unsanitized);
+    check("test_not_sanitizable_password_1", false, "Pa$$w0rd");
+    check("test_not_sanitizable_password_2", false, "A:^C,D");
+    check("test_not_sanitizable_password_3", false,
+          "Ftp://Example.Com/?si=12345");
   }
 
  protected:

diff --git a/chromium_src/content/browser/renderer_host/clipboard_host_impl.cc b/chromium_src/content/browser/renderer_host/clipboard_host_impl.cc
@@ -23,9 +23,13 @@ std::u16string Sanitize(content::ContentBrowserClient* client,
   }
 
   const GURL url(data);
-  if (url.is_valid() && !url.is_empty()) {
-    const GURL sanitized_url = client->SanitizeURL(render_frame_host, url);
-    return base::UTF8ToUTF16(sanitized_url.spec());
+  if (url.is_valid() && !url.is_empty() && url.SchemeIsHTTPOrHTTPS()) {
+    std::optional<GURL> sanitized_url =
+        client->SanitizeURL(render_frame_host, url);
+    if (!sanitized_url) {
+      return data;
+    }
+    return base::UTF8ToUTF16(sanitized_url->spec());
   }
   return data;
 }

diff --git a/chromium_src/content/public/browser/content_browser_client.cc b/chromium_src/content/public/browser/content_browser_client.cc
@@ -32,9 +32,9 @@ uint8_t ContentBrowserClient::WorkerGetBraveFarblingLevel(
   return 1 /* OFF */;
 }
 
-GURL ContentBrowserClient::SanitizeURL(content::RenderFrameHost*,
-                                       const GURL& url) {
-  return url;
+std::optional<GURL> ContentBrowserClient::SanitizeURL(content::RenderFrameHost*,
+                                                      const GURL& url) {
+  return std::nullopt;
 }
 
 }  // namespace content
diff --git a/chromium_src/content/public/browser/content_browser_client.h b/chromium_src/content/public/browser/content_browser_client.h
@@ -27,7 +27,8 @@
                                          BrowserContext* browser_context);   \
   virtual uint8_t WorkerGetBraveFarblingLevel(                               \
       const GURL& url, BrowserContext* browser_context);                     \
-  virtual GURL SanitizeURL(content::RenderFrameHost*, const GURL&);          \
+  virtual std::optional<GURL> SanitizeURL(content::RenderFrameHost*,         \
+                                          const GURL&);                      \
   virtual void SetBrowserStartupIsCompleteForTesting
 
 #include "src/content/public/browser/content_browser_client.h"  // IWYU pragma: export

diff --git a/test/data/url_sanitizer/js_api.html b/test/data/url_sanitizer/js_api.html
@@ -9,6 +9,8 @@
     </style>
 
     <script>
+        const TEXT_TO_COPY = 'hTtPs://Youtu.Be/B?si=oLb865I64uJlLRJX'
+
         function copy_from_text_area(name) {
             const ta = document.getElementById(name)
             ta.focus()
@@ -29,7 +31,7 @@
             textArea.style.outline = 'none'
             textArea.style.boxShadow = 'none'
             textArea.style.background = 'transparent'
-            textArea.value = 'https://youtu.be/?si=oLb865I64uJlLRJX'
+            textArea.value = TEXT_TO_COPY
             document.body.appendChild(textArea)
             textArea.focus()
             textArea.select()
@@ -48,7 +50,24 @@
         }
 
         async function navigator_copy() {
-            await navigator.clipboard.writeText('https://youtu.be/B?si=oLb865I64uJlLRJX')
+            await navigator.clipboard.writeText(TEXT_TO_COPY)
+        }
+
+        async function copy_url_like_password() {
+            // TEXT_TO_COPY contains capitalized chars.
+            await navigator.clipboard.writeText(TEXT_TO_COPY)
+        }
+
+        async function copy_password_1() {
+            await navigator.clipboard.writeText('Pa$$w0rd')
+        }
+
+        async function copy_password_2() {
+            await navigator.clipboard.writeText('A:^C,D')
+        }
+
+        async function copy_password_3() {
+            await navigator.clipboard.writeText('Ftp://Example.Com/?si=12345')
         }
     </script>
 
@@ -61,28 +80,27 @@
         <div>Put any text you want, then click 'Copy'. Data should be sanitized. </div>
         <div> API: document.exec('copy') </div>
         <div>
-            <textarea id="area_1">https://youtu.be/C?si=oLb865I64uJlLRJX</textarea>
+            <textarea id="area_1">hTtPs://Youtu.Be/B?si=oLb865I64uJlLRJX</textarea>
         </div>
         <button id="test_1" onclick="copy_from_text_area('area_1')"> Copy </button>
     </div>
 
     <br>
-    <br
+    <br <div>
+    <div>Copy from text area</div>
+    <div>Data should be sanitized. </div>
+    <div>API: document.exec('copy') </div>
     <div>
-        <div>Copy from text area</div>
-        <div>Data should be sanitized. </div>
-        <div>API: document.exec('copy') </div>
-        <div>
-            <textarea id="area_2" readonly="true">https://youtu.be/D?si=oLb865I64uJlLRJX</textarea>
-        </div>
-        <button id="test_2" onclick="copy_from_text_area('area_2')"> Copy </button>
+        <textarea id="area_2" readonly="true">hTtPs://Youtu.Be/B?si=oLb865I64uJlLRJX</textarea>
+    </div>
+    <button id="test_2" onclick="copy_from_text_area('area_2')"> Copy </button>
     </div>
 
     <br>
     <br>
     <div>
         <div>Navigator Clipboard API</div>
-        <div>https://youtu.be/E?si=oLb865I64uJlLRJX</div>
+        <div>hTtPs://Youtu.Be/B?si=oLb865I64uJlLRJX</div>
         <div>API: navigator.clipboard.writeText </div>
         <button id="test_3" onclick="navigator_copy()"> Copy </button>
     </div>
@@ -91,7 +109,7 @@
     <br>
     <div>
         <div>Copy from the temporary textarea</div>
-        <div>https://youtu.be/F?si=oLb865I64uJlLRJX</div>
+        <div>hTtPs://Youtu.Be/B?si=oLb865I64uJlLRJX</div>
         <div>API: document.exec('copy') </div>
         <button id="test_4" onclick="copy_from_temp()"> Copy </button>
     </div>
@@ -100,11 +118,38 @@
     <br>
     <div>
         <div>Copy from div </div>
-        <div id="text_div" tabindex="-1">https://youtu.be/G?si=oLb865I64uJlLRJX</div>
+        <div id="text_div" tabindex="-1">hTtPs://Youtu.Be/B?si=oLb865I64uJlLRJX</div>
         <div>API: document.exec('copy') </div>
         <button id="test_5" onclick="copy_from_div()"> Copy </button>
     </div>
 
+    <br>
+    <br>
+    <div>
+        <div>Copy sanitizable password</div>
+        <button id="test_sanitizable_password" onclick="copy_url_like_password()"> Copy </button>
+    </div>
+    <br>
+    <br>
+    <div>
+        <div>Copy not sanitizable password</div>
+        <button id="test_not_sanitizable_password_1" onclick="copy_password_1()"> Copy </button>
+    </div>
+
+    <br>
+    <br>
+    <div>
+        <div>Copy not sanitizable password</div>
+        <button id="test_not_sanitizable_password_2" onclick="copy_password_2()"> Copy </button>
+    </div>
+
+    <br>
+    <br>
+    <div>
+        <div>Copy not sanitizable password</div>
+        <button id="test_not_sanitizable_password_3" onclick="copy_password_3()"> Copy </button>
+    </div>
+
 </body>
 
 </html>