Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json

VERIFIED FIXED in Firefox 49

Status

P1
normal
VERIFIED FIXED
2 years ago
6 months ago

People

(Reporter: rpl, Assigned: bsilverberg)

Tracking

({addon-compat})

unspecified
mozilla51
addon-compat
Dependency tree / graph

Firefox Tracking Flags

(firefox49 verified, firefox50 verified, firefox51 verified)

Details

(Whiteboard: triaged)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
The behavior of the compatibility checks when there is a "*" in the `strict_min_version` compatibility field of a WebExtension manifest is currently confusing to the addon developer, e.g. a WebExtension with a strict_min_version set to "48.*" is not compatible with Firefox 48.0 (nor with 48.1 etc., but it is compatible with alpha version of the next version e.g. 49.0a2).

After a brief discussion on IRC, it seems that the most reasonable change can be to prevent the "*" to be used in the script_min_version at all.

A similar issue should be filed for web-ext lint (and the addons-linter), so that we can warn the addon developer that the strict_min_version have to be fixed, way before that the addon reaches any real user (and fails to install because we raise an error for the invalid strict_min_version format in the new Firefox versions, and because of the issue describe above on the older Firefox versions).
(Reporter)

Comment 2

2 years ago
Hi Jorge,
how it looks to you the above plan?

Feel free to add any additonal info/suggestions, this is still in a brainstorming phase, but we are pretty sure that it is something that we need to fix, and, at the same time, to be very very careful about how we are going to release the fix.
Flags: needinfo?(jorge)
Keywords: addon-compat
Priority: -- → P1
Another idea would be to fix the comparator to return the expected output when you compare 48.0 vs 48.*. Although it can have a bit more impact, I think it's reasonable to do. Especially because we will run into the issue again for temporary add-ons due to bug 1281884.
I don't think '.*' makes sense in min versions, so I'd rather forbid it than try to reverse the way that it currently works.
OK, but how are we going to solve the problem for temporary add-ons?
(In reply to Andreas Wagner [:TheOne] from comment #5)
> OK, but how are we going to solve the problem for temporary add-ons?

What problem, exactly?
(In reply to Kris Maglione [:kmag] from comment #6)
> (In reply to Andreas Wagner [:TheOne] from comment #5)
> > OK, but how are we going to solve the problem for temporary add-ons?
> 
> What problem, exactly?

I suspect that with bug 1281884 fixed, an add-on having a strict_min_version of 48.* for example will not be able to get temporarily loaded in 48.
(In reply to Andreas Wagner [:TheOne] from comment #7)
> I suspect that with bug 1281884 fixed, an add-on having a strict_min_version
> of 48.* for example will not be able to get temporarily loaded in 48.

Well, given that neither of those changes is going to be uplifted to release, I don't see how it could possibly be an issue. Even if they were, I don't see why it would be an issue for temporary add-ons in particular.
The numbers were just an example. I think we're going to have that problem in whatever release both bug 1281884 and this bug a re fixed.

So, when loading temporary add-ons is going to respect 'strict_min_version' and (example number) 55.* will be considered a higher version than 55.0, an add-on with 'strict_min_version' : '55.*', will refuse to load in 55.0 because Firefox says it's incompatible.

Where am I wrong?
(In reply to Luca Greco [:rpl] from comment #2)
> how it looks to you the above plan?

That looks good to me, to keep consistency with how we treat other add-ons.

If about:debugging is somehow bypassing the compatibility metadata, like Andreas is suggesting, there should be some visible error so developers know the add-on won't work correctly when installed normally.
Flags: needinfo?(jorge)
(In reply to Andreas Wagner [:TheOne] from comment #9)
> So, when loading temporary add-ons is going to respect 'strict_min_version'
> and (example number) 55.* will be considered a higher version than 55.0, an
> add-on with 'strict_min_version' : '55.*', will refuse to load in 55.0
> because Firefox says it's incompatible.

It will refuse to load anywhere where we reject .* in min versions. But I don't see what it has to do with temporary add-ons in particular. If anything, I'd be less worried about that case than about the permanent install case.

Comment 12

2 years ago
2 parts: firefox warning uplift to beta, rejection that ride trains 

andy will file the github issue (that's the need info)
Assignee: nobody → bob.silverberg
Flags: needinfo?(amckay)
Whiteboard: triaged

Comment 13

2 years ago
Done in comment 1
Flags: needinfo?(amckay)
(Assignee)

Updated

2 years ago
Iteration: --- → 51.3 - Sep 12
Summary: Prevent usage of "*" versions as strict_min_version in a WebExtension manifest.json → Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json
(Assignee)

Updated

2 years ago
Blocks: 1298923
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8786062 [details]
Bug 1297752 - Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json,

https://reviewboard.mozilla.org/r/75024/#review73294

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:369
(Diff revision 1)
> +    newId = "strict_min_star@tests.mozilla.org";
> +    apps = {

`let`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:378
(Diff revision 1)
> +          id: newId,
> +          strict_min_version: version,
> +        },
> +      },
> +    }
> +    testManifest = Object.assign(apps, MANIFEST);

`let`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:380
(Diff revision 1)
> +        },
> +      },
> +    }
> +    testManifest = Object.assign(apps, MANIFEST);
> +
> +    addonDir = yield promiseWriteWebManifestForExtension(testManifest, gTmpD,

`let`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:389
(Diff revision 1)
> +      yield AddonManager.installTemporaryAddon(addonDir);
> +    });
> +    ok(messages.some(msg => msg.message.includes("The use of '*' in strict_min_version is deprecated")),
> +       "Deprecation warning for strict_min_version with '*' was generated");
> +
> +    yield AddonManager.installTemporaryAddon(addonDir);

No need to do this twice.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:390
(Diff revision 1)
> +    });
> +    ok(messages.some(msg => msg.message.includes("The use of '*' in strict_min_version is deprecated")),
> +       "Deprecation warning for strict_min_version with '*' was generated");
> +
> +    yield AddonManager.installTemporaryAddon(addonDir);
> +    addon = yield promiseAddonByID(newId);

`let`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:392
(Diff revision 1)
> +    do_check_neq(addon, null);
> +    do_check_eq(addon.id, newId);

Please use `equal` and `notEqual` in new code, and always include an assertion message.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:395
(Diff revision 1)
> +    addon = yield promiseAddonByID(newId);
> +
> +    do_check_neq(addon, null);
> +    do_check_eq(addon.id, newId);
> +
> +    addon.uninstall();

Hum. There should really be a `promiseExtensionStartup` any time we install an extension, but it doesn't look like any of the other tests in this file do it either...
Attachment #8786062 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1299551
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
Try looks good. Requesting checkin.
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fffb67d17a59
Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json. r=kmag
Keywords: checkin-needed

Comment 20

2 years ago
Will also be blocked in the validator in the next AMO push.

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fffb67d17a59
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 23

2 years ago
Created attachment 8787646 [details] [diff] [review]
Patch for uplift to beta
(Assignee)

Comment 25

2 years ago
Created attachment 8787722 [details] [diff] [review]
Patch for uplift to beta
Attachment #8787646 - Attachment is obsolete: true
(Assignee)

Comment 26

2 years ago
Comment on attachment 8787722 [details] [diff] [review]
Patch for uplift to beta

Approval Request Comment
[Feature/regressing bug #]: 1297752
[User impact if declined]: This is a new warning being issued for a deprecated feature. Our plan is to implement this as a warning in beta, and promote it to an error in nightly. Without this uplift extension developers will not have an opportunity to see this warning before it becomes an error in a later version of Firefox.
[Describe test coverage new/current, TreeHerder]: This patch comes with its own test, and seems to have passed on try against beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=194cdc6974c9 
[Risks and why]: This is a simple change in a single file, which only introduces a warning. It also comes with a test. It therefore can be considered to be low risk.
[String/UUID change made/needed]: None
Attachment #8787722 - Flags: approval-mozilla-beta?
I think you mean for 50 (still aurora right now).
Attachment #8787722 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 28

2 years ago
Answering Liz's questions from IRC:

Q. Why do we need this?
A. Firefox's behaviour when one specifies an asterisk as part of a strict_min_version is counter-intuitive and confusing, and it doesn't really make sense to use an asterisk when specifying a min version. We want to eliminate the confusion by forbidding the use of an asterisk in strict_min_version. The long term solution is to forbid it and generate an error, but to give developers a bit of lead time to deal with it, we want to start by just issuing a warning. This patch adds the warning and we'd like it to land on beta to get the warning out to users. Another patch will land on nightly with the error, so that will give users a couple of releases before the warning turns into an error.

Q. How do we test it?
A. Create a web extension with a manifest in which strict_min_version [1] includes an asterisk (e.g., 1.0.* or 1.*.1). When you load the web extension you should see the warning in the console.

Please let me know if you have any other questions. 

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/applications
Flags: needinfo?(lhenry)
We are about to release 49. Do you need this in the 49 release that should happen next Tuesday? My question is because in comments you mention wanting this on beta. We are already building the release candidate.
Flags: needinfo?(lhenry)
Flags: needinfo?(bob.silverberg)
My point here is that I don't want to uplift anything else for 49 at the last minute. Users on beta would only see this for about 2 days. So it seems pointless to add more risk to the actual release for a couple of days of possible warning. I'm thinking devs can get the warning in beta 50.  Seem reasonable?
(Assignee)

Comment 31

2 years ago
Kris, can you reply to Liz please? I think we do want this to go out for 49, but I'm not totally confident in that assertion.
Flags: needinfo?(bob.silverberg) → needinfo?(kmaglione+bmo)
I'd still like to see it in 49, since it will get more attention from developers on release than on other channels. The patch is extremely simple, so I don't think it's a serious risk. But if we can't get it into 49, we should probably push the hard error back a release
Flags: needinfo?(kmaglione+bmo)
If we're already building the RC, this probably isn't urgent enough to uplift unless we do a second RC.
Let's give it a shot then, I can push back the build to a bit later this afternoon.
Comment on attachment 8787722 [details] [diff] [review]
Patch for uplift to beta

[Triage Comment]

Add a warning for webextensions developers. 
Andrei, can your team test once we have RC2 builds? 
Or, Andy, is this something for the Addons team to test?
Flags: needinfo?(andrei.vaida)
Flags: needinfo?(amckay)
Attachment #8787722 - Flags: approval-mozilla-release+
Attachment #8787722 - Flags: approval-mozilla-beta?
Attachment #8787722 - Flags: approval-mozilla-beta+
Attachment #8787722 - Flags: approval-mozilla-aurora?
Attachment #8787722 - Flags: approval-mozilla-aurora+
Add-ons QA should test this.

Krupa, can you or someone from QA test this in the next RC?

The following should trigger warnings in applications.gecko.strict_min_version, but not strict_max_version:

 - "*"
 - "48.*"
 - "48.*.0"

The following should *not* trigger warnings:

 - "48"
 - "48.*b2"
 - "48.0*"

Missing or null strict_min_version keys should also not trigger any warnings or errors.
Flags: needinfo?(amckay) → needinfo?(krupa.mozbugs)
Yes, on it.
When do you guys need this tested by?

Comment 40

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b4cc630c399
status-firefox50: --- → fixed
Flags: needinfo?(andrei.vaida)
(Assignee)

Comment 41

2 years ago
Thanks everyone for your help getting this landed and uplifted. :)
We confirm that the following warning appears on Firefox 49 RC unbranded build (https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-release-win64-add-on-devel/1473287416/) and Firefox 51.0a1 (2016-09-07/08) under Windows 10 64-bit, Ubuntu 12.04/14.04 64-bit and Mac OS X 10.10.4: “1473342800550	addons.xpi	WARN	The use of '*' in strict_min_version is deprecated”. Tomorrow we will also cover Firefox 50.0a2.

We tested as Kris suggested in Comment 36 and the testing results are available via: http://goo.gl/VEaXG3

We also noticed that the following invalid strict_min_version value is accepted on install: “abc.*” . Any thoughts about this?
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox51: fixed → verified
Flags: needinfo?(krupa.mozbugs) → needinfo?(kmaglione+bmo)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #42)
> We also noticed that the following invalid strict_min_version value is
> accepted on install: “abc.*” . Any thoughts about this?

I tested with this value as a minimum version, and it emitted a warning as expected. Can you provide more details?
Flags: needinfo?(kmaglione+bmo)
We also confirm that this issue is verified as fixed on FIrefox 50.0a1 (2016-09-08) under Windows 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.10.4. Testing details available via: http://goo.gl/VmavWb


(In reply to Kris Maglione [:kmag] from comment #43)
> I tested with this value as a minimum version, and it emitted a warning as
> expected. Can you provide more details?

The warning is triggered also for this strict_min_version = “abc.*”, but I was referring to its actually invalid value, because Firefox doesn’t have an “abc” version. 

Also there is no way to submit such an add-on that contains this strict_min_version because the platforms checkboxes are automatically disabled on amo(even an error would be more useful for a beginner - for which I’ve already file a Github bug - https://github.com/mozilla/addons-linter/issues/931). 


So the question is, should this invalid value be accepted at install or the installation should fail (same as for these values: “*”, “*.*”)?
status-firefox50: fixed → verified
Flags: needinfo?(kmaglione+bmo)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #44)
> (In reply to Kris Maglione [:kmag] from comment #43)
> > I tested with this value as a minimum version, and it emitted a warning as
> > expected. Can you provide more details?
> 
> The warning is triggered also for this strict_min_version = “abc.*”, but I
> was referring to its actually invalid value, because Firefox doesn’t have an
> “abc” version. 

That's expected. It's technically a valid version string, and Firefox doesn't
keep a list of known releases to check it against. AMO should reject it as an
unknown version, though.
Flags: needinfo?(kmaglione+bmo)

Updated

6 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.