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

Don't ignore internal requests in app/filtering #12256

Merged
merged 1 commit into from
Dec 15, 2017
Merged

Don't ignore internal requests in app/filtering #12256

merged 1 commit into from
Dec 15, 2017

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Dec 12, 2017

Cherry pick of #12255 for master, 0.21.x, 0.20.x, and 0.19.x

and return details.firstPartyUrl as a fallback for main frame URL.

Internal requests (tabId == -1) were ignored in order to fix #5934 in 6023abd. However (1) #5934 works without ignoring internal requests now and (2) generally speaking, non-webview requests should also be protected by HTTPS Everywhere, etc.

fix #12253
fix #12252

Test Plan:

  1. confirm HTTPS Everywhere and Safebrowsing are working using the test plans in Safe Browsing not functioning as expected #12253 and HTTPSE not working correctly  #12252
  2. confirm that Enable Pocket/Password managers doesn't enable the extension #5934 has not regressed by enabling Pocket

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

and return details.firstPartyUrl as a fallback for main frame URL.

Internal requests (tabId == -1) were ignored in order to fix #5934 in 6023abd. However (1) #5934 works without ignoring internal requests now and (2) generally speaking, non-webview requests should also be protected by HTTPS Everywhere, etc.

fix #12253
fix #12252

Test Plan:
1. confirm HTTPS Everywhere and Safebrowsing are working using the test plans in #12253 and #12252
2. confirm that #5934 has not regressed by enabling Pocket
@bsclifton bsclifton added this to the 0.19.x + C63 (Release Channel) milestone Dec 12, 2017
@bsclifton
Copy link
Member Author

bsclifton commented Dec 12, 2017

This PR can stay open until we merge C63 to master/0.19.x 😄 As noted above, it's already contained in the 0.19.x with Chromium 63 build (merged with #12255)

Copy link
Member

@darkdh darkdh 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 243c317 into brave:master Dec 15, 2017
@bsclifton bsclifton deleted the fix/c63httpse branch December 15, 2017 06:02
bsclifton added a commit that referenced this pull request Dec 15, 2017
Don't ignore internal requests in app/filtering
bsclifton added a commit that referenced this pull request Dec 15, 2017
Don't ignore internal requests in app/filtering
@bsclifton
Copy link
Member Author

master 243c317
0.21.x 5489cde
0.20.x cdf69d8

(0.19.x already has this- it was merged directly with #12255)

darkdh added a commit that referenced this pull request Jan 8, 2018
#12508 is caused by empty
url which will leads to sending {cancel: true} to callback of
`session.webRequest.onBeforeRequest` and download process will be
aborted.
Also verified this won't break the issues which
#12256 fixes

fix #12508

Auditors: @bridiver, @diracdeltas, @bsclifton
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants