Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed JS api url sanitation. (uplift to 1.71.x) #25689

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Fixed JS api url sanitation. (uplift to 1.71.x) #25689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions browser/brave_content_browser_client.cc
View file
Open in desktop
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
Open in desktop
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
60 changes: 35 additions & 25 deletions browser/url_sanitizer/url_sanitizer_browsertest.cc
View file
Open in desktop
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,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:
Expand Down
10 changes: 7 additions & 3 deletions chromium_src/content/browser/renderer_host/clipboard_host_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
6 changes: 3 additions & 3 deletions chromium_src/content/public/browser/content_browser_client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 2 additions & 1 deletion chromium_src/content/public/browser/content_browser_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
73 changes: 59 additions & 14 deletions test/data/url_sanitizer/js_api.html
View file
Open in desktop
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,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>

Expand All @@ -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>
Expand All @@ -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>
Expand All @@ -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>
Loading