333391 - install.php does not accept certain characters in URI argument

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect, P1)

Description

[James Ross]

This is a bug on behalf of AdvancedDork on IRC.

It appears that the download count for the Advanced Dork extension is refusing to budge. As best I can tell, this is becuase it is failing to match the download URI with that in the DB.

The URL for counting the download returns "nope":
https://addons.mozilla.org/install.php?uri=http://releases.mozilla.org/pub/mozilla.org/extensions/advanced_dork_/advanced_dork_-2.0%20beta-fx.xpi

There's at least two things I can think of that might be affecting this:
  - the colon in the name, although this has been here since before
  - the version number is "2.0 Beta"

According to imperical evidence, the download count stopped going up around the same time version 2.0 Beta was approved.

Comment 1

[Michael Morgan [:morgamic]]

Updating title -- this is not specific to one extension.

Comment 2

[Michael Morgan [:morgamic]]

I found that the problem exists in the developer control panel, where additem.php and approval.php manipulate and create filename strings.  Versions were assumed to be numeric, so the space screws things up -- since the URL is not decoded/encoded in install.php due to those assumptions.

See:
http://lxr.mozilla.org/update1.0/source/developers/additem.php#683

So -- the fix has multiple parts:
* Fix the preg_replace() to work on the entire string, not just parts of it.
* Fix install.php to accomodate the bug (in case older entries still have it).
* Fix additem.php to reject versions that have non-numeric strings?

How does that sound?

Comment 3

[James Ross]

I believe alpha characters in versions are allowed by Firefox 1.5, although I'm less certain that spaces should be accepted.

Comment 4

[Michael Morgan [:morgamic]]

Okay -- been staring at this for a while and here is what I found:

(In reply to comment #2)s:
> * Fix the preg_replace() to work on the entire string, not just parts of it.
This is not ideal, so this won't happen.

> * Fix install.php to accomodate the bug (in case older entries still have it).
This also will not happen, because we can't differentiate between the \s, + or %20 between apps and the one found in the version.  If we scrubbed for %20 --> + then our query would still fail.  urlrawdecode() also would not work.

> * Fix additem.php to reject versions that have non-numeric strings?
This is the proper fix, so I will modify additem.php (sad face) so that versions for extensions cannot contain spaces.  If a developer submits an extension with a space in the version, it will be rejected.

Comment 5

[Michael Morgan [:morgamic]]

Attachment: Reject extension versions from install.rdf that contain spaces

Patch in additem.php to reject improper versions that would break their install URI's.

Addons up to this point that have had spaces in their versions should resubmit versions without spaces.

There is no short-term dirty hack fix for this, since the +'s in the URI will conflict with any changes we could make to install.php itself -- see previous comments.

Comment 6

[James Ross]

Sounds good. The Advanced Dork extension has been resubmitted/admin-edited to have no space in the version, and it is now counting downloads happily.

Comment 7

[:Gijs (away/busy until Mar 10)]

So, erm, shouldn't this be marked FIXED?

Comment 8

[Michael Morgan [:morgamic]]

*** Bug 290637 has been marked as a duplicate of this bug. ***

Comment 9

[Mike Shaver (:shaver emeritus)]

Yeah, FIXED.