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

Disable flash in Tor #624

Merged
merged 1 commit into from
Oct 14, 2018
Merged

Disable flash in Tor #624

merged 1 commit into from
Oct 14, 2018

Conversation

yrliou
Copy link
Member

@yrliou yrliou commented Oct 12, 2018

Fix brave/brave-browser#1078
Flash is blocked by default right now, with this PR, users won't be able to enable it through content settings bubble or page info in Tor profile, so the settings should always stay blocked.
BTW, changing flash permissions in the normal profile won't be propagated to Tor profile either.

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).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Open a new Tor window
  2. Visit http://homestarrunner.com/main1.html
  3. Click the plugin blocked bubble, there should be no "Run flash this time" and "Manage" button

screen shot 2018-10-12 at 1 42 33 pm

4. Click the page info, there should be no flash entry to change permission.

screen shot 2018-10-12 at 1 44 43 pm

  1. Click on the flash element won't have any effect
  2. Right click on the flash element, should not be able to click "Run this plugin"

screen shot 2018-10-12 at 1 53 05 pm

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@yrliou yrliou self-assigned this Oct 12, 2018
@yrliou
Copy link
Member Author

yrliou commented Oct 12, 2018

Works fine on mac, I'm building it on window & linux now to test.

@yrliou yrliou requested a review from bbondy October 12, 2018 21:19
@yrliou
Copy link
Member Author

yrliou commented Oct 13, 2018

@bbondy This PR is ready for review, I can't do the manual test on linux though because flash seems to be not working on linux(brave/brave-browser#1562), but in theory I think this PR works for all platforms.

Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

LGTM

const PageInfoUI::PermissionInfo& info,
content::WebContents* web_contents) {
if ((info.type == CONTENT_SETTINGS_TYPE_PLUGINS ||
info.type == CONTENT_SETTINGS_TYPE_GEOLOCATION) &&
Copy link
Member

Choose a reason for hiding this comment

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

Corresponding issue just said about blocking plugin.
Do we also want to hide location info in page info bubble?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intentional cuz location permission is blocked in tor.

@simonhong
Copy link
Member

Or, how about storing flash plugin & geolocation setting to ContentSettingsStore?
Chromium code base has a policy that user can't edit configuration set by extension by default.
With this, we don't need to disable/hide explicitly. WDYT?

@yrliou
Copy link
Member Author

yrliou commented Oct 13, 2018

Or, how about storing flash plugin & geolocation setting to ContentSettingsStore?
Chromium code base has a policy that user can't edit configuration set by extension by default.
With this, we don't need to disable/hide explicitly. WDYT?

@simonhong I think it would be a bit weird that these default settings in Tor profile is "set by extension".
Especially that would be the message shown to users when they check the page info bubble.
We could probably use other providers that might make more sense, but I would prefer not to block this PR on how we set/store these settings and do it in a follow up since we also need to revise how we disable geolocation in Tor.
In any cases, I think the change in brave_content_setting_bubble_model.cc in this PR would still be needed.

@simonhong
Copy link
Member

@yrliou Yeah, using appropriate providers seems make sense. My above comment is non-blocking comment. So you can go ahead with current one.

@bbondy bbondy merged commit 9aa3374 into master Oct 14, 2018
bbondy added a commit that referenced this pull request Oct 14, 2018
bbondy added a commit that referenced this pull request Oct 14, 2018
@bbondy
Copy link
Member

bbondy commented Oct 14, 2018

master: 9aa3374
0.56.x: 236ff44
0.55.x: a2cdc86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable flash in Tor Mode
3 participants