372302 - Plus sign not allowed in URLs - prevents downloads

Status: RESOLVED FIXED

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

Description

[Wladimir Palant]

The XSS check in /app/config/bootstrap.php doesn't allow the plus sign. It is pretty common in add-on names however because of app compatibility info (ff+tb). Since the add-on name is part of the download name on trunk now (bug 370614) it will prevent some add-ons from downloading. Try the URL given above - it will simply redirect to the main page.

I would suggest changing the XSS check into:

if (array_key_exists('url',$_GET) && preg_match('/[^\w\d\/\.\-_:+]/',$_GET['url'])) {

This regular expression should be faster than /^[\w\d\/\.\-_:+]+$/.

Comment 1

[Andrei Hajdukewycz [:sancus]]

Actually, we need preg_match('/[^\w\d\/\.\-_: ]/',$_GET['url']) because +'s are converted into spaces in URLs if unencoded.

This would be a bigger problem if we were actually using those filenames for something, because it seems Cake decodes even encoded(%2B) +'s somewhere and turns them into spaces, which sucks, but thankfully doesn't matter for this.

Comment 2

[Wladimir Palant]

Reopening this bug because of bug 373514 comment 4 - parentheses in file names have the same effect. What are actually the allowed characters in file names?

Comment 3

[Fred Wenzel [:wenzel]]

Well, see the regex above. We probably have to add parentheses there as well.

Comment 4

[Wladimir Palant]

Adding parentheses is easy - but maybe we should check what other characters we need to allow. But from the look of it only spaces are removed. This is bad, it should be a whitelist rather than a blacklist - e.g. replace [^\w\-.()] by underscore (ideally this should be fixed for already existing add-ons as well).

Comment 5

[Fred Wenzel [:wenzel]]

Actually, the URL filter *is* a whitelist. But I agree that this should be fixed at upload time. Otherwise people will, like now, upload files with other characters and wonder why they are not downloadable.

CCing morgamic because old (migrated) files and their names are affected.

Comment 6

[Wladimir Palant]

I was talking about the file name filter for uploads - right now the only character we explicitly don't allow is a space (unless there is a more strict filter for extension names) and this raises security concerns.

Comment 7

[Oliver Saier]

(In reply to comment #4)
> Adding parentheses is easy

Then could you please do it, so that my 10-day old extension can eventually be downloaded? Thanks :D

Comment 8

[Wladimir Palant]

Now that bug 374057 is fixed, can we get this one fixed as well? I would suggest using the same regexp (with the exception of /) for file name construction as the one used for XSS prevention. Meaning that in function moveFiles in /app/controllers/components/developers.php we should have:

$baseFilename = preg_replace('[^\w\d\.\-_:+]', '_',
                             $addon['Translation']['name']['string']);

Since you probably don't want to fix the file names for all existing files (or at least not now), the same transformation should be applied to $file['filename'] in /app/views/elements/install.thtml.

Comment 9

[Fred Wenzel [:wenzel]]

Good call, thanks. I implemented a new constant for this regex and used it in the two places you suggested.

Comment 10

[Justin Scott [:fligtar]]

Filenames now have spaces in them and other characters that shouldn't be there. Something here isn't working.

Comment 11

[Justin Scott [:fligtar]]

-define('INVALID_FILENAME_CHARS', '[^\w\d\.\-_:+]');
+define('INVALID_FILENAME_CHARS', '/[^\w\d\.\-_:+]/');

Comment 12

[Christian Höltje]

I have a similar problem with my download link:
https://addons.mozilla.org/en-US/firefox/addon/4125

It has a ' in it...
https://addons.mozilla.org/en-US/firefox/downloads/file/14066/it's_all_text!-0.6.0-firefox+fl.xpi

Is there a work around?

Ciao!

Comment 13

[Christian Höltje]

I meant to re-open it before.  The sanitazation isn't complete because ' and ! are left in the url.

BTW:  A workaround for this is to change the download URL from 
https://addons.mozilla.org/en-US/firefox/downloads/file/14066/it's_all_text!-0.6.0-firefox+fl.xpi
to
https://addons.mozilla.org/en-US/firefox/downloads/file/14066/its_all_text-0.6.0-firefox+fl.xpi

But that is manual.

Hmm...links cannot download the URL as is, either.

Ciao!

Comment 14

[Wladimir Palant]

Frederic, either you forgot to change install.thtml or this change has been backed out - I can only see INVALID_FILENAME_CHARS used in developers.php.

Comment 15

[Andrei Hajdukewycz [:sancus]]

hm it's possible that I broke this somehow with the number of changes I was making to install.thtml

Comment 16

[Andrei Hajdukewycz [:sancus]]

OK, the original preg_replace in r2841 was typo'd so I guess that's why I removed it but I didn't replace it with a working one, fixed now in r2952.

Comment 17

[Fred Wenzel [:wenzel]]

Thanks, sancus. Did it fix this bug?

Comment 18

[Christian Höltje]

I haven't tested this yet, because I haven't uploaded a new version of my extension, yet.  I changed my addon's name back to include ' and !.

There is still one step needed to close this out.  Old download links are still in the DB which are invalid...

Example:
https://addons.mozilla.org/en-US/firefox/addons/versions/4125
version 0.6.0
Shows:
https://addons.mozilla.org/en-US/firefox/downloads/file/14066/it's_all_text!-0.6.0-firefox+fl.xpi

Ciao!

Comment 19

[Wladimir Palant]

This change hasn't been merged into the production branch yet.

Comment 20

[Christian Höltje]

Okay.  Two questions then:

1) When will it?
2) It will fix older links like the above?

Thanks.

Ciao!