271166 - Should be able to blacklist extensions centrally, in update.mozilla.org

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Brendan Eich [:brendan]]

Should we ship one that has a bad security hole, or even if one we don't host
becomes widespread and then compromised.  This is like the Windows registry's
ActiveX blacklist, but web-wide, not per-machine (might want per-machine too for
a layered administrative approach).

/be

Comment 1

[Prognathous]

Bug 250854 is related ("XPInstall Permission Manager: Blacklist feature").

Prog.

Comment 2

[Asa Dotzler [:asa]]

This seems like a really good one to try to get for 1.1. Any chance it's gonna
happen?

Comment 3

[Mike Connor [:mconnor]]

Benjamin, is this realistic at this point with everything else that's happening?

Comment 4

[Brendan Eich [:brendan]]

Prioritize what else is happening, and then let's schedule -- we don't have a
realistic schedule right now, so I would much rather we all sort out what must,
should, and would be nice to have for 1.1.  This is somewhere between should and
must in my book, so don't starve it for unknown "everthing elses".

/be

Comment 5

[Benjamin Smedberg]

This needs coordination with the extension update service, but after that gets
sorted out this is pretty simple to implement.

Comment 6

[alanjstr]

That would be bug 290759.  Tell us the format you want.  I guess it will be
something similar to 
http://lxr.mozilla.org/update1.0/source/update/VersionCheck.php#200

Comment 7

[Chris Beard]

shaver, you were going to take a look at this?  any reasonable chance of this
getting in to 1.5? e.g. it will need to land before we lockdown for 1.8b4 within
the next week or so.

/cb

Comment 8

[Mike Shaver (:shaver emeritus)]

I was going to look at 290759, which is the client side of this work.

Comment 9

[Mike Shaver (:shaver emeritus)]

(I could have sworn I made this comment the other day...)

This is indeed the client-side work.  I'm waiting for Ben's EM patch in 296566
to land, and then I'll be able to better scope and schedule the work.  I have
hopes that it will land today!

Comment 10

[Benjamin Smedberg]

-> robstrong per phone discussion

Comment 11

[Robert Strong (they/them - no direct email)]

Per the phone conversation I stated I would take on the EM portion of this bug
and not the other aspects (e.g. plugins, UM0 which I believe may be another bug,
service for retrieving or reading the blacklist, etc.). Shaver also promised a
sample file to work from that would be attached to this bug.

Comment 12

[Robert Strong (they/them - no direct email)]

Per the phone conversation I will take on the EM portion of this bug - I don't
have the time available to take on or drive the other aspects like disabling
plugins, server side work for the blacklist, etc. Time is running short and
without the base functionality of having the blacklist data to work from I may
not have the time available to finish the EM portion. A simplified description
of the EM portion includes:
ui for user notification that an item has been blacklisted
disabling of blacklisted extensions

What it does not include:
ui for and the disabling of plugins
server side work for the blacklist
periodically retrieving the blacklist or whatever is decided upon.
format of the blacklist

-> shaver

Comment 13

[Chris Beard]

Per drivers discussion, this is not going to make 1.5, but we will keep it on
the burner as something we'll implement in a dot release if need be to handle
any security or integrity issues introduced by Extensions or plugins.

Comment 14

[alanjstr]

http://wiki.mozilla.org/Firefox:1.1_Extension_Manager_Blacklist

Comment 15

[Mike Connor [:mconnor]]

Fx2 if not earlier.  (see comment 13)

Comment 16

[Robert Strong (they/them - no direct email)]

Since we are adding restart capability when blacklisted items are found during the background check it would be a good thing to have session restore capability... adding deps on bug 328154

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: patch

Benjamin, this adds some of what will be needed to support toolkit version knowledge in the EM... I can remove it if you think it is premature.

Darin, this affects app update as well and I'd appreciate your review on the code in general as well.

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: patch (fixed typo)

Had a typo in the last patch.

Also, the strings were provided by beltzner. If there are string changes I'd prefer to take care of them in bug 308916 instead of this bug though comments would be welcome.

Comment 19

[Benjamin Smedberg]

Comment on attachment 213134 [details] [diff] [review]
patch (fixed typo)

>   removeFile: function(file) {
>+    if (!file)
>+      return;

Why is this necessary? I understand being defensive, but I don't see how its good to end up with null here.

>+  /**
>+   * The blocklist XML file looks something like this:
>+   *
>+   * <blocklist xmlns="http://www.mozilla.org/2006/blocklist">
>+   *   <emItems>

What is the "emItems" for? Do you expect to extend this format in the future to return other information? Do we handle other items gracefully?

>+  _loadBlocklistFromFile: function(file) {

>+      var parser = Components.classes["@mozilla.org/xmlextras/domparser;1"]
>+                            .createInstance(Components.interfaces.nsIDOMParser);

nit: whitespace lineup

>+      var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml");
>+      
>+      for (var i = 0; i < doc.documentElement.childNodes.length; ++i) {
>+        var emItemsElement = doc.documentElement.childNodes.item(i);
>+        if (emItemsElement.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE ||
>+            emItemsElement.localName == "emItems")
>+          break;
>+      }

Why don't we test the namespace also (? What happens here if we don't find an <emItems> element? Seems to me we need some additional defense against malformed input. Why not put this code below inside the loop, with a helper func if you think that would make it more readable.

I'm vaguely worried about the fact that the EM service is popping up UI for blocked installs in various places; in an ideal world that would be handled by callbacks/observers from the client code, but I understand that's out of scope for 1.8.

>+   * @param   toolkitVersion
>+   *          The version of the toolkit we are checking for compatibility
>+   *          against. If this parameter is undefined, the version of the running
>+   *          application's toolkit is used.

>-  isCompatible: function (datasource, source, version) {
>+  isCompatible: function (datasource, source, appVersion, toolkitVersion) {

What are the situations where somebody would want/need to pass an explicit toolkitVersion instead of using gApp.platformVersion? Is this here to deal with what extensions would become incompatible in update scenarios? I don't see any usage of toolkitVersion later in the function, is that going to be added later as part of the other bug?

>+  _rdfGet_detailsURL: function(item, property) {
>+    var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
>+    if (this.getItemProperty(id, "blocklisted") == "false")
>+      return EM_L("none");

nit: "none"? seems to me it would be better to return NS_RDF_NO_VALUE or an empty string.

>Index: browser/app/profile/firefox.js

>+// Blocklist preferences
>+pref("extensions.blocklist.enabled", true);
>+pref("extensions.blocklist.interval", 86400);
>+pref("extensions.blocklist.url", "https://addons.mozilla.org/blocklist/firefox.xml");
>+pref("extensions.blocklist.detailsURL", "http://www.mozilla.com/blocklist/");

If these prefs are missing does the EM recover gracefully?

>+  var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                      .getService(Components.interfaces.nsIStringBundleService);
>+  var strings = sbs.createBundle(URI_BLOCKLIST_PROPERTIES);

Is there any particular reason you're using sbs directly instead of using <stringbundle> in the XUL? I'm not up on the decision to use one or the other, I just thought that the XUL version was preferred for some reason (it would block onload, perhaps, until it was loaded?).

>Index: toolkit/mozapps/extensions/content/blocklist.css
>===================================================================
>RCS file: toolkit/mozapps/extensions/content/blocklist.css
>diff -N toolkit/mozapps/extensions/content/blocklist.css
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/mozapps/extensions/content/blocklist.css	25 Feb 2006 00:42:55 -0000
>@@ -0,0 +1,5 @@
>+blocklistItem {
>+  -moz-binding: url("chrome://mozapps/content/extensions/blocklist.xml#blocklistItem");
>+  -moz-box-orient: vertical;
>+}

nit: in general I'd prefer not to invent new XUL element names. How about <richlistitem class="blocklistItem">?

Comment 20

[Robert Strong (they/them - no direct email)]

(In reply to comment #19)
> (From update of attachment 213134 [details] [diff] [review] [edit])
> >   removeFile: function(file) {
> >+    if (!file)
> >+      return;
> Why is this necessary? I understand being defensive, but I don't see how its
> good to end up with null here.
That snuck in from other tests I'm doing for EM failures... I'll remove it.

> >+  /**
> >+   * The blocklist XML file looks something like this:
> >+   *
> >+   * <blocklist xmlns="http://www.mozilla.org/2006/blocklist">
> >+   *   <emItems>
> 
> What is the "emItems" for? Do you expect to extend this format in the future to
> return other information? Do we handle other items gracefully?
When we add plugins this same file can be used (e.g. I dislike the idea of downloading a second file for plugins and potentially other items in the future).
Yes... with plugins being the only one planned.
Yes, other items are handled gracefully.

> >+  _loadBlocklistFromFile: function(file) {
> 
> >+      var parser = Components.classes["@mozilla.org/xmlextras/domparser;1"]
> >+                            .createInstance(Components.interfaces.nsIDOMParser);
> 
> nit: whitespace lineup
Will do.

> >+      var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml");
> >+      
> >+      for (var i = 0; i < doc.documentElement.childNodes.length; ++i) {
> >+        var emItemsElement = doc.documentElement.childNodes.item(i);
> >+        if (emItemsElement.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE ||
> >+            emItemsElement.localName == "emItems")
> >+          break;
> >+      }
> 
> Why don't we test the namespace also (? What happens here if we don't find an
> <emItems> element? Seems to me we need some additional defense against
> malformed input. Why not put this code below inside the loop, with a helper
> func if you think that would make it more readable.
I'll review this again this evening to make sure it does the right thing.

> I'm vaguely worried about the fact that the EM service is popping up UI for
> blocked installs in various places; in an ideal world that would be handled by
> callbacks/observers from the client code, but I understand that's out of scope
> for 1.8.
It is a bit of a PITA :/
I could move the blocked install notification into extensions.js but that would also require canceling installs when the ui is closed instead of just completing the install. I believe that notification would be a bit tricky unless we just didn't care about the edgecases where the client code didn't have an observer (e.g. options panel, other chrome apps, etc.).

> >+   * @param   toolkitVersion
> >+   *          The version of the toolkit we are checking for compatibility
> >+   *          against. If this parameter is undefined, the version of the running
> >+   *          application's toolkit is used.
> 
> >-  isCompatible: function (datasource, source, version) {
> >+  isCompatible: function (datasource, source, appVersion, toolkitVersion) {
> 
> What are the situations where somebody would want/need to pass an explicit
> toolkitVersion instead of using gApp.platformVersion? Is this here to deal with
> what extensions would become incompatible in update scenarios? I don't see any
> usage of toolkitVersion later in the function, is that going to be added later
> as part of the other bug?
Yes on all counts... this is the "adds some of what will be needed to support toolkit version knowledge in the EM... I can remove it if you think it is premature." as noted in comment #17

> >+  _rdfGet_detailsURL: function(item, property) {
> >+    var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
> >+    if (this.getItemProperty(id, "blocklisted") == "false")
> >+      return EM_L("none");
> 
> nit: "none"? seems to me it would be better to return NS_RDF_NO_VALUE or an
> empty string.
I used "none" to be consistent with how this was accomplished in bug 296566 for update notification... I've been working on cleaning up the ui and I believe I have a better approach.

> >Index: browser/app/profile/firefox.js
> 
> >+// Blocklist preferences
> >+pref("extensions.blocklist.enabled", true);
> >+pref("extensions.blocklist.interval", 86400);
> >+pref("extensions.blocklist.url", "https://addons.mozilla.org/blocklist/firefox.xml");
> >+pref("extensions.blocklist.detailsURL", "http://www.mozilla.com/blocklist/");
> 
> If these prefs are missing does the EM recover gracefully?
I believe so but I'll double check before resubmitting.

> >+  var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
> >+                      .getService(Components.interfaces.nsIStringBundleService);
> >+  var strings = sbs.createBundle(URI_BLOCKLIST_PROPERTIES);
> 
> Is there any particular reason you're using sbs directly instead of using
> <stringbundle> in the XUL? I'm not up on the decision to use one or the other,
> I just thought that the XUL version was preferred for some reason (it would
> block onload, perhaps, until it was loaded?).
No and I'll fix this.

> >Index: toolkit/mozapps/extensions/content/blocklist.css
> >===================================================================
> >RCS file: toolkit/mozapps/extensions/content/blocklist.css
> >diff -N toolkit/mozapps/extensions/content/blocklist.css
> >--- /dev/null	1 Jan 1970 00:00:00 -0000
> >+++ toolkit/mozapps/extensions/content/blocklist.css	25 Feb 2006 00:42:55 -0000
> >@@ -0,0 +1,5 @@
> >+blocklistItem {
> >+  -moz-binding: url("chrome://mozapps/content/extensions/blocklist.xml#blocklistItem");
> >+  -moz-box-orient: vertical;
> >+}
> 
> nit: in general I'd prefer not to invent new XUL element names. How about
> <richlistitem class="blocklistItem">?
Not a problem

Comment 21

[Robert Strong (they/them - no direct email)]

Benjamin, sorry for the delay and I should be done with this over the weekend. I talked over a couple of changes that beltzner would like and I am looking at if / how I can utilize the existing list.xul / list.js instead of creating blocklist.xul, etc. - needing to show this from a service is a bit of a PITA but there is no other way to do so without creating cases where we wouldn't show the dialog.

Comment 22

[Robert Strong (they/them - no direct email)]

Attachment: screenshot of blocklist dialogs

Comment 23

[alanjstr]

Do we have something for "More Info" to redirect to?

Comment 24

[Robert Strong (they/them - no direct email)]

It is in the works and will be hosted on mozilla.com. The xml file will be hosted on AMO.

Comment 25

[Robert Strong (they/them - no direct email)]

Attachment: patch

Comment 26

[Robert Strong (they/them - no direct email)]

Attachment: patch

Comment 27

[Robert Strong (they/them - no direct email)]

Comment on attachment 214204 [details] [diff] [review]
patch

Benjamin, I am not going to add the changes to app update in this bug since there are other more drastic changes needed to it in Bug 324121. Otherwise, this should address your review comments.

(In reply to comment #19)
> What is the "emItems" for? Do you expect to extend this format in the future to
> return other information? Do we handle other items gracefully?
When we add plugin blocklisting the format will almost certainly be different hence the use of emItems which handle EM managed items.

> >+      var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml");
> >+      
> >+      for (var i = 0; i < doc.documentElement.childNodes.length; ++i) {
> >+        var emItemsElement = doc.documentElement.childNodes.item(i);
> >+        if (emItemsElement.nodeType == Components.interfaces.nsIDOMNode.ELEMENT_NODE ||
> >+            emItemsElement.localName == "emItems")
> >+          break;
> >+      }
> 
> Why don't we test the namespace also (? What happens here if we don't find an
> <emItems> element? Seems to me we need some additional defense against
> malformed input. Why not put this code below inside the loop, with a helper
> func if you think that would make it more readable.
The only value I see in testing for the namespace is possibly for versioning and I added a check.
If we don't find an <emItems> element the blocklist will be empty and no items will be blocklisted.
I added a helper and though I could see adding a second it wouldn't provide much value.

> I'm vaguely worried about the fact that the EM service is popping up UI for
> blocked installs in various places; in an ideal world that would be handled by
> callbacks/observers from the client code, but I understand that's out of scope
> for 1.8.
I changed this slightly so we pass null for notifications and attempt to use the appropriate manager when blocking an install.

> What are the situations where somebody would want/need to pass an explicit
> toolkitVersion instead of using gApp.platformVersion? Is this here to deal with
> what extensions would become incompatible in update scenarios? I don't see any
> usage of toolkitVersion later in the function, is that going to be added later
> as part of the other bug?
I added it so the interface wouldn't need to be changed for when the EM supports a targetApplication for [e-mail] in bug 299716. Just to verify, is that still the name we want?

> 
> >+  _rdfGet_detailsURL: function(item, property) {
> >+    var id = stripPrefix(item.Value, PREFIX_ITEM_URI);
> >+    if (this.getItemProperty(id, "blocklisted") == "false")
> >+      return EM_L("none");
> 
> nit: "none"? seems to me it would be better to return NS_RDF_NO_VALUE or an
> empty string.
Fixed for the blocklist. I am planning on changing availableUpdateURL so it doesn't return "none" in bug 329045.

> If these prefs are missing does the EM recover gracefully?
Yes.
If blocklist url is not present it bails.
If blocklist enabled is not present it defaults to true
If blocklist interval is not present it defaults to 86400
If blocklist details url is not present the ui doesn't display the url

> nit: in general I'd prefer not to invent new XUL element names. How about
> <richlistitem class="blocklistItem">?
I modified lists.xul, etc. to support blocklist notification, added an info icon (I planned on doing this for 2.0 and needed it for this), and fixed a leak in list.js.

Comment 28

[Robert Strong (they/them - no direct email)]

Comment on attachment 214204 [details] [diff] [review]
patch

Darin, with Benjamin on paternity leave I'm hoping you would review this in his stead?

Comment 29

[Darin Fisher]

Comment on attachment 214204 [details] [diff] [review]
patch

>Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl
...
>+  /**
>+   * Checks for changes to the blocklist, application disables / enables items
>+   * that have been added / removed from the blocklist, and if there are
>+   * additions displays a list of the items added.
>+   */
>+  void checkForBlocklistChanges();

Just reading this comment, leaves me with several questions:

1) Is the checking asynchronous or synchronous?
2) If the checking is asynchronous, then what happens if I call this
method twice in quick succession?
3) How does someone using this API observe the changes to the blocklist?


>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in

>+const XMLURI_BLOCKLIST                = "http://www.mozilla.org/2006/blocklist";

Perhaps the namespace URL should include the word "extensions" or "addons":

  "http://www.mozilla.org/2006/addons-blocklist"


>+var Blocklist = {
>+  /**
>+   * Extension ID -> array of Version Ranges
>+   * Each value in the version range array is a JS Object that has the
>+   * following properties:
>+   *   "minVersion"  The minimum version in a version range (default = 0).
>+   *   "maxVersion"  The maximum version in a version range (default = *).
>+   *   "targetApps"  Application ID -> array of Version Ranges
>+   *                 (default = current application ID)
>+   *                 Each value in the version range array is a JS Object that
>+   *                 has the following properties:
>+   *   "minVersion"  The minimum version in a version range (default = 0).
>+   *   "maxVersion"  The maximum version in a version range (default = *).
>+   */
>+  entries: null,

This comment block seems out-of-date.


>+  notify: function() {
>+    var defaults = gPref.QueryInterface(Components.interfaces.nsIPrefService)
>+                        .getDefaultBranch(null);
>+    try {
>+      var dsURI = defaults.getCharPref(PREF_BLOCKLIST_URL);
>+      if (defaults.getBoolPref(PREF_BLOCKLIST_ENABLED) == false)
>+        return;

So, you do not allow users to easily override the default settings
for these prefs?  Why?


>+    request.channel.loadFlags |= Components.interfaces.nsIRequest.LOAD_BYPASS_CACHE;

Hrm... are we sure we don't want to support caching the blocklist?
Maybe the blocklist will grow large over time.  Maybe caching will
help reduce server load.  How often do we check for updates?  If we
only check once per day, then the blocklist can be cached for a day.
That could have a huge impact on performance.  Just food for thought
I guess.  Since we control the server hosting the blocklist, we could
choose to tune the cache options server-side.  But, this code change
prevents that altogether.


>+    request.onerror = function(event) { self.onXMLError(event); };
>+    request.onload  = function(event) { self.onXMLLoad(event);  };
>+    request.send(null);

By the way, if |send| throws an exception, then we end up leaking
the request object and the scope of those anonymous functions.  It
is therefore better (albeit awkward) to set onerror and onload after
calling send.


>+    var blocklistFile = getFile(KEY_PROFILEDIR, [FILE_BLOCKLIST]);
>+    if (blocklistFile.exists())
>+      blocklistFile.remove(false);

If the blocklistFile is busy (windows), then this will throw.  Do
things return to an okay state in that case?


>+    var em = Components.classes["@mozilla.org/extensions/manager;1"]
>+                       .getService(Components.interfaces.nsIExtensionManager)
>+                       .QueryInterface(Components.interfaces.nsIExtensionManager2);

Why not pass nsIExtensionManager2 directly to getService?
Also, for the 1.8 branch, we've been adopting the policy
of naming interfaces that are exclusively for the 1.8 branch:
nsIExtensionManager_MOZILLA_1_8_BRANCH

It's damn ugly, but it is done for good reason:  To avoid
conflicting with a future nsIExtensionManager2 interface
that might be introduced later on the trunk.


>+      var doc = parser.parseFromStream(fileStream, "UTF-8", fileStream.available(), "text/xml");

file.fileSize is probably better than fileStream.available() since
the available() method need not report the entire length of the
data stream.  (In this case it does, but best to avoid such
dependencies IMO)


>+      var itemNodes = this._getItemNodes(doc.documentElement);
>+      for (var i = 0; i < itemNodes.length; ++i) {
>+        var blocklistElement = itemNodes.item(i);
>+        if (blocklistElement.nodeType != Components.interfaces.nsIDOMNode.ELEMENT_NODE ||
>+            blocklistElement.localName != "emItem")
>+          continue;

since you're inside a loop, it might be good to create a temp
variable outside the loop body for nsIDOMNode.  that way you
don't pay for looking it up over and over.


>+        for (var x = 0; x < blocklistElement.childNodes.length; ++x) {
>+          var versionRangeElement = blocklistElement.childNodes.item(x);

you might want a local var for blocklistElement.childNodes as well
for similar reasons.


>+            // appDisabled is determined by an item being compatible,
>+            // satisfying its dependencies, and not being blocklisted

So, a user cannot override the blocklist?  That seems bad to me.


>+  checkForBlocklistChanges: function() {
...
>+    // show the blocklist notification window if there are new blocklist items.
>+    if (items.length > 0)
>+      showBlocklistMessage(items, false);
>+  },

Ah, so the user will be prompted by this function, and it just
consults the offline copy of the blocklist.  OK.  This should
be documented in the interface IMO.


>+    if (!responseXML || responseXML.documentElement.namespaceURI == XMLURI_PARSE_ERROR ||
>+        (request.status != 200 && request.status != 0)) {

This same test is repeated elsewhere.  Perhaps we should move it
into a subroutine.


Sorry, I am not the best person to review UI changes.  I suggest
getting UI review on screenshots of the feature, and then I
suggest getting a xul hacker like beng to review your xul+js+css
UI code changes.


r=darin on the extension manager portion of this patch

Comment 30

[Robert Strong (they/them - no direct email)]

Comment on attachment 213956 [details]
screenshot of blocklist dialogs

Mike, I went ahead and made the changes we discussed when you were at the office.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 213956 [details]
screenshot of blocklist dialogs

thanks, Rob. I'm pretty sure I'll eventually think of a better way to do the "More information" bit, but I can't think of it at this particular moment, and it seems like the sort of thing that's easy to do in a follow-on bug.

(Click here for _more information_? Find out _what is considered dangerous_? This is driving me _nuts_?)

Comment 32

[Robert Strong (they/them - no direct email)]

Comment on attachment 214204 [details] [diff] [review]
patch

Scott, the files left to be reviewed are extensions.js, extensions.xml, extensions.xul, list.js, list.xul, and extensions.css. The ui is currently for all practical purposes overloading an element for status info which is why some of the css is a bit funky. I'll be taking care of that in bug 329045.

Comment 33

[Scott MacGregor]

Comment on attachment 214204 [details] [diff] [review]
patch

you left me the easy part.

1) Please add the new blocklist prefs to all-thunderbird.js as well.

2) Instead of calling setAttribute to set the class attribute on infoIcon, you can use .className. You can also get rid of the switch statement, simplifying things:

if (iconClass in params)  document.getElementById("infoIcon").className = 'spaced ' + params["iconClass"];

that line may need tweaked to account for the default "alert-icon" case using a ?: syntax.

3) This:

+    message = document.getElementById("moreInfoBox")
+    message.hidden = false;

Can just be  document.getElementById("moreInfoBox").hidden = false;

4) message.setAttribute("value"
can you just do message.value = ...
here?

Comment 34

[Robert Strong (they/them - no direct email)]

Attachment: patch for checkin

This addresses the review comments... carrying over reviews

I'll be working with morgamic to improve the caching scheme.
I'm thinking of adding code to manage the XMLHttpRequests especially for extension update check where it is more of an issue.
It recovers gracefully if the file can't be removed and it throws.

Comment 35

[Robert Strong (they/them - no direct email)]

Checked in on trunk. Please file new bugs for any regressions and assign them to me. In case there is any confusion... the bug for creating / hosting the blocklist file on AMO is bug 290759 and has not been completed yet.

Comment 36

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214469 [details] [diff] [review]
patch for checkin

a=[e-mail] for the 1.8.1 branch

Comment 37

[Robert Strong (they/them - no direct email)]

Fixed on MOZILLA_1_8_BRANCH

Comment 38

[Alex Ihrig [:Thunderbird_Mail_DE]]

Could we get an example extension (blacklisted on a.m.o) to do l10n work?

Comment 39

[Robert Strong (they/them - no direct email)]

*** Bug 339949 has been marked as a duplicate of this bug. ***

Comment 40

[Robert Strong (they/them - no direct email)]

And this is why I went with blocklist way back when
https://9to5google.com/2020/06/12/google-android-chrome-blacklist-blocklist-more-inclusive/