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.

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.

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.

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...

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

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

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...

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

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

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.

Checked in to the trunk.

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

According to http://greenbytes.de/tech/tc2231/#emptydisposition Firefox is the only UA to "support" this. Recommendation: remove it.

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

(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 :-)

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 "=")

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.

(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.

> 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.

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

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

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

This patch has the desired effect on http://greenbytes.de/tech/tc2231/#emptydisposition ...but I didn't do a full regression test yet.

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

Comment on attachment 546134 [details] [diff] [review] proposed patch, incl minimal test case (also tested with my test suite)

(now incl the actual test case file)

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 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.

(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.

> 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.

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

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.

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.

> we should land bug 589292 first Yay! We all agree (my instructions above assumed that order).

updated patch (still using diff format though)

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

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 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!

Jason, thank you!

(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).

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".