Empty disposition type treated as "attachment"

RESOLVED FIXED in mozilla9

Status

()

Core
Networking
P3
normal
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: baoyang2000, Assigned: bz)

Tracking

(Blocks: 1 bug, {dev-doc-complete, qawanted})

Trunk
mozilla9
dev-doc-complete, qawanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

13 years ago
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.

Comment 2

13 years ago
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.
Assignee: bugs → darin
Component: Web Site → Networking
Keywords: qawanted
Product: Firefox → Core
QA Contact: benc
Summary: After login this web mail, when click one mail in mailbox, the mail can not be opened → Browser prompts to download attachment file (multipart file?)
Version: unspecified → Trunk
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

13 years ago
Created attachment 167541 [details]
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
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 167552 [details] [diff] [review]
Proposed patch
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...
Attachment #167552 - Flags: superreview?(darin)
Attachment #167552 - Flags: review?(cbiesinger)

Comment 8

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

Updated

13 years ago
Attachment #167552 - Flags: superreview?(darin) → superreview+
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
Attachment #167552 - Flags: review?(cbiesinger) → review+
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.
Assignee: darin → bzbarsky
Priority: -- → P2
Summary: Browser prompts to download attachment file (multipart file?) → [FIXr]Browser prompts to download attachment file (multipart file?)
Target Milestone: --- → mozilla1.8alpha6
Checked in to the trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
This didn't actually fix the bug: when this happens, GetParam throws.  We need to look at the first char by hand, I think....
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 430475
Target Milestone: mozilla1.8alpha6 → ---
Summary: [FIXr]Browser prompts to download attachment file (multipart file?) → Browser prompts to download attachment file (multipart file?)
Priority: P2 → P3
Duplicate of this bug: 618813
Summary: Browser prompts to download attachment file (multipart file?) → Empty disposition type treated as "attachment"
Blocks: 609667

Comment 14

6 years ago
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.

Comment 16

6 years ago
(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

6 years ago
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.

Comment 19

6 years ago
(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".

Comment 22

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

Comment 24

6 years ago
Created attachment 545701 [details]
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.
Attachment #545701 - Flags: review?(bzbarsky)

Comment 25

6 years ago
Created attachment 546134 [details] [diff] [review]
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.
Attachment #545701 - Attachment is obsolete: true
Attachment #546134 - Flags: review?(bzbarsky)
Attachment #545701 - Flags: review?(bzbarsky)

Comment 26

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

(also tested with my test suite)

Comment 27

6 years ago
Created attachment 546152 [details] [diff] [review]
proposed patch, incl minimal test case

(now incl the actual test case file)
Attachment #546134 - Attachment is obsolete: true
Attachment #546152 - Flags: review?(bzbarsky)
Attachment #546134 - Flags: review?(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.
Attachment #546134 - Attachment is obsolete: false
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.
Attachment #546152 - Flags: review?(bzbarsky) → review-

Comment 30

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

Comment 36

6 years ago
Created attachment 547365 [details] [diff] [review]
proposed patch, incl minimal test case

updated patch (still using diff format though)
Attachment #546134 - Attachment is obsolete: true
Attachment #546152 - Attachment is obsolete: true

Comment 37

6 years ago
Created attachment 547369 [details] [diff] [review]
proposed patch, incl minimal test case

updated patch (still using diff format though) - now actually the proper patch file
Attachment #547365 - Attachment is obsolete: true

Comment 38

6 years ago
Created attachment 559694 [details] [diff] [review]
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)
Attachment #547369 - Attachment is obsolete: true
Attachment #559694 - Flags: review?(bzbarsky)

Updated

6 years ago
OS: Windows XP → All
Hardware: x86 → All
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!
Attachment #559694 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/ddb49b079701
Whiteboard: [inbound]
Attachment #167552 - Attachment is obsolete: true
Jason, thank you!

Comment 42

6 years ago
(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).
https://hg.mozilla.org/mozilla-central/rev/ddb49b079701
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Blocks: 686558
Keywords: dev-doc-needed
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".
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.