click-to-play blocklisting: respect the "plugins" permission

RESOLVED FIXED in Firefox 17

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla18
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17 fixed, firefox18 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Initially I thought it would be a good idea for a click-to-play-blocklisted plugin to always be click-to-play, regardless of the "plugins" permission value. (The reasoning being, if a user has in the past said "always allow plugins" on a site, and then we make a plugin click-to-play, they should be made aware of the security implications of running that plugin.) However, since non-blocklist click-to-play isn't a first-class feature yet anyway, this a) won't happen to many users and b) isn't the right behavior anyway for the rest of users.
Ideally, we would have a more sophisticated plugins-permission manager that would handle all of these edge cases and actually differentiate between types of plugins and whether or not a plugin was vulnerable when it was allowed to run, but we're a ways away from that. In the meantime, it would be best if the permissions simply worked as expected.
Created attachment 666028 [details] [diff] [review]
patch

Josh - if you could review the nsObjectLoadingContent changes, that'd be great.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #666028 - Flags: review?(joshmoz)
Comment on attachment 666028 [details] [diff] [review]
patch

And Jared, if you could review the test changes, that would also be great.
Attachment #666028 - Flags: review?(jaws)
Comment on attachment 666028 [details] [diff] [review]
patch

Review of attachment 666028 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the test changes.
Attachment #666028 - Flags: review?(jaws) → review+
Comment on attachment 666028 [details] [diff] [review]
patch

Review of attachment 666028 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2525,5 @@
> +  // set the fallback reason
> +  // (if it's click-to-play, it might be because of the blocklist)
> +  uint32_t state;
> +  nsresult rv = pluginHost->GetBlocklistStateForType(mContentType.get(), &state);
> +  NS_ENSURE_SUCCESS(rv, false);

nit, if this call fails for some reason, we might return false here without setting aReason (an out param), which would result in callers using uninitialized values for fallback reasons.

Comment 5

5 years ago
Comment on attachment 666028 [details] [diff] [review]
patch

Review of attachment 666028 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a fix for the nit :johns pointed out
Attachment #666028 - Flags: review?(joshmoz) → review+
Created attachment 666608 [details] [diff] [review]
patch v2

Fixed it so the aReason gets set. Carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=0281ae49e05e (only running on one platform to conserve try resources and because there are no platform-specific changes. If this blows up in my face I'll go back to running on everything :)
Attachment #666028 - Attachment is obsolete: true
Attachment #666608 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/68bf9d057cf4

A real commit message would have been nice :-(
Flags: in-testsuite+
Keywords: checkin-needed

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/68bf9d057cf4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(In reply to Ryan VanderMeulen from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/68bf9d057cf4
> 
> A real commit message would have been nice :-(

Sorry about that :-/
Target Milestone: mozilla18 → ---
... and the target milestone got reset. I'm doing really well lately...
Target Milestone: --- → mozilla18
Created attachment 667549 [details] [diff] [review]
patch v2 with commit message

[Approval Request Comment]
Bug caused by (feature/regressing bug #): blocklist/click-to-play plugins (bug 760625)
User impact if declined: the "always activate plugins" option in the popup notification won't work for blocklisted plugins
Testing completed (on m-c, etc.): tested on m-c
Risk to taking this patch (and alternatives if risky): low (potentially could regress click-to-play plugins, but we've seen no evidence of that yet)
String or UUID changes made by this patch: none
Attachment #667549 - Flags: review+
Attachment #667549 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #666608 - Attachment is obsolete: true

Updated

5 years ago
Attachment #667549 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [patch needs uplift to Aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/29e52e3219a3
status-firefox17: --- → fixed
status-firefox18: --- → fixed
Keywords: checkin-needed
Whiteboard: [patch needs uplift to Aurora]
Is there a way to undo these settings?
Blocks: 738698
(In reply to Paul Silaghi [QA] from comment #13)
> Is there a way to undo these settings?

At the moment, Tools -> Clear Recent History -> Site Preferences :-/
I'm working on more accessible ui in bug 775857 and bug 821892.
(In reply to Paul Silaghi [QA] from comment #13)
> Is there a way to undo these settings?

You can also find the site in question in about:permissions and select "forget this site"
The plugin permissions are remembered. Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b4

But I personally think this shouldn't land until bug 775857 and bug 821892 are fixed. Our users won't know to undo their settings. Clicking "Never activate plugins" by mistake on youtube or facebook will have a big negative impact.
status-firefox18: fixed → verified
Actually, I looked into this a bit more. Bug 746374 made the change that caused "Page Info" and about:preferences to not work for this. That landed in 20, so until then, these two methods work just fine for undoing "always-" or "never activate for this site".
(In reply to David Keeler from comment #17)
> about:preferences

by which I mean about:permissions, of course
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=775857#c4
You need to log in before you can comment on or make changes to this bug.