136670 - [FIX]Content-Type header malformed on form-submit

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Marc Lehmann]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.9) Gecko/20020310
BuildID:    2002031008

For file-uploads, mozilla submits malformed content-types for some
content-types. Uploading a html files gives:

Content-Type: text/html

(correct), but uploading an .mpg gives:

Content-Type: type=video/mpeg

which (at least according to rfc2045, i haven't checked others), is malformed.

While we are at it: mozilla makes it impossible to develop internationalized
applications because it doesn't submit charset=-attributes for uploaded fields,
which make charset detection a guessing game (it's not possible to differentiate
ebtween windows-1258 and latin1 for example). This is only a "should" in the
html-recommendation, so this is not really a bug ;)

Reproducible: Always
Steps to Reproduce:
1. create a file upload form
2. upload a .mpg file (or a text file)
3. watch the content-type header in the form data

Actual Results:  Content-Type: type=video/mpeg

or (for text fields, not only file-uploads)

Content-Type: text/html
Content-Type: text/plain

Expected Results:  Content-Type: video/mpeg

or (for text fields, not only file-uploads, examples only)

Content-Type: text/html; charset=ISO-8859-1
Content-Type: text/plain; charset=UTF-8
Content-Type: text/plain; charset=UNKNOWN

http://rfc2045.x42.com/
http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2

Comment 1

[Darin Fisher]

-> jkeiser

Comment 2

[Boris Zbarsky [:bzbarsky]]

Could you please attach your /etc/mime.types file to this bug?  I strongly
suspect that you have a malformed entry in that file.

As for the charset on text/* types, that is covered by other bugs (apparently
some common web servers have major conniptions when it's present).

Comment 3

[John Keiser (jkeiser)]

charset is a different bug; webservers break right and left when we enable that
apparently :)

Comment 4

[Marc Lehmann]

Well, there are two alternatives for the charset issue:

1. some webservers are broken, therefore mozilla also must be broken
   in a way preventing internationalised applications from ever working.
   the microsoft way of fixing things ;)
2. mozilla gets fixed, and then the webservers also get fixed.
   the free-software way ;)

(2) would also catapult mozilla to the quality level of lynx (et al.)
right-away, which do submit charset information ever since, without any reported
breakages...

Comment 5

[Marc Lehmann]

Attachment: ~/.mime.types

Obviously, this .mime.types is "malformed" which results in the shown
behaviour. However, since there is no formal .mime.types syntax (at least I
couldn't find it), and netscape uses a different syntax, the resolution is
difficult. Since key=value pairs are _never_ a valid mime-type, maybe these
lines should simply be skipped.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Please move the charset discussion to the appropriate bug.

As for that .mime.types file...  As you say, there are two syntaxes for
.mime.types files -- the one that Netscape 4 uses and the one that Apache uses.
 The former has 'type=foo/bar desc="whatever" exts="a,b,c"' as entries.  This
type of file has a header that says it's a Netscape file on the first line.  The
latter has entries of the form 'foo/bar ext1 ext2 ext3'.

In your case, simply moving the staroffice entries to the _end_ of the file so
that the Netscape comment is on the first line will fix things (we fall back to
the "Apache" syntax on a line-by-line basis if the "Netscape" syntax fails,
precisely because of staroffice and realplayer adding "Apache" entries to this
file).

I suppose that we _could_ try to do fallback both directions.  That is, for
"Apache" files we could parse the line and see whether it starts with a valid
type.  If it does not, we could attempt to parse it using the "Netscape" rules.

Alternately, we could also simply drop the line if it's not parsing per the
"Apache" syntax, but that approach is less desirable since in your case it would
mean most of the entries are always dropped...

Were those lines at the top of the file added by you manually?  Or by staroffice
automatically?

Comment 7

[Darin Fisher]

i think we should try to handle switching between formats.  the apache format
doesn't allow '=' which means it is entirely possible to determine the format of
a particular mime type entry, right?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, the apache format would not allow "=" anywhere in the type, since that's
not a legal character in a mime type.

Taking bug; I'll make us fall back in both directions.  I'm not going to be able
to get to this till after May 10, however...

Comment 9

[Marc Lehmann]

What is the appropriate bug report for the charset issue? I submitted it here
because it is also part of the Content-Type header. Should I open a seperate bug
report for it (or is there an existing one? didn't find it...)

I didn't know that I had a .mime.types file, so none of the entries were added
by me. However, I know that the netscape entries are older than the openoffice
ones (the file date coincides with me installing openoffice 641C).

One could construe this as an openoffice bug, as it seems to add the entries at
the top of some file that might have a "magic number" (the first comment line).
But I guess the only sane way to survive is to handle both formats (apache is
more common, netscape's seems more powerful).

Comment 10

[Boris Zbarsky [:bzbarsky]]

See bug 116346 and bug 18643.

If openoffice munges the .mime.types file like that by default, we sorta have to
cope with it.  :(

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix

This also fixes bug 132688, adds some long-overdue logging capability, and
changes the way we match "major/*" to work better.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, and it gets rid of the nsIPref usage too, moving completely to nsIPrefBranch....

Comment 13

[John Keiser (jkeiser)]

Comment on attachment 87754 [details] [diff] [review]
Patch to fix

>Index: exthandler/unix/nsOSHelperAppService.cpp
>===================================================================
>+static nsresult
>+GetFileLocation(const char* aPrefName,
>+                const char* aEnvVarName,
>+                PRUnichar** aFileLocation) {
...
>+  nsCOMPtr<nsISupportsWString> prefFileName;
>+  PRBool isUserPref = !(aEnvVarName && *aEnvVarName);
>+  if (!isUserPref)
>+    prefBranch->PrefHasUserValue(aPrefName, &isUserPref);
>+  if (isUserPref) {

This extra logic is unnecessary since you get the ordinary pref later.	You
could just do:
PRBool isUserPref;
prefBranch->PrefHasUserValue(aPrefName, &isUserPref);
if (isUserPref)

>@@ -1059,31 +1195,37 @@
>         entry.Truncate();
>       }    
>     }
>     if (!more) {
>       break;
>+      rv = NS_ERROR_NOT_AVAILABLE;

Methinks you meant the other way around?

>@@ -1180,10 +1324,20 @@
>   mimeInfo->AppendExtension(aFileExt);
>   nsHashtable typeOptions; // empty hash table
>   rv = LookUpHandlerAndDescription(majorType, minorType, typeOptions,
>                                    handler, mailcap_description,
>                                    mozillaFlags);
>+  if (NS_FAILED(rv)) {
>+    // maybe we have an entry for "majorType/*"?
>+    rv = LookUpHandlerAndDescription(majorType, NS_LITERAL_STRING("*"),
>+                                     typeOptions, handler, mailcap_description,
>+                                     mozillaFlags);
>+  }

It seems to me that this is less efficient than before ... why did you make
this change?  Same just below, later in patch.

Other than that, all is good, r=jkeiser, feel free to carry r= when you fix
these.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch addressing jkeiser's comments

> This extra logic is unnecessary since you get the ordinary pref later.

I was trying to avoid the call to PrefHasUserValue() when it's not needed.  But
it's probably not that expensive and the code is confusing as-is... changed to
what you suggest.

> Methinks you meant the other way around?

Yep.

I made the major/* change to handle the case of a mailcap file that has the
following lines in it:

image/*; ee %s
image/tiff; xsetroot %s

in that order.	In such a case, we really want to pick up "xsetroot" for tiff
images; the old code would pick up "ee" instead.  I've added a comment to the
code explaining why it's done that way.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 87813 [details] [diff] [review]
Patch addressing jkeiser's comments

noting the r=jkeiser

Comment 16

[Darin Fisher]

Comment on attachment 87813 [details] [diff] [review]
Patch addressing jkeiser's comments

>Index: exthandler/unix/nsOSHelperAppService.cpp

>+  nsCOMPtr<nsIPrefService> prefService(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>+  if (!prefService)
>+    return NS_ERROR_NOT_AVAILABLE;

nit: you capture |rv| but you don't check it.  seems like it'd be better to
check |rv|
instead of |prefService|.


>+  prefService->GetBranch(nsnull, getter_AddRefs(prefBranch));
>+  if (!prefBranch)
>+    return NS_ERROR_FAILURE;

likewise, here how about checking |rv| instead?


>+  if (aEnvVarName && *aEnvVarName) {
>+    char* prefValue = PR_GetEnv(aEnvVarName);
>+    if (prefValue && *prefValue) {
>+      *aFileLocation = ToNewUnicode(nsDependentCString(prefValue));

hmm... ToNewUnicode assumes ISO-Latin-1 input, but the result returned fromo
PR_GetEnv is encoded in the native system charset.  seems like you need a 
charset conversion here instead, or is the env var guaranteed to be ASCII?

otherwise, the patch looks good to me.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the nits and conversion issue

Comment 18

[Boris Zbarsky [:bzbarsky]]

jkeiser, would you mind reviewing again?

Comment 19

[Darin Fisher]

Comment on attachment 87993 [details] [diff] [review]
Fix the nits and conversion issue

perhaps GetFileLocation should return nsAString& instead of PRUnichar**

aside from that, this would  be an excellent place to use
NS_CopyNativeToUnicode once that function is available cross-platform.

Comment 20

[Darin Fisher]

Comment on attachment 87993 [details] [diff] [review]
Fix the nits and conversion issue

sr=darin provided jkeiser is happy with the changes.

Comment 21

[John Keiser (jkeiser)]

Comment on attachment 87993 [details] [diff] [review]
Fix the nits and conversion issue

r=jkeiser

Comment 22

[Boris Zbarsky [:bzbarsky]]

checked into trunk.