Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mojo::UserAgent can't open https://www.justhappy.fr #1248

Closed
kraih opened this issue Aug 7, 2018 · 4 comments
Closed

Mojo::UserAgent can't open https://www.justhappy.fr #1248

kraih opened this issue Aug 7, 2018 · 4 comments

Comments

@kraih
Copy link
Member

kraih commented Aug 7, 2018

This seems to be a problem with our empty HTTP response detection code. It works if we remove the expect_close attribute. But that of course results in a few existing tests breaking and doesn't seem exactly compatible with the HTTP spec. This RFC section seems related.

diff --git a/lib/Mojo/Content.pm b/lib/Mojo/Content.pm
index 0a5788b91..a2b79629b 100644
--- a/lib/Mojo/Content.pm
+++ b/lib/Mojo/Content.pm
@@ -6,7 +6,7 @@ use Compress::Raw::Zlib qw(WANT_GZIP Z_STREAM_END);
 use Mojo::Headers;
 use Scalar::Util 'looks_like_number';

-has [qw(auto_decompress auto_relax expect_close relaxed skip_body)];
+has [qw(auto_decompress auto_relax relaxed skip_body)];
 has headers           => sub { Mojo::Headers->new };
 has max_buffer_size   => sub { $ENV{MOJO_MAX_BUFFER_SIZE} || 262144 };
 has max_leftover_size => sub { $ENV{MOJO_MAX_LEFTOVER_SIZE} || 262144 };
@@ -107,8 +107,7 @@ sub parse {
   my $len     = $headers->content_length // '';
   if ($self->auto_relax && !length $len) {
     my $connection = lc($headers->connection // '');
-    $self->relaxed(1)
-      if $connection eq 'close' || (!$connection && $self->expect_close);
+    $self->relaxed(1) if $connection eq 'close' || !$connection;
   }

   # Chunked or relaxed content
@@ -365,13 +364,6 @@ Decompress content automatically if L</"is_compressed"> is true.

 Try to detect when relaxed parsing is necessary.

-=head2 expect_close
-
-  my $bool = $content->expect_close;
-  $content = $content->expect_close($bool);
-
-Expect a response that is terminated with a connection close.
-
 =head2 headers

   my $headers = $content->headers;
diff --git a/lib/Mojo/Message/Response.pm b/lib/Mojo/Message/Response.pm
index 8cc9479bb..0998ce28f 100644
--- a/lib/Mojo/Message/Response.pm
+++ b/lib/Mojo/Message/Response.pm
@@ -101,7 +101,6 @@ sub extract_start_line {
   my $content = $self->content;
   $content->skip_body(1) if $self->code($2)->is_empty;
   defined $content->$_ or $content->$_(1) for qw(auto_decompress auto_relax);
-  $content->expect_close(1) if $1 eq '1.0';
   return !!$self->version($1)->message($3);
 }
@kraih
Copy link
Member Author

kraih commented Aug 7, 2018

It's very easy to see what's going on with MOJO_CLIENT_DEBUG=1 perl -Ilib script/mojo get https://www.justhappy.fr.

@jberger
Copy link
Member

jberger commented Aug 7, 2018

Reading the specs (snipped here) I think that the logical action is that in Mojo::Content::parse if no Content-Length header was received and not in one of the listed special cases, then we set relaxed parsing and possibly somehow taint the connect to prevent its reuse. That tainting might be unnecessary because it practice it doesn't appear that there is any way that the connection would attempt to be reused because nothing will cause the parsing phase to end until the connection is otherwise closed or timed out (and thus closed).

With relaxed manually set the body is indeed processed as seen by running

perl -Mojo -E 'my $ua = app->ua; my $tx = $ua->build_tx(GET => "https://www.justhappy.fr"); $tx->res->content->relaxed(1); $ua->start($tx); print $tx->res->body'

@dmanto
Copy link
Contributor

dmanto commented Aug 8, 2018

The following script could be the base for the corresponding test. It doesn't pass because there is not Content-Length header in the server answer.

use Mojo::Base -strict;
BEGIN { $ENV{MOJO_REACTOR} = 'Mojo::Reactor::Poll'; }
use Test::More;
use Mojo::IOLoop;
use Mojo::UserAgent;

# based on RFC7230 Server response example (after deleting Content-Length header)
my $ans = <<'FIN';
HTTP/1.1 200 OK
Date: Mon, 27 Jul 2009 12:28:53 GMT
Server: Apache
Last-Modified: Wed, 22 Jul 2009 19:15:56 GMT
ETag: "34aa387-d-1568eb00"
Accept-Ranges: bytes
Vary: Accept-Encoding
Content-Type: text/plain

Hello World! My payload includes a trailing CRLF.

FIN

my $buffer = '';
my $id     = Mojo::IOLoop->server(
  {address => '127.0.0.1'} => sub {
    my ($loop, $stream) = @_;
    $stream->on(
      read => sub {
        my ($stream, $chunk) = @_;
        $buffer .= $chunk;
        return unless $buffer =~ /^\s*$/m;
        $stream->write($ans => sub { shift->close });
      }
    );
  }
);
my $port = Mojo::IOLoop->acceptor($id)->port;
my $ua   = Mojo::UserAgent->new;
my $tx;
$ua->get_p("http://127.0.0.1:$port/")->then(sub {
  $tx = shift;
  Mojo::IOLoop->stop;
});
Mojo::IOLoop->start;
ok $tx->success, "get succeded";
is $tx->result->body, "Hello World! My payload includes a trailing CRLF.\n\n",
  "gets payload";
done_testing;

The test script will pass whith the current version of Mojo::UserAgent if you manually insert the rigth header:

Content-Length: 51

among the other headers in the $ans variable, as it is in the example taken from RFC 7230 section 2.1.

@fskale
Copy link

fskale commented Aug 8, 2018

Fyi it should be pointed out, that, when using curl, the alproxy sends the Content-Length Header.
So, Imho, there's some client Agent (or anything else) detection going on. It's a mass hosters environment with an own proxy solution. Didn't find anything out about alproxy.

curl -v https://www.justhappy.fr 2>&1

Request:
> GET / HTTP/1.1
> Host: www.justhappy.fr
> User-Agent: curl/7.61.0
> Accept: */*

Answer snippet:

{ [5 bytes data]
< HTTP/1.1 200 OK
< Content-Type: text/html; charset=utf-8
< Content-Length: 104807
< Vary: Accept-Encoding
< ETag: W/"f82d9172eb1954092555394d2709a09c"
< Cache-Control: max-age=0, private, must-revalidate
< Set-Cookie: < X-Request-Id: 73f47387-cbce-4050-9658-d14995093525
< Via: 1.1 alproxy
< Date: Wed, 08 Aug 2018 05:27:58 GMT
<

@kraih kraih closed this as completed in 8820911 Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants