272541 - Empty disposition type treated as "attachment"

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[baoyang2000]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

After login this web mail, when click one mail in mailbox, the mail can not be
opened, and "Open with / Save to disk" dialog poped up.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

This is very likely because the server is sending the data with the wrong
content type specified. Firefox is just obeying what the server says, and since
it is not saying "text/html" then Firefox is asking you what to do with it.

It is hard, however, to confirm whether this is the case without being able to
log in to that site.

Comment 2

[Daniel Wang]

test account: username:mozilla_test password:test1

error message (on Mozilla 1.7.3):

The site suggested that "超文本 的 欢迎使用TOM.COM免费邮箱!" be handled as an
attachment. It is of type text/html (HTML Document) and located at:
  http://bjapp.mail.tom.com/coremail/fcg/
What should Mozilla do with this file?

I think this is a multipart file, but I'm not sure. add qawanted.

Comment 3

[Matthias Versen [:Matti]]

Daniel: Could you please create a http log ?
( http://www.mozilla.org/projects/netlib/http/http-debugging.html )

It's verry difficult to me because I can't understand that language...

Comment 4

[Daniel Wang]

Attachment: http log

The URL in question is
http://bjapp3.mail.tom.com/coremail/fcg/ldmsapp?funcid=readlett&sid=NAwefGNNBmqAwXqB&mid=1tbiCgQMAT%252BWPijuzwAAbd%250A10%250A27%250A1&fid=1&ord=0&desc=1&start=0


because of attachment size limit, I deleted lines from the beginning of the
session

Comment 5

[Matthias Versen [:Matti]]

This seems to be the problem:
4028[a72f20]: http response [
4028[a72f20]:   HTTP/1.1 200 OK
4028[a72f20]:   Date: Wed, 01 Dec 2004 14:00:52 GMT
4028[a72f20]:   Server: Apache/1.3.32 (Unix) mod_fastcgi/2.4.2
4028[a72f20]:   content-disposition: ; filename="³¬Îı¾ µÄ »¶Ó­Ê¹ÓÃTOM.COMÃâ·ÑÓÊÏä!"
4028[a72f20]:   Keep-Alive: timeout=15
4028[a72f20]:   Connection: Keep-Alive
4028[a72f20]:   Transfer-Encoding: chunked
4028[a72f20]:   Content-Type: text/html; name="³¬Îı¾ µÄ »¶Ó­Ê¹ÓÃTOM.COMÃâ·ÑÓÊÏä!"
4028[a72f20]: ]

The disposition type is missing (inline or attachment) and the filename should
be in ASCII (rfc2183) but I'm not sure if this is a Mozilla bug or not.
comment#s assumes that Mozilla assumes "attachment" as Disposition-type....

confirming for investigation

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

Daniel, could you test that patch please?  I tried logging in, but after I did
that I could not load the URL in comment 4, and I don't know the language
involved, so couldn't make sense of the error message...

Comment 8

[Daniel Wang]

bz, I don't know how to compile Mozilla. Do you have a build w/ the patch?

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 167552 [details] [diff] [review]
Proposed patch

uriloader/base/nsURILoader.cpp
+	   (// Some broken sites just send

hmm, this comment does not line up with the others about broken sites... ah
well

same in uriloader/exthandler/nsExternalHelperAppService.cpp

sorry for the delay. r=biesi, with or without making the comments line up
somehow

Comment 10

[Boris Zbarsky [:bzbarsky]]

biesi, the existing comments are just mis-indented.  I didn't really feel like
adding more of the same... ;)

Daniel, I do have a build with the patch, but it's a Linux build...  I'll try to
get that patch checked in ASAP so you can test a nightly with it, I guess.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in to the trunk.

Comment 12

[Boris Zbarsky [:bzbarsky]]

This didn't actually fix the bug: when this happens, GetParam throws.  We need to look at the first char by hand, I think....

Comment 14

[Julian Reschke]

According to

  http://greenbytes.de/tech/tc2231/#emptydisposition

Firefox is the only UA to "support" this. Recommendation: remove it.

Comment 15

[Boris Zbarsky [:bzbarsky]]

This bug is about "removing" it, yes.  No one has suggested it be kept.

Comment 16

[Julian Reschke]

(In reply to comment #15)
> This bug is about "removing" it, yes.  No one has suggested it be kept.

I see. Sorry, it's getting late over here :-)

Comment 17

[Julian Reschke]

I believe this can be fixed by replacing 

  if (NS_FAILED(rv) ||

by

  if (NS_SUCCEEDED(rv) &&

This way, we don't switch to forceExternalHandling = true for cases where we couldn't extract the disposition type from the header field.

This fixes at least 

  http://greenbytes.de/tech/tc2231/#emptydisposition

and will fix more of my tests once we let GetHeader... throw in more cases then now (for instance when the disposition type contains fishy characters, such as "=")

Comment 18

[Boris Zbarsky [:bzbarsky]]

Are we guaranteed that failure is only returned for the cases that we care about here?

> and will fix more of my tests once we let GetHeader... throw in more cases then
> now (for instance when the disposition type contains fishy characters, such as
> "=")

See, when that happens I think we _want_ to treat it as "attachment".  Missing disposition and malformed disposition are NOT the same case security-wise.

Comment 19

[Julian Reschke]

(In reply to comment #18)
> See, when that happens I think we _want_ to treat it as "attachment". 
> Missing disposition and malformed disposition are NOT the same case
> security-wise.

That's why I mentioned it. Let's have a look at my tests and how FF compares to other UAs:


1) http://greenbytes.de/tech/tc2231/#attmissingdisposition
Content-Disposition: filename=foo.html

All UAs treat this as "inline". 

2) http://greenbytes.de/tech/tc2231/#attmissingdisposition2
Content-Disposition: x=y; filename=foo.html

Firefox and Chrome treat this as named attachment, all the other UAs as inline.

3) http://greenbytes.de/tech/tc2231/#attmissingdisposition3
Content-Disposition: "foo; filename=bar;baz"; filename=qux

See 2)

4) http://greenbytes.de/tech/tc2231/#attmissingdisposition4
Content-Disposition: filename=foo.html, filename=bar.html

All UAs ignore this one

5) http://greenbytes.de/tech/tc2231/#emptydisposition
Content-Disposition: ; filename=foo.html

All UAs except FF ignore this one


I'm not sure why we want to treat "missing" and "malformed" differently, but if we do, then we'll have to tune the return values of the method so that the caller can distinguish it. My preference would be to always broken/missing disposition types, and thus to align with everybody except Chrome.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I'm not sure why we want to treat "missing" and "malformed" differently,

Basically, my security instinct here is that _any_ sort of funny business in this header should lead to automatically treating as attachment.  Anything else allows someone to attack a server by forcing things to run with that server's origin by managing to inject garbage into the headers.

I'm sort of willing to make exceptions for commonly occurring patterns that cause web compat issues.  That does leave open a hole to attack a server by injecting things that happen to look like those exceptions, but that's at least somewhat of a constraint.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I would be fine with a special return value for "disposition is explicitly empty".

Comment 22

[Julian Reschke]

OK, what would I need to do in order to define a new status code? Can I just add to the list?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yep.  See netwerk/base/public/nsNetError.h

Comment 24

[Julian Reschke]

Attachment: proposed patch

This patch has the desired effect on 

  http://greenbytes.de/tech/tc2231/#emptydisposition

...but I didn't do a full regression test yet.

Comment 25

[Julian Reschke]

Attachment: proposed patch, incl minimal test case

Proposed patch, introducing a specific error code for empty header field component. Simple test case that checks this status code on GetParam.

Comment 26

[Julian Reschke]

Comment on attachment 546134 [details] [diff] [review]
proposed patch, incl minimal test case

(also tested with my test suite)

Comment 27

[Julian Reschke]

Attachment: proposed patch, incl minimal test case

(now incl the actual test case file)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 546134 [details] [diff] [review]
proposed patch, incl minimal test case

>+    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_NETWORK, 30)

That's already taken, by NS_ERROR_UNKNOWN_HOST.  The fact that this file is organized "logically" instead of in code order is a bit of a pain, but please use an error code that's not already taken.

test_bug272541.js is missing from the patch, so this would fail tests.  Did you forget to hg add?

Please include changeset metadata (commit message, From or User line) in the patch if possible?

For the actual code changes:

1)  I would prefer the logic to look more like this:

  if ((NS_SUCCEEDED(rv) && /* conditions on dispToken */) ||
      rv == NS_ERROR_FIRST_HEADER_FIELD_COMPONENT_EMPTY) {
    forceExternalHandling = PR_TRUE;
  }

2)  This will conflict with the patch in bug 589292.  Could you please coordinate with Nick in terms of whether it makes more sense to land this first or whether it makes more sense for him to land first and do this on top of his patch?  At that point you will only need to edit NS_GetContentDisposition in nsNetUtil.h instead of the multiple sports you need to edit now.

3)  No need to put the bug number in the comment.  That's what the changeset commit message is for.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 546152 [details] [diff] [review]
proposed patch, incl minimal test case

Ah, there's the test.  It should probably include a test without the leading ';' as well (testing that the "filename=foo.html" ends up as the disposition token in that case.

Comment 30

[Julian Reschke]

(In reply to comment #28)
> Comment on attachment 546134 [details] [diff] [review] [review]
> proposed patch, incl minimal test case
> 
> >+    NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_NETWORK, 30)
> 
> That's already taken, by NS_ERROR_UNKNOWN_HOST.  The fact that this file is
> organized "logically" instead of in code order is a bit of a pain, but
> please use an error code that's not already taken.

Will fix.

> test_bug272541.js is missing from the patch, so this would fail tests.  Did
> you forget to hg add?

Have fixed.

> Please include changeset metadata (commit message, From or User line) in the
> patch if possible?

Do you have a  pointer to instructions for that?

> For the actual code changes:
> 
> 1)  I would prefer the logic to look more like this:
> 
>   if ((NS_SUCCEEDED(rv) && /* conditions on dispToken */) ||
>       rv == NS_ERROR_FIRST_HEADER_FIELD_COMPONENT_EMPTY) {
>     forceExternalHandling = PR_TRUE;
>   }

Will do.

> 2)  This will conflict with the patch in bug 589292.  Could you please
> coordinate with Nick in terms of whether it makes more sense to land this
> first or whether it makes more sense for him to land first and do this on
> top of his patch?  At that point you will only need to edit
> NS_GetContentDisposition in nsNetUtil.h instead of the multiple sports you
> need to edit now.

Sounds good. I don't care a lot of the order in which we do this.

> 3)  No need to put the bug number in the comment.  That's what the changeset
> commit message is for.

(In reply to comment #29)
> Comment on attachment 546152 [details] [diff] [review] [review]
> proposed patch, incl minimal test case
> 
> Ah, there's the test.  It should probably include a test without the leading
> ';' as well (testing that the "filename=foo.html" ends up as the disposition
> token in that case.

Will add.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Do you have a  pointer to instructions for that?

The simplest way is to use the mq extension, put your patch into the mq and then |hg qref -m "Bug NNNN.  fix this that and the other, r=reviewer" -u "My Name <email@whatever>"|

Then either upload the patch directly from the mq's .hg/patches directory or use the bzexport extension from http://hg.mozilla.org/users/tmielczarek_mozilla.com/bzexport/

The other option it to locally commit the fix, with the right user and commit message, and then use hg export to create a diff file with the metadata.

Comment 32

[Jason Duell]

Blow-by-blow, in case you don't know hg:

1) Start from clean mozilla-central (ie w/o your fixes)

2) Set up according to: 

https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f

Instead of doing 'qnew', download the patch  bug 589292 and 'qimport' it, then 'qpush' to apply it (hopefully it applies cleanly). 

3) qimport your own patch, qpush it, and when it (presumably) gives you warnings about some of your patch hunks not applying, go edit foo.cpp and foo.cpp.ref files (which contain patches that didn't apply) until foo.cpp is right.  Then 'qref -e' (put bug # in msg), and you're ready to upload patch, per bz' last comment

Comment 33

[u408661]

So it feels to me as if it makes more sense to land bug 589292 and then land this on top of it. Either way, NS_GetContentDisposition (and friends) from 589292 will have to change a bit, but looking at the way I did it now, it makes more sense to change it anyway (to return an nsresult and pass back the disposition in a parameter, instead of just returning the disposition). Landing 589292 will have the advantage of less overall churn, since we won't be landing this, then "backing out" a good portion of it to refactor it when 589292 lands. Like Julian, though, I'm not dead set on the order in which we do this.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Actually, I think we want to keep returning the disposition.  Just return "inline" in the case this bug is about.

And yes, I do think we should land bug 589292 first.  Just need to get it reviewed.

Comment 35

[Jason Duell]

> we should land bug 589292 first

Yay!  We all agree (my instructions above assumed that order).

Comment 36

[Julian Reschke]

Attachment: proposed patch, incl minimal test case

updated patch (still using diff format though)

Comment 37

[Julian Reschke]

Attachment: proposed patch, incl minimal test case

updated patch (still using diff format though) - now actually the proper patch file

Comment 38

[Julian Reschke]

Attachment: proposed patch, incl test case

This updates the patch for trunk (now that the patch for 589292 is in).

Note this adds an extra test class; I'd like to cleanup the Content-Disposition test suite separately (test_MIME_headers.js is broken in that it uses bogus test values that include the header field name)

Comment 39

[Jason Duell]

Comment on attachment 559694 [details] [diff] [review]
proposed patch, incl test case

Review of attachment 559694 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, and I'm +r-ing for bz since we all agreed on the acceptable logic.

> netwerk/test/unit/test_bug272541.js 

I generally am encouraging people to use more descriptive names for tests ("test_disposition.js", etc), and to also consider re-using files for new test cases, just to slow down file proliferation (and maybe speed up xpcshell tests).  I have a "test_headers.js" file that you could use.  But it has a different structure (it actually does web requests with a channel, instead of just creating a MIMEParser), and I don't care enough to hold up this patch.  But if you're going to clear up test_MIME_params anyway, consider merging the file you're creating here into it.

Thanks!

Comment 40

[Jason Duell]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ddb49b079701

Comment 41

[Boris Zbarsky [:bzbarsky]]

Jason, thank you!

Comment 42

[Julian Reschke]

(In reply to Jason Duell (:jduell) from comment #39)
> > netwerk/test/unit/test_bug272541.js 
> 
> I generally am encouraging people to use more descriptive names for tests
> ("test_disposition.js", etc), and to also consider re-using files for new
> test cases, just to slow down file proliferation (and maybe speed up
> xpcshell tests).  I have a "test_headers.js" file that you could use.  But
> it has a different structure (it actually does web requests with a channel,
> instead of just creating a MIMEParser), and I don't care enough to hold up
> this patch.  But if you're going to clear up test_MIME_params anyway,
> consider merging the file you're creating here into it.

Yes, I plan to do that once 610054 is in (which modifies the tests a lot).

Comment 43

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ddb49b079701

Comment 44

[Eric Shepherd [:sheppy]]

Docs updated to include the new error code:

https://developer.mozilla.org/en/Table_Of_Errors

Firefox 9 for developers explains the effect that this has; that cases in which Content-Disposition is empty is now treated as "inline" instead of "attachment".