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

shields don't work after re-enabling in private browsing mode #1198

Closed
AndriusA opened this issue Sep 19, 2018 · 10 comments · Fixed by brave/brave-core#532
Closed

shields don't work after re-enabling in private browsing mode #1198

AndriusA opened this issue Sep 19, 2018 · 10 comments · Fixed by brave/brave-core#532

Comments

@AndriusA
Copy link

AndriusA commented Sep 19, 2018

Test Plan

See brave/brave-extension#68

Description

After disabling shields in private browsing mode on a website and re-enabling them, they don’t seem to be working even though the toggle is set to "on".

Shields continue working as expected in normal browsing mode, problem only occurs in private mode

Steps to Reproduce

  1. open browser
  2. open private mode window
  3. navigate to any website (reproduced on cnn.com and tmz.com)
  4. disable shields; Brave should reload page
  5. you should see ads / tracking / etc
  6. enable shields; Brave should reload page
  7. notice shields doesn't block anything

Actual result:

No requests blocked

Expected result:

10s of requests blocked on tested websites

Reproduces how often:

Reproduces consistently on OS X, confirmed by another person

Brave version (chrome://version info)

Brave 0.55.5 Chromium: 70.0.3538.16 (Official Build) dev (64-bit)
Revision 16ed95b41bb05e565b11fb66ac33c660b721f778-refs/branch-heads/3538@{#306}
OS Mac OS X
JavaScript V8 7.0.276.9
Flash (Disabled)
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.16 Safari/537.36
Command Line /Applications/Brave-Browser-Dev.app/Contents/MacOS/Brave Browser Dev --enable-tab-audio-muting --disable-domain-reliability --disable-chrome-google-url-tracking-client --no-pings --enable-features=EnableEmojiContextMenu,DesktopPWAWindowing,fill-on-account-select,NewExtensionUpdaterService --disable-features=SharedArrayBuffer --flag-switches-begin --flag-switches-end
Executable Path /Applications/Brave-Browser-Dev.app/Contents/MacOS/Brave Browser Dev

Reproducible on current release:

No

Website problems only:

N/A

@srirambv
Copy link
Contributor

cc: @bridiver

@bsclifton bsclifton self-assigned this Sep 25, 2018
@bsclifton
Copy link
Member

Reproduced. It carries over to new private windows and persists after quitting Brave and relaunching. Gonna dig in on this...

@bsclifton
Copy link
Member

bsclifton commented Sep 25, 2018

Content settings are being persisted to disk for incognito sessions (via indexeddb) via the Brave extension. This might have a similar root cause / fix as what I fixed in Muon with brave/muon#648

@bsclifton
Copy link
Member

bsclifton commented Sep 25, 2018

OK I believe I figured this out 😄

I had to make a few tweaks to get this working as expected. When testing this, I loaded the brave-extension manually from disk after making some changes.

  1. I added "incognito": "split", to the manifest.json for brave-extension. You can read more about it here, but basically we should be specifying this because I would think we want the extension in private tabs to have a different process than the extension in public tabs.
  2. Since I loaded the extension myself, I was able to manually turn on this switch:
    screen shot 2018-09-25 at 3 39 36 pm
    I don't know how we would do this programmatically though
  3. The calls in shieldsAPI.js need to have a scope set, specifying whether or not the call is for incognito session. If in incognito, scope of incognito_session_only should be sent.

I'm not sure why the scope can't be automatically determined, but it isn't. If you do steps 1 and 2, you'll run into an error when setting the content_setting value (because incognito sessions can't access regular sessions; different processes)

@LaurenWags
Copy link
Member

Seeing something similar when changing some individual shield items on Private/Guest/Tor windows:
Scenario 1:
Open a private/guest/tor window.
Navigate to nytimes.com
Allow Ads and Tracking from shields --> page refreshes, but Ads and Trackers still blocked, count is listed in shields/badge.
Scenario 2:
Open a private/guest/tor window.
Navigate to https://jsfiddle.net/7ke9r14a/9/
Allow all cookies from shields --> page refreshes and never loads properly. Same thing happens if you choose either of the other cookie options.
Scenario 3:
Open a private/guest/tor window.
Navigate to https://https-everywhere.badssl.com/
Turn off HTTPSE --> page refreshes but site still shows HTTPSE enabled. no count in shields/badge.

Note, some shield items functioned as expected in Private/Guest/Tor windows:
Open a private/guest/tor window.
Navigate to https://diafygi.github.io/webrtc-ips/
Change FP to Block All --> IP address is blocked as expected. shields count is updated as expected. able to change back to original setting w/o issue.
Open a private/guest/tor window.
Navigate to brianbondy.com
Enable block scripts --> nothing shown on page as expected. shields count is updated as expected. able to change back to original setting w/o issue.

@bsclifton
Copy link
Member

Quick update, RE: my bullet points above:

@bbondy
Copy link
Member

bbondy commented Sep 30, 2018

Can this be retested with master? I think maybe Simon's change fixes it.

@bsclifton
Copy link
Member

@bbondy no change; after brave/brave-core#525 was merged, any attempt to write to regular scope in a session scoped profile will fail. I rebuild latest and verified it fails as I expected to. Next step is to implement the step 3 above: basically providing the scope in all of the content_setting changes (in brave-extension)

@simonhong
Copy link
Member

Hmm, my PR was api changing from content settings store to user prefs. So, it wouldn't fix this issue.
As @bsclifton already did in new PR, I think scope should be set if we want to set settings separately on both mode. If not, settings from normal and incognito mode would be mixed in private window.

@srirambv
Copy link
Contributor

srirambv commented Oct 17, 2018

Verification Passed on

Brave 0.55.16 Chromium: 70.0.3538.54 (Official Build) (64-bit)
Revision 4f8e578b6680574714e9ed3bb9f02922b4dde40d-refs/branch-heads/3538@{#937}
OS Linux

Verification passed on

Brave 0.55.17 Chromium: 70.0.3538.67 (Official Build) (64-bit)
Revision 9ab0cfab84ded083718d3a4ff830726efd38869f-refs/branch-heads/3538@{#1002}
OS Windows 7 x64

Verified passed with

Brave 0.55.17 Chromium: 70.0.3538.67 (Official Build) (64-bit)
Revision 9ab0cfab84ded083718d3a4ff830726efd38869f-refs/branch-heads/3538@{#1002}
OS Mac OS X

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment