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

Properly get tab origin for brave shields. #9038

Merged
merged 1 commit into from
Jun 8, 2021
Merged

Conversation

iefremov
Copy link
Contributor

@iefremov iefremov commented Jun 7, 2021

see also #5925

Fixes brave/brave-browser#16265

Resolves

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:

Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

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

Just checked, this works on my build with the https://mybroadband.co.za/news/energy/400329-koeberg-nuclear-power-station-manager-suspended.html test case 👍

Copy link
Contributor

@mariospr mariospr left a comment

Choose a reason for hiding this comment

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

LGTM with a small coment

@@ -105,9 +105,11 @@ std::shared_ptr<brave::BraveRequestInfo> BraveRequestInfo::MakeCTX(
// |AddChannelRequest| provides only old-fashioned |site_for_cookies|.
// (See |BraveProxyingWebSocket|).
if (ctx->tab_origin.is_empty()) {
ctx->tab_origin = brave_shields::BraveShieldsWebContentsObserver::
GetTabURLFromRenderFrameInfo(ctx->frame_tree_node_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like you can completely remove BraveShieldsWebContentsObserver::GetTabURLFromRenderFrameInfo() from the repository now, since this was apparently the only caller.

@iefremov iefremov reopened this Jun 8, 2021
@iefremov iefremov merged commit 3123791 into master Jun 8, 2021
@iefremov iefremov deleted the ie_tab_origin branch June 8, 2021 17:09
@iefremov iefremov added this to the 1.27.x - Nightly milestone Jun 8, 2021
brave-builds pushed a commit that referenced this pull request Jun 8, 2021
@iefremov iefremov changed the title [WIP] 16256: An idea of a fix for missing tab_origin. Properly get tab origin for brave shields. Jun 8, 2021
@kjozwiak
Copy link
Member

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.27.51 Chromium: 91.0.4472.88 (Official Build) nightly (64-bit)
--- | ---
Revision | 109e9cd038b94a631aea7d40ee3d56c1278f2597-refs/branch-heads/4472@{#1385}
OS | Windows 10 OS Version 2009 (Build 19042.1023)

Test Case #1 (Original issue mentioned via )

  • load https://www.tecmint.com and disable shields
  • open the dev tools and select Network
  • refresh the page
  • ensure that none of the requests are Status: (blocked:)
1.25.70 Chromium: 91.0.4472.77 1.27.51 Chromium: 91.0.4472.88
issueReproduced issueFixed

Test Case #2

  • visit https://www.google.ca & searched for under armour
  • ensure that ads are being displayed as Trackers & ads blocking is set as standard via brave://settings/shields by default
  • change Trackers & ads blocking to aggressive via the shields panel and ensure that the tab refreshes and ads are not being displayed
  • change Trackers & ads blocking to Disabled via brave://settings/shields and ensure that Trackers & ads blocking is still set aggressive under https://www.google.ca

Test Case #3

  • visit https://www.blizzard.com and ensure that Block scripts is disabled and the page loads without any issues
  • enable Block scripts using the shields panel and ensure that scripts are being blocked once the tab is automatically refreshed

Test Case #4

Test Case #5

local storage:
"00"
session storage:
"00"
indexeddb:
cookies:
""
websql:
{"0":{"tbl_name":"__WebKitDatabaseInfoTable__"},"1":{"tbl_name":"todo"}}
filesystem API:
"https_fiddle.jshell.net_0:Persistent"

Test Case #6

Run through https://dev-pages.brave.software/fingerprinting/farbling.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Brave does not respect Shield settings for Web-Sites
4 participants