Issue 1549: Return constant results for navigator.plugins and navigator.mimeTypes #697

jumde · 2018-10-18T22:30:45Z

Description

From: brave/brave-browser#1549 (comment) - navigator.plugins and navigator.mimeTypes will return an empty array until Flash is installed and explicitly enabled. navigator.plugins and navigator.mimeTypes will return an empty array in tor mode.

Submitter Checklist:

Test Plan:

Without flash:

>navigator.mimeTypes
MimeTypeArray {length: 0}

>navigator.plugins
PluginArray {length: 0}

With Flash:

Install flash player: https://get.adobe.com/flashplayer/
Navigate to isflashinstalled.com and enable flash for the site.
The site will be reloaded and the output of the site should say Yes.

navigator.plugins
PluginArray {0: Plugin, Shockwave Flash: Plugin, length: 1}

navigator.mimeTypes
MimeTypeArray {0: MimeType, 1: MimeType, application/x-shockwave-flash: MimeType, application/futuresplash: MimeType, length: 2}

Verify that navigator.plugins and navigator.mimeTypes return empty arrays in tor mode.

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

jumde · 2018-10-30T09:23:43Z

test/data/navigatormimetypes.html

@@ -0,0 +1,5 @@
+<script>


This test works since flash is explicitly disabled by default

can we test to make sure it does show up when it is enabled?

When enabled: https://github.com/brave/brave-core/pull/697/files#diff-b0e5e9c3d35f52b63938805cd7ef86bb

diracdeltas · 2018-11-01T20:35:57Z

patches/third_party-blink-renderer-platform-plugins-plugin_data.cc.patch

+   Vector<mojom::blink::PluginInfoPtr> plugins;
+   registry->GetPlugins(false, main_frame_origin_, &plugins);
+   for (const auto& plugin : plugins) {
+    if (plugin->name != "Shockwave Flash") {


if i understand correctly, this will never add any plugins to navigator.plugins/mimeTypes unless it is the Flash plugin. but the behavior we want is:

if flash is not enabled for the site via click-to-play, navigator.plugins/mimeTypes is empty

if flash is enabled, navigator.plugins/mimeTypes only shows Flash

i will update https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode#privacy-protection-enabled-regardless-of-whether-fingerprinting-protection-mode-is-on to be more clear about this

@diracdeltas - GetPlugins does not return the flash plugin if its not enabled via click-to-play. If flash is not enabled via click-to-play, navigator.plugins/mimeTypes will return empty.

@jumde right, i'm saying that GetPlugins should return flash when it's enabled via click to play, which i don't think happens with this PR.

Resolved after offline conversation.

I think it would be better to implement this by subclassing PluginRegistryImpl and doing a chromium_src override to replace it in RenderProcessHostImpl

then we can filter it out in PluginRegistryImpl::GetPlugins without affecting the actual PluginList used by the browser process

diracdeltas · 2018-11-13T22:50:35Z

PJ showed me the behavior of this PR in-person and it lgtm. defer on code approval.

jumde · 2018-11-13T23:58:10Z

Updating reviewer from @yrliou to @bridiver after discussion with @yrliou since @bridiver reviewed the recent navigator PRs.

bridiver · 2018-11-20T20:26:22Z

content/browser/renderer_host/brave_plugin_registry_impl.cc

+
+  for(std::vector<blink::mojom::PluginInfoPtr>::iterator it = plugins.begin(); 
+    it != plugins.end(); ++it) {
+    if ((*it) && (*it)->name == base::ASCIIToUTF16("Shockwave Flash")) {


content::kFlashPluginName for "Shockwave Flash"

bridiver · 2018-11-20T20:34:20Z

content/browser/renderer_host/brave_plugin_registry_impl.cc

+                                    GetPluginsCallback callback) {
+  PluginRegistryImpl::GetPlugins(refresh, main_frame_origin,
+    base::BindOnce(
+      &BravePluginRegistryImpl::GetPluginsComplete, base::Unretained(this), 


you must use weak ptr here just like the superclass

actually this should be handled because the lifetime issue is already covered by the superclass WeakPtr in the original callback.

the embedded callback will be called synchronously so no lifetime issues at that point

bridiver

see comments

bridiver · 2018-11-20T21:00:02Z

chromium_src/content/browser/renderer_host/render_process_host_impl.cc

+
+#include "brave/content/browser/renderer_host/brave_plugin_registry_impl.h"
+
+#define PluginRegistryImpl BravePluginRegistryImpl


if you are overriding a class like this, you should always include the header for the class you are overriding before redefining it. Otherwise it's possible that it won't already be defined at this point and you'll change the original header file (likely resulting in a build failure)

it gets added from brave_plugin_registry_impl.h, but better to be explicit and make sure there are no issues

@diracdeltas

…or.mimeTypes auditors: @diracdeltas, @bridiver

petemill · 2018-11-26T08:45:33Z

Note that this introduced a build error on master as described in brave/brave-browser#2222. It's fixed by #952 which I've merged since it solves the issue. Please check it out and see if it's the preferred fix. If not, perhaps create a follow-up...

bbondy · 2018-11-26T13:38:04Z

was this tested with Widevine?

jumde · 2018-11-26T15:49:34Z

was this tested with Widevine?

Yes! For detecting widevine web-authors use navigator.requestMediaKeySystemAccess instead of navigator.plugins or navigator.mimeTypes

jumde · 2018-11-26T15:52:57Z

Note that this introduced a build error on master as described in brave/brave-browser#2222. It's fixed by #952 which I've merged since it solves the issue. Please check it out and see if it's the preferred fix. If not, perhaps create a follow-up...

I was trying a different patch that addresses this issue: https://github.com/brave/brave-core/compare/fixes_2222?expand=1. Was testing this on windows to see if linker errors were resolved.

jumde self-assigned this Oct 18, 2018

jumde requested a review from diracdeltas October 18, 2018 22:30

diracdeltas requested a review from yrliou October 18, 2018 22:38

jumde force-pushed the mimetypes_fingerprinting branch 2 times, most recently from b515e1b to 6be3441 Compare October 29, 2018 20:17

jumde commented Oct 30, 2018

View reviewed changes

jumde force-pushed the mimetypes_fingerprinting branch 3 times, most recently from 1e390a8 to 2e9acc2 Compare November 1, 2018 08:51

diracdeltas reviewed Nov 1, 2018

View reviewed changes

jumde requested review from bridiver and removed request for yrliou November 13, 2018 23:56

jumde force-pushed the mimetypes_fingerprinting branch 9 times, most recently from 21d7eea to f813bb8 Compare November 19, 2018 18:35

bridiver reviewed Nov 20, 2018

View reviewed changes

bridiver requested changes Nov 20, 2018

View reviewed changes

bridiver reviewed Nov 20, 2018

View reviewed changes

jumde force-pushed the mimetypes_fingerprinting branch 2 times, most recently from 7ac9edd to 2845757 Compare November 20, 2018 21:39

Issue 1549: Return constant results for navigator.plugins and navigat…

d039e8f

…or.mimeTypes auditors: @diracdeltas, @bridiver

jumde force-pushed the mimetypes_fingerprinting branch from 2845757 to d039e8f Compare November 21, 2018 18:21

bridiver approved these changes Nov 21, 2018

View reviewed changes

jumde merged commit 4807138 into master Nov 21, 2018

simonhong mentioned this pull request Nov 23, 2018

Build failure from brave_plugin_registry_impl.o brave/brave-browser#2222

Closed

petemill added the merged/0.59.x-Nightly label Nov 26, 2018

srirambv mentioned this pull request Jan 2, 2019

Return constant result for navigator.mimeTypes and navigator.plugins brave/brave-browser#1549

Closed

bbondy added this to the 0.59.x - Beta milestone Jan 14, 2019

bsclifton deleted the mimetypes_fingerprinting branch January 24, 2019 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1549: Return constant results for navigator.plugins and navigator.mimeTypes #697

Issue 1549: Return constant results for navigator.plugins and navigator.mimeTypes #697

jumde commented Oct 18, 2018 •

edited

Loading

jumde Oct 30, 2018

bridiver Nov 14, 2018

jumde Nov 19, 2018

diracdeltas Nov 1, 2018

jumde Nov 2, 2018

diracdeltas Nov 7, 2018

jumde Nov 13, 2018

bridiver Nov 14, 2018

bridiver Nov 14, 2018

diracdeltas commented Nov 13, 2018

jumde commented Nov 13, 2018

bridiver Nov 20, 2018

bridiver Nov 20, 2018

bridiver Nov 20, 2018

bridiver Nov 20, 2018

bridiver left a comment

bridiver Nov 20, 2018

bridiver Nov 20, 2018

petemill commented Nov 26, 2018

bbondy commented Nov 26, 2018

jumde commented Nov 26, 2018

jumde commented Nov 26, 2018


		#include "brave/content/browser/renderer_host/brave_plugin_registry_impl.h"

		#define PluginRegistryImpl BravePluginRegistryImpl

Issue 1549: Return constant results for navigator.plugins and navigator.mimeTypes #697

Issue 1549: Return constant results for navigator.plugins and navigator.mimeTypes #697

Conversation

jumde commented Oct 18, 2018 • edited Loading

Description

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Nov 13, 2018

jumde commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Nov 26, 2018

bbondy commented Nov 26, 2018

jumde commented Nov 26, 2018

jumde commented Nov 26, 2018

jumde commented Oct 18, 2018 •

edited

Loading