613568 - InstallTrigger.install accepts undefined as a URL and attempts to download the current page as an extension

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

This was reported in the Add-ons Forum: https://forums.mozilla.org/addons/viewtopic.php?f=7&t=1437&p=5300#p5300

Steps to reproduce:
1) Go to http://wmlbrowser.mozdev.org/installation/wmlbrowser.html
2) Click on the link to install.

Result:
Installation fails with an error. The Error Console shows:
Warning: WARN addons.xpi: Download failed: Downloaded file hash (4ee8e91e2b2151826a4c3ecc6b8e355f4fa606e61ce7037b386e449884910962) did not match provided hash (e52d9e020f2daceb197488b67fc2da01bf3d08778eda41a0a162722a3d48fe92)

Expected result:
The add-on installs correctly.


If you download the file and check the hash, it is correct. The link points to http://downloads.mozdev.org/wmlbrowser/wmlbrowser-0.7.25.xpi, which in run redirects to an FTP site, which is probably the reason this is failing.


Tested on Firefox 4 beta 7 on Mac OS.

Comment 1

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

Forgot to mention this works fine on 3.6.

Comment 2

[Dave Townsend [:mossop]]

Actually the hash is broken for me (perhaps the mirrors have differing content or something?):

lorenz ~$ wget http://downloads.mozdev.org/wmlbrowser/wmlbrowser-0.7.25.xpi
--2010-11-19 12:53:01--  http://downloads.mozdev.org/wmlbrowser/wmlbrowser-0.7.25.xpi
HTTP request sent, awaiting response... 302 Found
Location: http://mirror.umoss.org/mozdev/wmlbrowser/wmlbrowser-0.7.25.xpi [following]
--2010-11-19 12:53:01--  http://mirror.umoss.org/mozdev/wmlbrowser/wmlbrowser-0.7.25.xpi
HTTP request sent, awaiting response... 200 OK
Length: 92049 (90K) [application/x-xpinstall]
2010-11-19 12:53:03 (135 KB/s) - “wmlbrowser-0.7.25.xpi” saved [92049/92049]

lorenz ~$ openssl dgst -sha1 wmlbrowser-0.7.25.xpi 
SHA1(wmlbrowser-0.7.25.xpi)= edcbf423a9a043d825a1ddd0d814ddfd5c3343a5

The real problem though is that the webpage is broken, because there is a <span> inside the link that is what is being clicked on when the event handler is called, so aEvent.target.href is the href property of the span, not the link, probably giving null. In Firefox 3.6 InstallTrigger decides that this is an illegal call giving the error:

Error: Incorrect arguments to InstallTrigger.Install()
Source File: http://wmlbrowser.mozdev.org/installation/wmlbrowser.html
Line: 129

3.6 falls back to just following the link when the InstallTrigger call throws which is why it appears to work, it just isn't doing a hash check there at all.

In 4.0 it looks like we're assuming null is ok and using it as a relative URL from the main page and so it's trying to download and install http://wmlbrowser.mozdev.org/installation/wmlbrowser.html as the XPI (which of course fails the hash check).

Since this is caused by a bogus InstallTrigger call I suspect it doesn't block, both 3.6 and 4.0 behaviours are kind of sane. But I'm going to look at just what restrictions 3.6 placed on the urls passed and see if it makes sense to reproduce them in 4.0.

Comment 3

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

(In reply to comment #2)
> Actually the hash is broken for me (perhaps the mirrors have differing content
> or something?):
> lorenz ~$ openssl dgst -sha1 wmlbrowser-0.7.25.xpi 
> SHA1(wmlbrowser-0.7.25.xpi)= edcbf423a9a043d825a1ddd0d814ddfd5c3343a5

Actually, the dev is using SHA256, not SHA1. Either way, the hash computed by Firefox does match either of those.

Comment 4

[Dave Townsend [:mossop]]

Err yes. The hash is correct so it should work fine if the webpage is fixed.

Comment 6

[Dave Townsend [:mossop]]

Going to block on this, looks like we should check that the url and iconurl aren't undefined

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 1

The old code actually used JSVAL_IS_VOID on the properties which tests whether they are undefined, null gets accepted so this just duplicates that and tests the main URL case.

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Failed to include the test file

Comment 9

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

Comment on attachment 492787 [details] [diff] [review]
patch rev 1

>+++ b/toolkit/mozapps/extensions/test/xpinstall/browser_badargs2.js
>@@ -0,0 +1,27 @@
>+// ----------------------------------------------------------------------------
>+// Test whether passing aundefined url InstallTrigger.install throws an
nit: an undefined url causes InstallTrigger.install to throw

Comment 10

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/d744f676a758

Comment 11

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, is there any way for me to manually verify this fix? I tried to reproduce with a beta7 build on OS X but wasn't successful in doing so.

Comment 13

[Matthew Wilson]

http://wmlbrowser.mozdev.org/installation/testcase.html should have the old installation code, so it should be reproducible from there I would have thought.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sadly not, and I'm testing with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7) Gecko/20100101 Firefox/4.0b7. Could this be related to the mirror I'm getting?

Comment 15

[Dave Townsend [:mossop]]

A fairly simple test would be to just load a webpage and then put this into the url bar it should display an error in the error console about a missing URL property:

javascript:InstallTrigger.install({addon: { URL: undefined }});

Comment 16

[Matthew Wilson]

Reproducible for me in Mozilla/5.0 (X11; Linux i686; rv:2.0b7) Gecko/20100101 Firefox/4.0b7 at http://wmlbrowser.mozdev.org/installation/testcase.html, assuming Javascript is enabled, selecting either link.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with the hint from comment 15 with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre ID:20110209030359