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

HttpServer returns a body with a HEAD request if an error occurs #3116

Closed
pvanek opened this Issue Nov 15, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@pvanek
Copy link
Contributor

pvanek commented Nov 15, 2018

I'm calling sequence of HTTP requests against a minimalistic HTTP server (qore's one) using minimalistic handler, which always returns 501 Not Implemented with result:

  • OPTIONS = ok
  • GET = ok
  • HEAD = ok
  • POST = malformed response (?)

Test script:

#!/usr/bin/env qore

%new-style
%enable-all-warnings

%requires HttpServer
%requires QUnit

%exec-class Main

# logger for http server
sub http_log(string str) {
    #printf("%N: %s\n", now_us(), vsprintf(str, argv));
}

# minimal HTTP server for this test suite
class Server inherits HttpServer {
    constructor() : HttpServer(\http_log(), \http_log()) {
        MyHandler handler();
        setHandler("webdav", "/", NOTHING, handler);
        setDefaultHandler("webdav", handler);
        addListener(8888);
    }
} # class Server

class MyHandler inherits public HttpServer::AbstractHttpRequestHandler {
    constructor() : HttpServer::AbstractHttpRequestHandler() {
    }
    auto handleRequest(hash cx, hash hdr, *data body) {
        return new hash<HttpResponseInfo>({
            "code": 501,
            "body": "Not Implemented",
        });
    }
}


class Client inherits HTTPClient
{
    constructor(string url)
        : HTTPClient({"url": url,}) {
    }
}

class Main inherits QUnit::Test {

    constructor() : QUnit::Test("test", "1.0") {
        addTestCase("test", \test());
        set_return_value(main());
    }

    private test() {
        Server srv();
        on_exit {
            srv.stop();
        }

        # everything works if I move Client instance into the while
        # loop to be initialized for each call
        Client http("http://localhost:8888");

        # order of methods is critical here to reproduce it
        hash methods = {
            "OPTIONS": True,
            "GET": True,
            "HEAD": True,
            "POST": True, # <-- this method gets completely wrong response
        };

        HashIterator it(methods);
        while (it.next()) {
            try {
                if (m_options.verbose) {
                    printf("sending %s - %s\n", it.getKey(), it.getValue());
                }
                auto ret = http.send(NOTHING, it.getKey(), "/");
                # some methods are passing through (OPTIONS)
                assertEq(200, ret.status_code);
            }
            catch (hash ex) {
#            printf("%N\n", ex);
                assertEq("HTTP-CLIENT-RECEIVE-ERROR", ex.err, it.getKey() + ": " + ex.err);
                assertEq(True, ex.desc =~ /501.*Not Implemented/, it.getKey() + ": " + ex.desc);
            }
        }
    }
} # class Main

There is running this curl command in parallel always returning correct response. It's using one connection, so I assume the problem is in Qore's HTTPCleint:

pvanek@spongebob:~> curl -I -X OPTIONS http://localhost:8888 -X GET http://localhost:8888 -X HEAD http://localhost:8888 -I -X POST http://localhost:8888
HTTP/1.1 501 Not Implemented
Server: Qore-HTTP-Server/0.3.11.1
X-Powered-By: Qore/0.9.0
Content-Type: text/html;charset=utf8
Connection: Keep-Alive
Content-Length: 187

HTTP/1.1 501 Not Implemented
Server: Qore-HTTP-Server/0.3.11.1
X-Powered-By: Qore/0.9.0
Content-Type: text/html;charset=utf8
Connection: Keep-Alive
Content-Length: 187

HTTP/1.1 501 Not Implemented
Server: Qore-HTTP-Server/0.3.11.1
X-Powered-By: Qore/0.9.0
Content-Type: text/html;charset=utf8
Connection: Keep-Alive
Content-Length: 187

HTTP/1.1 501 Not Implemented
Server: Qore-HTTP-Server/0.3.11.1
X-Powered-By: Qore/0.9.0
Content-Type: text/html;charset=utf8
Connection: Keep-Alive
Content-Length: 187

This is a result of plain qore run:

pvanek@spongebob:~> qore ~/t.q
QUnit Test "test" v1.0
FAILURE: test: 8 assertions, 7 succeeded
Assertion "POST: no HTTP status code received in response" failure at /home/pvanek/t.q:83 [Main::test()]
-----
        >> Expected: True, Actual: False
-----
Ran 1 test case, 1 error (8 assertions, 7 succeeded)

This is a result of qore with some printfs:

pvanek@spongebob:~/src/qore/qore/test/qlib/WebDAV (feature/2995-webdav *$%=)> qore ~/t.q
QUnit Test "test" v1.0
1: send - OPTIONS - 
2: /
3: (null)
HDR: 1: 0
HDR: 2: OPTIONS / HTTP/1.1
Accept: text/html
Accept-Encoding: deflate,gzip,bzip2
Connection: Keep-Alive
User-Agent: Qore-HTTP-Client/0.9.0
Host: localhost:8888

HDR: 1: 0
HDR: 2: HTTP/1.1 501 Not Implemented
Server: Qore-HTTP-Server/0.3.11.1
X-Powered-By: Qore/0.9.0
Content-Type: text/html;charset=utf8
Connection: Keep-Alive
Content-Length: 187

info: no info
 ans: hash: (8 members)
    http_version : "1.1"
    status_code : 501
    status_message : "Not Implemented"
    server : "Qore-HTTP-Server/0.3.11.1"
    x-powered-by : "Qore/0.9.0"
    content-type : "text/html;charset=utf8"
    connection : "Keep-Alive"
    content-length : "187"
1: send - GET - 
2: /
3: (null)
HDR: 1: 0
HDR: 2: GET / HTTP/1.1
Accept: text/html
Accept-Encoding: deflate,gzip,bzip2
Connection: Keep-Alive
User-Agent: Qore-HTTP-Client/0.9.0
Host: localhost:8888

HDR: 1: 0
HDR: 2: HTTP/1.1 501 Not Implemented
Server: Qore-HTTP-Server/0.3.11.1
X-Powered-By: Qore/0.9.0
Content-Type: text/html;charset=utf8
Connection: Keep-Alive
Content-Length: 187

info: no info
 ans: hash: (8 members)
    http_version : "1.1"
    status_code : 501
    status_message : "Not Implemented"
    server : "Qore-HTTP-Server/0.3.11.1"
    x-powered-by : "Qore/0.9.0"
    content-type : "text/html;charset=utf8"
    connection : "Keep-Alive"
    content-length : "187"
1: send - HEAD - 
2: /
3: (null)
HDR: 1: 0
HDR: 2: HEAD / HTTP/1.1
Accept: text/html
Accept-Encoding: deflate,gzip,bzip2
Connection: Keep-Alive
User-Agent: Qore-HTTP-Client/0.9.0
Host: localhost:8888

HDR: 1: 0
HDR: 2: HTTP/1.1 501 Not Implemented
Server: Qore-HTTP-Server/0.3.11.1
X-Powered-By: Qore/0.9.0
Content-Type: text/html;charset=utf8
Connection: Keep-Alive
Content-Length: 187

info: no info
 ans: hash: (8 members)
    http_version : "1.1"
    status_code : 501
    status_message : "Not Implemented"
    server : "Qore-HTTP-Server/0.3.11.1"
    x-powered-by : "Qore/0.9.0"
    content-type : "text/html;charset=utf8"
    connection : "Keep-Alive"
    content-length : "187"
1: send - POST - 
2: /
3: (null)
HDR: 1: 0
HDR: 2: POST / HTTP/1.1
Accept: text/html
Accept-Encoding: deflate,gzip,bzip2
Connection: Keep-Alive
User-Agent: Qore-HTTP-Client/0.9.0
Host: localhost:8888

HDR: 1: 0
HDR: 2: <html><head><title>501 Not Implemented</title></head><body><h1>Not Implemented</h1><pre>Not Implemented</pre><p><hr><address>Qore-HTTP-Server/0.3.11.1 on spongebob</address></body></html>HTTP/1.1 501 Not Implemented
Server: Qore-HTTP-Server/0.3.11.1
X-Powered-By: Qore/0.9.0
Content-Type: text/html;charset=utf8
Connection: Keep-Alive
Content-Length: 187

info: no info
 ans: hash: (8 members)
    http_version : "1.1"
    method : "<html><head><title>501"
    path : "Not"
    server : "Qore-HTTP-Server/0.3.11.1"
    x-powered-by : "Qore/0.9.0"
    content-type : "text/html;charset=utf8"
    connection : "Keep-Alive"
    content-length : "187"
FAILURE: test: 8 assertions, 7 succeeded
Assertion "POST: no HTTP status code received in response" failure at /home/pvanek/t.q:83 [Main::test()]
-----
        >> Expected: True, Actual: False
-----
Ran 1 test case, 1 error (8 assertions, 7 succeeded)

@davidnich davidnich changed the title HTTPClient wrong response in particular call sequence HttpServer returns a body with a HEAD request if an error occurs Nov 15, 2018

@davidnich

This comment has been minimized.

Copy link
Contributor

davidnich commented Nov 15, 2018

The problem is that the HttpServer module is violating RFC-2616 4.4 p1:

        1.Any response message which "MUST NOT" include a message-body (such
        as the 1xx, 204, and 304 responses and any response to a HEAD
        request) is always terminated by the first empty line after the
        header fields, regardless of the entity-header fields present in
        the message.

@davidnich davidnich added this to the 0.8.13.9 milestone Nov 15, 2018

@davidnich davidnich added not-c++ and removed invisible c++ labels Nov 15, 2018

@davidnich davidnich self-assigned this Nov 15, 2018

@davidnich davidnich removed the not-c++ label Nov 15, 2018

@davidnich

This comment has been minimized.

Copy link
Contributor

davidnich commented Nov 15, 2018

there is also a bug in the HTTPClient class regarding 304 Not Modified message handling

@pvanek

This comment has been minimized.

Copy link
Contributor Author

pvanek commented Nov 15, 2018

proposed fix for HTTPServer

diff --git a/qlib/HttpServer.qm b/qlib/HttpServer.qm
index 999abc97b..e6ee8ec69 100644
--- a/qlib/HttpServer.qm
+++ b/qlib/HttpServer.qm
@@ -1643,14 +1643,19 @@ http.setListenerLogOptionsID(0, LLO_RECV_HEADERS|LLO_SEND_HEADERS);
         rv.hdr = self.hdr + rv.hdr;
         if (rv.code >= 300 && rv.code < 400) {
             # handle redirect msgs (RFC 2616 section 10.3 http://tools.ietf.org/html/rfc2616#section-10.3)
-            s.sendHTTPResponse(rv.code, HttpServer::HttpCodes.(rv.code), "1.1", rv.hdr, rv.body);
+            s.sendHTTPResponse(rv.code, HttpServer::HttpCodes.(rv.code), "1.1", rv.hdr,
+                               (head ? NOTHING : rv.body));
             listener.logResponse(cx, rv);
         }
         else if (rv.code >= 400) {
             if (!rv.body)
                 rv.body = sprintf("unknown error in %s handler", cx.handler_name);
 
-            sendHttpError(listener, cx, s, rv.code, rv.body, rv.hdr, cx."response-encoding");
+            if (head) {
+                s.sendHTTPResponse(rv.code, HttpServer::HttpCodes{rv.code}, "1.1", rv.hdr);
+            } else {
+                sendHttpError(listener, cx, s, rv.code, rv.body, rv.hdr, cx."response-encoding");
+            }
         }
         else {
             HttpServer::http_set_reply_headers(s, cx, \rv);

davidnich added a commit that referenced this issue Nov 15, 2018

refs #3116 fixed handling 304 Not Modified responses in the HTTPClien…
…t class, fixed sending responses to HEAD messages + 100, 204, 304 responses (cannot be sent with a message body) in the HttpServer module

davidnich added a commit that referenced this issue Nov 15, 2018

pvanek added a commit that referenced this issue Nov 15, 2018

Merge pull request #3117 from qorelanguage/bugfix/3116_HttpServer_fix
refs #3116 fixed handling 304 Not Modified responses in the HTTPClien…

davidnich added a commit that referenced this issue Nov 15, 2018

@davidnich davidnich added the fixed label Nov 15, 2018

@davidnich davidnich closed this Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment