795387 - Plugin blocks don't allow simultaneous plugin version and application version filters

Status: VERIFIED FIXED

(Toolkit :: Blocklist Policy Requests, defect)

Description

[Jorge Villalobos [:jorgev] (he/him)]

If we want to block a plugin and restrict if by plugin version, we use the Min and Max fields, like in:
https://addons.mozilla.org/admin/models/blocklist/blocklistdetail/138/

If we want to restrict by application version, we use the GUID, Min and Max fields, like in:
https://addons.mozilla.org/admin/models/blocklist/blocklistdetail/27/

Since they use the same fields, they can't be used simultaneously.

Extension blocks work differently. You add the blocklist detail, and then you create a separate blocklist app entry (https://addons.mozilla.org/admin/models/blocklist/blocklistapp/) that handles the application version range. You can link blocklist app entries to extension blocks, but not to plugin blocks.

There are a couple of possible solutions here. One is to expand the form in the plugin section, so that it has separate Min / Max entries for application and plugin version. The other one would be to allow blocklist app entries to be associated with plugin blocks.

This was brought up in bug 793273, and it limited our options for keeping CTP blocks enabled in Firefox 16.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

This is limiting our possibilities when planning plugin blocks, so we need it fixed soon.

Wil, Wraithan, can you look into this?

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Jorge Villalobos [:jorgev] from comment #0)
> The other one would be to allow blocklist
> app entries to be associated with plugin blocks.

To answer a question from IRC, I prefer this approach because it is consistent with the way we block extensions.

Comment 3

[Wil Clouser [:clouserw]]

Wraithan volunteered to check this out.

Comment 4

[Wraithan (Chris McDonald) [:wraithan]]

Have working code, need to write tests and a migration.

Comment 5

[Wraithan (Chris McDonald) [:wraithan]]

Wont be done with enough time for code review/QA, pushing to next week.

WIP code:
https://github.com/wraithan/zamboni/tree/795387

Comment 6

[Alex Keybl [:akeybl]]

(In reply to Wraithan from comment #5)
> Wont be done with enough time for code review/QA, pushing to next week.
> 
> WIP code:
> https://github.com/wraithan/zamboni/tree/795387

Still on track for this week? This is blocking critical CTP testing on FF17.

Comment 7

[Wraithan (Chris McDonald) [:wraithan]]

This slipped in favor of getting PINs working for paid apps in the Firefox Marketplace. I should have said something earlier this week and got it handed off. I apologize for that.

Comment 8

[Wraithan (Chris McDonald) [:wraithan]]

https://github.com/mozilla/zamboni/commit/5e99328f6a0210554f0f8d4ec703fbc4d291dc8c
Here's the patch.

Comment 9

[Wil Clouser [:clouserw]]

thanks wraithan

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

Reopening because https://addons-dev.allizom.org/en-US/admin/models/blocklist/blocklistapp/ isn't accessible, and I am unable to add new app items.

Comment 11

[Jorge Villalobos [:jorgev] (he/him)]

A migration script wasn't run when pushing. This is fixed now.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Will wait until this is in production for marking status-firefox17 fixed.

Comment 13

[Wraithan (Chris McDonald) [:wraithan]]

Steps for testing:

You'll be entering BlocklistPlugins and BlocklistApps in the admin here:

http://addons-dev.allizom.org/en-US/admin/models/blocklist/

And verifying it works by looking at the blocklist url:

http://addons-dev.allizom.org/en-US/blocklist/2/<guid>/<version>

guid - This will be a value you come up with.
version - this is a number you come up with.

Try adding a BlocklistPlugin with just a name and details (all that is required)

Make sure it shows up in the blocklist url.

Try adding a BlocklistApp to the BlocklistPlugin, make sure to set the guid to a value you came up with and the version range (min and max) so that the version number will be inside.

Make sure it shows up in your blocklist url.

Change the version numbers in the BlocklistApp that you made to exclude the version number you came up with.
Make sure it isn't in your blocklist url.

Change the version range on the BlocklistApp that you made back to including the version.
Make sure it is in your blocklist url.

Change the guid on the BlocklistApp to something different.
Make sure it isn't in your blocklist url.

Comment 14

[Jorge Villalobos [:jorgev] (he/him)]

These blocks are supposed to have both filters, but they only show up the application version filter:

<pluginItem  blockID="p163">
  <match name="filename" exp="(NPSWF32\.dll)|(Flash\ Player\.plugin)" />
  <versionRange  severity="0" vulnerabilitystatus="1">
    <targetApplication  id="{ec8030f7-c20a-464f-9b0e-13a3a9e97384}">
      <versionRange  minVersion="17.0" maxVersion="*" />
    </targetApplication>
  </versionRange>
</pluginItem>

<pluginItem  blockID="p165">
  <match name="filename" exp="(NPSWF[0-9_]*\.dll)|(Flash\ Player\.plugin)" />
  <versionRange  severity="0" vulnerabilitystatus="1">
    <targetApplication  id="{ec8030f7-c20a-464f-9b0e-13a3a9e97384}">
      <versionRange  minVersion="17.0" maxVersion="*" />
    </targetApplication>
  </versionRange>
</pluginItem>

Comment 15

[Wraithan (Chris McDonald) [:wraithan]]

https://github.com/mozilla/zamboni/commit/d0089feb215791eed993a362d7d83e6bc5b31b9d

forgot to account for plugin.min and plugin.max properly

Comment 16

[Jorge Villalobos [:jorgev] (he/him)]

Verified in prod.