Bug #79239 for HTTP-Message: leaving end boundary without \n

Mon Aug 27 05:29:01 2012 MARKOV [...] cpan.org - Ticket created

CC:	c.deignan [...] rsd.com
Subject:	leaving end boundary without \n

Hi Gisle, In HTTP::Message, when you parse multiparts, You use this: (around line 1000) $str =~ s/\r?\n--\Q$b\E--\r?\n.*//s; if ($str =~ s/(^|.*?\r?\n)--\Q$b\E\r?\n//s) { $self->{_parts} = [map HTTP::Message->parse($_). split(/\r?\n--\Q$b\E\r?\n/, $str)] } According to RFC2046 section 5.1.1 page 19: --gc0pJq0M:08jU534c0p-- NOTE TO IMPLEMENTORS: Boundary string comparisons must compare the boundary value with the beginning of each candidate line. An exact match of the entire candidate line is not required; it is sufficient that the boundary appear in its entirety following the CRLF. It is a bit hard to read, but I (and some implementation) read this as: there doesn't need to be a \n after the -- of the closing boundary. The regex is therefore to strict, requiring an \n - $str =~ s/\r?\n--\Q$b\E--\r?\n.*//s; + $str =~ s/\r?\n--\Q$b\E--.*//s; [[ side note: I think OO likes to see HTTP::Message->parse($_) to be rewritten als (ref $self)->parse($_) ]] Thanks in advance, MarkOv

Sat Oct 20 18:04:34 2012 GAAS [...] cpan.org - Correspondence added

Your sugged fix was applied as https://github.com/libwww-perl/http-message/commit/c1bac162b83094764ed4f8d8737c5367ec02305c

Regarding your side note, I think it should stay as HTTP::Message->parse($_). The actual class of $self is likely to be HTTP::Request or HTTP::Response, but that's not the kind of stuff to expect in the parts.

Sat Oct 20 18:04:34 2012 The RT System itself - Status changed from 'new' to 'open'

Sat Oct 20 18:04:34 2012 GAAS [...] cpan.org - Status changed from 'open' to 'resolved'

Sat Oct 20 18:18:43 2012 MARKOV [...] cpan.org - Correspondence added

On Sat Oct 20 18:04:34 2012, GAAS wrote: Show quoted text

> Your sugged fix was applied as > https://github.com/libwww-perl/http- > message/commit/c1bac162b83094764ed4f8d8737c5367ec02305c > > Regarding your side note, I think it should stay as HTTP::Message-

> >parse($_).

> The actual class of $self is likely to be HTTP::Request or > HTTP::Response, but > that's not the kind of stuff to expect in the parts.

Thanks for fixing. Also, I am really happy with the split-up of LWP you did last year. Thanks. On the latter: the difference shows itself when one of the extensions has additional needs for parse(). Your own extensions do not need that... however, other people (like me) may also extend your classes. You never know. There is no reason to force method lookup here. The usual way is to make parse() a combined class/instance method. In that case you can simply call $self->parse()

Sat Oct 20 18:18:43 2012 The RT System itself - Status changed from 'resolved' to 'open'

Sun Oct 21 03:17:53 2012 GAAS [...] cpan.org - Correspondence added

On Sat Oct 20 18:18:43 2012, MARKOV wrote:
Show quoted text

> On the latter: the difference shows itself when one of the extensions
> has additional needs for parse(). Your own extensions do not need
> that... however, other people (like me) may also extend your classes.
> You never know. There is no reason to force method lookup here.

That's right. I was thinking about arguing for why I would not remove this hardcoding until somebody came up with a use case for it, but decided that it was easier to explain it by how it would have to be implemented; so I just did that https://github.com/libwww-perl/http-message/commit/5f0b18944eabcd2883e797818a84374138b40140.

The fact that ref($self)->new() wasn't the right solution here is an indication that it was a mistake to subclass HTTP::Request and HTTP::Response from HTTP::Message. A more proper design would have been to use composition. An HTTP::Request would then consist of a "request line" and a message and would then just use delegation to make it as easy to use. After all I did not make HTTP::Message a subclass of HTTP::Headers and it's basically the same relationship there. Oh, well. Next time I'll design this better. :-)

When this design came about HTTP::Message was abstract. It was there purely for sharing the implementation between requests and responses. The multipart support came later and suddenly it made sense to also instantiate pure messages.

Sun Oct 21 10:20:22 2012 solutions [...] overmeer.net - Correspondence added

CC:	c.deignan [...] rsd.com
Subject:	Re: [rt.cpan.org #79239] leaving end boundary without \n
Date:	Sun, 21 Oct 2012 16:20:02 +0200
To:	Gisle_Aas via RT <bug-HTTP-Message [...] rt.cpan.org>
From:	Mark Overmeer <solutions [...] overmeer.net>

* Gisle_Aas via RT (bug-HTTP-Message@rt.cpan.org) [121021 07:18]: Show quoted text

> The fact that ref($self)->new() wasn't the right solution here is an indication > that it was a mistake to subclass HTTP::Request and HTTP::Response from > HTTP::Message.

IMO, that is a valid design. To parties talk in messages, which are just slightly different... the HTTP::Message is the important class, not these tiny extensions. I do not see any benefit in _part_class. But... there no single truth in OO. For Mail::Box, I had to implement the same thing (different kinds of mail-folders, in this case). The multipart-part are in Mail::Message::Part which extends Mail::Message. That works very well. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net

Wed Jan 16 13:39:47 2013 ether [...] cpan.org - Cc ETHER added

Wed Jul 08 20:38:28 2015 ether [...] cpan.org - Correspondence added

RT-Send-CC:

c.deignan [...] rsd.com, solutions [...] overmeer.net

The changes that were already in git have been released in 6.07. Please verify!

Thu Mar 30 20:59:17 2017 olaf [...] wundersolutions.com - Correspondence added

Ticket migrated to github as https://github.com/libwww-perl/HTTP-Message/issues/71

Thu Mar 30 20:59:28 2017 olaf [...] wundersolutions.com - Status changed from 'open' to 'resolved'