Plus sign not allowed in URLs - prevents downloads

RESOLVED FIXED

Status

addons.mozilla.org Graveyard
Public Pages
--
critical
RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: Wladimir Palant, Assigned: wenzel)

Tracking

Details

(URL)

(Reporter)

Description

12 years ago
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\/\.\-_:+]+$/.
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 2

11 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

11 years ago
Blocks: 373958
(Assignee)

Comment 3

11 years ago
Well, see the regex above. We probably have to add parentheses there as well.
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

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

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

Comment 8

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

Updated

11 years ago
Assignee: nobody → fwenzel
Status: REOPENED → NEW
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

11 years ago
Good call, thanks. I implemented a new constant for this regex and used it in the two places you suggested.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
Filenames now have spaces in them and other characters that shouldn't be there. Something here isn't working.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
-define('INVALID_FILENAME_CHARS', '[^\w\d\.\-_:+]');
+define('INVALID_FILENAME_CHARS', '/[^\w\d\.\-_:+]/');
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 12

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

11 years ago
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!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 14

11 years ago
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.
hm it's possible that I broke this somehow with the number of changes I was making to install.thtml
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.
(Assignee)

Comment 17

11 years ago
Thanks, sancus. Did it fix this bug?
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 18

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

Comment 19

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

Comment 20

11 years ago
Okay.  Two questions then:

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

Thanks.

Ciao!
Duplicate of this bug: 376009
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.