Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

HEAD, 1xx, 204, 304 have content-length but do not have body #251

Open
huyuguang opened this issue Jun 6, 2015 · 5 comments · May be fixed by #464
Open

HEAD, 1xx, 204, 304 have content-length but do not have body #251

huyuguang opened this issue Jun 6, 2015 · 5 comments · May be fixed by #464

Comments

@huyuguang
Copy link

There are some comments in http_parser.cc, line 1837:
/* Here we call the headers_complete callback. This is somewhat
* different than other callbacks because if the user returns 1, we
* will interpret that as saying that this message has no body. This
* is needed for the annoying case of recieving a response to a HEAD
* request.
*
* We'd like to use CALLBACK_NOTIFY_NOADVANCE() here but we cannot, so
* we have to simulate it by handling a change in errno below.
*/
Excludes the HEAD request, the 1xx, 204, 304 response also have Content-Length but do not have HTTP body.

So maybe the following code should add:

    if (settings->on_headers_complete) {
      switch (settings->on_headers_complete(parser)) {
        case 0:
         if (status_code / 100 == 1 || status_code == 204 || status_code == 304)
            parser->flags |= F_SKIPBODY;
          break;
@huyuguang
Copy link
Author

Now I add the check in on_headers_complete which looks like:
settings_.on_headers_complete = this {
headers_complete_cb_called_ = true;

// if HEAD, 1xx, 204, 304, return 1 which means F_SKIPBODY
if (request_is_head_)
  return 1;

if ((status_code / 100) == 1 || status_code == 204 || status_code == 304) {
  return 1;
}
return 0;

};

But I think the check about the 1xx, 204, 304 should move to http_parser.c.

For HEAD, maybe should add a callback function will be better:

int get_request_flags() {
return HTTP_HEAD | xxx;
}

@indutny
Copy link
Member

indutny commented Jul 5, 2015

Hm... I think you are right. What are your thoughts on this @mscdex @tatsuhiro-t ?

@tatsuhiro-t
Copy link
Contributor

I'm doing same thing for those status codes in my projects.
Doing that in http-parser would be nice, and secure. It does not break sane application which already checks those codes itself.
Not sure for other non-standard application, which may break if we do this in the library.

I prefer HEAD handling as is, since adding another callback would break compatibility, unless we leave current interface.

@indutny
Copy link
Member

indutny commented Jul 16, 2015

Ok, I'm convinced! :) Let's do this thing.

@vinniefalco
Copy link
Contributor

Did this ever happen?

azaretsky added a commit to azaretsky/http-parser that referenced this issue Mar 18, 2019
@azaretsky
Always skip body for 1xx, 204, and 304 responses
227a6cb 
Fixes: nodejs#251
@azaretsky azaretsky linked a pull request Mar 18, 2019 that will close this issue
Open
azaretsky added a commit to azaretsky/http-parser that referenced this issue May 4, 2019
@azaretsky
Always skip body for 1xx, 204, and 304 responses
4b6da4f 
Fixes: nodejs#251
azaretsky added a commit to azaretsky/http-parser that referenced this issue Oct 10, 2019
@azaretsky
Always skip body for 1xx, 204, and 304 responses
068f054 
Fixes: nodejs#251
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Always skip body for 1xx, 204, and 304 responses azaretsky/http-parser
4 participants
@indutny @tatsuhiro-t @vinniefalco @huyuguang