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

Disable shields for extensions #975

Merged
merged 2 commits into from
Nov 28, 2018
Merged

Disable shields for extensions #975

merged 2 commits into from
Nov 28, 2018

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Nov 28, 2018 •
edited
Loading

The reasoning is they have APIs to do much worse tracking than our shields would protect them from. Users install them knowing they will have elevated permissions

Fix brave/brave-browser#1380

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@bbondy bbondy self-assigned this Nov 28, 2018
@bbondy bbondy mentioned this pull request Nov 28, 2018
Closed
@bbondy
Disable shields for extensions
8b8c9d9 
The reasoning is they have APIs to do much worse tracking than our shields would protect them from. Users install them knowing they will have elevated permissions

Fix brave/brave-browser#1380
@bbondy bbondy force-pushed the breaking-extensions branch from 638fc5a to 8b8c9d9 Compare November 28, 2018 10:04
bbondy
bbondy commented Nov 28, 2018
View reviewed changes
browser/brave_content_browser_client.cc
brave_shields::IsAllowContentSettingWithIOData(
io_data, tab_origin, tab_origin, CONTENT_SETTINGS_TYPE_PLUGINS,
brave_shields::kBraveShields) &&
!first_party.SchemeIs(kChromeExtensionScheme);
Copy link
Member Author

Choose a reason for hiding this comment

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

The idea to implement this was just to follow where we allow things because of Brave shields down.

bbondy
bbondy commented Nov 28, 2018
View reviewed changes
browser/extensions/brave_extension_functional_test.cc
@@ -39,6 +40,7 @@ void ExtensionFunctionalTest::InstallExtensionSilently(ExtensionService* service

const Extension* extension = registry_observer.WaitForExtensionReady();
EXPECT_TRUE(extension);
return extension;
Copy link
Member Author

Choose a reason for hiding this comment

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

I return this just so I can log the generated extension ID more easily in debugging.

bbondy
bbondy commented Nov 28, 2018
View reviewed changes
browser/extensions/brave_extension_provider_browsertest.cc
@@ -65,4 +67,68 @@ IN_PROC_BROWSER_TEST_F(BraveExtensionProviderTest, PDFJSInstalls) {
ASSERT_TRUE(pdfjs_exists);
}

// Load an extension page with an ad image, and make sure it is NOT blocked.
// It would otherwise be blocked though if it wasn't an extension.
IN_PROC_BROWSER_TEST_F(BraveExtensionProviderTest, AdsNotBlockedByDefaultBlockerInExtension) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Nearly the same test is done in the adblock service tests, but with the result of ads blocked 1 and a different call to setExpectations to indicate that the test expects an item to be blocked.

bbondy
bbondy commented Nov 28, 2018
View reviewed changes
browser/extensions/brave_extension_provider_browsertest.cc
contents,
"canSetCookie('test', 'testval', 'http://a.com')",
&as_expected));
EXPECT_TRUE(as_expected);
Copy link
Member Author

Choose a reason for hiding this comment

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

We already didn't block this, but I added a test to make sure it doesn't break.

bbondy
bbondy commented Nov 28, 2018
View reviewed changes
browser/extensions/brave_extension_provider_browsertest.cc
contents,
"canGetCookie('test', 'http://a.com')",
&as_expected));
EXPECT_TRUE(as_expected);
Copy link
Member Author

Choose a reason for hiding this comment

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

We already didn't block this, but I added a test to make sure it doesn't break.

bbondy
bbondy commented Nov 28, 2018
View reviewed changes
browser/net/brave_ad_block_tp_network_delegate_helper.cc
// https://developer.att.com/.../file.pdf
// So if the tab origin is chrome-extension, set it to that of the PDF only for PDFJS
std::string tab_host = brave::GetURLOrPDFURL(ctx->tab_url).host();
std::string tab_host = ctx->tab_origin.host();
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 longer need the last fix for the PDF being seen as 3p.

bbondy
bbondy commented Nov 28, 2018
View reviewed changes
browser/net/brave_site_hacks_network_delegate_helper.cc
@@ -28,6 +29,9 @@ bool ApplyPotentialReferrerBlock(net::URLRequest* request) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
GURL target_origin = GURL(request->url()).GetOrigin();
GURL tab_origin = request->site_for_cookies().GetOrigin();
if (tab_origin.SchemeIs(kChromeExtensionScheme)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't do referrer blocking for ApplyPotentialReferrerBlock

bbondy
bbondy commented Nov 28, 2018
View reviewed changes
browser/net/brave_site_hacks_network_delegate_helper_unittest.cc
@@ -271,4 +271,32 @@ TEST_F(BraveSiteHacksNetworkDelegateHelperTest, ReferrerCleared) {
});
}

TEST_F(BraveSiteHacksNetworkDelegateHelperTest, ReferrerWouldBeClearedButExtensionSite) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Verifies ApplyPotentialReferrerBlock

bbondy
bbondy commented Nov 28, 2018
View reviewed changes
test/data/extension-compat-test-extension.pem
@@ -0,0 +1,28 @@
-----BEGIN PRIVATE KEY-----
Copy link
Member Author

Choose a reason for hiding this comment

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

Intentionally not private, useful if we want to re-generate the crx file for tests after doing changes.

bbondy
bbondy commented Nov 28, 2018
View reviewed changes
test/data/extension-compat-test-extension/blocking.html
@@ -0,0 +1,99 @@
<html>
Copy link
Member Author

@bbondy bbondy Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Copy of blocking.html in test/data with some additions for cookies. I intentionally kept some functions that are unused like xhr because we might use them for more tests later. Repackaging an extension is a pain and you have to get a new CSP SHA.

@bbondy bbondy changed the title WIP: Disable shields for extensions Disable shields for extensions Nov 28, 2018
emerick
emerick approved these changes Nov 28, 2018
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@bbondy bbondy merged commit d670fa8 into master Nov 28, 2018
bbondy added a commit that referenced this pull request Nov 28, 2018
@bbondy
Merge pull request #975 from brave/breaking-extensions
302062f 
Disable shields for extensions
@bbondy
Copy link
Member Author

bbondy commented Nov 28, 2018

master: d670fa8
0.58.x: 302062f

@kjozwiak kjozwiak deleted the breaking-extensions branch November 28, 2018 19:19
bbondy added a commit that referenced this pull request Nov 28, 2018
@bbondy
Merge pull request #975 from brave/breaking-extensions
48bfd75 
Disable shields for extensions
@tildelowengrimm tildelowengrimm mentioned this pull request Nov 28, 2018
Closed
@bbondy
Copy link
Member Author

bbondy commented Nov 28, 2018

0.57.x: 48bfd75

@bsclifton bsclifton mentioned this pull request Dec 31, 2018
Closed
@bbondy bbondy added this to the 0.57.x - Release milestone Jan 14, 2019
@martinklepsch martinklepsch mentioned this pull request Apr 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emerick emerick emerick approved these changes

@diracdeltas diracdeltas Awaiting requested review from diracdeltas

Assignees

@bbondy bbondy

Labels
None yet
Projects
None yet
Milestone
0.57.x - Release
Development

Successfully merging this pull request may close these issues.

Some of Brave's Default Shields settings breaking extensions
2 participants
@bbondy @emerick