Last Comment Bug 474536 - The jar: scheme ignores the content-disposition: header on the inner URI
: The jar: scheme ignores the content-disposition: header on the inner URI
Status: VERIFIED FIXED
[sg:moderate] XSS against sites who u...
: verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: Networking: JAR (show other bugs)
: Trunk
: Other All
: P2 normal (vote)
: mozilla1.9.1
Assigned To: Jason Duell [:jduell] (needinfo me)
:
:
Mentors:
Depends on: 224209
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-20 19:58 PST by Daniel Veditz [:dveditz]
Modified: 2012-10-26 11:06 PDT (History)
30 users (show)
jst: blocking1.9.1+
jst: wanted1.9.1+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x?
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial fix (11.67 KB, patch)
2009-02-10 16:43 PST, Jason Duell [:jduell] (needinfo me)
no flags Details | Diff | Splinter Review
Second attempt at fix, using Boris' advice (13.26 KB, patch)
2009-02-18 18:17 PST, Jason Duell [:jduell] (needinfo me)
bzbarsky: review-
Details | Diff | Splinter Review
Incorporate Boris' feedback (12.47 KB, patch)
2009-02-19 09:30 PST, Jason Duell [:jduell] (needinfo me)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
branch patch (12.21 KB, patch)
2009-03-12 13:19 PDT, Dave Camp (:dcamp)
dveditz: approval1.9.0.9+
Details | Diff | Splinter Review
unit test for this bug (3.74 KB, patch)
2009-03-17 17:49 PDT, Jason Duell [:jduell] (needinfo me)
dcamp: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Version of last patch that works on trunk (3.68 KB, patch)
2009-03-23 17:16 PDT, Jason Duell [:jduell] (needinfo me)
dcamp: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
for 1.8 (15.81 KB, patch)
2009-04-27 10:16 PDT, Alexander Sack
no flags Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2009-01-20 19:58:48 PST
When the jar: scheme wraps a URI that returns a Content-disposition: attachment header that will be ignored and the inner zip file unpacked and the specified file interpreted anyway. Since Content-disposition: attachment is often used to prevent potentially dangerous or unknown content from being interpreted by browsers as part of the serving host's domain this can lead to security issues as in bug 472206 comment 27
Comment 1 Daniel Veditz [:dveditz] 2009-01-20 22:18:31 PST
We probably want to fix this soonish or the bugzilla fix in bug 472206 is pointless.
Comment 2 Denis Roy 2009-01-21 06:49:40 PST
Thanks for cc'ing us, but I wouldn't mind seeing what the impact of this is.  I can't access the URL in bug 472206 comment 27 because I don't have a "Bugzilla Test Instances - LDAP Login". I tried this on our local Bugzilla install but I'm unsure of what it is I'm looking for.

Or maybe I just don't need to know.
Comment 3 Daniel Veditz [:dveditz] 2009-01-22 14:28:11 PST
That test URL should result in a dialog prompting you to save the file, and instead it opens the HTML content in the browser due to this bug. Since bug 472206 is about using Content-disposition: attachment as an anti-XSS measure this is a problem. The specific impact of that should be discussed in that other bug since that tool will have to figure out a workaround until this client bug is fixed.
Comment 4 Daniel Veditz [:dveditz] 2009-01-22 14:42:21 PST
view-source: has the same issue although it might take a pretty contrived situation to make it dangerous. Would be nice if we could fix it in a central place for all nested schemes rather than have to worry that a future scheme might forget to do the same check, but maybe that's not possible.
Comment 5 Daniel Veditz [:dveditz] 2009-01-22 14:43:44 PST
I guess this doesn't "block" 472206 since it looks like they'll ship regardless, since they've got a compelling upgrade with or without this problem.
Comment 10 Jason Duell [:jduell] (needinfo me) 2009-02-10 16:43:19 PST
Created attachment 361669 [details] [diff] [review]
Initial fix

OK, I think I have an initial fix.  Patch attached.

The patch is pretty simple conceptually:  in the existing URILoader check for content-disposition, we pull out the 'transport channel' from JARChannel's (i.e. the channel that actually does the network transport of the file), and use it for the check.  This should work for both HTTP and 'multipart' (Question:  can JAR URLS use multipart?  If so I think there may be some other places I want to check for security holes).

Most of the code changes are to change the JARChannel to keep a reference to its inner channel (these can be nested, so they ought to work for nested "jar:jar:http//..." urls:  I will test these soon).  JST tells me there's a good chance we could wind up with circular references to the inner channel, so I manually release the reference in OnStopRequest().  There is a new nsIJARChannel2.idl file to export the new GetTransportChannel() method, since we can't add the method to the regular IDL (it's frozen).

I've tested this on HTTP on my laptop, and we now see a download dialog when you try to download a JAR that has "content-disposition: attach" in its headers. (I'd make this test public, but I need to be able to tweak http.conf--a regular htaccess file can't do what's needed).  The non-attach case still loads the JAR.  So things seem to be working.  I still need to run this through the triserver, plus look into making a mochitest (?), plus test the nested (and multipart?) cases.  But I figured I'd post this patch so I can get some feedback in case I'm using the wrong approach.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2009-02-11 07:40:55 PST
> Question:  can JAR URLS use multipart?

Multipart handling is triggered by the MIME type, so if there is an extension that would map to that type then yes.

nsIJARChannel isn't frozen, unless I'm missing something...

As far as the patch goes, I was wondering whether it's instead time to bite the bullet and finally introduce an interface that exposes content-disposition information (or at least just the whole disposition string) that all three of the relevant channels (jar, http, part) would implement.  That would also let extensions expose content-disposition without having to change the core to do so...  I thought we had an existing bug on this, but can't find it; it might be that we do not.

Note also that we have other callers in our tree that get content-disposition that should probably also be adjusted.

As far as the patch goes, you probably need to drop the mInnerChannel reference if AsyncOpen fails, since you won't get an OnStopRequest in that case.  In GetTransportChannel you'd want to use NS_ADDREF, not an explicit AddRef call to be consistent with other code.  Your indentation in nsJARChannel.h for the "public nsIJARChannel2" line is off; did tabs sneak in?

This setup doesn't seem to deal with cases when the underlying channel is an HTTP channel that gets a 3xx response that redirects to an HTTP 200 response with Content-Disposition, since you'll try to get disposition off the pre-redirect channel.  If we do decide to keep track of the channel, we'd need to listen for redirects and update on redirect.  But if we just hand out the disposition string, it seems like we could do well enough by just grabbing it off the channel passed in to us in OnDownloadComplete, like we do for security info and so forth.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2009-02-11 08:08:41 PST
Ah, I found it.  See bug 224209 and bug 356086.
Comment 13 Jason Duell [:jduell] (needinfo me) 2009-02-18 18:17:37 PST
Created attachment 363038 [details] [diff] [review]
Second attempt at fix, using Boris' advice

OK, here's a second attempt at a patch.  I've tested it and it appears to work, including on nested jar:jar:http urls.

Patch notes:

> finally introduce an interface that exposes content-disposition
> information (or at least just the whole disposition string) that 
> all three of the relevant channels (jar, http, part) would implement.

I've made a start at this, but to keep the patch small, I've so far only made the change to nsJARChannel.  With a little extra work I could make make multichannels also inherit from nsHashPropertyBag, and then make both it and nsHttpChannel store the Content-Disposition using the same property key, and we'll have a unified interface.  This would actually simplify the code a bit, but would also mean more overall code changed.  Let me know if that's appropriate, of if we'd rather keep the patch small for the release branch.  I'd vote for just making the additional changes.

I've changed nsJARChannel to inherit from nsHashPropertyBag (just like the nsHttpChannel and nsBaseChannel classes already do).   Rather than trying to keep a pointer to the transport channel (as in the last patch), we now grab the Content-Disposition header from the HTTP channel (or inner JARChannel for nested jar:jar URLs) in OnDownloadComplete, and stuff it into the JARChannel's properties.  

There are no checks for either Content-Disposition or Content-Type within nsJARChannel::OnDownloadComplete for the multipart case (see comment in patch).  Should there be?  I created a CGI script that feeds a JAR as the first part of a multipart reply, but it winds up failing (since the root content-type is multipart, not application/java-archive).  So I'm not sure it's even a possible case.  But Boris mentioned that a MIME extension could be mapped to multipart--I'm not sure if I understand how that would work, or if it's relevant.

Also haven't addressed the fact that we're currently using (slightly different) duplicated logic to parse the Content-disposition header in about 3-4 places in the codebase--at some point I'd like to centralize this in a function in nsNetUtils.h.  I thought of storing logical values (ATTACHMENT, INLINE) in the properties, rather than the raw Content-Disposition header, so we could truly centralize the parsing, but I'm not sure I know all the use cases, so for now it's the raw header.

I'm using Christian's scheme for centralizing Channel property names in nsChannelProperties.h.  This required eliminating a define of IMPL_NS_NET in a test program's Makefile, which shouldn't have had it set anyway, as near as I can tell (the only side effect of not having it set is that the program will call NS_LITERAL_STRING, which is slightly less efficient).

Finally, while poking through the code I realized we didn't have "Content-Disposition" as a static HTTP_ATOM, so I added it.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2009-02-18 19:06:39 PST
Comment on attachment 363038 [details] [diff] [review]
Second attempt at fix, using Boris' advice

>+++ b/modules/libjar/nsJARChannel.cpp    Wed Feb 18 18:16:18 2009 -0800
>+#include "nsChannelProperties.h"

Oh, nice.  I hadn't realized we had this, actually.  ;)

We should add other properties here; there's a whole bunch that some channels expose...  (base URIs and referrers come to mind offhand).  File a followup bug on doing that?

>-NS_IMPL_ISUPPORTS6(nsJARChannel,

You could use

  NS_IMPL_ISUPPORTS_INHERITED6(nsJARChannel,
                               nsHashPropertyBag,
                               ....

>@@ -747,41 +763,54 @@ nsJARChannel::OnDownloadComplete(nsIDown
>+    nsCAutoString header;

The indentation here is weird.  Did tabs sneak in?

>+        // bug 474536: URILoader checks disposition of page-level JAR load
>+        rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("Content-Disposition"),
>+            header);
>+        if (NS_SUCCEEDED(rv)) 
>+        

More weird indentation, especially for "header);".

For what it's worth, I wouldn't bother putting the bug# in the comments, or adding comments to the effect that this fixes some bug.  That's what hg/cvs blame is for.

>+        // TODO: also check for multipart channel?
>+        // -- not possible for inner channel to be Multipart? JCD

More weird indent.  At the moment, the inner channel here in fact cannot be multipart; the only way to get a multipart channel is via the stream converter that decodes multipart responses.  So while the underlying channel of a part channel could be a jar channel, the reverse can't happen.

>                 mIsUnsafe = unsafe;
>+

Why add the blank line?

>             }
>+        // Soon-to-be common way to get Disposition: right now only nsIJARChannel
>+        rv = NS_GetContentDisposition(request, header);
>+        if (NS_SUCCEEDED(rv)) 
>+        SetPropertyAsACString(NS_CHANNEL_PROP_CONTENT_DISPOSITION, header);

Again, weird indent.

>+++ b/netwerk/base/public/nsChannelProperties.h    Wed Feb 18 

>+ * HTTP Content-Disposition header of channel.  

s/HTTP/MIME/, since for part channels there might be no HTTP involved?

>+ * Only available from Channels that use HTTP (nsIHttpChannel, nsIJARChannel,
>+ * nsIMultiPartChannel).

This part is just not true (in that multipart channels may not use http).  I'd just take this sentence out.

>+++ b/netwerk/base/public/nsNetStrings.h    Wed Feb 18 18:16:18 

There should be a corresponding change to nsNetStrings.cpp, right?

>+++ b/netwerk/protocol/http/src/nsHttpAtomList.h    Wed Feb 18 >+HTTP_ATOM(Content_Disposition,       "Content-Disposition")

I guess this is just an optimization, right?

>+++ b/netwerk/streamconv/test/Makefile.in    Wed Feb 18 18:16:18 >-DEFINES += -DIMPL_NS_NET

Why is this change needed?

>+++ b/uriloader/base/nsURILoader.cpp    Wed Feb 18 18:16:18 2009 

I guess this fixes the issue with showing inline but doesn't actually use the filename for anything.  That might be ok, I guess.

Fix the above (esp. the nsNetStrings) issue, and looks good.  Let's get this in on trunk and branch and file a followup bug for making all three channels use the property on trunk?  I'd love to see some code simplification happening here.  ;)
Comment 15 Jason Duell [:jduell] (needinfo me) 2009-02-19 09:30:20 PST
Created attachment 363135 [details] [diff] [review]
Incorporate Boris' feedback

OK, here's another patch.

I've made all the changes that Boris suggested.

Comments/notes:

Weird indentations: yes they were tabs.

yes, HTTP_ATOM(Content_Disposition) is an optimization.  Otherwise we dynamically allocate an atom (I haven't looked to see if that's per lookup, or just the first lookup).

> multipart channels may not use http

What else can they use?  Are we using nsMultipartChannel to handle ftp requests?  multipart emails?  

> There should be a corresponding change to nsNetStrings.cpp, right?

Thanks--nice catch.  Previously I wasn't initializing this, so any code where IMPL_NS_NET isn't defined (i.e any code outside the same shared object as gNetStrings) would have wound up not finding the property, since we'd be using different strings.

Which brings me to my main question:  why are we storing channel properties this way (ie. in a hashtable)?  It seems like a big kludge to me, compared with something like a new .idl interface (nsIChannelProperties) with separate accessor methods for each stored property.  In the latter case the compiler will guarantee that we add logic to every channel when we add a new property, so no channel gets "missed."  It will also be more efficient:  (accessing a member variable, rather than kludging around with the IMPL_NS_NETWORK hack in order to skip an 8-16 bit string conversion, only to wind up still incurring a hashtable lookup).  On all fronts (amount of code, maintainability, efficiency) the whole nsChannelProperties.h/nsNetString.h strategy looks like a stinker to me.  Anyone care to defend it?  Otherwise I propose adding that our new bug about additional properties to add also include converting to using a new .idl interface.

>+++ b/uriloader/base/nsURILoader.cpp    Wed Feb 18 18:16:18 2009 
> I guess this fixes the issue with showing inline but doesn't actually 
> use the filename for anything.  That might be ok, I guess.

I can grab the filename if you tell me how to then pass it up within nsURILoader.  Do we want this for the immediate patch?  Also, looking at the logic it appears that we treat malformed "Content-Disposition: filename=foo.txt" and/or "name=foo.txt"  as "inline", rather than "attachment".  Is it common to pass a filename with "inline", or should those be considered to imply attachment instead?  I haven't changed it because I'm assuming the logic was added this way for a reason.

Finally, the removal of the IMPL_NS_NETWORK from the test makefile is because the code will not compile with it, since the test does not link against the network shared lib, and thus can't find the global gNetStrings variable.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2009-02-19 12:22:19 PST
> Are we using nsMultipartChannel to handle ftp requests?

We're handling it for any load in a docshell (browser area) or through imagelib which comes back with the MIME type application/x-mixed-replace.  This could be an HTTP channel, a data: channel, some extension-implemented channel, an FTP channel pointing to a file whose extension maps to that MIME type for some reason, etc, etc.

> why are we storing channel properties this way (ie. in a hashtable)

I think mainly because it lets us add new properties without breaking binary compatibility...  In particular, we want to be able to add properties that are "optional" in the sense that channels may, but do not have to, set them.  This property is a good example of such.  I think the nsChannelProperties/nsNetStrings thing doesn't save us all that much; we could be using NS_LITERAL_STRING at all callsites just like we do when not IMPL_NS_NET.  It's just good to have a source of documentation for properties people might look for on channels, which is why I suggested adding the other properties to this file.  Just in the documentation section is probably fine; I wouldn't worry about adding to gNetStrings, I think.

> I can grab the filename if you tell me how to then pass it up within
> nsURILoader.  Do we want this for the immediate patch? 

No; let's do that as a followup.  It'll be easier once the code for content-disposition is simpler...  The relevant callers are scattered about (e.g. nsExternalHelperAppService), and as we unify the API things should Just Work for them for free.  We might want to expose the filename/disposition separately from the entire header too, as discussed in that other bug I pointed you at; we can think about that in the followup bug.

> we treat malformed "Content-Disposition: filename=foo.txt" and/or
> "name=foo.txt"  as "inline

Yeah.  That's unfortunately fairly common, and needs to be treated inline for web compat.  :(
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2009-02-19 12:22:46 PST
Comment on attachment 363135 [details] [diff] [review]
Incorporate Boris' feedback

Looks good.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2009-02-19 12:27:16 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/4bd7dd7645c2 so we can get some testing on this.  Jason, do you want to write up some unit tests here too?

We should also get this in on branches once it's baked on trunk a bit.  Poke me to land on 1.9.1 in a few days, and at that point we should ask for 1.9.0 approval too.
Comment 19 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-02-19 12:40:11 PST
Backed out because it didn't compile:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235075319.1235075381.26575.gz
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2009-02-19 12:45:34 PST
Pushed again as http://hg.mozilla.org/mozilla-central/rev/e22bfb6a442e with this bustage fix added:

-  : NS_LITERAL_STRING_INIT(kContentLength, NS_CHANNEL_PROP_CONTENT_LENGTH_STR)
-  : NS_LITERAL_STRING_INIT(kContentDisposition, NS_CHANNEL_PROP_CONTENT_DISPOSITION_STR)
+  : NS_LITERAL_STRING_INIT(kContentLength, NS_CHANNEL_PROP_CONTENT_LENGTH_STR),
+    NS_LITERAL_STRING_INIT(kContentDisposition, NS_CHANNEL_PROP_CONTENT_DISPOSITION_STR)
 
Jason, I'd really like to see some tests here, at least for the jar: part.  If you can write some for the URILoader part too, that would be excellent, but it might be a bit of work to detect that we'd put up the helper app dialog without hanging up the browser doing it...
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2009-03-06 18:26:31 PST
Jason, ping?  We need those tests, and need to backport this to 1.9.1 and probably 1.9.0....
Comment 22 Dave Camp (:dcamp) 2009-03-12 13:19:34 PDT
Created attachment 367099 [details] [diff] [review]
branch patch

The original patch applies fine to 1.9.1/1.9.0, this patch just includes the bustage fix too.

I can push this to 1.9.1 today unless we want to wait on those unit tests...
Comment 24 Daniel Veditz [:dveditz] 2009-03-16 14:55:47 PDT
Comment on attachment 367099 [details] [diff] [review]
branch patch

Approved for 1.9.0.8, a=dveditz for release-drivers
Comment 25 Dave Camp (:dcamp) 2009-03-16 23:19:33 PDT
Checking in modules/libjar/nsJARChannel.cpp;
/cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v  <--  nsJARChannel.cpp
new revision: 1.129; previous revision: 1.128
done
Checking in modules/libjar/nsJARChannel.h;
/cvsroot/mozilla/modules/libjar/nsJARChannel.h,v  <--  nsJARChannel.h
new revision: 1.48; previous revision: 1.47
done
Checking in netwerk/base/public/nsChannelProperties.h;
/cvsroot/mozilla/netwerk/base/public/nsChannelProperties.h,v  <--  nsChannelProperties.h
new revision: 1.3; previous revision: 1.2
done
Checking in netwerk/base/public/nsNetStrings.h;
/cvsroot/mozilla/netwerk/base/public/nsNetStrings.h,v  <--  nsNetStrings.h
new revision: 1.2; previous revision: 1.1
done
Checking in netwerk/base/public/nsNetUtil.h;
/cvsroot/mozilla/netwerk/base/public/nsNetUtil.h,v  <--  nsNetUtil.h
new revision: 1.112; previous revision: 1.111
done
Checking in netwerk/base/src/nsNetStrings.cpp;
/cvsroot/mozilla/netwerk/base/src/nsNetStrings.cpp,v  <--  nsNetStrings.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in netwerk/protocol/http/src/nsHttpAtomList.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpAtomList.h,v  <--  nsHttpAtomList.h
new revision: 1.6; previous revision: 1.5
done
Checking in netwerk/streamconv/test/Makefile.in;
/cvsroot/mozilla/netwerk/streamconv/test/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done
Checking in uriloader/base/nsURILoader.cpp;
/cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v  <--  nsURILoader.cpp
new revision: 1.150; previous revision: 1.149
done
Comment 26 Dave Camp (:dcamp) 2009-03-16 23:54:36 PDT
One chunk failed to apply added:

Checking in nsJARChannel.cpp;
/cvsroot/mozilla/modules/libjar/nsJARChannel.cpp,v  <--  nsJARChannel.cpp
new revision: 1.130; previous revision: 1.129
done
Comment 27 Jason Duell [:jduell] (needinfo me) 2009-03-17 17:49:01 PDT
Created attachment 367935 [details] [diff] [review]
unit test for this bug

OK, at long last, here's a test for this bug.  Includes a JAR file and ^headers^ file.   Right now just tests that JARChannel contains "content-disposition" property, in PropertyBag interface.   Will eventually extend test to cover HTTP and Multi: I'm creating a new bug for that work.

I've tested the test, and it works.
Comment 28 Jason Duell [:jduell] (needinfo me) 2009-03-18 11:55:14 PDT
OK, I've moved the further work for this bug into bug 356086.

I assume someone needs to merge the test code into the 1.9.0 and 1.9.1 branches?   I'm reopening this so someone will notice this:  feel free to mark closed again if we don't need to merge the test into the branches
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2009-03-18 12:11:04 PDT
Yes, we do need to and the tests, but bug should stay closed.  I'll do it after I review them, so it'll certainly be on my todo-list (in my review queue, to be exact).
Comment 30 Al Billings [:abillings] 2009-03-23 14:15:58 PDT
I wanted to verify it with the unit test but it isn't reviewed and checked in. Dveditz and I went over bug 472206 comment 27 and it isn't a valid testcase anymore.  

Can someone suggest a valid testcase to verify this fix for 1.9.0.8 since we're making builds in a week?
Comment 31 Jason Duell [:jduell] (needinfo me) 2009-03-23 17:16:16 PDT
Created attachment 368988 [details] [diff] [review]
Version of last patch that works on trunk

We've changed how https.js gets loaded in unit tests on the trunk.  This version of the patch takes care of this change (and one other file load).

The previous patch is still the one to use for older branches.
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2009-03-26 13:04:25 PDT
Comment on attachment 367935 [details] [diff] [review]
unit test for this bug

Looks good.
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2009-03-26 13:04:34 PDT
Comment on attachment 368988 [details] [diff] [review]
Version of last patch that works on trunk

Looks good.
Comment 34 Al Billings [:abillings] 2009-03-31 17:42:35 PDT
(In reply to comment #30)
> I wanted to verify it with the unit test but it isn't reviewed and checked in.
> Dveditz and I went over bug 472206 comment 27 and it isn't a valid testcase
> anymore.  
> 
> Can someone suggest a valid testcase to verify this fix for 1.9.0.8 since we're
> making builds in a week?

Any suggestions on how to validate this fix for 1.9.0.9?
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2009-03-31 18:59:43 PDT
Set up an HTTP server that serves up .jar files with a "content-disposition:attachment" header and then see whether you can open them via a jar:// URI or whether you get a "save as" dialog instead.  Then repeat the test without the header to make sure that the header is in fact affecting the behavior you see.
Comment 36 Jason Duell [:jduell] (needinfo me) 2009-04-01 12:39:23 PDT
Just in case it matters, I've done the exact tests that Boris has described on my laptop.

If the code needs verification by someone else, I can email you the tarball and HTML page with test links (you'd still need to configure apache httpd.conf to add the disposition header for one directory).

But like I said, I've already tested myself.
Comment 37 Al Billings [:abillings] 2009-04-01 12:57:17 PDT
That's good enough for me, Jason. What build did you use?
Comment 38 Jason Duell [:jduell] (needinfo me) 2009-04-01 13:40:32 PDT
I tested with the trunk.   Give me a URL for whatever version you want me to test and it'll take me 3 minutes to verify.
Comment 39 Al Billings [:abillings] 2009-04-01 16:23:44 PDT
Sweet. Give it a try with the 1.9.0 nightly at ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/ (use the 3.0.9pre build, not the 3.0.8).
Comment 40 Jason Duell [:jduell] (needinfo me) 2009-04-02 14:43:39 PDT
I've verified that this is fixed for 3.0.9pre, using the tests on my laptop.

Marking as VERIFIED--remove if that's wrong.
Comment 41 Al Billings [:abillings] 2009-04-02 14:45:53 PDT
Marking as verified for 1.9.0.9, which is what you checked. Leaving resolved otherwise.

We should verify this in a 1.9.1 nightly as well.
Comment 42 Jason Duell [:jduell] (needinfo me) 2009-04-02 14:52:33 PDT
Just verified with 3.5b4pre.   Now do we mark it VERIFIED?  (I'll let you do it :)
Comment 43 Al Billings [:abillings] 2009-04-02 14:56:04 PDT
Yes, now we mark it verified. :-)
Comment 44 Samuel Sidler (old account; do not CC) 2009-04-02 15:57:37 PDT
Actually, VERIFIED refers to trunk verification which happened in comment 36 and comment 38. For 1.9.1 verification, we use the "verified1.9.1" keyword, replacing "fixed1.9.1".

Ah, Bugzilla, how I love you so.
Comment 45 Al Billings [:abillings] 2009-04-02 16:19:25 PDT
The Version field has been confusing here too.
Comment 46 Martin Stránský 2009-04-14 02:10:07 PDT
1.8.0 doesn't look affected, it refuses to load jar: from http.
Comment 47 Boris Zbarsky [:bz] (still a bit busy) 2009-04-14 07:23:51 PDT
Then you're doing something wrong.  It works fine, assuming your server is sending the right content-type for the jar.
Comment 48 Daniel Veditz [:dveditz] 2009-04-16 18:32:13 PDT
At-risk sites would have to
 1) allow arbitrary user-uploaded content
 2) allow users to set arbitrary MIME types, or specifically support
    .jar files as application/java-archive or application/x-jar
 3) Serve them from a domain with sensitive content they need to protect
    using Content-disposition: attachment to prevent XSS

The MIME type restriction was added in bug 369814 (MFSA 2007-37) http://www.mozilla.org/security/announce/2007/mfsa2007-37.html
Comment 49 Alexander Sack 2009-04-27 10:16:55 PDT
Created attachment 374783 [details] [diff] [review]
for 1.8

backport attachment 367099 [details] [diff] [review] for 1.8.

I was able to verify that "Content-disposition: attachment" does not trigger the helper dialog on current 1.8 branch; with this patch it properly triggers the helper dialog.
Comment 50 Jason Duell [:jduell] (needinfo me) 2009-09-01 17:23:22 PDT
The test for this fix somehow never got checked in.  Patch marked "Version of last patch that works on trunk" should be checked into trunk.
Comment 51 Boris Zbarsky [:bz] (still a bit busy) 2009-09-02 08:36:39 PDT
Pushed the test as http://hg.mozilla.org/mozilla-central/rev/6ee269c6c118
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2009-09-02 10:35:28 PDT
Backed out the test, because it hangs on tinderbox.
Comment 53 Jeff Walden [:Waldo] (remove +bmo to email) 2009-09-02 14:11:18 PDT
The test is pre-bug 396226, and in particular it assumes nsIHttpServer.stop is synchronous, returning only when shutdown is complete.  Control flow should now continue in a callback function passed to the stop() method; using do_test_finished as the argument and replacing the break with a return will probably do the trick, at a glance.

Note You need to log in before you can comment on or make changes to this bug.