The add-on blocklist service needs to support the concept of severities for blocks and allow for customising what level is a hard block and what is a warning.

First round of this patch is on its way and will cover notification when the blocklist is updated for plugins and add-ons and notification during installation of xpis. Plugin detection and add-ons detected during startup will not have any notification in the first patch, instead those that are blocked above the threshold will be app disabled, the others that are still listed in the blocklist will be disabled but can be re-enabled later by the user. Screenshots for UI review to follow...

I need a skim over the words used in the example screenshots, my wording is probably not right, trying to clarify the difference between the soft and hard block to the user is not easy.

a few testcases to be created here once this is finalized.

Comment on attachment 344303 [details] User attempts to install an xpi that is soft blocked Shouldn't the default button be cancel since we are recommending not to install soft blocked add-ons?

These look pretty good. I had a few suggestions, which mainly attempt to be more consistent with our language and description to the user of what's going on. 1. New items have been hard blocked In this screenshot, I think it's a bit unclear whether the the user is disabling the add-ons by restarting, or whether they have already been disabled. The texts says they have "now been disabled," and the text asks the user to "disable and restart." Mossop, have these hard-block addons been disabled at the point the user sees this message, or are they disabled at the next restart? If the former, the message should be more informative - telling the user what has happened and clicking OK. If the latter, then even if the user dismisses this message the add-ons will still be disabled at the next restart - that's more of a "later" than "cancel," which suggests the user is canceling the block itself. I'd change the title of this window based on which one is the case as well. 2. New items have been soft-blocked The language we're using to describe soft-block addons here is inconsistent. We haven't verified them as dangerous, so my though is that we should be steering more towards they "may be a security risk" than they are "dangerous add-ons." I would change the text to "It is highly recommended that you restart with these add-ons disabled for your protection." Would change title to: "Warning: Add-ons may be instable or insecure." 3. Some soft-block, some hard-block As with #2 above, I'd change text to: "The add-ons that have been verified as security threats have been blocked. It is highly recommended that you restart with the other add-ons disabled for your protection." Would change title to "Warning: Add-ons may be instable or insecure." 4. Soft-block install Would change title to: "Warning: Add-on may be insecure or unstable" 5. Hard-block install Would change title to: Warning: Add-on cannot be installed."

(In reply to comment #10) > These look pretty good. I had a few suggestions, which mainly attempt to be > more consistent with our language and description to the user of what's going > on. > > 1. New items have been hard blocked > > In this screenshot, I think it's a bit unclear whether the the user is > disabling the add-ons by restarting, or whether they have already been > disabled. The texts says they have "now been disabled," and the text asks the > user to "disable and restart." > > Mossop, have these hard-block addons been disabled at the point the user sees > this message, or are they disabled at the next restart? If the former, the > message should be more informative - telling the user what has happened and > clicking OK. If the latter, then even if the user dismisses this message the > add-ons will still be disabled at the next restart - that's more of a "later" > than "cancel," which suggests the user is canceling the block itself. I'd > change the title of this window based on which one is the case as well. You must restart for them to be properly unloaded and so the user be safe. Once this dialog appears they have been marked to never load again but they remain in memory till the next time Firefox is closed. > 2. New items have been soft-blocked > > The language we're using to describe soft-block addons here is inconsistent. > We haven't verified them as dangerous, so my though is that we should be > steering more towards they "may be a security risk" than they are "dangerous > add-ons." I would change the text to "It is highly recommended that you > restart with these add-ons disabled for your protection." Would change title > to: "Warning: Add-ons may be instable or insecure." I'm a little wary of just describing things as potential security risks. We use the blocklist to cover both security and stability issues, there isn't one word to describe that I think. I also don't think we will be adding things to the blocklist unless we have verified them as a problem. Another problem is that the user can change what level the block becomes soft or hard, meaning the specific issue with an add-on could be different. > 3. Some soft-block, some hard-block > > As with #2 above, I'd change text to: "The add-ons that have been verified as > security threats have been blocked. It is highly recommended that you restart > with the other add-ons disabled for your protection." Would change title to > "Warning: Add-ons may be instable or insecure." > > 4. Soft-block install > > Would change title to: "Warning: Add-on may be insecure or unstable" > > 5. Hard-block install > > Would change title to: Warning: Add-on cannot be installed." These titles feel very inconsistent with what we use elsewhere in the product to me.

Still need to do further revisions on the strings but that shouldn't stop us getting the code reviewed and it's not the smallest patch in the world... There are a couple of interface changes to note. nsIBlocklistService has additive changes and shouldn't break any existing js users. nsIExtensionManager has a breaking change, however the method removed should never be getting called by anyone. If that is a real problem though we can just leave it in and make it do nothing. There will probably need to be a change to the blocklist url, that will come when the server side is updated. I've created the new blocklist.xul UI for the notification, list.xul was getting too overloaded for it to make sense anymore. nsIBlocklistService.idl has been moved to the gecko tier to make it visible to the plugin host (final location still to be determined). This allows the plugin host to check for blocklisting when it detects new plugins as opposed to the blocklist having to go through the entire list whenever it changes. Updated the test for bug 455906 to also cover notification for plugins, and added a test for the new notifications including blocking and warning.

Comment on attachment 344469 [details] [diff] [review] patch rev 1 jst, can you review the changes to nsPluginHostImpl.cpp. Also bsmedberg said you might have a better idea of where to put nsIBlocklistService.idl?

Thanks for the helpful clarifications, Mossop. 1. New Items have been hard blocked The language regarding whether or not the add-ons have been blocked is still a bit confusing. How about for the text under the list: "The add-ons that have been verified as security risks have been blocked, but a restart is required to remove them completely." Button: "Remove Add-ons and Restart Minefield" 2. New items have been soft-blocked. Mossop said that soft-blocks are for verified problems, not just potential risks, so I agree the language should be stronger. Only potential problem with the current form is that "the dangerous add-ons" seems to imply a subset of those listed in the box. And, we've already said that they may be unstable or insecure (above the box) So, how about for text under the list: "For your protection, it is highly recommended that you restart with these add-ons disabled." 3. Some hard, some soft To be consistent with above: "The add-ons that have been verified as security risks have been blocked. For your protection, it is highly recommended that you restart with the other add-ons disabled." 4. - 5. Soft & hard block install After looking through Firefox's other error messages, the language in these in their current form looks good!

(In reply to comment #14) > Button: > > "Remove Add-ons and Restart Minefield" This isn't really appropriate since we aren't removing the add-ons we are simply blocking them from loading. They will still appear in the add-ons manager and the user can later update to a new stable version. Fixing up the patch for the other bits now.

I'm also going to add a warning in the add-ons manager for items that have been warned about: "This add-on may expose you to security risks."

Re Comment 15: Fair enough. How about: "The add-ons that have been verified as security risks have been blocked, but a restart is required to disable them completely." Button: "Disable Add-ons and Restart Minefield" Re Comment 16: Sounds good

Updated screenshots for the previous views above, these should be good to go now I assume. This is the warning in the add-ons manager that hopefully will notify people that they shouldn't be using certain add-ons.

Slightly better view. The warning displays even when the item is unselected to indicate to the user that they are running with something that may be dangerous

Updated patch with the new strings and additional warning. Rob if you could look at the string usage so I can land them before the freeze on Thursday that would be great.

I should be able to get to it by tomorrow evening.

Comment on attachment 345072 [details] Add-ons manager warning when unselected One possible way to draw attention besides the already overloaded exclamation point (which is really to draw attention to there being additional info) is to use a different background / text color for add-ons in this state... just a thought. Also, did you intentionally make the text right align as in the screenshot?

(In reply to comment #25) > (From update of attachment 345072 [details]) > One possible way to draw attention besides the already overloaded exclamation > point (which is really to draw attention to there being additional info) is to > use a different background / text color for add-ons in this state... just a > thought. I'll chat with Boriss about this since we have all sorts of different ways we want to draw attention now and she has already been thinking about some of them. I think for now this will probably do but if something good comes up I'll add it in. > Also, did you intentionally make the text right align as in the screenshot? That is how those statuses have always displayed on mac

These all look good, let's push them out. Re Comment 26 - I do think there's better ways to highlight add-ons in this list, but since this is already standard perhaps we should deal with this in another bug or overhaul it completely when we fix the add-on manager.

(In reply to comment #26) >... > > Also, did you intentionally make the text right align as in the screenshot? > > That is how those statuses have always displayed on mac I'm surprised there isn't a bug on this... they should all be left aligned and there should be an exclamation point image to the left (red for issues such as blocklist, incompatible, etc. and green for info such as update available).

(In reply to comment #28) > (In reply to comment #26) > >... > > > Also, did you intentionally make the text right align as in the screenshot? > > > > That is how those statuses have always displayed on mac > I'm surprised there isn't a bug on this... they should all be left aligned and > there should be an exclamation point image to the left (red for issues such as > blocklist, incompatible, etc. and green for info such as update available). Sorry I can add the exclamation point to the warning items, I hadn't noticed it wasn't there. I'll try to figure out where the right align came from

Adds the red notification for the warning state. The right alignment on mac came from bug 397723, specifically http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/toolkit/themes/pinstripe/mozapps/extensions&command=DIFF_FRAMESET&file=extensions.css&rev2=1.33&rev1=1.32 I'll catch up with Boriss later and see what she thinks about that, I'll file it separately if it warrants a change.

Comment on attachment 345120 [details] [diff] [review] patch rev 3 A lot more has changed than I expected so I am adding my string change recommendations before I've finished the review >diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/blocklist.dtd b/toolkit/locales/en-US/chrome/mozapps/extensions/blocklist.dtd >new file mode 100644 >--- /dev/null >+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/blocklist.dtd >@@ -0,0 +1,10 @@ >+<!ENTITY blocklist.title "Add-ons may be causing problems"> >+<!ENTITY blocklist.style "width: 45em; height: 30em"> >+<!ENTITY blocklist.summary "&brandShortName; has determined that the following add-ons may be unstable or insecure:"> For consistency with the other new strings: &brandShortName; has determined that the following add-ons are known to cause stability or security problems. >+<!ENTITY blocklist.warning "For your protection, it is highly recommended that you restart with these add-ons disabled."> "with these add-ons disabled" is a tad awkward. Perhaps: For your protection, it is highly recommended that you restart to disable these add-ons. along with a note that unchecking disable is not recommended. >+<!ENTITY blocklist.blocked "The add-ons that have been verified as security risks have been blocked, but a restart is required to disable them completely."> If the above is changed perhaps: For your protection, it is highly recommended that you restart to disable these add-ons. >+<!ENTITY blocklist.blockedandwarn "The add-ons that have been verified as security risks have been blocked. For your protection, it is highly recommended that you restart with the other add-ons disabled."> "with the other add-ons disabled" is awkward. >+<!ENTITY blocklist.moreinfo "More information"> >+ >+<!ENTITY blocklist.accept.label "Disable Add-ons and Restart &brandShortName;"> This is a tad wordy for a button especially since it is already stated immediately above that restarting is required to disable the blocked add-ons. How about just "Restart &brandShortName;"? >diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd >--- a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd >+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd >@@ -107,16 +107,17 @@ > <!ENTITY includeUpdate.label "Include this update"> > <!ENTITY includeUpdate.accesskey "n"> > <!ENTITY includeUpdate.tooltip "Include this Add-on when installing the updates"> > > <!-- Status Messsages --> > <!ENTITY insecureUpdate.label "Does not provide secure updates."> > <!ENTITY needsDependencies.label "Requires additional items."> > <!ENTITY blocklisted.label "Disabled for your protection."> >+<!ENTITY blocklistWarning.label "This add-on may expose you to security risks."> "expose you" seems wrong though it may be appropriate since using you may indicate to the user the importance of this. "This add-on" seems somewhat superfluous in that all other messages within the add-on don't use "This add-on" since it is obvious in order to save space. I'm a tad concerned about how elsewhere stability and security problems is called out and that here it only states "security risks" since if it is important to call out both elsewhere then it should be just as important to call out both here. I'm not happy with it but it is a start: Known to cause security or stability problems. Can you provide a screenshots of the case where the add-on is disabled and selected / unselected to the blocklisted.label is also shown?

Comment on attachment 345120 [details] [diff] [review] patch rev 3 >diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties >--- a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties >+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties >@@ -54,21 +54,20 @@ incompatibleExtension=Disabled - not com > incompatibleExtension=Disabled - not compatible with %S %S > incompatibleAddonMsg=Not compatible with %S %S > insecureUpdateMessage="%S" will not be installed because it does not provide secure updates > > invalidGUIDMessage="%S" could not be installed because of an error in its Install Manifest ("%S" is not a valid GUID). Please contact the author of this item about the problem. > invalidVersionMessage="%S" could not be installed because of an error in its Install Manifest ("%S" is not a valid Version String). Please contact the author of this item about the problem. > incompatiblePlatformMessage="%S" could not be installed because it is not compatible with your %S build type (%S). Please contact the author of this item about the problem. > >-blocklistedInstallTitle=This extension is not secure >-blocklistedInstallMsg=The extension %S is known to be dangerous, and can't be installed. >-blocklistNotifyTitle2=Add-ons may be causing problems >-blocklistNotifyMsg2=%S has determined that the following add-ons may be unstable or insecure. >-blocklistRestartMsg2=You should restart %S so that these add-ons can be disabled. >+blocklistedInstallTitle2=This add-on is not secure Since these are being changed I suggest consistency between the two titles though it may be a good thing to use something more descriptive than dangerous in these strings This add-on is dangerous to use >+blocklistedInstallMsg2=The add-on %S causes stability or security problems and can't be installed. >+warningInstallTitle=This add-on may be dangerous to use >+warningInstallMsg=The add-on %S is known to cause stability or security problems and should not be installed. Do you wish to continue with the installation?

Currently it still displays the warning message even when disabled. Perhaps a better option there might be to hide the warning message but display the warning dialog when they try to enable the same as when they install? Or maybe only the warning on selection when disabled.

Comment on attachment 345120 [details] [diff] [review] patch rev 3 >diff --git a/toolkit/themes/winstripe/mozapps/extensions/blocklist.css b/toolkit/themes/winstripe/mozapps/extensions/blocklist.css >new file mode 100644 >--- /dev/null >+++ b/toolkit/themes/winstripe/mozapps/extensions/blocklist.css >@@ -0,0 +1,16 @@ >+richlistitem { >+ padding-top: 6px; >+ padding-bottom: 6px; >+ -moz-padding-start: 7px; >+ -moz-padding-end: 7px; >+ border-bottom: 1px solid #C0C0C0; >+} >+ >+.addon-name-version { >+ font-size: 110%; >+} nit: is this really better than going with the default size? I'll leave it up to you to decide. >+.blockedLabel { >+ color: red; >+ font-style: italic; >+} Hardcoding the color will cause problems with custom themes (in this case if my list background color is red then the text won't be displayed Also, this appears in blocklist.css and the class blockedLabel is only used in extensions.xml which does not include blocklist.css. >diff --git a/toolkit/themes/winstripe/mozapps/extensions/extensions.css b/toolkit/themes/winstripe/mozapps/extensions/extensions.css >--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css >+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css >@@ -152,16 +152,17 @@ richlistitem[selected="true"]:not([opTyp > display: none; > } > > richlistitem[availableUpdateURL][updateable="true"] .updateBadge, > richlistitem[availableUpdateURL][updateable="true"] .updateAvailableBox, > richlistitem[compatible="false"] .notifyBadge, > richlistitem[providesUpdatesSecurely="false"] .notifyBadge, > richlistitem[blocklisted="true"] .notifyBadge, >+richlistitem[warning="true"] .notifyBadge, > richlistitem[satisfiesDependencies="false"] .notifyBadge { > display: -moz-box; > } > > /* Selected Add-on buttons > See content/extensions.css to hide / display buttons */ > .selectedButtons { > margin-top: 4px; >@@ -184,17 +185,20 @@ richlistitem[satisfiesDependencies="fals > .uninstallButton, .cancelUninstallButton { > -moz-margin-start: 5px; > } > > /* Selected Add-on status messages and images */ > richlistitem[compatible="true"] .incompatibleBox, > richlistitem[providesUpdatesSecurely="true"] .insecureUpdateBox, > richlistitem[satisfiesDependencies="true"] .needsDependenciesBox, >-richlistitem[blocklisted="false"] .blocklistedBox, >+richlistitem:not([blocklisted="true"]):not([warning="true"]) .blocklistedBox, >+richlistitem[warning="false"]:not([selected="true"]) .blocklistedBox, >+richlistitem[blocklisted="false"] .blocklistedLabel, >+richlistitem[warning="false"] .warningLabel, What do you think about going with blocklistedhard and blocklistedsoft instead of blocklisted and warning? My concern is that warning is too generic and can be applied to other things. > richlistitem[opType="needs-uninstall"] .blocklistedBox, > richlistitem[opType="needs-uninstall"] .incompatibleBox, > richlistitem[opType="needs-uninstall"] .needsDependenciesBox, > richlistitem[opType="needs-uninstall"] .blocklistedBox { > display: none; > } > > richlistitem[loading="true"] .updateBadge { >diff --git a/toolkit/mozapps/extensions/public/nsIBlocklistService.idl b/xpcom/system/nsIBlocklistService.idl >rename from toolkit/mozapps/extensions/public/nsIBlocklistService.idl >rename to xpcom/system/nsIBlocklistService.idl >--- a/toolkit/mozapps/extensions/public/nsIBlocklistService.idl >+++ b/xpcom/system/nsIBlocklistService.idl >@@ -34,19 +34,29 @@ > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > > #include "nsISupports.idl" > >-[scriptable, uuid(0c3fe697-d50d-4f42-b747-0c5855cfc60e)] >+interface nsIPluginTag; >+ >+[scriptable, uuid(8439f9c0-da03-4260-8b21-dc635eed28fb)] > interface nsIBlocklistService : nsISupports > { >+ // Indicates that the item does not appear in the blocklist. >+ const unsigned long STATE_UNBLOCKED = 0; Should be NOT_BLOCKED or something similar since UNBLOCKED implies that it was unblocked when it may have never been blocked. >+ // Indicates that the item is in the blocklist but the problem is not severe >+ // enough to warant forcibly blocking. >+ const unsigned long STATE_WARN = 1; >+ // Indicates that the item should be blocked and never used. >+ const unsigned long STATE_BLOCKED = 2; >+ > /** > * Determine if an item is blocklisted > * @param id > * The GUID of the item. > * @param version > * The item's version. > * @param appVersion > * The version of the application we are checking in the blocklist. >@@ -55,10 +65,49 @@ interface nsIBlocklistService : nsISuppo > * @param toolkitVersion > * The version of the toolkit we are checking in the blocklist. > * If this parameter is null, the version of the running toolkit > * is used. > * @returns true if the item is compatible with this version of the > * application or this version of the toolkit, false, otherwise. > */ > boolean isAddonBlocklisted(in AString id, in AString version, >- in AString appVersion, in AString toolkitVersion); >+ [optional] in AString appVersion, >+ [optional] in AString toolkitVersion); >+ >+ /** >+ * Determine the blocklist state of an add-on >+ * @param id >+ * The GUID of the item. nit: Since we changed to our ID format these are technically no longer GUIDs... just change it to ID >+ * @param version >+ * The item's version. >+ * @param appVersion >+ * The version of the application we are checking in the blocklist. >+ * If this parameter is null, the version of the running application >+ * is used. >+ * @param toolkitVersion >+ * The version of the toolkit we are checking in the blocklist. >+ * If this parameter is null, the version of the running toolkit >+ * is used. >+ * @returns The STATE constant.. nit: extra trailing period >+ */ >+ unsigned long getAddonBlocklistState(in AString id, in AString version, >+ [optional] in AString appVersion, >+ [optional] in AString toolkitVersion); >+ >+ /** >+ * Determine the blocklist state of a plugin >+ * @param plugin >+ * The plugin to get the state for >+ * @param appVersion >+ * The version of the application we are checking in the blocklist. >+ * If this parameter is null, the version of the running application >+ * is used. >+ * @param toolkitVersion >+ * The version of the toolkit we are checking in the blocklist. >+ * If this parameter is null, the version of the running toolkit >+ * is used. >+ * @returns The STATE constant.. nit: extra trailing period

(In reply to comment #35) > (From update of attachment 345120 [details] [diff] [review]) > >diff --git a/toolkit/themes/winstripe/mozapps/extensions/blocklist.css b/toolkit/themes/winstripe/mozapps/extensions/blocklist.css > >new file mode 100644 > >--- /dev/null > >+++ b/toolkit/themes/winstripe/mozapps/extensions/blocklist.css > >@@ -0,0 +1,16 @@ >... > >+.blockedLabel { > >+ color: red; > >+ font-style: italic; > >+} > Hardcoding the color will cause problems with custom themes (in this case if my > list background color is red then the text won't be displayed > Also, this appears in blocklist.css and the class blockedLabel is only used in > extensions.xml which does not include blocklist.css. I see now that it is getting the blocklist.css from blocklist.xul which though somewhat confusing is ok. The comment regarding color still stands

do we really want to have an IDL that knows about plugins, app names, toolkit names in xpcom/system? wouldn't /toolkit be more appropriate?

(In reply to comment #37) > do we really want to have an IDL that knows about plugins, app names, toolkit > names in xpcom/system? wouldn't /toolkit be more appropriate? The plugin host cannot use it if it is in toolkit. As I've stated I'm open to better suggestions for where to put it but it must be in the main build tier.

Comment on attachment 345120 [details] [diff] [review] patch rev 3 +<!ENTITY blocklist.accept.accesskey "r"> Shouldn't this be R? >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js >--- a/browser/app/profile/firefox.js >+++ b/browser/app/profile/firefox.js >@@ -75,16 +75,17 @@ pref("extensions.getAddons.recommended.b > pref("extensions.getAddons.recommended.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/recommended"); > pref("extensions.getAddons.recommended.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/list/featured/all/10/%OS%/%VERSION%"); > pref("extensions.getAddons.search.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/search?q=%TERMS%"); > pref("extensions.getAddons.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/search/%TERMS%/all/10/%OS%/%VERSION%"); > > // Blocklist preferences > pref("extensions.blocklist.enabled", true); > pref("extensions.blocklist.interval", 86400); >+pref("extensions.blocklist.severity", 2); A note describing what this pref does would be a good thing. Also, severity doesn't describe the purpose very well and elsewhere the term level is used. >diff --git a/toolkit/mozapps/extensions/content/blocklist.js b/toolkit/mozapps/extensions/content/blocklist.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/mozapps/extensions/content/blocklist.js >@@ -0,0 +1,81 @@ >... nit: warnAddon seems a bit generic as noted elsewhere. >diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js >--- a/toolkit/mozapps/extensions/content/extensions.js >+++ b/toolkit/mozapps/extensions/content/extensions.js >@@ -293,16 +293,17 @@ function showView(aView) { > // Using disabled to represent add-on state in regards to the EM causes evil > // focus behavior when used as an element attribute when the element isn't > // really disabled. > var bindingList = [ [ ["aboutURL", "?aboutURL"], > ["addonID", "?addonID"], > ["availableUpdateURL", "?availableUpdateURL"], > ["availableUpdateVersion", "?availableUpdateVersion"], > ["blocklisted", "?blocklisted"], >+ ["warning", "?warning"], nit: warning seems a bit generic as noted elsewhere... not going to note any additional parts where warning is used. >diff --git a/toolkit/mozapps/extensions/public/nsIExtensionManager.idl b/toolkit/mozapps/extensions/public/nsIExtensionManager.idl >--- a/toolkit/mozapps/extensions/public/nsIExtensionManager.idl >+++ b/toolkit/mozapps/extensions/public/nsIExtensionManager.idl >... >@@ -467,23 +467,23 @@ interface nsIExtensionManager : nsISuppo > void getDependentItemListForID(in AString id, > in boolean includeDisabled, > out unsigned long itemCount, > [retval, array, size_is(itemCount)] out nsIUpdateItem items); > > /** > * Checks for changes to the blocklist using the local blocklist file, > * application disables / enables items that have been added / removed from >- * the blocklist, and if there are additions to the blocklist this will >- * inform the user by displaying a list of the items added. >+ * the blocklist. > * >- * XXXrstrong - this method is not terribly useful and was added so we can >- * trigger this check from the additional timer used by blocklisting. >+ * @returns An array of nsIUpdateItems that are blocklisted or the user should >+ * be warned about but are currently enabled. > */ >- void checkForBlocklistChanges(); >+ void getNewlyBlocklistedItems(out unsigned long itemCount, >+ [retval, array, size_is(itemCount)] out nsIUpdateItem items); Would it be worthwhile to change this so it has an includeDisabled param? If so, how about naming it getBlocklistedItems and updating the description accordingly? If not, I think getNewBlocklistedItems is a better name though I'm fine with it as is. >diff --git a/toolkit/mozapps/extensions/src/nsBlocklistService.js b/toolkit/mozapps/extensions/src/nsBlocklistService.js >--- a/toolkit/mozapps/extensions/src/nsBlocklistService.js >+++ b/toolkit/mozapps/extensions/src/nsBlocklistService.js >@@ -48,25 +48,34 @@ Components.utils.import("resource://gre/ >... >+const URI_BLOCKLIST_DIALOG = "chrome://mozapps/content/extensions/blocklist.xul" >+const DEFAULT_SEVERITY = 3; >+const DEFAULT_BLOCK_LEVEL = 2; >+const MAX_SEVERITY = 3; The mixing of the terms severity and level makes this somewhat confusing >+const STATE_UNBLOCKED = Ci.nsIBlocklistService.STATE_UNBLOCKED; >+const STATE_WARN = Ci.nsIBlocklistService.STATE_WARN; >+const STATE_BLOCKED = Ci.nsIBlocklistService.STATE_BLOCKED; nit: I'd prefer not defining these and just going with Ci.nsIBlocklistService.STATE_etc where needed for consistency if nothing else. >+ /** >+ * Private version of getAddonBlocklistState that allows the caller to pass in >+ * the blocklist entries. the blocklist entries to compare against. >+ * >+ * @param id >+ * The id of the xpi item to get the blocklist state for. Are you really getting an "xpi item" here and below? >+ * @param version >+ * The version of the xpi item to get the blocklist state for. >+ * @param addonEntries >+ * The blocklist entries to compare against. The addon blocklist entries to compare against. >+ * @param appVersion >+ * The application version to compare to, will use the current >+ * version if null. >+ * @param toolkitVersion >+ * The toolkit version to compare to, will use the current version if >+ * null. >+ * @param The blocklist state for the item, one of the STATE constants @returns >+ * defined in nsIBlocklistService. nit: as defined in nsIBlocklistService.idl. >+ /** >+ * Private version of getPluginBlocklistState that allows the caller to pass in >+ * the blocklist entries. >+ * >+ * @param plugin >+ * The nsIPluginTag to get the blocklist state for. >+ * @param pluginEntries >+ * The blocklist entries to compare against. >+ * @param appVersion >+ * The application version to compare to, will use the current >+ * version if null. >+ * @param toolkitVersion >+ * The toolkit version to compare to, will use the current version if >+ * null. >+ * @param The blocklist state for the item, one of the STATE constants @returns >+ * defined in nsIBlocklistService. nit: as defined in nsIBlocklistService.idl. > >- _checkPluginsList: function() { >... >+ var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"]. >+ getService(Ci.nsIWindowWatcher); >+ ww.openWindow(null, URI_BLOCKLIST_DIALOG, "", >+ "chrome,centerscreen,dialog,modal,titlebar", args); >+ >+ for (let i = 0; i < addonList.length; i++) { >+ if (!addonList[i].disable) >+ continue; >+ >+ if (addonList[i].item instanceof Ci.nsIUpdateItem) >+ em.disableItem(addonList[i].item.id); >+ else if (addonList[i].item instanceof Ci.nsIPluginTag) >+ addonList[i].item.disabled = true; Not sure if it is worth it but perhaps add a log statement if either of these two fail?

(In reply to comment #39) >... > >+ * @param version > >+ * The version of the xpi item to get the blocklist state for. > >+ * @param addonEntries > >+ * The blocklist entries to compare against. > The addon blocklist entries to compare against. Note: if these are just addonEntries then the new comment should be used. If they are blocklist entries perhaps change the param name to blocklistEntries here and for pluginEntries?

Comment on attachment 345120 [details] [diff] [review] patch rev 3 Suggestion: perhaps just use the theme's extensions.css instead of adding blocklist.css since you are already loading extensions.css via extensions.xml? >diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in >--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in >+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in >@@ -147,16 +147,17 @@ const INSTALLERROR_INVALID_GUID > const INSTALLERROR_INVALID_GUID = -2; > const INSTALLERROR_INCOMPATIBLE_VERSION = -3; > const INSTALLERROR_PHONED_HOME = -4; > const INSTALLERROR_INCOMPATIBLE_PLATFORM = -5; > const INSTALLERROR_BLOCKLISTED = -6; > const INSTALLERROR_INSECURE_UPDATE = -7; > const INSTALLERROR_INVALID_MANIFEST = -8; > const INSTALLERROR_RESTRICTED = -9; >+const INSTALLERROR_SOFTBLOCKED = -10; You use SOFTBLOCKED here and warning elsewhere. Can this be made consistent with whatever is chosen for warning? >@@ -764,43 +765,40 @@ function showMessage(titleKey, titlePara > else > message = extensionStrings.GetStringFromName(messageKey); > var ps = Cc["@mozilla.org/embedcomp/prompt-service;1"]. > getService(Ci.nsIPromptService); > ps.alert(null, title, message); > } > > /** >- * Shows a dialog for blocklisted items. >- * @param items >- * An array of nsIUpdateItems. >- * @param fromInstall >- * Whether this is called from an install or from the blocklist >- * background check. >- */ >-function showBlocklistMessage(items, fromInstall) { >- var win = null; >+ * Shows a dialog for blocklisted items. For warning items returns true if the Shows a dialog for a blocklisted item during installation. >+ * item should still be installed >+ * @param item >+ * The nsIUpdateItem to warn about The nsIUpdateItem to that is blocklisted. >+ getNewlyBlocklistedItems: function EM_getNewlyBlocklistedItems(itemCount) { Another thing that concerns me is that typically getETC methods don't perform additional actions such as enable / disable items as this does.

(In reply to comment #41) >... > >+ * item should still be installed > >+ * @param item > >+ * The nsIUpdateItem to warn about > The nsIUpdateItem to that is blocklisted. The nsIUpdateItem that is blocklisted.

Had a long discussion with Boriss about the strings and hopefully we've reached a consensus though there are a few changes. Going to answer all the comments here then attach the updated patch shortly. (In reply to comment #35) > (From update of attachment 345120 [details] [diff] [review]) > >+.addon-name-version { > >+ font-size: 110%; > >+} > nit: is this really better than going with the default size? I'll leave it up > to you to decide. I was going for matching the add-ons manager UI. On OSX we increase the font size. Elsewhere we bold the name. I've updated to match. > >+.blockedLabel { > >+ color: red; > >+ font-style: italic; > >+} > Hardcoding the color will cause problems with custom themes (in this case if my > list background color is red then the text won't be displayed Yeah this is annoying, gone with bold italics which stands out ok though not great. > > /* Selected Add-on status messages and images */ > > richlistitem[compatible="true"] .incompatibleBox, > > richlistitem[providesUpdatesSecurely="true"] .insecureUpdateBox, > > richlistitem[satisfiesDependencies="true"] .needsDependenciesBox, > >-richlistitem[blocklisted="false"] .blocklistedBox, > >+richlistitem:not([blocklisted="true"]):not([warning="true"]) .blocklistedBox, > >+richlistitem[warning="false"]:not([selected="true"]) .blocklistedBox, > >+richlistitem[blocklisted="false"] .blocklistedLabel, > >+richlistitem[warning="false"] .warningLabel, > What do you think about going with blocklistedhard and blocklistedsoft instead > of blocklisted and warning? My concern is that warning is too generic and can > be applied to other things. Gone with blocklisted and blocklistedsoft as per IRC. > >-[scriptable, uuid(0c3fe697-d50d-4f42-b747-0c5855cfc60e)] > >+interface nsIPluginTag; > >+ > >+[scriptable, uuid(8439f9c0-da03-4260-8b21-dc635eed28fb)] > > interface nsIBlocklistService : nsISupports > > { > >+ // Indicates that the item does not appear in the blocklist. > >+ const unsigned long STATE_UNBLOCKED = 0; > Should be NOT_BLOCKED or something similar since UNBLOCKED implies that it was > unblocked when it may have never been blocked. Works better yes (In reply to comment #39) "?availableUpdateVersion"], > > ["blocklisted", "?blocklisted"], > >+ ["warning", "?warning"], > nit: warning seems a bit generic as noted elsewhere... not going to note any > additional parts where warning is used. I've updated everywhere to use the soft term as appropriate. > >- void checkForBlocklistChanges(); > >+ void getNewlyBlocklistedItems(out unsigned long itemCount, > >+ [retval, array, size_is(itemCount)] out nsIUpdateItem items); > Would it be worthwhile to change this so it has an includeDisabled param? If > so, how about naming it getBlocklistedItems and updating the description > accordingly? > If not, I think getNewBlocklistedItems is a better name though I'm fine with it > as is. I'm not sure it is worth it. Really I'd prefer to get rid of this method, it only exists because the blocklist service can't get what it needs without learning RDF which I don't want to do. I've renamed it and added a note to the comment that people shouldn't be using it > >+ * > >+ * @param id > >+ * The id of the xpi item to get the blocklist state for. > Are you really getting an "xpi item" here and below? Yeah I was trying to differentiate between EM items and plugins, probably not all that worth it. > >+ if (addonList[i].item instanceof Ci.nsIUpdateItem) > >+ em.disableItem(addonList[i].item.id); > >+ else if (addonList[i].item instanceof Ci.nsIPluginTag) > >+ addonList[i].item.disabled = true; > Not sure if it is worth it but perhaps add a log statement if either of these > two fail? Hopefully we should never hit it, only if someone were to do something weird with the window I think, but there is no harm in adding it. (In reply to comment #41) > (From update of attachment 345120 [details] [diff] [review]) > Suggestion: perhaps just use the theme's extensions.css instead of adding > blocklist.css since you are already loading extensions.css via extensions.xml? I'll have to see what that does. I'm a bit wary that we might start making changes to extensions.css not realising they impact the blocklist dialog too. As far as I understand it the extension.css inclusion in extensions.xml would only affect the bindings. I might be wrong there though. > >+ getNewlyBlocklistedItems: function EM_getNewlyBlocklistedItems(itemCount) { > Another thing that concerns me is that typically getETC methods don't perform > additional actions such as enable / disable items as this does. Yeah that isn't ideal. Not many non-get methods return things though. Maybe |updateAndReturnBlocklistedItems| though it's a bit long?

Sounds good. (In reply to comment #43) >... > > >+ getNewlyBlocklistedItems: function EM_getNewlyBlocklistedItems(itemCount) { > > Another thing that concerns me is that typically getETC methods don't perform > > additional actions such as enable / disable items as this does. > > Yeah that isn't ideal. Not many non-get methods return things though. Maybe > |updateAndReturnBlocklistedItems| though it's a bit long? I'm fine with updateAndGetNewBlocklistedItems or just getNewBlocklistItems with a prominent note in the idl that states specifically what happens to the new blocklisted items when calling this method.

Updating screenshots

Comment on attachment 345403 [details] Blocklist updated, new items have been hard blocked This says "remove them completely" but you just disable them... true? Whatever the case I'm only going to make comments on strings if they appear incorrect or are awkwardly worded but I won't minus based on the strings. Also, if a new patch is submitted I'll do the review tonight.

Comment on attachment 345403 [details] Blocklist updated, new items have been hard blocked Ignore the ".:" in the text at the top of this, I've corrected it already.

(In reply to comment #51) > (From update of attachment 345403 [details]) > This says "remove them completely" but you just disable them... true? Yeah that isn't very good, replaces "remove" with "disable".

Comment on attachment 345415 [details] [diff] [review] patch rev 4 It looks good Dave.

Pushed: http://hg.mozilla.org/mozilla-central/rev/bbccbf2c7456

Backed out due to test failures

Re-landed: http://hg.mozilla.org/mozilla-central/rev/8291b82dd323 Had to remove my use of Cc and Ci from extensions.js which are only defined on OSX due to the mac browser overlay weirdness.

Some notes from a localizer's perspective. softBlocklisted.label is the only string with the form "security or stability", all the other strings have "stability or security". Maybe it could make sense to have uniformity across all strings.

Dave, do you have any tests that you can attach here that i can verify by hand? Thanks

(In reply to comment #59) > Dave, do you have any tests that you can attach here that i can verify by hand? > Thanks Sorry forgot to add this. This is the basic test I was developing with. It includes extensions: DOM Inspector at severity 3 Storage Explorer at severity 3 Nightly Tester Tools at severity 2 Fire Gestures at severity 1 Also plugins: Anything with "DivX" in the name at severity 3 Anything with "Silverlight" in the name at severity 3 Anything with "QuickTime" in the name at severity 2 Anything with "Shockwave" in the name at severity 1 By default anything at a severity lower than 2 will only be warned about, equal to or above will be blocked, but this is controlled with the extensions.blocklist.level preference. You can just set extensions.blocklist.url to the url of this attachment and update the blocklist in the normal way. When it is updated, you should be able to adjust extensions.blocklist.level and extensions.blocklist.enabled and it should automatically update the state of items. One issue I am aware of is that if the add-ons manager is opened when the blocklist updates it often won't display the restart message as necessary, reopening the add-ons manager would solve that. This is an old issue though.

I should add that if needs be I can put together a blocklist the blocks any particular items you like for testing purposes, just let me know what you need.

(In reply to comment #61) > I should add that if needs be I can put together a blocklist the blocks any > particular items you like for testing purposes, just let me know what you need. Thanks Dave. question: does blocklisting severity levels affect the versions of the plugins? or as long as its a quicktime plugin severity 2 (using your example), all versions of quicktime will be affected?

(In reply to comment #62) > (In reply to comment #61) > > I should add that if needs be I can put together a blocklist the blocks any > > particular items you like for testing purposes, just let me know what you need. > > Thanks Dave. question: does blocklisting severity levels affect the versions > of the plugins? or as long as its a quicktime plugin severity 2 (using your > example), all versions of quicktime will be affected? The simple test blocklist I have there will block any of those add-ons regardless of the version. We can block different versions of a single add-on at different severities though if we choose.

I'm Unsure how to trigger the hard block dialog from comment #50. I have set the blocklist pointed to the testcase in comment #60, and restarted. i then installed Fire Gestures (sev level 1) and Dom Inspector (sev level 3) from addons manager. 1) Fire Gestures gave me the soft block installation error as expected. Check. 2) But Dom Inspector gave me the "incompatible extension" with Fx3.1b1 error. Is that right? Shouldn't I be expecting to see the Hard Block Error dialog instead? (FWIW, if the blocklist is not enabled, Dom Inspector installs just fine without an error) Screenshot of error attached.

Sorry, wrong attachment in prior comment. This is the right screenshot.

Comment on attachment 346965 [details] Incompatible Extension Screenshot invalid

(In reply to comment #64) > Created an attachment (id=346965) [details] > Incompatible Extension Screenshot > > I'm Unsure how to trigger the hard block dialog from comment #50. > > I have set the blocklist pointed to the testcase in comment #60, and restarted. > i then installed Fire Gestures (sev level 1) and Dom Inspector (sev level 3) > from addons manager. > > 1) Fire Gestures gave me the soft block installation error as expected. > Check. > 2) But Dom Inspector gave me the "incompatible extension" with Fx3.1b1 error. > Is that right? Shouldn't I be expecting to see the Hard Block Error dialog > instead? > > (FWIW, if the blocklist is not enabled, Dom Inspector installs just fine > without an error) Screenshot of error attached. Filed this as bug 463819.

Marking this bug verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081110 Minefield/3.1b2pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081110 Minefield/3.1b2pre

copy and paste fail. I meant Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081110 Minefield/3.1b2pre.

tests covered in-testsuite.