Fennec uses old blocklisting URL

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: reed, Assigned: wesj)

Tracking

Bug Flags:
in-testsuite ?
in-litmus +

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

8 years ago
I'm cleaning up mobile.js, and I noticed this:

pref("extensions.blocklist.url", "https://addons.mozilla.org/blocklist/2/%APP_ID%/%APP_VERSION%/%PRODUCT%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/");

v2 is the old blocklisting URL. We're at v3 now. v3 has things like soft blocking and other stuff, iirc.
Since v1.0 doesn't actually use the blocklisting service, this shouldn't block that release, but it should be fixed for the future version.
Nevermind... was thinking of the malware site blocklist, not this one.

Updated

8 years ago
Depends on: 537186
(In reply to comment #0)
> v2 is the old blocklisting URL. We're at v3 now. v3 has things like soft
> blocking and other stuff, iirc.

We don't have UI for softblocking. Is there any other difference? Do both versions contain the same data (apart from the soft blocks)?
Someone needs to test that blocklisting is working right now. We're certainly pinging for the updates, I think, but there may be UI hooks it depends on?
Morgamic: need a web-dev answer to comment 3 about the content differences between the v2 and v3 blocklisting URL
(Reporter)

Comment 6

8 years ago
(In reply to comment #5)
> Morgamic: need a web-dev answer to comment 3 about the content differences
> between the v2 and v3 blocklisting URL

http://viewvc.svn.mozilla.org/vc/addons/trunk/site/app/webroot/services/blocklist.php?view=markup

Looks like the differences are that v3:
* Supports specifying specific applications
* Supports specifying version ranges (and only for specific products and their version ranges)

Mossop, is that correct?
v3 adds support for targetApplication information for plugins. Fennec should update to use it. You'll be getting softblockling and stuff regardless of using v2 or v3 urls.

Updated

8 years ago
tracking-fennec: ? → 1.1+
(In reply to comment #4)
> Someone needs to test that blocklisting is working right now. We're certainly
> pinging for the updates, I think, but there may be UI hooks it depends on?

Looks like there is:
 * http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#68
 * http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/list.js#80
tracking-fennec: 1.1+ → ?

Updated

8 years ago
tracking-fennec: ? → 2.0b1+
Assignee: nobody → mark.finkle
Assignee: mark.finkle → wjohnston
(Assignee)

Comment 9

8 years ago
Created attachment 469358 [details] [diff] [review]
Mozilla Central changes

Mozilla-central changes. Adds an nsIBlockedAddonPrompt called when addons are blocked due to a newly downloaded blocklist.
(Assignee)

Comment 10

8 years ago
Created attachment 469359 [details] [diff] [review]
Mobile-browser changes

Implemented the nsIBlockedAddonPrompt interface. I made this show a notification popup for now, although I'm guessing that the preferred UI is debatable.

This also checks for addoninstalls that are blocked due to a blocklist, and shows an alert indicating why they were rejected. There's probably a million little side cases here. Posting these for initial feedback.
Comment on attachment 469358 [details] [diff] [review]
Mozilla Central changes

Some drive by feedback:
* I was initially thinking nsIBlocklistPrompt, but as long as Mossop is happy, we are happy
* Why not put nsIBlockedAddonPrompt in the nsIBlocklistService.idl file?
* You remove some blank lines that should probably stay.
Comment on attachment 469359 [details] [diff] [review]
Mobile-browser changes


>diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js

>-    this._showAlert(true);
>+    this._showIntallCompleteAlert(true);

showIntallCompleteAlert -> showInstallCompleteAlert

>+  _showAlert: function xpidm_showAlert(aMessage) {
>     if (ExtensionsView.visible)
>       return;
>-
>     let strings = Elements.browserBundle;
>-    let message = aSucceeded ? strings.getString("alertAddonsInstalled") :
>-                               strings.getString("alertAddonsFail");
>-
>     let observer = {

Don't kill the nice blank lines

I need to look more closely at how we handle various blocklist situations.
(Assignee)

Comment 13

8 years ago
Created attachment 469501 [details] [diff] [review]
Mozilla Central changes

Moved the interface. This should actually work.
Attachment #469358 - Attachment is obsolete: true
(Assignee)

Comment 14

8 years ago
Created attachment 469502 [details] [diff] [review]
Mobile-browser changes
Attachment #469359 - Attachment is obsolete: true
Comment on attachment 469501 [details] [diff] [review]
Mozilla Central changes

Let's get Mossop to review. We also need to find a way to test this out for Fennec.
Attachment #469501 - Flags: review?(dtownsend)
Attachment #469501 - Flags: feedback+
Comment on attachment 469502 [details] [diff] [review]
Mobile-browser changes


>diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js

>+  onDownloadCancelled: function(aInstall, aAddon) {
>+    if(aInstall.addon.blocklistState == Ci.nsIBlocklistService.STATE_BLOCKED) {
>+      this._showBlockedAlert();
>+    }

nits:
* Space between "if" and "("
* No {} needed around one liner

>-    this._showAlert(true);
>+    this._showIntallCompleteAlert(true);

Typo: showInstallCompleteAlert

>diff --git a/components/BlockedAddonPrompt.js b/components/BlockedAddonPrompt.js

>+BlockedAddonPrompt.prototype = {
>+  prompt: function(aAddons, aCount) {
>+    let win = Services.wm.getMostRecentWindow("navigator:browser");
>+    win.ExtensionsView.showRestart("blocked");
>+    if (win.ExtensionsView.visible)
>+      return;

Should we only show the ExtensionsView restart _if_ the ExtensionView is visible? Why show the ExtensionView restart _and_ the main notification area restart? If we really should then ok, but I'm just checking.

nit: add a blank line after the "return;"

>+    notifyBox.appendNotification(bundle.GetStringFromName("notificationRestart.blocked"),
>+                                 "blocked-add-on",
>+                                 "chrome://browser/skin/images/alert-addons-30.png",
>+                                 "PRIORITY_CRITICAL_HIGH",
>+                                 buttons);

I think we ignore any image passed to appendNotification. You can just pass ""


I have not worked much with blocking add-ons, so we need to find some good test cases (look in Firefox and talk to Mossop) so we can test the various situation where blocklisting can happen.


r+, but let's fix the nits and figure out the restart in ExtensionView behavior. Oh and find ways to test! We can't land this before we have tested various situations.
Attachment #469502 - Flags: review+
Comment on attachment 469501 [details] [diff] [review]
Mozilla Central changes

This part of the blocklist service really needs to be updated to use the new add-ons manager API more fully but it didn't seem worthwhile previously. I'm guessing you guys don't have the time to do it right now either so I suppose we'll muddle on as-is and do it post-Firefox 4.

The prompting API proposed here doesn't seem right to me. It looks like you're doing your work asynchronously yet as soon as you return the service goes ahead and disables the soft-blocked items. I think instead if the nsIBlockedAddonPrompt service exists then it should handle the disabling of soft-blocked items itself, so just call into it and do nothing afterwards. This would be cleaner if the code was using the new API's but should just work as-is anyway.

As for naming I think "@mozilla.org/addons/blocklist-prompt;1" and "nsIBlocklistPrompt" is probably a better choice.
Attachment #469501 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 18

8 years ago
Created attachment 470825 [details] [diff] [review]
Mobile browse

Addressed I think. The download canceled code is now stolen from Firefox:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#718

and handles a few other situations. Added support to showing blocked and softblocked in the add-ons manager. This probably needs some UX-review. There are two cases:

1.) User installs addon, blocklist is downloaded later the blocks (hard or soft blocking) the add-on
    In this case, this shows a notification bar with a restart button added.
    If you dismiss that alert, and later visit the add-ons manager (without having restarted), there is a notification bar telling you that something has been blocklisted and you should restart (this is probably overkill)
2.) User tries to install an addon that we've already blocklisted
    If the addon is hard-blocked we refuse to install and show a slide-in notification
    If the addon is soft-blocked we just allow the install like always, but the addon will have a badge in the add-ons manager.

For testing this, I have been using some stuff on my dropbox:

https://dl.dropbox.com/u/72157/Testing/extensions/installExtensions.html

Two extensions, and a custom blocklist. The first one ("Hello World") is hard blocked in the list. The second is soft-blocked. You have to change some prefs to point the blocklist to a local file. Testing the second case above is easy. Testing the first, you have to delete your blocklist. Flipping the extensions.blocklist.enabled pref should trigger that, but it never does for me. So I just set the timer to be short and wait a bit. It's a pain.

Related Firefox bugs: bug 455906, bug 391714
Attachment #469502 - Attachment is obsolete: true
Attachment #470825 - Flags: review?(mark.finkle)
(Assignee)

Comment 19

8 years ago
Created attachment 470829 [details] [diff] [review]
Mozilla Central changes

Name change. And we can handle our own soft blocking.
Attachment #469501 - Attachment is obsolete: true
Attachment #470829 - Flags: review?(dtownsend)
Comment on attachment 470825 [details] [diff] [review]
Mobile browse


>         let updateable = (addon.permissions & AddonManager.PERM_CAN_UPDATE) > 0;
>         let uninstallable = (addon.permissions & AddonManager.PERM_CAN_UNINSTALL) > 0;

Add the | let blockedString =""; | section of code here, not below. And rename "blockedString" -> "blocked"

Oh, and make sure a blank line is above and below the section :)

>         listitem.setAttribute("updateable", updateable);
>         listitem.setAttribute("isReadonly", !uninstallable);
>+        let blockedString = "";
>+        switch(addon.blocklistState) {
>+          case Ci.nsIBlocklistService.STATE_BLOCKED:
>+            blockedString = strings.getString("addon.item.blocked")
>+            break;
>+          case Ci.nsIBlocklistService.STATE_SOFTBLOCKED:
>+            blockedString = strings.getString("addon.item.softBlocked");
>+            break;
>+          case Ci.nsIBlocklistService.STATE_OUTDATED:
>+            blockedString = srings.getString("addon.item.outdated");
>+            break;
>+        }            
>+        listitem.setAttribute("blockedStatus", blockedString);

Just the | listitem.setAttribute("blockedStatus", blocked); | should be here. No blank lines needed :)


>+  onDownloadCancelled: function(aInstall, aAddon) {
>+    let strings = Elements.browserBundle;
>+    let brandBundle = document.getElementById("bundle_brand");
>+    let brandShortName = brandBundle.getString("brandShortName");
>+    let host = (aInstall.originatingURI instanceof Ci.nsIStandardURL) &&
>+               aInstall.originatingURI.host;

No need to wrap

>+    if (!host)
>+      host = (aInstall.sourceURI instanceof Ci.nsIStandardURL) &&
>+             aInstall.sourceURI.host;

No need to wrap

>diff --git a/components/BlocklistPrompt.js b/components/BlocklistPrompt.js

"If you dismiss that alert, and later visit the add-ons manager (without
having restarted), there is a notification bar telling you that something has
been blocklisted and you should restart (this is probably overkill)"

I agree, it's probably overkill. Does that happen here?

>+  prompt: function(aAddons, aCount) {
>+    let win = Services.wm.getMostRecentWindow("navigator:browser");
>+    win.ExtensionsView.showRestart("blocked");
>+    if (win.ExtensionsView.visible)
>+      return;

Is the overkill part here? Maybe we should just do the | win.ExtensionsView.showRestart("blocked"); | _if_ the extensions view is visible. That way we only show the user one notification. If they are in the Add-on Manager they get the add-on notification. If they are in the browser, they get the notificationBox one.

>+    // Disable softblocked items automatically
>+    for (let i = 0; i < aAddons.length; i++) {
>+      if (aAddons[i].item instanceof Ci.nsIPluginTag)
>+        addonList[i].item.disabled = true;
>+      else
>+        aAddons[i].item.userDisabled = true;
>+    }

In this patch, we don't disable softblocked items if the extension view is open, because we return early. Is that a problem? or do we disbale them elsewhere?

>diff --git a/locales/en-US/chrome/browser.properties b/locales/en-US/chrome/browser.properties
> alertAddonsFail=Installation failed

Move the "alertAddonsBlocked" line here, with the other add-on alert strings

>+alertAddonsBlocked=Unsafe add-ons installed
>+addon.item.blocked=Blocked
>+addon.item.softBlocked=Known to cause security or stability issues
>+addon.item.outdated=Out of date

Instead of "addon.item." let's use "addonBlocked." and move up to a new group just below the "addonUpdate." section.

>+addonError-1=The add-on could not be downloaded because of a connection failure on #2.
>+addonError-2=The add-on from #2 could not be installed because it does not match the add-on #3 expected.
>+addonError-3=The add-on downloaded from #2 could not be installed because it appears to be corrupt.
>+addonError-4=#1 could not be installed because #3 cannot modify the needed file.

We need a localization note here too

>+# LOCALIZATION NOTE (addonLocalError-1, addonLocalError-2, addonLocalError-3, addonLocalError-4, addonErrorIncompatible, addonErrorBlocklisted):
>+# #1 is the add-on name, #3 is the application name, #4 is the application version
>+addonLocalError-1=This add-on could not be installed because of a filesystem error.
>+addonLocalError-2=This add-on could not be installed because it does not match the add-on #3 expected.
>+addonLocalError-3=This add-on could not be installed because it appears to be corrupt.
>+addonLocalError-4=#1 could not be installed because #3 cannot modify the needed file.
>+addonErrorIncompatible=#1 could not be installed because it is not compatible with #3 #4.
>+addonErrorBlocklisted=#1 could not be installed because it has a high risk of causing stability or security problems.

Let's move these two error blocks above the download manager section, keeping the addon manager strings together.


Overall, this patch looks good to me. Just some nits to clean up. Also, the "overkill" and "auto softblock disable questions.
Attachment #470825 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 21

8 years ago
Created attachment 471176 [details] [diff] [review]
Mobile-browser changes

Nits addressed and tested again. Thanks for catching the Extensions view open case.
Attachment #470825 - Attachment is obsolete: true
Attachment #471176 - Flags: review?(mark.finkle)
Comment on attachment 471176 [details] [diff] [review]
Mobile-browser changes

looks good
Attachment #471176 - Flags: review?(mark.finkle) → review+
Comment on attachment 470829 [details] [diff] [review]
Mozilla Central changes

Want to see the updated comments but otherwise this is fine

>diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/nsBlocklistService.js
>@@ -857,32 +857,40 @@ Blocklist.prototype = {
>               disable: false,
>               blocked: state == Ci.nsIBlocklistService.STATE_BLOCKED,
>               item: plugins[i]
>             });
>           }
>         }
>         plugins[i].blocklisted = state == Ci.nsIBlocklistService.STATE_BLOCKED;
>       }
>-

Please don't remove this empty line

>       if (addonList.length == 0)
>         return;
> 
>+      if ("@mozilla.org/addons/blocklist-prompt;1" in Cc) {
>+        try {
>+          let blockedPrompter = Cc["@mozilla.org/addons/blocklist-prompt;1"]
>+                                 .getService(Ci.nsIBlocklistPrompt);
>+          blockedPrompter.prompt(addonList);
>+        } catch (e) {
>+          LOG(e);
>+        }
>+        return;
>+      }

Add an empty line after this

>       var args = {
>         restart: false,
>         list: addonList
>       };
>       // This lets the dialog get the raw js object
>       args.wrappedJSObject = args;
> 
>       var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>                getService(Ci.nsIWindowWatcher);
>       ww.openWindow(null, URI_BLOCKLIST_DIALOG, "",
>                     "chrome,centerscreen,dialog,modal,titlebar", args);
>-

Please don't remove this empty line

>diff --git a/xpcom/system/nsIBlocklistService.idl b/xpcom/system/nsIBlocklistService.idl

>+/**
>+ * nsIBlockedAddonPrompt is used, if available, by the default implementation of 
>+ * nsIBlocklistService to display a confirmation UI to the user before blocking
>+ * extensions/plugins.

Correct the naming in the comment

>+[scriptable, uuid(36f97f40-b0c9-11df-94e2-0800200c9a66)]
>+interface nsIBlocklistPrompt : nsISupports
>+{
>+  /**
>+   * Prompt the user about newly blocked addons. The prompt is then resposible
>+   * for soft-blocking any addons that need to be afterwards
>+   *
>+   * @param  aAddons
>+   *         An array of addons and plugins that are blocked

Could you add a little to the comment to describe the nature of the objects being passed here.

>+   * @param  aCount
>+   *         The number of addons
>+   */
>+  void prompt([array, size_is(aCount)] in nsIVariant aAddons,
>+                 [optional] in PRUint32 aCount);

Line up the arguments
Attachment #470829 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 24

8 years ago
Created attachment 471270 [details] [diff] [review]
Mozilla Central changes

Whitespace returns!  Sorry about that...
Attachment #470829 - Attachment is obsolete: true
Attachment #471270 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #471270 - Flags: review? → review?(dtownsend)
Attachment #471270 - Flags: review?(dtownsend) → review+
pushed m-c:
http://hg.mozilla.org/mozilla-central/rev/d81c3c24a14b

pushed m-b:
http://hg.mozilla.org/mobile-browser/rev/f482950a917d
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Flags: in-litmus?
Assigning to self to create a litmus testcase.
Assignee: wjohnston → tchung
(Reporter)

Comment 27

7 years ago
(In reply to comment #26)
> Assigning to self to create a litmus testcase.

The assignee field is only for the developer who wrote the patch.
Assignee: tchung → wjohnston

Updated

7 years ago
Flags: in-litmus? → in-litmus?(tchung)
Verified fix on 0921 android and n900 nightly

Added litmus test: https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=12939
Status: RESOLVED → VERIFIED

Updated

7 years ago
Flags: in-litmus?(tchung) → in-litmus+
You need to log in before you can comment on or make changes to this bug.