If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in mozilla2.0b8

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: jorgev, Assigned: mossop)

Tracking

({regression})

Trunk
mozilla2.0b8
regression
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Forgot to mention this works fine on 3.6.
blocking2.0: --- → ?
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Updated

7 years ago
Summary: Hash validation fails for download redirecting to FTP → InstallTrigger.install accepts null as a URL and attempts to download the current page as an extension
(Reporter)

Comment 3

7 years ago
(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.
(Assignee)

Comment 4

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

Updated

7 years ago
Duplicate of this bug: 612649
(Assignee)

Comment 6

7 years ago
Going to block on this, looks like we should check that the url and iconurl aren't undefined
Assignee: nobody → dtownsend
blocking2.0: ? → beta9+
(Assignee)

Updated

7 years ago
Summary: InstallTrigger.install accepts null as a URL and attempts to download the current page as an extension → InstallTrigger.install accepts undefined as a URL and attempts to download the current page as an extension
(Assignee)

Comment 7

7 years ago
Created attachment 492762 [details] [diff] [review]
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.
Attachment #492762 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
Whiteboard: [has patch][needs review rs]
(Assignee)

Comment 8

7 years ago
Created attachment 492787 [details] [diff] [review]
patch rev 1

Failed to include the test file
Attachment #492762 - Attachment is obsolete: true
Attachment #492787 - Flags: review?(robert.bugzilla)
Attachment #492762 - Flags: review?(robert.bugzilla)
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
Attachment #492787 - Flags: review?(robert.bugzilla) → review+
Whiteboard: [has patch][needs review rs] → [has patch]
(Assignee)

Comment 10

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/d744f676a758
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8

Comment 11

7 years ago
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)
blocking2.0: beta9+ → betaN+
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.
Keywords: regression
Version: unspecified → Trunk

Comment 13

7 years ago
http://wmlbrowser.mozdev.org/installation/testcase.html should have the old installation code, so it should be reproducible from there I would have thought.
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?
(Assignee)

Comment 15

7 years ago
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

7 years ago
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.
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.