Bug #112231 for Net-Jifty: Fails with recent Encode versions

Sun Feb 21 08:23:32 2016 ANDK [...] cpan.org - Ticket created

Subject:

Fails with recent Encode versions

Sample fail report with Encode 2.80: http://www.cpantesters.org/cpan/report/64879870 Downgrading to 2.79 does as a workaround. HTH&&Thanks,

Sun Feb 21 15:06:44 2016 SREZIC [...] cpan.org - Correspondence added

On 2016-02-21 08:23:32, ANDK wrote: Show quoted text

> Sample fail report with Encode 2.80: > http://www.cpantesters.org/cpan/report/64879870 > > Downgrading to 2.79 does as a workaround. >

Looks like another victim of https://rt.cpan.org/Ticket/Display.html?id=111853

Sun Feb 21 15:06:45 2016 The RT System itself - Status changed from 'new' to 'open'

Sun Feb 21 15:07:10 2016 SREZIC [...] cpan.org - Reference to ticket #111853 added

Fri Apr 01 15:17:01 2016 pali [...] cpan.org - Cc PALI added

Fri Apr 01 15:18:06 2016 pali [...] cpan.org - Correspondence added

On Ned Feb 21 08:23:32 2016, ANDK wrote: Show quoted text

> Sample fail report with Encode 2.80: > http://www.cpantesters.org/cpan/report/64879870 > > Downgrading to 2.79 does as a workaround. > > HTH&&Thanks,

This looks like a broken test. You are MIME encoding name and filename attributes, but you expect it in plain (non MIME encoded) format.

Sun Nov 06 11:26:01 2016 gregoa [...] cpan.org - Correspondence added

Trivial patch attached.

Subject:

mime-encode.patch

Description: Encode::MIME::HeEader::encode encodes everything with MIME-Q since 2.83. Adjust expected output. Origin: vendor Bug: https://rt.cpan.org/Public/Bug/Display.html?id=112231 Forwarded: https://rt.cpan.org/Public/Bug/Display.html?id=112231 Bug-Debian: https://bugs.debian.org/825608 Author: gregor herrmann <gregoa@debian.org> Last-Update: 2016-11-06 --- a/t/006-uploads.t +++ b/t/006-uploads.t @@ -33,7 +33,7 @@ is( scalar @parts, 1, "has one part" ); is $parts[0]->content_type, 'text/plain', 'correct type of the part'; is $parts[0]->header('Content-Disposition'), - 'form-data; name="file"; filename="test.txt"', + 'form-data; name="=?UTF-8?Q?file?="; filename="=?UTF-8?Q?test=2Etxt?="', 'checked disposition'; is $parts[0]->content, 'stub', 'checked content'; @@ -58,7 +58,7 @@ is( scalar @parts, 1, "has one part" ); is $parts[0]->content_type, 'application/octet-stream', 'correct type of the part'; is $parts[0]->header('Content-Disposition'), - 'form-data; name="file"; filename="test.txt"', + 'form-data; name="=?UTF-8?Q?file?="; filename="=?UTF-8?Q?test=2Etxt?="', 'checked disposition'; is $parts[0]->content, 'stub', 'checked content'; @@ -87,12 +87,12 @@ is $parts[0]->content_type, 'application/octet-stream', 'correct type of the part'; is $parts[0]->header('Content-Disposition'), - 'form-data; name="file"; filename="test.txt"', + 'form-data; name="=?UTF-8?Q?file?="; filename="=?UTF-8?Q?test=2Etxt?="', 'checked disposition'; is $parts[0]->content, 'stub', 'checked content'; is $parts[1]->header('Content-Disposition'), - 'form-data; name="some_arg"', + 'form-data; name="=?UTF-8?Q?some=5Farg?="', 'checked disposition'; is $parts[1]->content, 'some_value', 'checked content'; } @@ -117,7 +117,7 @@ is $parts[0]->content_type, 'application/octet-stream', 'correct type of the part'; is $parts[0]->header('Content-Disposition'), - 'form-data; name="file"; filename="=?UTF-8?Q?=D1=82=2Ebin?="', + 'form-data; name="=?UTF-8?Q?file?="; filename="=?UTF-8?Q?=D1=82=2Ebin?="', 'checked disposition'; is $parts[0]->content, 'stub', 'checked content'; }

Sun Nov 06 11:26:16 2016 gregoa [...] cpan.org - Cc GREGOA added

Sun Nov 06 11:34:16 2016 pali [...] cpan.org - Correspondence added

On Ned Nov 06 11:26:01 2016, GREGOA wrote: Show quoted text

> Trivial patch attached.

Show quoted text

> - 'form-data; name="file"; filename="test.txt"', > + 'form-data; name="=?UTF-8?Q?file?="; filename="=?UTF-8?Q?test=2Etxt?="',

That is not only incorrect, but also violate standards! name/filename is ASCII plain text, not MIME encoded.

Sun Nov 06 11:46:02 2016 gregoa [...] debian.org - Correspondence added

Subject:	Re: [rt.cpan.org #112231] Fails with recent Encode versions
Date:	Sun, 6 Nov 2016 17:45:48 +0100
To:	Pali via RT <bug-Net-Jifty [...] rt.cpan.org>
From:	gregor herrmann <gregoa [...] debian.org>

On Sun, 06 Nov 2016 11:34:21 -0500, Pali via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=112231 > > > On Ned Nov 06 11:26:01 2016, GREGOA wrote:

> > Trivial patch attached.

>

> > - 'form-data; name="file"; filename="test.txt"', > > + 'form-data; name="=?UTF-8?Q?file?="; filename="=?UTF-8?Q?test=2Etxt?="',

> > That is not only incorrect, but also violate standards! name/filename is ASCII plain text, not MIME encoded.

Ok, then I interpreted your comment from April wrong, where you wrote: This looks like a broken test. You are MIME encoding name and filename attributes, but you expect it in plain (non MIME encoded) format. So this needs changes in the code itself, I guess? Cheers, gregor -- .''`. https://info.comodo.priv.at/ - Debian Developer https://www.debian.org : :' : OpenPGP fingerprint D1E1 316E 93A7 60A8 104D 85FA BB3A 6801 8649 AA06 `. `' Member of VIBE!AT & SPI, fellow of the Free Software Foundation Europe `- NP: Flying Pickets: She Drives Me Crazy

Download signature.asc
application/pgp-signature 931b

Message body not shown because it is not plain text.

Sun Nov 06 11:59:59 2016 pali [...] cpan.org - Correspondence added

On Ned Nov 06 11:46:02 2016, gregoa@debian.org wrote: Show quoted text

> Ok, then I interpreted your comment from April wrong, where you > wrote: > > This looks like a broken test. You are MIME encoding name and > filename attributes, but you expect it in plain (non MIME encoded) > format.

Yes, that is incorrect. You should not MIME-encode name and filename attributes. Show quoted text

> So this needs changes in the code itself, I guess?

I have not looked into code and tests... but it depends on where is that explicit encode() function called. If it is in tests (e.g. by mocking or another way), then problem is in tests. But if it is called in code itself, then module is broken and code needs to be fixed. IIRC I saw in other code/modules that tests were broken, they incorrectly mocked some more complicated structure and pre-filled MIME-encoded all attributes. So this is reason why I wrote back in April, that it looks like a broken test...

Sat Nov 12 11:33:43 2016 gregoa [...] debian.org - Correspondence added

Subject:	Re: [rt.cpan.org #112231] Fails with recent Encode versions
Date:	Sat, 12 Nov 2016 17:33:21 +0100
To:	Pali via RT <bug-Net-Jifty [...] rt.cpan.org>
From:	gregor herrmann <gregoa [...] debian.org>

On Sun, 06 Nov 2016 12:00:00 -0500, Pali via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=112231 > > > On Ned Nov 06 11:46:02 2016, gregoa@debian.org wrote:

> > Ok, then I interpreted your comment from April wrong, where you > > wrote: > > > > This looks like a broken test. You are MIME encoding name and > > filename attributes, but you expect it in plain (non MIME encoded) > > format.

> > Yes, that is incorrect. You should not MIME-encode name and filename attributes. >

> > So this needs changes in the code itself, I guess?

> > I have not looked into code and tests... but it depends on where is > that explicit encode() function called. If it is in tests (e.g. by > mocking or another way), then problem is in tests. But if it is > called in code itself, then module is broken and code needs to be > fixed.

It's in the code: #v+ 209 sub form_form_data_args { 210 my $self = shift; 211 212 my @res; 213 while (my ($key, $value) = splice @_, 0, 2) { 214 my $disposition = 'form-data; name="'. Encode::encode( 'MIME-Q', $key ) .'"'; 215 unless ( ref $value ) { 216 push @res, HTTP::Message->new( 217 ['Content-Disposition' => $disposition ], 218 $value, 219 ); 220 next; 221 } 222 223 if ( $value->{'filename'} ) { 224 $value->{'filename'} = Encode::encode( 'MIME-Q', $value->{'filename'} ); 225 $disposition .= '; filename="'. delete ( $value->{'filename'} ) .'"'; 226 } 227 push @res, HTTP::Message->new( 228 [ 229 'Content-Type' => $value->{'content_type'} || 'application/octet-stream', 230 'Content-Disposition' => $disposition, 231 ], 232 delete $value->{content}, 233 ); 234 } 235 return @res; 236 } #v- Cheers, gregor -- .''`. https://info.comodo.priv.at/ - Debian Developer https://www.debian.org : :' : OpenPGP fingerprint D1E1 316E 93A7 60A8 104D 85FA BB3A 6801 8649 AA06 `. `' Member of VIBE!AT & SPI, fellow of the Free Software Foundation Europe `- NP: The Doors: Twentieth Century Fox

Download signature.asc
application/pgp-signature 963b

Message body not shown because it is not plain text.

Sat Nov 12 11:51:38 2016 pali [...] cpan.org - Correspondence added

On Sob Nov 12 11:33:43 2016, gregoa@debian.org wrote: Show quoted text

> It's in the code:

So... then code is incorrect. Show quoted text

> #v+ > 209 sub form_form_data_args { > 210 my $self = shift; > 211 > 212 my @res; > 213 while (my ($key, $value) = splice @_, 0, 2) { > 214 my $disposition = 'form-data; name="'. Encode::encode( 'MIME-Q', $key ) .'"';

Here can be only US-ASCII data, no MIME. Show quoted text

> 215 unless ( ref $value ) { > 216 push @res, HTTP::Message->new( > 217 ['Content-Disposition' => $disposition ], > 218 $value, > 219 ); > 220 next; > 221 } > 222 > 223 if ( $value->{'filename'} ) { > 224 $value->{'filename'} = Encode::encode( 'MIME-Q', $value->{'filename'} ); > 225 $disposition .= '; filename="'. delete ( $value->{'filename'} ) .'"';

Same here, filename can be only US-ASCII. Show quoted text

> 226 } > 227 push @res, HTTP::Message->new( > 228 [ > 229 'Content-Type' => $value->{'content_type'} || 'application/octet-stream', > 230 'Content-Disposition' => $disposition, > 231 ], > 232 delete $value->{content}, > 233 ); > 234 } > 235 return @res; > 236 } > #v-

If you want to pass non-ASCII characters into filename, then you need to use "filename*" specified by RFC 2231. It can contains MIME encoded string, but with percent encoding. So MIME-Q cannot be used as it produce =20 intead %20. I think RFC 2231 is what implementation should use if it aims to support Unicode characters. Other option is to allow only US-ASCII filenames and Encode::encode('ASCII', $filename) can be used to convert string into ascii. All non-ascii chars are replaced by '?'.