275529 - Extension Manager does not reject invalid GUIDs

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jens Bannmann]

Currently, the Extension Manager does enforce valid version numbering for
packages, but not valid GUIDs. There are packages out there (also listed on
update.mozilla.org) that use stuff like {noia_yop-76b1-4bcf-aff9-90e1a5_win32},
which clearly violates the specification. This should be fixed soon, before more
authors rely on EM's misbehaviour.

Steps to reproduce:
1. Install the "Open link in..." extension from
https://addons.update.mozilla.org/extensions/moreinfo.php?application=firefox&id=379&vid=1211

Actual results:
Firefox installs the package without complaining.

Expected results:
Firefox should reject the package, because it uses the invalid GUID
"{openlink-0fa3-4886-8adf-24a3e6301ec5}".

Patch coming up.

Comment 1

[Jens Bannmann]

Attachment: patch for nsExtensionManager.js.in and extensions.properties (in locales/en-US)

Proposing patch. Should the malformed GUID should be shown to the user? With
this patch, the text wrapping in the error message looks a bit ugly.

Comment 2

[Wolf [:wolf]]

I seriously hope the Firefox team wontfixes this bug.

Comment 3

[Wolf [:wolf]]

IMO, the specification intends <em:id> to be globally unique value, a GUID
simply furthers that cause because of it's low chances of two authors using the
same one, (w/o copy/paste errors anyway). I don't see a valid reason why the
unique identifier must be a valid GUID, when Firefox doesn't depend on it being
valid, it depends on it being unique. Its used for the directory name under
extensions/ in user's profiles and in communications with the extension/theme
update service. The root issue here is obviously that some authors find the GUID
difficult to remember and find that adding human-readble aids to their items ID
assists them in development. If that causes no problems, then I don't see why
the ID can't be flexable, as it is now.

Though more importantly, the extensions/themes that users have installed..
ignoring authors and developers for a moment, are going to have their upgrade
path broken as far as E/T Update is concerned if the new items suddenly are
forced into having new GUIDs. With regard to UMO, the DB changes will cause the
old items to be not-found and therefore the users will still have versions that
will be defunct with Firefox 1.1, if they upgrade. and smooth migration will
fail to find an updated version, even if it exists. If authors update their
custom update.rdfs, the same should occur there as well. Is this change worth
breaking the update paths for the installed-user base for those items? 

And... Before you say the authors were at fault for using invalid GUIDs and
so-be-it. Consider the user who has no clue what a GUID is at all, who expects
things to work as-advertised. Without breakage caused by internal struggles over
the meaning of a specification within the organization that created the browser
they use.

Comment 4

[Jens Bannmann]

I think some numbers would help here - whether we're talking about 10 or 100
affected packages could make a huge difference. Would you mind doing a statistic
about valid/invalid GUIDs in update.mozilla.org's database? (The appropriate
regex is at bug 271270 comment 4, but I'd be happy to help, provided you put up
a DB export containing all package IDs somewhere).

Comment 5

[alanjstr]

Valid to invalid is approximates 20:1, last time I poked this issue.

Comment 6

[O. Atsushi (Torisugari)]

IMHO, notify and continue (or confirm) install is
the way to go, instead of screening out all
malformed ids.

(In reply to comment #1)
> Created an attachment (id=169284) [edit]

As long as ids are case sensitive,
it's not "clear-cut specification", anyway.
We can install
{DF8E5247-8E0A-4de6-B393-0735A39DFD80} and
{df8e5247-8e0a-4de6-b393-0735a39dfd80} at the same time.

Comment 7

[James Ross]

GUIDs/UUIDs are, by definition, not case-sensitive. Whether the EM handles them
in a case-insensitive way is another matter, though... I'd love to try
installing two extensions with IDs differing only by case on a computer with a
case-insensitive file system. ;)

Comment 8

[Jens Bannmann]

Attachment: patch matching new revision 1.75 of nsExtensionManager.js.in

Comment 9

[Przemyslaw Bialik]

Could this bug be extended to also reject extensions which GUID is the same as
GUID which belong to the one of mozilla applications (extra: nvu, netscape,
'new' mozilla suite...).
The warning could be sth like:
"%S could not install "%S" because its ID ("%S") is used by "%S". Please contact
the author about this problem."
I saw at least one case when author used FF GUID for his extension ID (either by
a mistake or lack of knowledge) and imagine what would happen if user would
install 2 such 'buggy' extensions...
Or I should file another bug ?

Comment 10

[Marius Gundersen]

A GUID database for all extensions and themes would be an amazing. I has 
happened that someone used the same GUID as another extension and a searchable 
database would be a good idea. I could set up this on my server with a simple 
form to add the GUID, the name of the extension/theme, the type (theme for 
Firefox/Thunderbird...), The author and a link to a place where I could 
download it. Then there could be a regex search field so you could check if 
the GUID is in use.

Comment 11

[Benjamin Smedberg]

It's a GUID... if you're using a guid-generating tool, the chances of a conflict
are in the 1/many billion range. But I'm leaning towards allowing these IDs to
be pretty strings (and even suggesting that authors use contractid-like strings):

@myextension.mozdev.org/1

I still need to work out some details with Ben about directory paths in the EM.

Comment 12

[Jens Bannmann]

(In reply to comment #11)
> I'm leaning towards allowing these IDs to be pretty strings

Benjamin: I'm looking forward to having a final answer on the GUIDs-or-not
question soon, as it is a much discussed topic. Oh, and if you and Ben go for
pretty strings, I'd propose to have them not fully free-form, but with some
rules (e.g. startswith/contains @, "domain" with at least one dot, valid tld, at
least one slash etc.) This would prevent people from just using something simple
as "bettertabs".

(In reply to comment #10)
> A GUID database for all extensions and themes (...)

Marius, this bug is about rejecting *invalid* (meaning malformed) GUIDs, not
*conflicting* GUIDs. As Benjamin wrote, proper usage of GUID-generating tools
results in a near-zero possibility of collisions.

(In reply to comment #9)
> Could this bug be extended to also reject extensions which GUID is the same as
> GUID which belong to the one of mozilla applications

Przemyslaw, this should be a separate bug. However, I guess it's very likely to
be WONTFIXed, as it either requires a hard-coded list of the GUIDs of other
products (which will never include the whole set), or an on-line service, which
is overkill. (OTOH, you can just try filing it, I'm not the one who decides)

Comment 13

[alanjstr]

Please keep in mind that there already is a database of all of these GUIDs;
they're in UMO.

If you change the size of the GUID to allow it to be longer, it will impact UMO.
 The nice thing about GUIDs is that they follow a specific format, are of
constant length, and are easily generated.

If the concern is the name of the folder on disk, then maybe that shouldn't be
based on the GUID.  Perhaps some name space can be used and all extensions from
one author could go into that same namespaced folder.

Comment 14

[Benjamin Smedberg]

Alan, the concern is not the filename on disk, it's the fact that GUIDs are not
readable. If UMO has fixed-length fields, I will need to talk to them about
making it a varchar or somesuch instead. UMO currently holds invalid GUIDs, so
I'm pretty  sure that they aren't doing rigorous guid checking.

Comment 15

[alanjstr]

You're referring to bug 271270 and bug 273550

Comment 16

[Jens Bannmann]

(In reply to comment #14)
> If UMO has fixed-length fields, I will need to talk to them about
> making it a varchar or somesuch instead. UMO currently holds invalid GUIDs, so
> I'm pretty  sure that they aren't doing rigorous guid checking.

The GUID field is a varchar(50), so no problems here.

Note that the request to change UMO so it enforces valid GUIDs - bug 271270 - is
blocked by this bug.

Comment 17

[Przemyslaw Bialik]

This bug was fixed by a checkin for bug 286034 .

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050426
Firefox/1.0+

Comment 18

[alanjstr]

(In reply to comment #17)
> This bug was fixed by a checkin for bug 286034 .

Can you point to the section in LXR that does this?

Comment 19

[Benjamin Smedberg]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.93&root=/cvsroot&mark=163-166,927,960,2924#160

We're still discussing whether or what we want to enforce.

Comment 20

[Ria Klaassen (not reading all bugmail)]

Would be nice: 
{forecast-7E9B-4d49-8831-A227C80A7AD3}
{prefbutt-E113-4f5f-B052-EDB33FE943FF}
and everyone would be happy (there's not much choice between A and F to make the
extension more recognizable)!

Comment 21

[Benjamin Smedberg]

The two "examples" in comment 20 are very bad and should not be allowed: if you
have something GUID-like, it should be a GUID, generated properly by a guidgen
tool, not a hand-edited text string that is likely not to be unique.

My current thinking (after 1.1a) is to allow two forms of "id":

{GUID}                       (a real GUID)
@myextension.mozdev.org;1    (something vaguely contractid-like)

The second form would need to avoid characters that shouldn't be in filesystems,
so it will allow only the following characters:

a-z 0-9 - . @ , ; _

So I think a decent regex would be:
m/^(\{[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\}|\@[a-z0-9\.\-\@;_]+)$/i

Does this make sense?

Comment 22

[Ria Klaassen (not reading all bugmail)]

Another idea for an ID would be this fix form:

[e-mail]
[e-mail]
[e-mail]
externalapp@torisugari

I think this kind of human-readable ID is more user friendly than digits.
Just an idea. Please ignore this comment if not usable.

Comment 23

[Benjamin Smedberg]

The patch I posted in bug 293461 "fixes" this so that email-like IDs can be used
in addition to {GUID}s

Comment 24

[Henrik Gemal]

(In reply to comment #23)
> The patch I posted in bug 293461 "fixes" this so that email-like IDs can be used
> in addition to {GUID}s

could you please give an example of an email-like IDs

Comment 25

[Teune van Steeg]

[e-mail] works as ID

Comment 26

[Jens Bannmann]

Fixed by bsmedberg in bug 293461, resolving.

Comment 27

[Ria Klaassen (not reading all bugmail)]

Sorry, I wasn't aware of the new possibility (see comment #23 ) for a long time.
There are problems with extensions with human readable names:
All renamed extensions show up, also the chrome shows up, but most of them don't
(fully) work.

I tried renaming Search Button 0.4.8:
http://www.extensionsmirror.nl/index.php?showtopic=95  
Make Link 2.0.1: http://www.extensionsmirror.nl/index.php?showtopic=64  and
Paste Happy 0.6.1: http://www.extensionsmirror.nl/index.php?showtopic=531  

Only Paste Happy seems to work. Make Link works partially (only "Configure...")
and the Search Button does show up, but doesn't work.

Comment 28

[Ria Klaassen (not reading all bugmail)]

I see the same when installing an xpi via my desktop with a changed id: it
installs but doesn't work entirely.

I appreciate this new feature very much. Thanks for that. Makes it much easier
for me to edit extensions. 

Comment 29

[Ria Klaassen (not reading all bugmail)]

Only Makelink and Searchbutton seem to have problems, the rest was due to a
spoiled registration.