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

HTTPSE not working correctly #12252

Closed
LaurenWags opened this issue Dec 11, 2017 · 3 comments · Fixed by #12256
Closed

HTTPSE not working correctly #12252

LaurenWags opened this issue Dec 11, 2017 · 3 comments · Fixed by #12256
Assignees
@darkdh
Labels
0.19.x issue first seen in 0.19.x bug cr63 feature/HTTPSEverywhere QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified regression release-notes/exclude
Milestone
0.19.x Hotfix 8 (...

Comments

@LaurenWags
Copy link
Member

LaurenWags commented Dec 11, 2017 •
edited by luixxiul
Loading

Test plan

#12255 (comment)

Description

HTTPSE does not seem to be functioning correctly in C63 build.

Steps to Reproduce

  1. Clean profile on 0.19.113
  2. Navigate to https://https-everywhere.badssl.com/
  3. Open shields, turn off HTTPS Everywhere.
  4. View page which indicates HTTPSE is off.
  5. Change URL from https to http, page turns red.
  6. Go to shields, enable HTTPSE again.

Actual result:
Page remains Red and indicates that HTTPS Everywhere is off or not working.
httpse019113

Expected result:
Page should turn green as you have enabled HTTPS Everywhere again.
This is how this is working in 0.19.105

httpse019105

Reproduces how often:
Easily

Brave Version

about:brave info:
Brave | 0.19.113
rev | 043c506
Muon | 4.5.25

Reproducible on current live release:
no, not reproducible on 0.19.105

Additional Information

Can also be reproduced with these steps:

  1. Clean profile on 0.19.113
  2. Navigate to https://https-everywhere.badssl.com/
  3. Disable shields.
  4. View page which indicates HTTPSE is off.
  5. Change URL from https to http, page turns red.
  6. Enable shields again.
  7. Page remains red, indicates HTTPSE is off or not working.

Reproduced on Ubuntu by @kjozwiak
Found on MacOS.
@LaurenWags LaurenWags added this to the 0.19.x + C63 (Release Channel) milestone Dec 11, 2017
@bsclifton
Copy link
Member

bsclifton commented Dec 11, 2017 •
edited
Loading

Confirmed this works great on C62 using 0.19.x (basically, I reverted to Muon 4.5.16 and it works great)

This does appear to be C63 related
cc: @darkdh

@darkdh darkdh self-assigned this Dec 11, 2017
@diracdeltas
Copy link
Member

@darkdh seems that the problem is that the tabId of the mainFrame request is set to -1 after shields is disabled and enabled again

diracdeltas added a commit that referenced this issue Dec 12, 2017
@diracdeltas
Don't ignore internal requests in app/filtering
405b64a 
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.

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
diracdeltas added a commit that referenced this issue Dec 12, 2017
@diracdeltas
Don't ignore internal requests in app/filtering
09cbb1e 
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
@diracdeltas diracdeltas mentioned this issue Dec 12, 2017
Merged
8 tasks
@bsclifton bsclifton mentioned this issue Dec 12, 2017
Merged
8 tasks
@bsclifton
Copy link
Member

Fixed with #12255

This was referenced Dec 12, 2017
Closed
Closed
Closed
Closed
@bsclifton bsclifton modified the milestones: 0.19.x + C63 (Release Channel), 0.19.x Private Search Dec 14, 2017
@kjozwiak kjozwiak added cr63 and removed cr63 labels Dec 14, 2017
@kjozwiak kjozwiak modified the milestones: 0.19.x Private Search, 0.19.x + C63 (Release Channel) Dec 14, 2017
bsclifton pushed a commit that referenced this issue Dec 14, 2017
@diracdeltas @bsclifton
Don't ignore internal requests in app/filtering
3ec1fc4 
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@darkdh darkdh

Labels
0.19.x issue first seen in 0.19.x bug cr63 feature/HTTPSEverywhere QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified regression release-notes/exclude
Projects
None yet
Milestone
0.19.x Hotfix 8 (Release Channel)
Development

Successfully merging a pull request may close this issue.

Don't ignore internal requests in app/filtering
7 participants
@diracdeltas @kjozwiak @luixxiul @bsclifton @darkdh @srirambv @LaurenWags