275108 - Content-disposition header is incorrect, violates RFC

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Josh Aas]

Load any page that is a list of bugs (do a query), and look at the
content-disposition part of the HTTP response header (use an HTTP log). It will
look something like this:

"inline; filename=bugs-2004-12-17.html"[CRLF]

According to RFCs 2183 and 2616, it should be this:

inline; filename="bugs-2004-12-17.html"[CRLF]

You shouldn't quote a whole Content-disposition value, only internal values such
as the actual filename. This causes problems with saving query pages - see bug
243981 (extra quote proposed as part of the save file filename). This would also
be a problem in Firefox, but they have incorrectly put in a hack to trim quotes
from GetParameter(), which is doing the right thing by returning the quote after
the filename because the filename does not begin with a quote. That also
violates the RFCs. I will file a bug on that after this.

Comment 1

[Dave Miller [:justdave]]

Doh!  wtf does that even work in Firefox?

Comment 2

[Josh Aas]

It works in Firefox because they added an RFC-violating hack in response to the
problem. I'm filing a bug on that now.

Comment 3

[Dave Miller [:justdave]]

ummm...  examining our code, we're using the Perl CGI module to generate the
headers.  Either we're incorrectly using the API for the content-disposition
header or it's a bug in the Perl CGI module.  I'd be shocked if it's the latter.

Comment 4

[Dave Miller [:justdave]]

indeed, CGI has a special API for the Content-Disposition header, go figure.

"The -attachment parameter can be used to turn the page into an attachment.
Instead of displaying the page, some browsers will prompt the user to save it to
disk. The value of the argument is the suggested name for the saved file."

No mention of "-inline" in the docs anywhere...  and a quick scan of the source
to CGI.pm confirms it's not directly supported.

However, the way we're doing it currently is to use the generic header mechanism
(which can be used to generate any header that CGI.pm doesn't support directly).

 738     print $cgi->multipart_init(-content_disposition => "inline;
filename=$filename");

 994     print $cgi->header(-type => $contenttype,
 995                        -content_disposition => "$disp; filename=$filename");

Comment 5

[Dave Miller [:justdave]]

and so far, I fail to see anywhere in the code path in CGI.pm where it's adding
quotes...

Comment 6

[:glob ✱]

the bug only appears with serverpush.

looking at multipart_init:

  return $self->header(
      -nph => 0,
      -type => $type,
      (map { split "=", $_, 2 } @other),
  ) ...

that map looks suspect to me.  anyone know why it's there?

replacing the map with just @other appears to resolve this issue without any
side effects.  ie.

  return $self->header(
      -nph => 0,
      -type => $type,
      @other,
  ) ...

Comment 7

[:glob ✱]

Attachment: serverpush disposition fix v1

following more testing, i can't see anything wrong with my findings, so there
it is as a patch.

before patch:

Content-disposition: "inline; filename=bugs-2004-12-19.html"
Content-Type: multipart/x-mixed-replace;boundary="------- =_aaaaaaaaaa0"

after patch:

Content-disposition: inline; filename=bugs-2004-12-19.html
Content-Type: multipart/x-mixed-replace;boundary="------- =_aaaaaaaaaa0"

Comment 8

[Joel Peshkin]

Didn't we have to add those quotes in attachment downloads to avoid chopping off
filenames at the first whitespace?

Comment 9

[Joel Peshkin]

Doh!  I re-read the change.    Let's just doublecheck this against what we are
doing for attachments.

Comment 10

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

(In reply to comment #8)
> Didn't we have to add those quotes in attachment downloads to avoid chopping off
> filenames at the first whitespace?

err, for those, you should quote just the filename parameter, e.g.:

Content-Disposition: inline; filename="asdf asdf.html"

Comment 11

[Marc Schumann [:Wurblzap]]

Comment on attachment 169052 [details] [diff] [review]
serverpush disposition fix v1

This works for me, and it makes sense to me -- multipart_init is called using a
hash parameter in buglist.cgi, so I can't see a need for a conversion by map().
Otherwise, comment 5 applies to me, too :)
Stephen, as the original author in bug 226251, is there a chance you can
second-review this?

Joel, the change here may not apply to attachments because the filename is
mangled by s/\s/_/g in buglist.cgi.

Byron, please remove the part about .cvsignore in a final patch file :)

Comment 12

[Dave Miller [:justdave]]

Please examine the code in Perl's CGI module...

the map {} on @other is cloned from there.  Note that CGI->header() still has
that map, but does it differently than CGI->multipart_init() does.  Perhaps we
need the version from CGI->header().

(Has something to do with undoing damage done by CGI::Util::rearrange())

Comment 13

[:glob ✱]

Attachment: serverpush disposition fix v2

the quotes are added by the rearrange call, specifically by make_attributes()

according to the CGI::Util manpage, the routines are not public, so it's
possible (however unlikely) that we may be bit by internal CGI changes in the
future.

so rather than use an internal CGI routine which doesn't work exactly as we
need, this patch brings rearrange into Bugzilla::Util and applies the fixup
there instead of multipart_init.

i've also added quotes around the filename.

Comment 14

[Dave Miller [:justdave]]

Comment on attachment 169148 [details] [diff] [review]
serverpush disposition fix v2

>Index: buglist.cgi

>-    print $cgi->multipart_init(-content_disposition => "inline; filename=$filename");
>+    print $cgi->multipart_init(-content_disposition => qq#inline; filename="$filename"#);

This doesn't deal with the possibility (however remote) that the filename might
contain a quote mark.  Since, if the user is running a saved query, we use
their saved query name for the filename, we need to deal with that, just in
case.  Compare with the code in attachment.cgi, where I believe we already deal
with that.

>Index: Bugzilla/Util.pm

OK, the basic problem here is that multipart_init() in the CGI module is
broken.  It's a really major hack, and it was submitted by a third party and
added to CGI without much testing, according to the comments in the module. 
Our best bet here really *is* to fix the multipart_init() routine.  Being that
we are part of the Mozilla organization, and our browser is the one that
introduced "server push" to the world, it would be very appropriate if we were
the ones to submit fixes for the multipart routines so they actually work right
to the CGI folks.

That said, I'd much prefer that we stick with our override of the related
routines in Bugzilla::CGI so that we have a well-encapsulated chunk of working
and tested code that we can submit to get it included in the CGI module (and
then drop it from ours once we require the version of CGI that includes it).

Comment 15

[:glob ✱]

Attachment: serverpush disposition fix v3

- escapes backslashes and quotes if present in the filename
 - moves rearrange() and make_attributes() into Bugzilla::CGI
 - adds better description of changes to make_attributes()

Comment 16

[Dave Miller [:justdave]]

(In reply to comment #15)
>  - moves rearrange() and make_attributes() into Bugzilla::CGI
>  - adds better description of changes to make_attributes()

I guess I wasn't clear enough before.  I don't like the idea of overriding
rearrange() and make_attributes() at all.  Doing so will make it *very*
difficult to get this pushed upstream, since other things may depend on that
behavior in rearrange().  CGI->header doesn't have this problem, only
CGI->multipart_init does.  Looking at the code that deals with @other in
CGI->header, I *do* see it explicitly removing quotes from around the value.  I
suspect the real problem here is that rearrange() is actually getting called on
the contents of @other twice, once from multipart_init(), and once from header()
after we fall through to it.  Since our @other is returned from rearrange() and
is badly mangled at that point, we need to do a better job at converting @other
back to a form that rearrange() won't re-mangle worse the second time. 
(Borrowing the exact code that header() is using to decode the @other array
would likely work here, I suspect).

On the other hand (and why I'm not denying review on this yet), rearrange() is
obviously being used for a purpose it wasn't originally intended for within CGI,
so perhaps making a header-specific version of it that doesn't require all these
screwy workarounds (and then convincing them to use that for CGI->header() as
well) is really the best thing to do.  We shouldn't replace the existing one
because other things probably use it, but rather introduce a new one
(rearrange_header?) instead if we go that route.

Comment 17

[:glob ✱]

my concern with the mangle/demangle option is if it is fixed upstream, it may
render our workaround broken.  i don't think we should be relying on the
functionality of internal-only functions, especially if they are broken :)

i've files a perlbug: http://guest:[e-mail]/rt3/index.html?q=33118

Comment 18

[:glob ✱]

Attachment: serverpush disposition fix - rearrange_headers version

same as v3, but function is renamed to rearrange_headers(), and is also used by
multipart_start()

Comment 19

[:glob ✱]

Attachment: serverpush disposition fix - rearrange fixup version

this version fixes up @other before calling $self->header()

using the same fixup code from CGI->header() doesn't result in the correct
output.

Comment 20

[:glob ✱]

Attachment: serverpush disposition fix - no rearrange

after all that, we don't actually need to call rearrange.

multipart_init:

as we never specify the boundary, or do anything with nph, there's no reason to
use rearrange to extract them.

multipart_start:

rearrange funcioned only to extract a default value for $type.

Comment 21

[Dave Miller [:justdave]]

Comment on attachment 169168 [details] [diff] [review]
serverpush disposition fix - no rearrange

That's certainly a hell of a lot cleaner :)

The only down side is that it takes advantage of the fact that we aren't using
that stuff now, and prevents us from using it in the future.

It also doesn't produce working functions that would be usable in the upstream
CGI module if they took them (though I'll probably give in on that one since
it's so clean otherwise :)

Comment 22

[Jacob Steenhagen]

(In reply to comment #4)
> No mention of "-inline" in the docs anywhere...  and a quick scan of the source
> to CGI.pm confirms it's not directly supported.

So is this actually the real problem... that CGI.pm doesn't support an -inline
option?


(In reply to comment #14)
> Our best bet here really *is* to fix the multipart_init() routine.  Being that
> we are part of the Mozilla organization, and our browser is the one that
> introduced "server push" to the world, it would be very appropriate if we were
> the ones to submit fixes for the multipart routines so they actually work right
> to the CGI folks.

When you say "fix the multipart_init() routine," do you mean adding an -inline
option? Or is "inline" used by something other than server push? If "inline" is
used only by server push, then I suppose it makes sense that we provide the
working code to the CGI module.

(In reply to comment #21)
> It also doesn't produce working functions that would be usable in the upstream
> CGI module if they took them (though I'll probably give in on that one since
> it's so clean otherwise :)

Perhaps, then, what we can do is use this for 2.18 (we REALLY need to get that
out the door) and make a blocking 2.20 bug for something that fixes it in an
upstream compatible manner.

Comment 23

[Dave Miller [:justdave]]

*** Bug 275102 has been marked as a duplicate of this bug. ***

Comment 24

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

note that the just duped bug is about a slightly different problem: that bug is
caused by the fact that the content-disposition header is put on the "outside
wrapper" of the multipart document; it would be fixed by adding the header to
the actual buglist part of the document.

Comment 25

[:glob ✱]

Attachment: serverpush disposition fix v7

this patch:

  - correctly escapes and quotes the filename in content-disposition

  - adds content-disposition header to "main" part

  - updates multipart_init to support all functions of current version
    (basically you can override the default boundary)

  - updates multipart_start to support adding extra headers

the new versions of multipart_* should be suitable to be updated upstream.

Comment 26

[Vlad Dascalu]

Comment on attachment 169483 [details] [diff] [review]
serverpush disposition fix v7

+	 $var =~ s/^-//

I never quite fully understood why this is required here; it seems ok; I could
wonder if a 'g' could fit on its end:

+	 $var =~ s/^-//g

Also, I wonder if those two (this one and the next) should be switched, like
this:

+	 $var =~ tr/_/-/;
+	 $var =~ s/^-//g;

Anyway, I'm a little clueless on this section, so those are just ramblings :-)

Here:

+    foreach my $var (qw(-type -content_type -content-type)) {
+	 $type ||= $param{$var};

I'd leave -type the last one, because that's the least "standard" version that
we'll meet, and it makes sense to give priority to content-type. But that's a
nit.

Looks good to me.

Comment 27

[:glob ✱]

> +	 $var =~ s/^-//
> 
> I never quite fully understood why this is required here; it seems ok; 
> I could wonder if a 'g' could fit on its end:

the arguments are passed in as

  ->header(-content_disposition => $dispostion)

which means the key to the hash is '-content_disposition'.  this needs to become
'content-disposition', so i strip the leading hyphen and then convert
underscores to hyphens.

> +    foreach my $var (qw(-type -content_type -content-type)) {
> +	 $type ||= $param{$var};
> 
> I'd leave -type the last one, because that's the least "standard" version that
> we'll meet, and it makes sense to give priority to content-type. But that's a
> nit.

i wanted to keep the priority of arguments the same as the original code.

> Looks good to me.

:)

Comment 28

[Dave Miller [:justdave]]

Comment on attachment 169483 [details] [diff] [review]
serverpush disposition fix v7

hate to keep kicking this in search of perfection, but this is a chunk of code
where we probably need to have it in order to avoid confusion...

>Index: Bugzilla/CGI.pm
>===================================================================
>@@ -115,16 +114,17 @@
> sub multipart_init {
>-    my($self,@p) = @_;
>-    my($boundary,$nph,@other) = rearrange(['BOUNDARY','NPH'],@p);
>-    $boundary = $boundary || '------- =_aaaaaaaaaa0';
>+    my $self = shift;
>+    my %param = @_;
>+    my $boundary = $param{'-boundary'} || '------- =_aaaaaaaaaa0';
>+    delete $param{'-boundary'};

The original code here also had the effect of deleting the passed -nph param if
there was one.	Since we're supplying our own value for -nph, it wouldn't hurt
to make sure we don't have a duplicate.


>@@ -151,10 +151,15 @@
> sub multipart_start {
>@@ -166,6 +171,15 @@
>+    # Add any other headers
>+    foreach my $var (keys %param) {
>+        my $value = $param{$var};
>+        $var =~ s/^-//;
>+        $var =~ tr/_/-/;
>+        $var = ucfirst $var;
>+        push @header, "$var: $value";
>+    }
>+

This is effectively including all of the headers that we passed in, regardless
of what they are, right?  (Which is a change from the existing multipart_start
in Perl's CGI because it only allows specific headers.)  What would be the
difference here if instead of generating all these headers ourselves, if we
fell through to $cgi->header() instead, like we do in multipart_init()?  That
would allow us to eliminate the cookie hack here, too, and have a routine that
would really and truly be able to be sent upstream cleanly.  Or would that add
other headers that we don't want here?

Also, we need to change the comment above multipart_start that explains why we
overrode it, since the cookie hack is no longer the only reason.

Comment 29

[:glob ✱]

Attachment: serverpush disposition fix v8

> The original code here also had the effect of deleting the passed -nph 
> param if there was one.  Since we're supplying our own value for -nph, it
> wouldn't hurt to make sure we don't have a duplicate.

no problems.

i just realised that the original code is case insensitive, so i've fixed that
as well.

> This is effectively including all of the headers that we passed in,
regardless
> of what they are, right?

yes.

> Which is a change from the existing multipart_start in Perl's CGI because 
> it only allows specific headers.

if i'm reading it right, CGI 2.43 (earliest version i could find) includes all
passed headers.

> What would be the difference here if instead of generating all these 
> headers ourselves, if we fell through to $cgi->header() instead

a lot less code :)




here's the headers produced by this revision of the patch:

Content-disposition: inline; filename="bugs-2004-12-31.html"
Content-Type: multipart/x-mixed-replace;boundary="------- =_aaaaaaaaaa0"

WARNING: YOUR BROWSER DOESN'T SUPPORT THIS SERVER-PUSH TECHNOLOGY.
--------- =_aaaaaaaaaa0
Content-Type: text/html

<html>
  <head>
    <title>Bugzilla is pondering your search</title>
  </head>
  <body>
    <h1 style="margin-top: 20%; text-align: center;">Please stand by ...</h1>
  </body>
</html>
--------- =_aaaaaaaaaa0
Set-Cookie: LASTORDER=relevance%20desc; path=/; expires=Fri, 01-Jan-2038
00:00:00 GMT
Set-Cookie: BUGLIST=; path=/; expires=Fri, 01-Jan-2038 00:00:00 GMT
Date: Fri, 31 Dec 2004 08:10:47 GMT
Content-disposition: inline; filename="bugs-2004-12-31.html"
Content-Type: text/html

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">

Comment 30

[Dave Miller [:justdave]]

Comment on attachment 169969 [details] [diff] [review]
serverpush disposition fix v8

>+# Override multipart_start to ensure our cookies are added

But that has nothing at all to do with why we're overriding it now (which is
just to make sure the content-disposition shows up correctly, if I'm not
mistaken).

I see that "updated by Andrew Benham" in the comment in CGI.pm now...  and the
code that's there now looks like it should handle cookies okay.  But yours is
WAY cleaner.

Comment 31

[Dave Miller [:justdave]]

or to avoid bad quoting or something

Comment 32

[:glob ✱]

Attachment: serverpush disposition fix v9

updated comment to

"Override multipart_start to ensure our cookies are added and avoid bad quoting
of CGI's multipart_start (bug 275108)"

Comment 33

[Max Kanat-Alexander]

Hey bbaetz -- justdave suggested that you might be able to do this review. This
is the only really important remaining blocker to the 2.18 release, AFAIK.

Comment 34

[Dave Miller [:justdave]]

bbaetz pointed out http://rt.perl.org/rt3/index.html?q=24542 on IRC...  if we're
intending to push our multipart_init upstream, we probably need to take that
into account in this patch.

Comment 35

[Bradley Baetz (:bbaetz)]

As discussed, that has already been fixed upstream.

We should mention this problem upstream, and patch it that way, even if we
backport it too our cloned version afterwards.

Longer term we should get the header() stuff moved upstream too so that we don't
need this ugly sort of patching.

Comment 36

[:glob ✱]

Attachment: serverpush disposition fix v10

this revision addresses comment #34 by honouring the nph parameter if it is
supplied.

Comment 37

[Vlad Dascalu]

This is getting ridiculous. A simple thing like moving some quotes around turned
out to be a patch with lots of lines of code, blocking 2.18, too big to be
reviewed in a timely matter. It seems we nitpick everything nowadays. :-(

Comment 38

[Kevin Benton]

How does this handle characters that are not valid for a filename?

-    print $cgi->multipart_init(-content_disposition => "inline;
filename=$filename");
+    $filename =~ s/\\/\\\\/g; # escape backslashes
+    $filename =~ s/"/\\"/g; # escape quotes
+    $disposition = qq#inline; filename="$filename"#;

Comment 39

[Max Kanat-Alexander]

Comment on attachment 170342 [details] [diff] [review]
serverpush disposition fix v10

justdave vouches for the code quality (I asked him on IRC), and I can vouch for
the fact that it works (I just tested it on my 2.18 installation on landfill).

It doesn't break cookies, and it doesn't break attachments.

My only question is -- does setting $CGI::HEADERS_ONCE = 0; mean that we can't
ever call $cgi->print_headers() more than once? I think that we actually might
do that in some places, but I understood that CGI deals with it properly. (I'm
only saying this because of some comments on a bug that I saw a while ago...)

Also, I would have used qq( instead of qq#. I think that's what we use
elsewhere.

Comment 40

[Max Kanat-Alexander]

I think that this code is pretty much ready to check in. justdave wanted to do
some theoretical tests on it, but I think that we can do theoretical tests on
the branch, and get the practical fix in now so that we can release.

Comment 41

[Marc Schumann [:Wurblzap]]

(In reply to comment #39)
> My only question is -- does setting $CGI::HEADERS_ONCE = 0; mean that we can't
> ever call $cgi->print_headers() more than once? I think that we actually might
> do that in some places, but I understood that CGI deals with it properly. (I'm
> only saying this because of some comments on a bug that I saw a while ago...)

There are places that will break if we cannot call print_headers more than once
any more. For example, what has been done in bug 276605 relies on being able to
do so.

Comment 42

[Marc Schumann [:Wurblzap]]

(In reply to comment #41)
> any more. For example, what has been done in bug 276605 relies on being able to
> do so.

This should have said bug 277013.

Comment 43

[:glob ✱]

> There are places that will break if we cannot call print_headers more than once
> any more. For example, what has been done in bug 277013 relies on being able to
> do so.

$CGI::HEADERS_ONCE is set only by multipart_init, which means that setting is
only applicable to buglist.cgi, as that's the only multipart script we have.

bug 277013, which is about votes.cgi, won't be affected.

Comment 44

[Marc Schumann [:Wurblzap]]

(In reply to comment #43)
> $CGI::HEADERS_ONCE is set only by multipart_init, which means that setting is

That's good then :)

Comment 45

[Shane H. W. Travis]

Thanks for doggedly shepherding this one through ten revisions, Byron!

2.18:
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.255.2.7; previous revision: 1.255.2.6
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.10.2.3; previous revision: 1.10.2.2
done

Tip:
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.267; previous revision: 1.266
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.13; previous revision: 1.12
done