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

Tie CSP rule injection to Shields ads/tracker setting #9053

Merged
merged 4 commits into from
Jun 10, 2021
Merged

Tie CSP rule injection to Shields ads/tracker setting #9053

merged 4 commits into from
Jun 10, 2021

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Jun 8, 2021 •
edited
Loading

Resolves brave/brave-browser#16283
Resolves brave/brave-browser#16251

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Fix for brave/brave-browser#16283

Note the test plan from #8281:

Visit http://pirateiro.com/. Refresh the page while the developer console is open, and check in the console to verify that the browser refused to load scripts violating the policy script-src 'self' 'unsafe-inline' https://hcaptcha.com *.hcaptcha.com.

On Android, it will be sufficient to observe 0 blocked items in the Shields panel rather than 5 (the requests are still blocked, but will not increment the counter).

This behavior should hold when Shields are up and ads/trackers blocked setting is either standard or aggressive mode. When Shields are down, or ads/trackers are allowed, the above should no longer be observed.

Fix for brave/brave-browser#16251

From brave/brave-browser#16251 (comment):

  • Create a basic .html file with the following contents: 
    <html><body><a href="https://duckduckgo.com/">Link</a></body></html>
  • Open the html file in Brave as a file:// URL, and left click on the Link anchor
  • Verify that the DuckDuckGo page loads rather than displaying a full white screen

@antonok-edm antonok-edm self-assigned this Jun 8, 2021
@antonok-edm antonok-edm force-pushed the csp-shields-control branch from aad75dd to f7dcfde Compare June 9, 2021 16:37
iefremov
iefremov reviewed Jun 9, 2021
View reviewed changes
browser/brave_shields/ad_block_service_browsertest.cc Outdated
browser()->tab_strip_model()->GetActiveWebContents();

auto res = EvalJs(contents, "await window.allLoaded");
ASSERT_EQ(true, EvalJs(contents, "!!window.loadedNonceScript"));
Copy link
Contributor

Choose a reason for hiding this comment

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

ASSERT_TRUE()
personally i prefer EXPECT_TRUE

@antonok-edm antonok-edm merged commit ba5b5b0 into master Jun 10, 2021
@antonok-edm antonok-edm deleted the csp-shields-control branch June 10, 2021 03:19
@antonok-edm antonok-edm added this to the 1.27.x - Nightly milestone Jun 10, 2021
brave-builds pushed a commit that referenced this pull request Jun 10, 2021
@brave-browser-releases
Uplift of #9053 (squashed) to beta
a8e2b94
brave-builds pushed a commit that referenced this pull request Jun 10, 2021
@brave-browser-releases
Uplift of #9053 (squashed) to release
abea753
This was referenced Jun 10, 2021
Merged
Merged
@kjozwiak
Copy link
Member

kjozwiak commented Jun 10, 2021 •
edited
Loading

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.27.56 Chromium: 91.0.4472.101 (Official Build) nightly (64-bit)
--- | --
Revision | af52a90bf87030dd1523486a1cd3ae25c5d76c9b-refs/branch-heads/4472@{#1462}
OS | Windows 10 OS Version 2009 (Build 19042.1023)

Test Cases for brave/brave-browser#16283

Verified the STR/Cases outlined via #9053 (comment) as per the following:

Fixed (Blocking) Standard Aggressive Allowed Disabled
fixed standard aggressive allowed disabled

Test Cases for brave/brave-browser#16251

Verified the STR/Cases outlined via #9053 (comment) as per the following:

Link After clicking (DDG)
link ddg

@kjozwiak
Copy link
Member

Verification PASSED using Samsung S10+ running Android 11 using the following build:

1.27.56 Chromium: 91.0.4472.101

Verified the STR/Cases outlined via #9053 (comment) as per the following:

Screenshot_20210610-121706_Brave - Nightly

@GeetaSarvadnya GeetaSarvadnya mentioned this pull request Jun 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iefremov iefremov iefremov approved these changes

@bridiver bridiver Awaiting requested review from bridiver

@ryanbr ryanbr Awaiting requested review from ryanbr

Assignees

@antonok-edm antonok-edm

Labels
None yet
Projects
None yet
Milestone
1.27.x - Release
3 participants
@antonok-edm @kjozwiak @iefremov