471891 - non-ascii characters are converted to underscores in XPI filenames

Status: RESOLVED FIXED

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

Description

[Dave Garrett]

https://addons.mozilla.org/en-US/firefox/addon/3867/
http://ftp.mozilla.org/pub/mozilla.org/addons/3867/

Note the XPI filenames of the most recent couple of versions of the Foxkeh pseudo-theme add-on. It's mostly underscores. The name in the install.rdf is "フォクすけといっしょ", so I'm pretty sure that it's Unicode getting mangled here. This effect is also evident with the add-on noted for bug 376009.

These files can be downloaded and installed just fine. The problem is purely that they have broken filenames. Either we should let developers name things as they want and have them work, or rejections should require the name be changed to something else that's allowed and useful.

(I feel like I've got a decent chance that this is a dupe, but I can't find it; also, this doesn't really fit under any of the AMO components well...)

Comment 1

[Fred Wenzel [:wenzel]]

That's (currently) intended behavior, as we are restricting the characters used in (some) URLs.

Might be fixed in the 4.x refactoring though.

Comment 2

[Wil Clouser [:clouserw]]

We're addressing this right now in the new code.  The latest proposal is to remove the name altogether, so on disk you'd have:

$addonid/$version-$platform-$app.xpi

The example from comment 0 would be:

3867/0.1.7-fx-win.xpi

When we serve the file to the user, we'll add the Content-Disposition header with filename=$name, where $name is the name of the add-on (from the database) prepended to the actual filename with the version, app, and platform info.

Any objections to the above?

Comment 3

[Justin Scott [:fligtar]]

I would rather not remove add-on names from the filenames entirely just because of this bug. Can't we keep the names and still change the disposition name to something pleasant?

Comment 4

[Wil Clouser [:clouserw]]

We can certainly change the disposition to whatever we want, but you're saying you'd like to keep the underscores in the filenames?

(In actuality, we could use our slugify() function which is more lenient on what it lets through, and removes what it doesn't, but the point is still the same - you're going to have filenames that don't match what people enter)

Comment 5

[Dave Garrett]

(In reply to comment #2)
> $addonid/$version-$platform-$app.xpi
> ...
> 3867/0.1.7-fx-win.xpi

Um, slash in a filename? Did you really mean that or were you proposing a folder be created? Very few file systems can stand slashes in filenames.

Also, your example swapped the platform and app. ($platform-$app would be win-fx not fx-win) Which one is the proposed?

I'm good with $addonid-$version-$platform-$app.xpi so long as the platform is only in there when the file specifies something other than "all" supported.

Comment 6

[Wil Clouser [:clouserw]]

(In reply to comment #5)
> (In reply to comment #2)
> > $addonid/$version-$platform-$app.xpi
> > ...
> > 3867/0.1.7-fx-win.xpi
> 
> Um, slash in a filename? Did you really mean that or were you proposing a
> folder be created? Very few file systems can stand slashes in filenames.
> 
> Also, your example swapped the platform and app. ($platform-$app would be
> win-fx not fx-win) Which one is the proposed?
> 
> I'm good with $addonid-$version-$platform-$app.xpi so long as the platform is
> only in there when the file specifies something other than "all" supported.

I'm proposing the add-on id is the directory, as it is today.  I'd keep the platform and app in the same places they are today, so, app then platform.  The rules for adding platform would remain the same (something other than all).

Comment 7

[Dave Garrett]

Oh, you're meaning directory on the server, not the filename for the user. If that's the case, then my misunderstanding. This bug was filed about what the user downloads so I assumed you meant that would be what you'd get from clicking the download button. So the actual filename would not have the addon ID then. Gotcha.

Comment 8

[Jeff Balogh (:jbalogh)]

(In reply to comment #2)
> When we serve the file to the user, we'll add the Content-Disposition header

We're getting a fileserver and won't be using Content-Disposition anymore (bug 590821).

Comment 9

[Wil Clouser [:clouserw]]

(In reply to comment #8)
> (In reply to comment #2)
> > When we serve the file to the user, we'll add the Content-Disposition header
> 
> We're getting a fileserver and won't be using Content-Disposition anymore (bug
> 590821).

I was hoping Content-Disposition could roll through a redirect, but I just tested it and it looks like it doesn't.  So, are there other options for making the filename pretty?  Otherwise we're stuck with what's on disk.

Comment 10

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

slug of addon name...+all the other stuff

Comment 11

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

So in master this is what happens:


In [1]: import tower
In [2]: tower.activate('ja')
In [3]: a = Addon.objects.get(pk=3867)

In [4]: a.name
Out[4]: <Translation: フォクすけといっしょ>

In [5]: a.current_version.files.get().generate_filename()
Out[5]: u'\u30d5\u30a9\u30af\u3059\u3051\u3068\u3044\u3063\u3057\u3087-0.1.7-fx-windows.xpi'

Which will make a pretty filename with Japanese characters.

Here's a functional test for good measure:

http://github.com/jbalogh/zamboni/commit/60881f3

Comment 12

[Vishal Kamdar [:vish_moz]]

According to Dave Dash, this bug is not testable until devtools is done. Marking [qa-] for now.