search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
125 stars 41 forks source link

Align version string acceptance and comparison #1264

Open wagnerand opened 3 years ago

wagnerand commented 3 years ago

Within the ecosystem there currently exist several implementations for which add-on version strings are accepted and how they are compared to each other, to determine update-paths and blocklist-ranges.

This leads to several issues:

As a first step, at least on the AMO side, we should unify acceptance and comparison criteria to make sure we only accept versions that we can tell apart from each other. In a second step, we could look at streamlining version comparison in Firefox to remove complexity and improve reliability.

/cc diox eviljeff jvillalobos rpl

┆Issue is synchronized with this Jira Task

eviljeff commented 3 years ago

I still think moving to much simpler Chrome style version strings is most straightforward way to resolve this. https://bugzilla.mozilla.org/show_bug.cgi?id=1593316

eviljeff commented 3 years ago

On:

  • AMO cuts of part of the version string for comparison, ... during creation of application versions, which are auto-generated by releng through an API.

I think this is an edge case - Firefox version strings have been very consistent for a long time. If we do want to cover it though, application versioning will be a different area of code in m-c than add-on support (right?)

  • Firefox accepts version strings that AMO would not.

Now everything is signed the version would have to get through AMO first, so I assume this only applies to internally signed add-ons that have bypassed AMO?

wagnerand commented 3 years ago

I still think moving to much simpler Chrome style version strings is most straightforward way to resolve this. bugzilla.mozilla.org/show_bug.cgi?id=1593316

While that would make lots of things easier for both Firefox and AMO, I suppose we do want to support alpha/beta versions. We could probably drop the pre part though.

On:

  • AMO cuts of part of the version string for comparison, ... during creation of application versions, which are auto-generated by releng through an API.

I think this is an edge case - Firefox version strings have been very consistent for a long time. If we do want to cover it though, application versioning will be a different area of code in m-c than add-on support (right?)

I agree this is an edge-case and @diox said it's dangerous enough to fix it. It does not affect m-c since since Firefox application versions are ok already. The issue here is that our API also allows automated created of bogus application versions.

  • Firefox accepts version strings that AMO would not.

Now everything is signed the version would have to get through AMO first, so I assume this only applies to internally signed add-ons that have bypassed AMO?

Xes, privileged add-ons and langpacks are examples. Also, it affects the development process since such a version string could be used during development where it works fine and then developers would hit a roadblock when trying to submit their add-on to AMO.

rpl commented 3 years ago

Firefox accepts version strings that AMO would not. Firefox uses a different version comparator (with lots of backwards compatibility cruft) that could come to a different decision than AMO when comparing two version strings.

Using a different version comparator on the Firefox side may make fixing this mismatch a bit riskier (changing nsIVersionComparator isn't clearly an option because it is also being used to compare app versions which is unrelated to the extensions).

I think that if we want to apply changes on the Firefox side, the simplest and safer option would be to:

diox commented 3 years ago

AMO cuts of part of the version string for comparison, ... during creation of application versions, which are auto-generated by releng through an API.

I think this is an edge case - Firefox version strings have been very consistent for a long time. If we do want to cover it though, application versioning will be a different area of code in m-c than add-on support (right?)

I agree this is an edge-case and @diox said it's dangerous enough to fix it. It does not affect m-c since since Firefox application versions are ok already. The issue here is that our API also allows automated created of bogus application versions.

I'm not that worried about applications versions. Releng is the sole caller of the API that creates those, if a bogus version is released (or just a bogus API call made) we can deal with it and fix it manually.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

wagnerand commented 3 years ago

@jvillalobos is this something we could consider for MV3?

willdurand commented 3 years ago

AMO blog: mozilla/addons-moz-compare

It will be addons-frontend, too. This (new) implementation matches the FF one.

wagnerand commented 3 years ago

AMO blog: mozilla/addons-moz-compare

It will be addons-frontend, too. This (new) implementation matches the FF one.

Thanks, updated!

jvillalobos commented 3 years ago

@jvillalobos is this something we could consider for MV3?

It would be a good point in time to introduce a backwards-incompatible change, yes. I like the idea of aligning with the Chrome format, but it sounds like it would require quite a bit of effort on the Firefox side to create a different version comparator. This requires discussion with the WebExtensions team to figure out what's practical to do here.

rpl commented 3 years ago

one option that we may evaluate to keep things simple (especially on the Firefox side) is to restrict the allowed version format at a schema level, but still use the same Services.vc.compare on the strings that are considered valid based on the more restrictive format validation.

By doing it at a schema level, we may also get the same kind of validation on the linter side through the usual Firefox JSONSchema import.

By leveraging the ongoing work on both Firefox and addons-linter side to allow us to have different validations for manifest_version 2 and manifest_version 3 extensions manifests we should be able to have different format validation based on the schema manifest.

The part that would still concern me a bit (and that I think we may want to look more closely into) is what issue we may have if an extension is being updated or downgraded between manifest_version 2 and 3, because in that case we would be comparing an old version format with the new more restrictive version format.

mixedpuppy commented 3 years ago

In chrome there are two manifest entries where versionString is used, that is version (the addon version) and min_chrome_version (we don't support).

In firefox, we have a strict_min/max_version. Those must be able to match against the firefox version strings.

It think it is fine to limit addons to using versionString as Chrome defines it for version, but we cannot really do that for the min/max. MV3 provides an "easy" time to do it, but if it's a problem then I don't see why we couldn't deprecate and change in V2 on AMO, just start enforcing it for all new addons and updates to addons. Firefox can still support the current V2 version formats so we don't break currently installed (or even installing addons from non-AMO locatinos with old version string formats).

mixedpuppy commented 3 years ago

In a second step, we could look at streamlining version comparison in Firefox to remove complexity and improve reliability.

I'd need a much better explanation of issues to understand why this would be necessary.

zombie commented 3 years ago

I asked Apple/Microsoft folks today, and they all use the Chrome version string format (despite Apple linking to MDN for all extensions docs, which only mentions our old Toolkit format).

The part that would still concern me a bit (and that I think we may want to look more closely into) is what issue we may have if an extension is being updated or downgraded between manifest_version 2 and 3, because in that case we would be comparing an old version format with the new more restrictive version format.

For an addon to be downgraded, it would still need to be a newer version, in either format.

Since Chrome version string format is a subset of ours, we can just continue using Services.vc.compare() and everything will work as expected.

The only place where we would limit is when a newer version is MV3, it would be restricted to Chrome format. If they go back to a MV2, that version could be old format (just needs to be higher according to the same Services.vc rules).

zombie commented 3 years ago

MV3 provides an "easy" time to do it, but if it's a problem then I don't see why we couldn't deprecate and change in V2 on AMO, just start enforcing it for all new addons and updates to addons.

I don't think this is necessary, it's much simpler and cleaner to just do the check in the linter for MV3, and in our schema for the firefox side.

Rob--W commented 3 years ago

MV3 provides an "easy" time to do it, but if it's a problem then I don't see why we couldn't deprecate and change in V2 on AMO, just start enforcing it for all new addons and updates to addons.

Note that the linter rejects invalid version numbers. AMO doesn't sign add-ons that doesn't pass the validator, IIRC.

mixedpuppy commented 3 years ago

MV3 provides an "easy" time to do it, but if it's a problem then I don't see why we couldn't deprecate and change in V2 on AMO, just start enforcing it for all new addons and updates to addons.

I don't think this is necessary, it's much simpler and cleaner to just do the check in the linter for MV3, and in our schema for the firefox side.

If we can wait a year for GA of V3, then 1-2 more years for transition, then I'd suggest that this isn't really a problem to solve.

eviljeff commented 3 years ago

if we agree it's a good idea in general, so will be using the Chrome version string format for MV3, then what's really blocking us from enforcing it straight away for new uploads for MV2? As long as we maintain the existing version comparison code for a certain period of time then upgrades will still work as expected.

When we do finally simplify the comparison code in AMO/Firefox, if we wanted to we could identify problem versions that would fail in some way ahead of time and make a decision about what to do with them at that point.

mixedpuppy commented 3 years ago

what's really blocking us from enforcing it straight away for new uploads for MV2

nothing technical, AMO can enforce it, and it will work in firefox as-is.

When we do finally simplify the comparison code in AMO/Firefox

The (version compare) code in firefox wont change, at least not by us.

jvillalobos commented 3 years ago

Do we support version_name? That's a mitigation Chrome has to support alpha/beta versions when presenting them to users. If we don't have that, then things would get complicated for devs that have multiple update channels for their add-ons.

Rob--W commented 3 years ago

Do we support version_name?

We don't.

eviljeff commented 3 years ago

Do we support version_name? That's a mitigation Chrome has to support alpha/beta versions when presenting them to users. If we don't have that, then things would get complicated for devs that have multiple update channels for their add-ons.

We could implement support? Doesn't seem complicated - just fiddly to make sure we're exposing it everywhere.

Rob--W commented 3 years ago

We recently encountered an issue where langpacks meant for a dotrelease were not propagated to users who had already received the initial version. For details, see https://bugzilla.mozilla.org/show_bug.cgi?id=1732676.

Some findings from investigating that issue:

AMO should ideally limit reject versions whose numbers are 2^31-1. For comparison purposes, we can treat larger values as 0 since Firefox does the same.

eviljeff commented 3 years ago

AMO (addons-server) and the linter both use a very simplistic method of parsing the version strings that end up just ignoring parts of the string that don't fit in the format they understand. e.g. on addons-server, for version comparison purposes, 92.0buildid20210903235534 is effectively parsed as just 92.0b (as would 92.0buildid20210922161155).

When a numeric part is captured the max is 65535 (2^16 -1). Given the alternatives of making the code on AMO much more complicated to match Firefox to handle a few edge cases; or restricting the version strings further (if not going all the way to Chrome's version string format) I don't see much benefit in the former.

If the lang packs in https://bugzilla.mozilla.org/show_bug.cgi?id=1732676 weren't using valid toolkit version strings (I don't think they are? but it's complicated) then IMO it's to be expected the behaviour is undefined/unpredictable.

Rob--W commented 3 years ago

@eviljeff Is it possible to see the distribution of version numbers on AMO?

What happens when a large version number is used, and what would you like to see instead?

In the near term we'd like to find a limit and enforce a limit in the addons-linter. For now there are two limits, 31-bit (Firefox) and 16-bit (AMO server), and I'd like to explore which direction we should be heading towards.

eviljeff commented 3 years ago

@eviljeff Is it possible to see the distribution of version numbers on AMO?

everything is in redash but there are 3.2m versions of add-ons (358k unique version strings) so it would take a little work to get some useful data out of it. What are you looking for?

What happens when a large version number is used, and what would you like to see instead?

In most cases it wouldn't matter. AMO doesn't try to do much with the version string - it's just checked for uniqueness (literal string uniqueness, where the format doesn't matter) and compared in some cases, e.g. when working out what versions should be blocked if an add-on has been added to the blocklist. It's the latter case where having a format that isn't understood would potentially cause versions to be matched when they shouldn't be, or the admin wouldn't be able to specify an exact version string to be blocked, but over matching isn't really a problem in reality (the vast majority of blocks are all versions; in other cases we can communicate with the developer if necessary).

Afaik on the linter a large number would be rejected.

In the near term we'd like to find a limit and enforce a limit in the addons-linter. For now there are two limits, 31-bit (Firefox) and 16-bit (AMO server), and I'd like to explore which direction we should be heading towards.

imo, definitely 16 bit. There isn't any use case for such a single large part of a version that can't be refactored into part.part.part.part

escapewindow commented 2 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1733396#c1

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

wagnerand commented 2 years ago

Fwiw, this is blocked on Product (@mconnormoz), see also https://mozilla-hub.atlassian.net/browse/FIDE-620.