286765 - Download and download count is broken for extensions containing spaces in their name

Status: RESOLVED FIXED

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

Description

[Giorgio Maone [:ma1]]

A silly bug in install.php (related to the even more silly direct usage of
hardcoded full URIs in queries) breaks download count.
This bug doesn't break JavaScript install trigger (Firefox and Mozilla
extensions can be properly installed), but it breaks download by redirection, so
affected Thunderbird extensions can't be downloaded in an obvious way (see
https://addons.update.mozilla.org/extensions/moreinfo.php?id=170 ).

Comment 1

[Giorgio Maone [:ma1]]

Attachment: This one fixes the bug, removing an useless (and harmful) str_replace()

Comment 2

[Giorgio Maone [:ma1]]

Attachment: This one prevents a regression, properly encoding URIs passed to install.php

Since I wrote attachment 177902 [details] [diff] [review], I've been wondering why that strange
str_replace() had been coded in first place. Then the enlightment: a lot of
URIs containing the "+" character are passed to install.php *not encoded*, so
someone thought this had to be fixed converting spaces (which *may* come from
PHP automatic urldecoding of "+") back to "+". Obviously, in such a situation
you should patch the code that produces malformed URLs, rather than trying to
guess a sane one. And it happens that this URI building code, which used to be
scattered all around, recently has been refactored in a centralized function by
myself: yes, I overlooked the urlencoding bug when I moved that line, but
thanks God now there's only one place to patch :-)

Comment 3

[Giorgio Maone [:ma1]]

Notice that patch in attachment 177912 [details] [diff] [review] is a diff on a function born in
attachment 172144 [details] [diff] [review], hence the dependency on bug 275900 which is fixed but whose
patches are not on the live site yet.

Comment 4

[Michael Morgan [:morgamic]]

Comment on attachment 177912 [details] [diff] [review]
This one prevents a regression, properly encoding URIs passed to install.php

Looks good.  

In the future, could we use single quotes instead of double quotes, and cat the
strings (or at least use {$filename}).	It makes the code easier to read.

Comment 5

[Michael Morgan [:morgamic]]

Comment on attachment 177902 [details] [diff] [review]
This one fixes the bug, removing an useless (and harmful) str_replace()

Looks good - thanks, mao.

Again, the single vs. double quotes thing - $_GET['uri'] is a tad more proper
than $_GET["uri"] since 'uri' is not a variable or constant but an associative
index.	Not a big deal, but a good thing to know.

Comment 6

[Mark Edgington]

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

Comment 7

[Colin Ogilvie [:cso]]

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

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Please un-bitrot the patches.

Comment 9

[Giorgio Maone [:ma1]]

Attachment: unbitrots previous patch 177912

unbirotting as requested

Comment 10

[Michael Morgan [:morgamic]]

Check in to branch.