Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

apply brave UA whitelist to first-party subresources #10904

Merged
merged 2 commits into from
Sep 15, 2017
Merged

apply brave UA whitelist to first-party subresources #10904

merged 2 commits into from
Sep 15, 2017

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Sep 11, 2017 •
edited by bsclifton
Loading

fix #10800

Test Plan:
load adobe.com within Brave
open the developer tools via "option + command + I"
select the "Network" tab and reload adobe.com via "command + r"
select on the first resource and you'll notice the correct Brave Chrome/60.0.3112.101 UA
select another resource that's underneath the first one and is from Host:www.adobe.com
the Brave UA should be there

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).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@diracdeltas
apply brave UA whitelist to first-party subresources
e166b26 
fix #10800

Test Plan:
load adobe.com within Brave
open the developer tools via "option + command + I"
select the "Network" tab and reload adobe.com via "command + r"
select on the first resource and you'll notice the correct Brave Chrome/60.0.3112.101 UA
select another resource that's underneath the first one and is from Host:www.adobe.com
the Brave UA should be there
@diracdeltas diracdeltas added this to the 0.20.x (Developer Channel) milestone Sep 11, 2017
@diracdeltas diracdeltas self-assigned this Sep 11, 2017
@kjozwiak kjozwiak mentioned this pull request Sep 12, 2017
Closed
@ghost ghost added the sprint/1 label Sep 13, 2017
@codecov-io
Copy link

codecov-io commented Sep 14, 2017 •
edited
Loading

Codecov Report

Merging #10904 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10904      +/-   ##
==========================================
+ Coverage   54.14%   54.18%   +0.04%     
==========================================
  Files         248      249       +1     
  Lines       21751    21798      +47     
  Branches     3383     3392       +9     
==========================================
+ Hits        11777    11812      +35     
- Misses       9974     9986      +12
Flag Coverage Δ
#unittest 54.18% <100%> (+0.04%) ⬆️
Impacted Files Coverage Δ
app/siteHacks.js 48.93% <100%> (ø)
app/filtering.js 19.25% <0%> (+0.67%) ⬆️
js/data/siteHacks.js 43.93% <0%> (+12.12%) ⬆️

@bsclifton
Added unit test which ensures:
25c9492 
- filtering.getMainFrameUrl is called (which also queries webcontents; no test for that yet)
- hack considers subdomain

Auditors: @diracdeltas

.
Auditors:

Test Plan:
bsclifton
bsclifton reviewed Sep 14, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good- I added a few unit tests 😄

When I manually tested, most resources looked great. However, I did notice a few which didn't have their UA updated. Is this expected? (looks like all of them have provisional headers)

screen shot 2017-09-14 at 12 29 26 am

@brave brave deleted a comment from NejcZdovc Sep 14, 2017
@diracdeltas
Copy link
Member Author

@bsclifton IIUC provisional headers are shown before the filtering is applied, so that seems fine

bsclifton
bsclifton approved these changes Sep 15, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

++

@bsclifton bsclifton merged commit d5f0ebd into master Sep 15, 2017
@bsclifton bsclifton deleted the fix/10800 branch September 15, 2017 15:28
bsclifton added a commit that referenced this pull request Sep 15, 2017
@bsclifton
Merge pull request #10904 from brave/fix/10800
3968362 
apply brave UA whitelist to first-party subresources
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bsclifton bsclifton bsclifton approved these changes

Assignees

@diracdeltas diracdeltas

Labels
QA/checked-Linux QA/test-plan-specified release-notes/include
Projects
None yet
Milestone
0.20.x (Release Channel)
Development

Successfully merging this pull request may close these issues.

Brave UA whitelist only applies to top level page loads
5 participants
@diracdeltas @codecov-io @bsclifton @luixxiul @NejcZdovc