Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

Fix extension popup shows rulesets from previous pages #17025

Merged
merged 2 commits into from
Nov 5, 2018
Merged

Fix extension popup shows rulesets from previous pages #17025

merged 2 commits into from
Nov 5, 2018

Conversation

cschanaj
Copy link
Collaborator

@cschanaj cschanaj commented Nov 4, 2018

STR:

  1. Visit http://time.com/ using a build from b056c3d from Early return for URLS that are already HTTPS #17010
  2. Visit https://bloomberg.com/
  3. Notice that rulesets applied to http://time.com/ shows up when the URL is changed to https://bloomberg.com/
  4. Expect rulesets from previous page are clear and only rulesets applied to current page are shown.

Issues

02a9220 breaks browser test because #17010 contains a bugs. Step 3 can be repeated infinitely for different domains, as long as all the subsequent pages are HTTPS only. So this bug cannot be ignored.

  1. For non-HTTPS pages, if main_frame is redirected by HTTPSE, no ruleset will be shown on the popup. Otherwise, rulesets for page's resources will be shown (if any).

  2. For HTTPS pages, no ruleset be shown unless the page serve mixedcontent

  3. No rulesets will be shown for most of the time (at least for me), this might confuse some users that HTTPSE is not working. Perhaps there should be a brief explanation in the release notice on this matter.

To enable test/selenium/test_popup.py::TestPopup::test_rule_shown, we need to perform the test against a mixedcontent pages ... which seem an anti-pattern for me. So I have disabled the test in 2bf5f79, I am open for discussions on this.

cc @jsha @zoracon @Hainish This PR will fix a bug from #17010 and should be merged before the next release. thanks.

@Bisaloo Bisaloo added this to the Before the Next Release milestone Nov 4, 2018
@jsha
Copy link
Member

jsha commented Nov 4, 2018

Thanks for spotting this, @cschanaj! I'll take a look at this PR with @zoracon on Monday.

@Hainish
Copy link
Member

Hainish commented Nov 5, 2018

Thanks @jsha and @cschanaj. Please flag me for review when you open the PR.

@zoracon
Copy link
Contributor

zoracon commented Nov 5, 2018

@Hainish FYI this is already a PR but we reviewed and tested and it looks good to go. Just going to merge this in with some urgency since PR #17010 is in fact breaking the build

@zoracon zoracon merged commit 35ba0fe into EFForg:master Nov 5, 2018
@cschanaj cschanaj deleted the fix-bug-17010 branch November 10, 2018 07:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants