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

BUG: HEAD requests return a body #5

Open
yaronyg opened this issue Oct 26, 2013 · 7 comments
Open

BUG: HEAD requests return a body #5

yaronyg opened this issue Oct 26, 2013 · 7 comments

Comments

@yaronyg
Copy link
Contributor

yaronyg commented Oct 26, 2013

CBLRouter.java intentionally treats a HEAD as a GET which means a body gets returned which causes all sorts of problems for libraries like Ektorp.

@tleyden
Copy link
Contributor

tleyden commented Oct 28, 2013

Reposting comment from mailing list in case this is useful:

In CBLRouter.java on line 256 HEAD is turned into a GET. That decision turns out to have some ugly implications. Specifically it means that the response to the HEAD request contains a body. Per section 9.4 of RFC 2616 that is explicitly illegal. But lots of illegal things happen all the time. The reason this one matters is that it causes Ektorp to lose its mind. Or maybe it's the StdHttpClient that is losing its mind. Hard to say. But I get the same broken behavior when I use AndroidHttpClient.

I have traces below that document the stack losing its mind. I ran exactly the same Ektorp code against Erlang CouchDB and it worked just fine. So this is caused by the behavior of the Android Listener. Note btw, especially in the first request below, that the Android Listener also loses its mind. So I don't know if the HEAD request fix is enough. There might be other evil things going on here. See the traces for details.

My plan right now is to try and figure out how to build couchbase-lite-android-core to fix the bug in CBLRouter.java. Then to build the Listener project with the new couchbase lite core and then see if that fixes anything.

I'm posting this just in case someone already knows about this and can save me time with statements like: No! Stop! That won't work! :)

Or better yet: Yeah, we know, here's the fix!

In any case the proof of the evil this causes is below. Also note at the very end of this mail I have the actual code I'm using.

Please take a look at the trace below. What do you notice about it? There are at least two errors. The first error is that the Android Listener responded with a response body to a HEAD request. Per section 9.4 of RFC 2616 "The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response." So what is that response body doing there? Now look at the POST. We just did a HEAD request for /test/ that return 200 OK. Now we do a POST to /test/ and we get a 404? There were no other requesters so um... what the heck?

Now, please, scroll down past the example. Because it gets worse.

HEAD /test/ HTTP/1.1
Via: 1.1 localhost (Apache-HttpClient/4.2.5 (cache))
Host: 127.0.0.1:9898
Connection: Keep-Alive

HTTP/1.1 200 Ok
keep-alive: timeout=30, max=100
server: D. Rogatkin's TJWS with Android support (Acme.Serve)/Version 1.94, $Revision: 1.236 $,Couchbase Lite 1.0.0-beta
date: Fri, 25 Oct 2013 21:50:57 GMT
mime-version: 1.0
content-type: application/json
connection: keep-alive
transfer-encoding: chunked

73
{"disk_size":102400,"update_seq":4,"doc_count":4,"db_uuid":"7ea8b20e-9c03-40be-9c9e-75d29f51ceab","db_name":"test"}
0

POST /test/ HTTP/1.1
Via: 1.1 localhost (Apache-HttpClient/4.2.5 (cache))
Content-Length: 56
Content-Type: application/json
Host: 127.0.0.1:9898
Connection: Keep-Alive
Expect: 100-continue

HTTP/1.1 404 Not found
keep-alive: timeout=30, max=100
server: D. Rogatkin's TJWS with Android support (Acme.Serve)/Version 1.94, $Revision: 1.236 $,Couchbase Lite 1.0.0-beta
date: Fri, 25 Oct 2013 21:51:07 GMT
mime-version: 1.0
content-type: application/json
connection: keep-alive
transfer-encoding: chunked

80
{"error":"not_found","reason":"CBLRouter unable to route request to do_POST_Databasejava.lang.reflect.InvocationTargetException"
1
}
0

I repeated the exact same code, literally no changes. But now see what happened. First, I got a bunch of protocol errors from my sniffer, it complained:

6 [HTTPLint] The HTTP Chunked response body was incomplete; most likely lacking the final 0-size chunk.
7 Content-Length mismatch: Request Header indicated 56 bytes, but client sent 0 bytes.
7 Cannot parse HTTP response; Status line contains no spaces. Data:
{"disk_size":102400,"update_seq":4,"doc_count":4,"db_uuid":"7ea8b20e-9c03-40be-9c9e-75d29f51ceab","db_name":"test"}

To see what it's complaining about please look at the trace below. Notice what's happening now. Now the HEAD suddenly doesn't have a response body. Which is good, but um... why was it in the first response? But wait, it gets better. Do you notice that transfer-encoding: chunked? Why is there a transfer encoding on a response with no content? Except... well it sorta does have content. A chunk with 0x73 (aka 115) but nothing attached to it. What's interesting is that if you scroll up and look at the HEAD response above it was also exactly 0x73 bytes. So what seems to be happening is that the server wants to return a response that is 0x73 bytes long but doesn't. Thus producing malformed HTTP. Note, btw, that if you look at the second POST it somehow 'knocked loose' that content but now it's just a completely syntactic mess so the whole connection collapses.

Please, just one more scroll! Please scroll past this example.

HEAD /test/ HTTP/1.1
Via: 1.1 localhost (Apache-HttpClient/4.2.5 (cache))
Host: 127.0.0.1:9898
Connection: Keep-Alive

HTTP/1.1 200 Ok
keep-alive: timeout=30, max=100
server: D. Rogatkin's TJWS with Android support (Acme.Serve)/Version 1.94, $Revision: 1.236 $,Couchbase Lite 1.0.0-beta
date: Fri, 25 Oct 2013 22:01:12 GMT
mime-version: 1.0
content-type: application/json
connection: keep-alive
transfer-encoding: chunked

73
POST /test/ HTTP/1.1
Via: 1.1 localhost (Apache-HttpClient/4.2.5 (cache))
Content-Length: 56
Content-Type: application/json
Host: 127.0.0.1:9898
Connection: Keep-Alive
Expect: 100-continue

HTTP/1.1 500 Fiddler - Bad Response
Date: Fri, 25 Oct 2013 22:01:24 GMT
Content-Type: text/html; charset=UTF-8
Connection: close
Timestamp: 15:01:24.247

[Fiddler] Response Header parsing failed. This can be caused by an illegal HTTP response earlier on this reused server socket-- for instance, a HTTP/304 response which illegally contains a body. Response Data: <plaintext> 7B 22 64 69 73 6B 5F 73 69 7A 65 22 3A 31 30 32 34 30 30 2C 22 75 70 64 {"disk_size":102400,"upd
61 74 65 5F 73 65 71 22 3A 34 2C 22 64 6F 63 5F 63 6F 75 6E 74 22 3A 34 ate_seq":4,"doc_count":4
2C 22 64 62 5F 75 75 69 64 22 3A 22 37 65 61 38 62 32 30 65 2D 39 63 30 ,"db_uuid":"7ea8b20e-9c0
33 2D 34 30 62 65 2D 39 63 39 65 2D 37 35 64 32 39 66 35 31 63 65 61 62 3-40be-9c9e-75d29f51ceab
22 2C 22 64 62 5F 6E 61 6D 65 22 3A 22 74 65 73 74 22 7D 0D 0A 30 0D 0A ","db_name":"test"}..0..
0D 0A ..

So I try the requests again and now get different errors!

8 [HTTPLint] The HTTP Chunked response body was incomplete; most likely lacking the final 0-size chunk.
9 Cannot parse HTTP response; Status line contains no spaces. Data: 73

Now look. Same HEAD requests but now the response really don't have a response body at all. Although it does claim to use transfer-encoding: chunked which isn't good. But look carefully at the POST response. Other than the fact that it completely screwed up the syntax the response is a success!!!!!! I think this is the point where I break down into tears.

HEAD /test/ HTTP/1.1
Via: 1.1 localhost (Apache-HttpClient/4.2.5 (cache))
Host: 127.0.0.1:9898
Connection: Keep-Alive

HTTP/1.1 200 Ok
keep-alive: timeout=30, max=100
server: D. Rogatkin's TJWS with Android support (Acme.Serve)/Version 1.94, $Revision: 1.236 $,Couchbase Lite 1.0.0-beta
date: Fri, 25 Oct 2013 22:07:18 GMT
mime-version: 1.0
content-type: application/json
connection: keep-alive
transfer-encoding: chunked

POST /test/ HTTP/1.1
Via: 1.1 localhost (Apache-HttpClient/4.2.5 (cache))
Content-Length: 56
Content-Type: application/json
Host: 127.0.0.1:9898
Connection: Keep-Alive
Expect: 100-continue

{"blogArticleName":"foo","blogArticleContent":"AbcDef!"}
HTTP/1.1 500 Fiddler - Bad Response
Date: Fri, 25 Oct 2013 22:07:21 GMT
Content-Type: text/html; charset=UTF-8
Connection: close
Timestamp: 15:07:21.565

HTTP/1.1 201 Created
etag: "1-67c151c0-19ce-4583-aac5-0e8be709ffe2"
location: cblite:///test/
mime-version: 1.0
content-type: application/json
keep-alive: timeout=30, max=100
server: D. Rogatkin's TJWS with Android support (Acme.Serve)/Version 1.94, $Revision: 1.236 $,Couchbase Lite 1.0.0-beta
date: Fri, 25 Oct 2013 22:07:20 GMT
connection: keep-alive
transfer-encoding: chunked

66
{"id":"daa1ecd5-403a-433f-9748-57b72002062d","rev":"1-67c151c0-19ce-4583-aac5-0e8be709ffe2","ok":true}
0

   final Logger log = LoggerFactory.getLogger(Main.class);

    String hostName = "127.0.0.1";

    int port = 9898; // Port I use on Android
    //int port = 5984; // Default couch port
    HttpClient httpClient = new StdHttpClient.Builder().host(hostName).port(port).proxy("127.0.0.1").proxyPort(8888).build(); // Routing through Fiddler2
    //HttpClient httpClient = new StdHttpClient.Builder().host(hostName).port(port).build(); // Direct connection

    CouchDbInstance couchDbInstance = new StdCouchDbInstance(httpClient);
    CouchDbConnector couchDbConnector = couchDbInstance.createConnector("test", true);

    TestBlogClass testArticle = new TestBlogClass();
    String blogArticleName = "foo";
    String blogArticleContent = "AbcDef!";
    testArticle.setBlogArticleName(blogArticleName);
    testArticle.setBlogArticleContent(blogArticleContent);
    couchDbConnector.create(testArticle);

    String id = testArticle.getId();
    String revision = testArticle.getRevision();

    TestBlogClass checkArticle = couchDbConnector.get(TestBlogClass.class, id);

    if (checkArticle.getId().equals(id) == false || checkArticle.getRevision().equals(revision) == false ||
            checkArticle.getBlogArticleName().equals(blogArticleName) == false ||
            checkArticle.getBlogArticleContent().equals(blogArticleContent) == false) {
        log.error("The article we got back didn't match the one we sent.");
        throw new RuntimeException("oops");
    }

@tleyden
Copy link
Contributor

tleyden commented Oct 28, 2013

Comment from @snej:

Sounds like a simple oversight in the Java port; no need for length arguments to convince us! The Objective-C version skips sending the response body if the request is a HEAD (see CBL_Router.m:593 in the current sources) but that probably got lost in translation.

Could you file an issue against Couchbase Lite-Android in Github? Just note that HEAD shouldn’t return a body.

@tleyden
Copy link
Contributor

tleyden commented Oct 28, 2013

@tahmmee would it be possible to add a test to the REST api tests to detect this issue?

@andreibaranouski
Copy link

yeah, just today I noticed that we do not have tests for HEAD request
note, the CouchDB API Reference contains only one mention about HEAD:
http://docs.couchdb.org/en/latest/api/reference.html
http://docs.couchdb.org/en/latest/api/documents.html#head-db-doc

@yaronyg
Copy link
Contributor Author

yaronyg commented Oct 30, 2013

The specs don't need to reference HEAD because any resource that supports GET must support HEAD and the behavior is specified by RFC 2616. So HEAD support is 'automatic'. For what it's worth Ektorp uses Head to test if a DB exists.

From: Andrei Baranouski [mailto:notifications@github.com]
Sent: Wednesday, October 30, 2013 10:20 AM
To: couchbase/couchbase-lite-android-listener
Cc: Yaron Goland
Subject: Re: [couchbase-lite-android-listener] HEAD requests return a body (#5)

yeah, just today I noticed that we do not have tests for HEAD request
note, the CouchDB API Reference contains only one mention about HEAD:
http://docs.couchdb.org/en/latest/api/reference.html
http://docs.couchdb.org/en/latest/api/documents.html#head-db-doc


Reply to this email directly or view it on GitHubhttps://github.com//issues/5#issuecomment-27414835.

@tleyden
Copy link
Contributor

tleyden commented Oct 30, 2013

@andreibaranouski / @tahmmee seems like we should add a test for correct HEAD behavior on at least a few of the REST api calls. What do you guys think?

@andreibaranouski
Copy link

ticket for implementation http://www.couchbase.com/issues/browse/CBQE-1618

@hideki hideki added the backlog label Jun 23, 2016
@hideki hideki added icebox and removed backlog labels Oct 8, 2016
@hideki hideki removed this from the Future milestone Oct 8, 2016
@djpongh djpongh added this to the 1.4.2 milestone Nov 28, 2017
@djpongh djpongh modified the milestones: 1.4.2, 1.4.x Dec 7, 2017
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

6 participants