Closed p5pRT closed 16 years ago
as part of an investigate of a bugzilla bug (see https://bugzilla.mozilla.org/show_bug.cgi?id=275108) it appears that multipart_init() in CGI is incorrectly adding quotes to any "other" headers passed to it.
when called like
$cgi->multipart_init(-content_disposition => "inline; filename=bugs-2004-12-17.html");
the header comes back as
"inline; filename=bugs-2004-12-17.html"[CRLF]
the quotes break RFCs 2183 and 2616. the ideal structure of the header is
inline; filename="bugs-2004-12-17.html"[CRLF]
the crux of the problem is multipart_init is calling CGI::Util::rearrange()\, which in turn calls CGI::Util::make_attributes()\, which is hard coded to always wrap the value in quotes.
multipart_init shouldn't add quotes around any extra fields -- this should be left up to the caller.
it looks like rearrange is being used for a purpose that it wasn't intended\, resulting in strange results and fixups; CGI->header() contains the comment "rearrange() was designed for the HTML portion\, so we need to fix it up a little.".
a fix may be to create a new version of rearrange that isn't designed for the html portion.
here's a version of rearrange_headers() which is a modified version of rearrange() that doesnt not perform html escaping\, or wrap the value in quotes.
sub rearrange_headers { my($order\, @param) = @_; return () unless @param;
if (ref($param[0]) eq 'HASH') { @param = %{$param[0]}; } else { return @param unless (defined($param[0]) && substr($param[0]\, 0\, 1) eq '-'); }
# map parameters into positional indices my ($i\, %pos); $i = 0; foreach (@$order) { foreach (ref($_) eq 'ARRAY' ? @$_ : $_) { $pos{lc($_)} = $i; } $i++; }
my (@result \,%leftover); $#result = $#$order; # preextend while (@param) { my $key = lc(shift(@param)); $key =~ s/^\-//; if (exists $pos{$key}) { $result[$pos{$key}] = shift(@param); } else { $leftover{$key} = shift(@param); } }
if (%leftover) { foreach (keys %leftover) { my($key) = $_; $key =~ s/^\-//; ($key = "\L$key") =~ tr/_/-/; my $value = $leftover{$_}; push(@result\, defined($leftover{$_}) ? qq/$key=$value/ : qq/$key/); } }
@result; }
Hi *\,
attached is a patch that adds "rearrange_header" to CGI::Util and multipart_init() uses this new function.
Cheers\, Renee
The RT System itself - Status changed from 'new' to 'open'
Hi Lincoln\,
attached is a revised patch for CGI::Util. All the CGI.pm tests pass...
$ make test PERL_DL_NONLAZY=1 /usr/bin/perl5.8.8 "-MExtUtils::Command::MM" "-e" "test_harness(0\, 'blib/lib'\, 'blib/arch')" t/*.t t/apache................ok t/can...................ok t/carp..................ok t/cookie................ok t/fast..................ok t/form..................ok t/function..............ok t/html..................ok t/no_tabindex...........ok t/pretty................ok t/push..................ok 1/12 skipped: various reasons t/request...............ok t/start_end_asterisk....ok t/start_end_end.........ok t/start_end_start.......ok t/switch................ok t/upload................ok t/uploadInfo............ok t/util-58...............ok t/util..................ok All tests successful\, 1 subtest skipped. Files=20\, Tests=521\, 1 wallclock secs ( 0.51 cusr + 0.08 csys = 0.59 CPU)
Cheers\, Renee
Lincoln Stein wrote:
Hi\,
This patch appears to break the regression tests. I am investigating now\, but I may need to back it out.
Lincoln
Hi Lincoln\,
3.42 looks fine! Thanks...
Renee
Lincoln Stein wrote:
Hi Renee\,
I just uploaded version 3.42 to CPAN (about 20 minutes ago). I made various fixes to get things running again. Could you check your patch against that?
Lincoln
fixed with CGI.pm 3.42
module@renee-baecker.de - Status changed from 'open' to 'resolved'
On Mon\, Sep 08\, 2008 at 08:21:56PM +0200\, Rene Ba"cker wrote:
Hi Lincoln\,
3.42 looks fine! Thanks...
I've updated blead to 3.42 with change 34320.
Lincoln\, in your changes you credit me with the Windows patch. I merely acted as postman - the patch was actually from Steve Hay.
Also\, Craig Berry submitted a patch via RT https://rt.cpan.org/Public/Bug/Display.html?id=36158 which fixes tests that fail on VMS. Are you able to check whether that can go in to your source tree\, ready for the next release?
Nicholas Clark
Hi Renee\,
I just uploaded version 3.42 to CPAN (about 20 minutes ago). I made various fixes to get things running again. Could you check your patch against that?
Lincoln
On Mon\, Sep 8\, 2008 at 10:36 AM\, Renée BÀcker \< renee.baecker@smart-websolutions.de> wrote:
Hi Lincoln\,
attached is a revised patch for CGI::Util. All the CGI.pm tests pass...
$ make test PERL_DL_NONLAZY=1 /usr/bin/perl5.8.8 "-MExtUtils::Command::MM" "-e" "test_harness(0\, 'blib/lib'\, 'blib/arch')" t/*.t t/apache................ok t/can...................ok t/carp..................ok t/cookie................ok t/fast..................ok t/form..................ok t/function..............ok t/html..................ok t/no_tabindex...........ok t/pretty................ok t/push..................ok 1/12 skipped: various reasons t/request...............ok t/start_end_asterisk....ok t/start_end_end.........ok t/start_end_start.......ok t/switch................ok t/upload................ok t/uploadInfo............ok t/util-58...............ok t/util..................ok All tests successful\, 1 subtest skipped. Files=20\, Tests=521\, 1 wallclock secs ( 0.51 cusr + 0.08 csys = 0.59 CPU)
Cheers\, Renee
Lincoln Stein wrote:
Hi\,
This patch appears to break the regression tests. I am investigating now\, but I may need to back it out.
Lincoln
--- /home/rbaecker/Util.pm.orig 2008-08-26 16:46:10.000000000 +0200 +++ CGI/Util.pm 2008-09-08 16:32:26.000000000 +0200 @@ -4\,7 +4\,7 @@ use vars qw($VERSION @EXPORT_OK @ISA $EBCDIC @A2E @E2A); require Exporter; @ISA = qw(Exporter); -@EXPORT_OK = qw(rearrange make_attributes unescape escape +@EXPORT_OK = qw(rearrange rearrange_header make_attributes unescape escape expires ebcdic2ascii ascii2ebcdic);
$VERSION = '1.5_01'; @@ -70\,16 +70\,41 @@ }
# Smart rearrangement of parameters to allow named parameter -# calling. We do the rearangement if: +# calling. We do the rearrangement if: # the first parameter begins with a - + sub rearrange { + my ($order\,@param) = @_; + + my ($result\, $leftover) = _rearrange_params( $order\, @param ); + + return () unless $result; + + push @$result\, make_attributes( $leftover\, defined $CGI::Q ? $CGI::Q->{escape} : 1 ) if keys %$leftover; + + @$result; +} + +sub rearrange_header { + my ($order\,@param) = @_; + + my ($result\,$leftover) = _rearrange_params( $order\, @param ); + + return () unless $result; + + push @$result\, make_attributes( $leftover\, 0\, 1 ) if keys %$leftover; + + @$result; +} + +sub _rearrange_params { my($order\,@param) = @_; return () unless @param;
if \(ref\($param\[0\]\) eq 'HASH'\) \{ @​param = %\{$param\[0\]\}; \} else \{
- return @param + return \@param unless (defined($param[0]) && substr($param[0]\,0\,1) eq '-'); }
@@ -103\,14 +128\,17 @@ } }
- push (@result\,make_attributes(\%leftover\,defined $CGI::Q ? $CGI::Q->{escape} : 1)) if %leftover; - @result; + return \@result\, \%leftover; }
sub make_attributes { my $attr = shift; return () unless $attr && ref($attr) && ref($attr) eq 'HASH'; my $escape = shift || 0; + my $do_not_quote = shift; + + my $quote = $do_not_quote ? '' : '"'; + my(@att); foreach (keys %{$attr}) { my($key) = $_; @@ -122\,7 +150\,7 @@ ($key="\L$key") =~ tr/_/-/; # parameters are lower case\, use dashes
my $value = $escape ? simple\_escape\($attr\->\{$\_\}\) : $attr\->\{$\_\};
- push(@att\,defined($attr->{$_}) ? qq/$key="$value"/ : qq/$key/); + push(@att\,defined($attr->{$_}) ? qq/$key=$quote$value$quote/ : qq/$key/); } return @att; }
-- Lincoln D. Stein
Ontario Institute for Cancer Research 101 College St.\, Suite 800 Toronto\, ON\, Canada M5G0A3 416 673-8514 Assistant: Stacey Quinn \Stacey\.Quinn@​oicr\.on\.ca
Cold Spring Harbor Laboratory 1 Bungtown Road Cold Spring Harbor\, NY 11724 USA (516) 367-8380 Assistant: Sandra Michelsen \michelse@​cshl\.edu
Hi\,
This patch appears to break the regression tests. I am investigating now\, but I may need to back it out.
Lincoln
On Mon\, Sep 8\, 2008 at 4:07 AM\, Renée BÀcker \< renee.baecker@smart-websolutions.de> wrote:
Hi *\,
attached is a patch that adds "rearrange_header" to CGI::Util and multipart_init() uses this new function.
Cheers\, Renee
--- Util.pm.orig 2008-08-26 16:46:10.000000000 +0200 +++ cpan/lib/perl5/5.8.8/CGI/Util.pm 2008-09-08 09:58:43.000000000 +0200 @@ -4\,7 +4\,7 @@ use vars qw($VERSION @EXPORT_OK @ISA $EBCDIC @A2E @E2A); require Exporter; @ISA = qw(Exporter); -@EXPORT_OK = qw(rearrange make_attributes unescape escape +@EXPORT_OK = qw(rearrange rearrange_header make_attributes unescape escape expires ebcdic2ascii ascii2ebcdic);
$VERSION = '1.5_01'; @@ -70\,16 +70\,35 @@ }
# Smart rearrangement of parameters to allow named parameter -# calling. We do the rearangement if: +# calling. We do the rearrangement if: # the first parameter begins with a - + sub rearrange { + my ($order\,@param) = @_; + + my ($result\, $leftover) = _rearrange_params( $order\, @param ); + push @$result\, make_attributes( $leftover\, defined $CGI::Q ? $CGI::Q->{escape} : 1 ) if keys %$leftover; + + @$result; +} + +sub rearrange_header { + my ($order\,@param) = @_; + + my ($result\,$leftover) = _rearrange_params( $order\, @param ); + push @$result\, make_attributes( $leftover\, 0\, 1 ) if keys %$leftover; + + @$result; +} + +sub _rearrange_params { my($order\,@param) = @_; return () unless @param;
if \(ref\($param\[0\]\) eq 'HASH'\) \{ @​param = %\{$param\[0\]\}; \} else \{
- return @param + return \@param unless (defined($param[0]) && substr($param[0]\,0\,1) eq '-'); }
@@ -103\,14 +122\,17 @@ } }
- push (@result\,make_attributes(\%leftover\,defined $CGI::Q ? $CGI::Q->{escape} : 1)) if %leftover; - @result; + return \@result\, \%leftover; }
sub make_attributes { my $attr = shift; return () unless $attr && ref($attr) && ref($attr) eq 'HASH'; my $escape = shift || 0; + my $do_not_quote = shift; + + my $quote = $do_not_quote ? '' : '"'; + my(@att); foreach (keys %{$attr}) { my($key) = $_; @@ -122\,7 +144\,7 @@ ($key="\L$key") =~ tr/_/-/; # parameters are lower case\, use dashes
my $value = $escape ? simple\_escape\($attr\->\{$\_\}\) : $attr\->\{$\_\};
- push(@att\,defined($attr->{$_}) ? qq/$key="$value"/ : qq/$key/); + push(@att\,defined($attr->{$_}) ? qq/$key=$quote$value$quote/ : qq/$key/); } return @att; }
--- CGI.pm.orig 2008-08-26 16:46:10.000000000 +0200 +++ cpan/lib/perl5/5.8.8/CGI.pm 2008-09-08 09:46:17.000000000 +0200 @@ -24\,7 +24\,7 @@ # HARD-CODED LOCATION FOR FILE UPLOAD TEMPORARY FILES. # UNCOMMENT THIS ONLY IF YOU KNOW WHAT YOU'RE DOING. # $CGITempFile::TMPDIRECTORY = '/usr/tmp'; -use CGI::Util qw(rearrange make_attributes unescape escape expires ebcdic2ascii ascii2ebcdic); +use CGI::Util qw(rearrange rearrange_header make_attributes unescape escape expires ebcdic2ascii ascii2ebcdic);
#use constant XHTML_DTD => ['-//W3C//DTD XHTML Basic 1.0//EN'\, # ' http://www.w3.org/TR/xhtml-basic/xhtml-basic10.dtd'\<http://www.w3.org/TR/xhtml-basic/xhtml-basic10.dtd%27> ]; @@ -1381\,7 +1381\,7 @@ 'multipart_init' => \<\<'END_OF_FUNC'\, sub multipart_init { my($self\,@p) = self_or_default(@_); - my($boundary\,@other) = rearrange([BOUNDARY]\,@p); + my($boundary\,@other) = rearrange_header([BOUNDARY]\,@p); $boundary = $boundary || '------- =_aaaaaaaaaa0'; $self->{'separator'} = "$CRLF--$boundary$CRLF"; $self->{'final_separator'} = "$CRLF--$boundary--$CRLF";
-- Lincoln D. Stein
Ontario Institute for Cancer Research 101 College St.\, Suite 800 Toronto\, ON\, Canada M5G0A3 416 673-8514 Assistant: Stacey Quinn \Stacey\.Quinn@​oicr\.on\.ca
Cold Spring Harbor Laboratory 1 Bungtown Road Cold Spring Harbor\, NY 11724 USA (516) 367-8380 Assistant: Sandra Michelsen \michelse@​cshl\.edu
My pleasure. Thanks for your help with the bug fixes.
Lincoln
On Mon\, Sep 8\, 2008 at 2:21 PM\, Renée Ba"cker \< renee.baecker@smart-websolutions.de> wrote:
Hi Lincoln\,
3.42 looks fine! Thanks...
Renee
Lincoln Stein wrote:
Hi Renee\,
I just uploaded version 3.42 to CPAN (about 20 minutes ago). I made various fixes to get things running again. Could you check your patch against that?
Lincoln
-- Lincoln D. Stein
Ontario Institute for Cancer Research 101 College St.\, Suite 800 Toronto\, ON\, Canada M5G0A3 416 673-8514 Assistant: Stacey Quinn \Stacey\.Quinn@​oicr\.on\.ca
Cold Spring Harbor Laboratory 1 Bungtown Road Cold Spring Harbor\, NY 11724 USA (516) 367-8380 Assistant: Sandra Michelsen \michelse@​cshl\.edu
Hi Renee\,
Ok\, I'll fix the credits. Apologize to Steve for the oversight.
Yes\, I got the Berry patches into 3.42 but since I don't have a VMS machine I couldn't test that they work.
Lincoln
On Mon\, Sep 8\, 2008 at 3:19 PM\, Nicholas Clark \nick@​ccl4\.org wrote:
On Mon\, Sep 08\, 2008 at 08:21:56PM +0200\, Rene Ba"cker wrote:
Hi Lincoln\,
3.42 looks fine! Thanks...
I've updated blead to 3.42 with change 34320.
Lincoln\, in your changes you credit me with the Windows patch. I merely acted as postman - the patch was actually from Steve Hay.
Also\, Craig Berry submitted a patch via RT https://rt.cpan.org/Public/Bug/Display.html?id=36158 which fixes tests that fail on VMS. Are you able to check whether that can go in to your source tree\, ready for the next release?
Nicholas Clark
-- Lincoln D. Stein
Ontario Institute for Cancer Research 101 College St.\, Suite 800 Toronto\, ON\, Canada M5G0A3 416 673-8514 Assistant: Stacey Quinn \Stacey\.Quinn@​oicr\.on\.ca
Cold Spring Harbor Laboratory 1 Bungtown Road Cold Spring Harbor\, NY 11724 USA (516) 367-8380 Assistant: Sandra Michelsen \michelse@​cshl\.edu
On Mon\, Sep 8\, 2008 at 2:32 PM\, Lincoln Stein \lstein@​cshl\.edu wrote:
Yes\, I got the Berry patches into 3.42 but since I don't have a VMS machine I couldn't test that they work.
Berry will be testing that real soon now. Thanks.
On Mon\, Sep 8\, 2008 at 7:02 PM\, Craig A. Berry \craig\.a\.berry@​gmail\.com wrote:
On Mon\, Sep 8\, 2008 at 2:32 PM\, Lincoln Stein \lstein@​cshl\.edu wrote:
Yes\, I got the Berry patches into 3.42 but since I don't have a VMS machine I couldn't test that they work.
Berry will be testing that real soon now. Thanks.
And I can now confirm that there are no CGI test failures on VMS since 3.42 came into blead.
Migrated from rt.perl.org#33118 (status was 'resolved')
Searchable as RT33118$