391633 - Extend blocklist to support plugins

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Plugins, defect)

Description

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

mwu has added support for blocklisting plugins in bug 330511.

Comment 1

[Michael Wu [:mwu]]

Bug 271559 actually adds the code that reads and uses plugins blocklisting information from blocklist.xml.

Comment 2

[Michael Morgan [:morgamic]]

Is there anything about plugins that extends past the scope of the current blocklist.xml schema?  See:
http://wiki.mozilla.org/Extension_Blocklisting:Code_Design

Comment 3

[Michael Wu [:mwu]]

The plugin blocklisting update to the blocklist.xml schema are fairly simple. Here's an example:

<blocklist>
  <pluginItems>
    <pluginItem>
      <!-- All match tags must match a plugin to blocklist a plugin -->
      <match name="name" exp="some plugin"/>
      <match name="description" exp="1[.]2[.]3"/>
      </pluginItem>
  </pluginItems>
</blocklist> 

The name attribute in the match tag can match any string attribute defined in http://lxr.mozilla.org/mozilla/source/modules/plugin/base/public/nsIPluginTag.idl . The exp attribute contains a regular expression which is used to match an attribute in the plugin tag. If all the match tags in a pluginitem match, the plugin is blocklisted.

Comment 4

[Tony Chung [:tchung]]

hi, just wondering what the status and timeline for this feature.  is there plans to target beta 3?  

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

I thought plugin blocklisting works, since bug 388445 was verified; see https://bugzilla.mozilla.org/show_bug.cgi?id=388445#c19

Comment 6

[Tony Chung [:tchung]]

thats what i thought too.  when i verified that bug, at least the notification part was working.   If this is really fixed, can we get this bug properly commented and status changed?

Comment 7

[Michael Morgan [:morgamic]]

TODO:
* update blocklist schema wiki page (see comment #2)
* create new table for pluginitems (kind of like blitems, but blpluginitems instead)
* add logic in script to pull from plugin table
* add logic in script to iterate over results and display plugin list in the XML

ETA is Feb 7-ish.

Comment 8

[Michael Morgan [:morgamic]]

Stephen/Tony -- plugins don't have GUIDs, so the name/regex matching in pluginItem entries is a fix for that.  Since they followed a different install method for a pretty long time before we centered on the method used for today's extensions, they are a special case and aren't covered by the existing schema.

Comment 9

[Michael Morgan [:morgamic]]

mwu -- so just description, filename and name, right?

Comment 10

[Michael Morgan [:morgamic]]

One more quesiton -- if I have a <pluginItem> entry is it possible to define an application range for it, or is blocklisting of a plugin application-agnostic?

For extensions it was possible to define this:
    <emItem id="item_4@domain">
      <versionRange>
        <targetApplication>
          <versionRange minVersion="1.5" maxVersion="1.5.*"/>
        </targetApplication>
      </versionRange>
    </emItem>

I'm guessing it's app-agnostic since they are pure binaries, but want to confirm.

Comment 11

[Michael Morgan [:morgamic]]

Answers:
#9: yes, just those
#10: yes, app-agnostic

Wanted to make sure.  So we'll move forward with those assumptions.  I updated the wiki to reflect this:
http://wiki.mozilla.org/Extension_Blocklisting:Code_Design#Blocklist_syntax

Comment 12

[Michael Morgan [:morgamic]]

Attachment: first stab at schema and blocklist.php patch

Will add tests tomorrow -- Fred can you take a look at the schema and basics?

Comment 13

[Fred Wenzel [:wenzel]]

Comment on attachment 308590 [details] [diff] [review]
first stab at schema and blocklist.php patch

In general, the patch looks good.

But please use a MySQL index that spans the two columns we are querying here together, not each separately.

(Also, if you want to remove a typo, line 240 says "errer" for "error").

Comment 14

[Michael Morgan [:morgamic]]

K, fixed typo -- pretty sure that a multicolumn index won't make any difference, but will check...

Comment 15

[Mike Connor [:mconnor]]

Mike, ETA on getting this into production?

Comment 16

[Michael Morgan [:morgamic]]

Just gotta write test so tomorrow looks peachy.

Comment 17

[Michael Morgan [:morgamic]]

Tony - will work with you to stage it first as well.

Comment 18

[Tony Chung [:tchung]]

thanks, i just need to know which plugin we plan to blocklist, version, and where to point my settings to.   I'll have time to test this after we ship beta 5

Comment 19

[Michael Morgan [:morgamic]]

Actually I need to know what we're blocklisting as far as plugins... so I can create a record for it?

Comment 20

[Michael Morgan [:morgamic]]

Attachment: v2, proper index, tests and test data, fixed typo

To test, have to set SERVICE_URL and make sure you have TEST_DB_* setup correctly in your config (which you should already!!!) >:P

Comment 21

[Fred Wenzel [:wenzel]]

Comment on attachment 313592 [details] [diff] [review]
v2, proper index, tests and test data, fixed typo

Passes the test suite, looks good. Good job.

Comment 22

[Michael Morgan [:morgamic]]

Checked in on trunk.  Tony, let's meet on Friday to test a random plugin -- I'll ping ya.

Comment 23

[Michael Morgan [:morgamic]]

We need to add support for version _ranges_ in the app (Firefox can't parse them as with blitems/blapps).  So reopening until version range checking is added.  For now we can test with a specific version, though.  Won't push until range support is added.

Comment 24

[Michael Morgan [:morgamic]]

ETA on this is Saturday.

Comment 25

[Michael Morgan [:morgamic]]

S/Saturday/Monday/ -- started the patch but wasn't able to finish this weekend.  Patch later this PM.

Comment 26

[Michael Morgan [:morgamic]]

Attachment: v3, adding support for version ranges

Patch on top of v2 which adds support for version ranges, tests included.

Comment 27

[Fred Wenzel [:wenzel]]

Comment on attachment 314194 [details] [diff] [review]
v3, adding support for version ranges

WFM. New tests look good and pass as well.

Not something to r- it for, but maybe you want to avoid the relative path in the require_once since it relies on the CWD and make it relative to dirname(__FILE__) instead?

Comment 28

[Michael Morgan [:morgamic]]

Yeah, added dirname(__FILE__) thanks Fred.  Checked in on r12064.

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED; I tested this today using Tony's Litmus testcases, and they all passed (Java, QuickTime, etc.), with rc1.