Skip to content

Commit

Permalink
Fixed JS api url sanitation.
Browse files Browse the repository at this point in the history
  • Loading branch information
@boocmp
boocmp committed Sep 19, 2024
1 parent 666a658 commit a0d059a
Show file tree
Hide file tree
Showing 7 changed files with 88 additions and 51 deletions.
13 changes: 9 additions & 4 deletions browser/brave_content_browser_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions browser/brave_content_browser_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
54 changes: 29 additions & 25 deletions browser/url_sanitizer/url_sanitizer_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -161,48 +162,51 @@ 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", should_sanitize, "Pa$$w0rd");

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", should_sanitize, unsanitized);
check("test_not_sanitizable_password", should_sanitize, "Pa$$w0rd");
}

protected:
Expand Down
8 changes: 6 additions & 2 deletions chromium_src/content/browser/renderer_host/clipboard_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@ 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());
std::optional<GURL> sanitized_url =
client->SanitizeURL(render_frame_host, url);
if (!sanitized_url) {
return data;
}
return base::UTF8ToUTF16(sanitized_url->spec());
}
return data;
}
Expand Down
6 changes: 3 additions & 3 deletions chromium_src/content/public/browser/content_browser_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ ContentBrowserClient::WorkerGetBraveShieldSettings(
return brave_shields::mojom::ShieldsSettingsPtr();
}

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
3 changes: 2 additions & 1 deletion chromium_src/content/public/browser/content_browser_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
virtual brave_shields::mojom::ShieldsSettingsPtr \
WorkerGetBraveShieldSettings(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
Expand Down
51 changes: 37 additions & 14 deletions test/data/url_sanitizer/js_api.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -48,7 +50,16 @@
}

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() {
await navigator.clipboard.writeText('Pa$$w0rd')
}
</script>

Expand All @@ -61,28 +72,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>
Expand All @@ -91,7 +101,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>
Expand All @@ -100,11 +110,24 @@
<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" onclick="copy_password()"> Copy </button>
</div>

</body>

</html>

0 comments on commit a0d059a

Please sign in to comment.