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.

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

PDF checks for blocking should use proper tab origin #958

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion browser/net/url_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <string>

#include "brave/common/url_util.h"
#include "brave/components/brave_shields/browser/brave_shields_util.h"
#include "brave/components/brave_shields/common/brave_shield_constants.h"
#include "content/public/browser/resource_request_info.h"
Expand All @@ -23,7 +24,7 @@ void BraveRequestInfo::FillCTXFromRequest(const net::URLRequest* request,
std::shared_ptr<brave::BraveRequestInfo> ctx) {
ctx->request_identifier = request->identifier();
ctx->request_url = request->url();
ctx->tab_origin = request->site_for_cookies().GetOrigin();
ctx->tab_origin = brave::GetURLOrPDFURL(request->site_for_cookies()).GetOrigin();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is ctx->tab_origin used for anything security-related? if so it probably shouldn't be set to the pdfjs pseudo-origin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no only our internal filtering, but I'll update this to scope it to only adblock+TP or just exclude TP.

auto* request_info = content::ResourceRequestInfo::ForRequest(request);
if (request_info) {
ctx->resource_type = request_info->GetResourceType();
Expand Down
65 changes: 65 additions & 0 deletions browser/net/url_context_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/browser/net/url_context.h"
#include "chrome/test/base/chrome_render_view_host_test_harness.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
#include "net/url_request/url_request_test_util.h"
#include "url/gurl.h"

namespace {

class URLContextTest: public testing::Test {
public:
URLContextTest() :
thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
context_(new net::TestURLRequestContext(true)) {
}

~URLContextTest() override {}

void SetUp() override {
context_->Init();
}

net::TestURLRequestContext* context() { return context_.get(); }

protected:

private:
content::TestBrowserThreadBundle thread_bundle_;
std::unique_ptr<net::TestURLRequestContext> context_;
};

TEST_F(URLContextTest, TabHostResolvesProperlyForTabContext) {
GURL url("https://www.brave.com/prime_numbers/127");
net::TestDelegate test_delegate;
std::unique_ptr<net::URLRequest> request =
context()->CreateRequest(url, net::IDLE, &test_delegate,
TRAFFIC_ANNOTATION_FOR_TESTS);
request->set_site_for_cookies(GURL("https://be.brave.com/test.html"));

std::shared_ptr<brave::BraveRequestInfo>
brave_request_info(new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(), brave_request_info);
ASSERT_EQ(brave_request_info->tab_origin, "https://be.brave.com/");
}

TEST_F(URLContextTest, PDFJSTabHostResolvesProperlyForTabContext) {
GURL url("https://www.brave.com/prime_numbers/131");
net::TestDelegate test_delegate;
std::unique_ptr<net::URLRequest> request =
context()->CreateRequest(url, net::IDLE, &test_delegate,
TRAFFIC_ANNOTATION_FOR_TESTS);
request->set_site_for_cookies(GURL("chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/https://example.com/test.pdf"));

std::shared_ptr<brave::BraveRequestInfo>
brave_request_info(new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(), brave_request_info);
ASSERT_EQ(brave_request_info->tab_origin, "https://example.com/");
}

} // namespace

2 changes: 2 additions & 0 deletions common/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ source_set("common") {
"shield_exceptions.h",
"url_constants.cc",
"url_constants.h",
"url_util.cc",
"url_util.h",
]

public_deps = [
Expand Down
28 changes: 28 additions & 0 deletions common/url_util.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/common/extensions/extension_constants.h"
#include "brave/common/url_util.h"
#include "url/gurl.h"

namespace brave {

GURL GetURLOrPDFURL(const GURL& url) {
if (url.SchemeIs("chrome-extension") &&
url.host() == pdfjs_extension_id) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] it would be helpful to set chrome-extension://<pdfjs_extension_id>/ as a constant pdfjs_extension_prefix in extension_constants since it is repeated a lot in this PR and will also be needed in the PR to hide the pdfjs prefix.

then this check can be replaced with if (url.GetOrigin() == pdfjs_extension_prefix)

static size_t pdfjs_substring_len = (std::string("chrome-extension://") +
pdfjs_extension_id + "/").length();
size_t http_pos = url.spec().find(std::string("chrome-extension://") +
pdfjs_extension_id + "/http://");
size_t https_pos = url.spec().find(std::string("chrome-extension://") +
pdfjs_extension_id + "/https://");
if (http_pos != std::string::npos || https_pos != std::string::npos) {
return GURL(url.spec().substr(pdfjs_substring_len,
url.spec().length() - pdfjs_substring_len));
}
}
return url;
}

} // namespace brave
18 changes: 18 additions & 0 deletions common/url_util.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_COMMON_URL_UTIL_H_
#define BRAVE_COMMON_URL_UTIL_H_

class GURL;

namespace brave {

// Returns the location of the PDF if this URL is a PDFJS extension URL.
// Otherwise simply just returns the same URL as passed in.
GURL GetURLOrPDFURL(const GURL& url);

} // namespace brave

#endif // BRAVE_COMMON_URL_UTIL_H_
34 changes: 34 additions & 0 deletions common/url_util_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "brave/common/url_util.h"
#include "chrome/test/base/chrome_render_view_host_test_harness.h"
#include "url/gurl.h"

typedef testing::Test BraveUrlUtilTest;

namespace brave {

TEST_F(BraveUrlUtilTest, GetURLOrPDFURL) {
std::vector<GURL> unchanged_urls({
// PDFJS URL but not to a PDF
GURL("chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/test.html"),
// PDFJS ID but not chrome-extension scheme
GURL("chrome://oemmndcbldboiebfnladdacbdfmadadm/https://test.html"),
// Not PDFJS ID but format of a PDFJS PDF URL
GURL("chrome-extension://aaamndcbldboiebfnladdacbdfmadaaa/https://example.com/test.html"),
// Random other URL
GURL("https://example.com")
});
std::for_each(unchanged_urls.begin(), unchanged_urls.end(),
[this](GURL url){
EXPECT_EQ(brave::GetURLOrPDFURL(url), url);
});
EXPECT_EQ(brave::GetURLOrPDFURL(GURL("chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/http://example.com?test")),
GURL("http://example.com?test"));
EXPECT_EQ(brave::GetURLOrPDFURL(GURL("chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/https://example.com?test")),
GURL("https://example.com?test"));
}

} // namespace
4 changes: 3 additions & 1 deletion test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ test("brave_unit_tests") {
"//brave/browser/brave_resources_util_unittest.cc",
"//brave/browser/brave_stats_updater_unittest.cc",
"//brave/browser/download/brave_download_item_model_unittest.cc",
"//brave/browser/net/url_context_unittest.cc",
"//brave/browser/tor/mock_tor_profile_service_impl.cc",
"//brave/browser/tor/mock_tor_profile_service_impl.h",
"//brave/browser/tor/mock_tor_profile_service_factory.cc",
Expand All @@ -50,7 +51,7 @@ test("brave_unit_tests") {
"//brave/browser/resources/settings/reset_report_uploader_unittest.cc",
"//brave/chromium_src/chrome/browser/external_protocol/external_protocol_handler_unittest.cc",
"//brave/chromium_src/chrome/browser/signin/account_consistency_disabled_unittest.cc",
"//brave\chromium_src/chrome/browser/ui/bookmarks/brave_bookmark_context_menu_controller_unittest.cc",
"//brave/chromium_src/chrome/browser/ui/bookmarks/brave_bookmark_context_menu_controller_unittest.cc",
"//brave/chromium_src/components/search_engines/brave_template_url_prepopulate_data_unittest.cc",
"//brave/chromium_src/components/search_engines/brave_template_url_service_util_unittest.cc",
"//brave/chromium_src/components/version_info/brave_version_info_unittest.cc",
Expand All @@ -59,6 +60,7 @@ test("brave_unit_tests") {
"//brave/common/shield_exceptions_unittest.cc",
"//brave/common/tor/tor_test_constants.cc",
"//brave/common/tor/tor_test_constants.h",
"//brave/common/url_util_unittest.cc",
"//brave/components/assist_ranker/ranker_model_loader_impl_unittest.cc",
"//brave/components/brave_shields/browser/ad_block_regional_service_unittest.cc",
"//brave/components/brave_sync/bookmark_order_util_unittest.cc",
Expand Down