Closed Bug 795397 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Plug-ins, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox17 --- fixed
firefox18 --- verified

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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 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+
Attached patch patch v2 (obsolete) — Splinter Review
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/68bf9d057cf4

A real commit message would have been nice :-(
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/68bf9d057cf4
Status: ASSIGNED → RESOLVED
Closed: 7 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
[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?
Attachment #666608 - Attachment is obsolete: true
Attachment #667549 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed
Whiteboard: [patch needs uplift to Aurora]
Is there a way to undo these settings?
(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.
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
You need to log in before you can comment on or make changes to this bug.