Handle HTTP Response with no content-length header

[telliottosceola]

I ran into an issue with a Google Rest API which did not send back a content-length header or a transfer-encoding header. If this happens the HTTPClient gives the user no access to the response body in any way. So my solution is to set the _size variable to the remaining length of data available on _client which makes sense and works perfectly for me now.

[Jeroen88]

This PR is not OK in my opinion: available() just returns the number of bytes currently in the buffer, not the size of the payload. I think this coincidentally only goes right if the total data size send (headers + payload) is less than one tcp fragment length.

[me-no-dev]

I agree with @Jeroen88 here. this will only work if the whole response in in one packet.

[telliottosceola]

We need some sort of solution for this. What would be your suggestion? How do we deal with a response with no content-length? There has to be a way to handle this.

[Jeroen88]

There was quite some discussion about this at the esp8266. The conclusion is: either a server should send a Content-Length or it should send a Transfer-Encoding: chunked and the size of each chunk as a header of every chunk. No length and no encoding does not adhere to the standards and thus is not supported.

If you really need it, you could wait for the timeout and read the response anyway. Maybe you have to do some small changes in the library. Or stick to the solution you found if you are sure that your payload is less than one frame. But do not send a PR for these solutions, because both are non-standard non generic.

[telliottosceola]

100% agree that this response is not following standard protocol and thus is invalid. Oddly enough though this response is coming from Google Cloud Services which is very strange it does not follow the http response outlined protocol.

I validated that the response indeed does not have transfer-encoding or content-length headers by printing the full received packet out inside the sendRequest function just prior to the call to handleHeaderResponse. Here is the modified sendRequest function simply for diagnostic purposes:

int HTTPClient::sendRequest(const char * type, uint8_t * payload, size_t size)
{
    // connect to server
    if(!connect()) {
        return returnError(HTTPC_ERROR_CONNECTION_REFUSED);
    }

    if(payload && size > 0) {
        addHeader(F("Content-Length"), String(size));
    }

    // send Header
    if(!sendHeader(type)) {
        return returnError(HTTPC_ERROR_SEND_HEADER_FAILED);
    }

    // send Payload if needed
    if(payload && size > 0) {
        if(_client->write(&payload[0], size) != size) {
            return returnError(HTTPC_ERROR_SEND_PAYLOAD_FAILED);
        }
    }

    while(_client->connected()){
      while(_client->available() < 1);
      delay(200); //Wait for full transmission to be received
      size_t len = _client->available();
      char receivedData[len+1];
      _client->read((uint8_t*)receivedData, len);
      Serial.printf("Received Data:%s\n", receivedData);
      return 200; //return 200 just for testing purposes
    }

    // handle Server Response (Header)
    // return returnError(handleHeaderResponse());
}

Here is the printout of the received response:

Received Data:HTTP/1.1 200 OK
Content-Type: application/json; charset=utf-8
Vary: X-Origin
Vary: Referer
Date: Tue, 24 Sep 2019 15:18:17 GMT
Server: scaffolding on HTTPServer2
Cache-Control: private
X-XSS-Protection: 0
X-Frame-Options: SAMEORIGIN
X-Content-Type-Options: nosniff

Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"
Accept-Ranges: none
Vary: Origin,Accept-Encoding
Connection: close

{
  "access_token": "******Removed for privacy******",
  "expires_in": 3600,
  "token_type": "Bearer"
}

I'm not crazy right? Google is not following standard protocols on responses?

[atanisoft]

There is no requirement for the response to have a "Content-Length" header in HTTP 1.1 (ref) unless it is for backward compatibility to HTTP 1.0. Since the HTTPClient is likely sending an HTTP/1.1 GET request the server assumes it can send back the payload without the length since it is closing the connection after the payload.

Can you test with HTTP/1.0 and see if it returns a Content-Length?

[telliottosceola]

@atanisoft
Just gave that a try. Here is the response after calling useHTTP10

Received Data:HTTP/1.0 200 OK
Content-Type: application/json; charset=utf-8
Vary: X-Origin
Vary: Referer
Date: Tue, 24 Sep 2019 15:43:48 GMT
Server: scaffolding on HTTPServer2
Cache-Control: private
X-XSS-Protection: 0
X-Frame-Options: SAMEORIGIN
X-Content-Type-Options: nosniff
Alt-Svc: quic=":443"; ma=2592000; v="46,43,39"
Accept-Ranges: none

Vary: Origin,Accept-Encoding

{
  "access_token": "ya29.c.Kl6NB3_rB7cC21gtt4kSfTMDroxnAXDecpH5cXqwrcfh6IPIX0xNcDSAmEDaFa421IPl6WKH1L5rLBsVnY4Kf5rMCDlnpxBjd56m_V-u7Yj0KTLbOHlXNm26DLSKhgHU",
  "expires_in": 3600,
  "token_type": "Bearer"
}

Still no content-length or transfer-encoding headers. That said oddly enough if I send the request using Post Man there is a transfer-encoding header which is set to chunked. I have validated I am sending all the same headers to the same endpoint with the same body. The only difference I can see is Post man had an additional header like this:
Postman-Token: XXXXXXXX-XXXXXXXX-XXXXXXXX-XXXXXXXX-XXXXXXXX
That however seems arbitrary.

[telliottosceola]

For reference here is documentation on the endpoint I am sending the request to:
https://developers.google.com/identity/protocols/OAuth2ServiceAccount#makingrequest

[atanisoft]

That is odd that it doesn't return a Content-Length when there is a payload, but it doesn't technically violate the RFCs. Did you check the request headers as well?

Likely the only option will be to read until there is no data left to read (ie: available() returns zero or connection closed).

[telliottosceola]

@atanisoft,
Thanks for your input. That's pretty much what I'm doing. It's working perfectly fine for me. I just hate the thought of others running into this same problem in the future. I won't be the last person to run into this library limitation interacting with Google Cloud Services. Hopefully someone will come up with an elegant solution to this dilemma.

[atanisoft]

@telliottosceola Can you share the headers sent by PostMan and by HTTPClient? I suspect there is a different in the request headers themselves which is leading to this inconsistent behavior in the response.

[Jeroen88]

@telliottosceola I think you are not crazy ;)

@atanisoft I think it does violate what is stated in paragraph 4.4:

1. does not apply
2. Transfer-Encoding header is not present, so skip
3. Content-Length is not present, so skip
   4 Media type is not "multipart/byteranges", so skip
4. By the server closing the connection. Where on HTTP/1.2, reuse is assumed if no Connection: close is sent. close is not sent.

I may think that this line could be changed to make this use case work. If not chunked and no content length is available then asume the content is complete, else return the error. Agree?

@telliottosceola Could you test this and if it works make a PR for it

[atanisoft]

@Jeroen88 In the case of HTTP/1.1 "Connection: close" is present in the response headers so it is fine to not have the "Content-Length" header present. In the case of HTTP/1.0 though it is not returned but it is sent as part of the request as close (ref).

As for your proposed fix, I'm not sure it will do what is expected. The problem is that without a known length of the response it will be required to read until end of stream.

[Jeroen88]

@atanisoft

In the case of HTTP/1.1 "Connection: close" is present in the response headers so it is fine to not have the "Content-Length" header present

Agree, but the close header is also not sent in this case

As for your proposed fix, I'm not sure it will do what is expected. The problem is that without a known length of the response it will be required to read until end of stream.

Again agree with the second part, but reading the first reference you gave:

5.By the server closing the connection. (Closing the connection cannot be used to indicate the end of a request body, since that would leave no possibility for the server to send back a response.)

I think this is what the client will do with the change: instead of reporting a lost connections, it regards the content received as complete.

[atanisoft]

5.By the server closing the connection. (Closing the connection cannot be used to indicate the end of a request body, since that would leave no possibility for the server to send back a response.)

I think this is what the client will do with the change: instead of reporting a lost connections, it regards the content received as complete.

The RFC is more inline with the Request not the Response. The Server can (and will) do whatever it wants after it sends the last byte of the Response. It is then up to the client side to determine what the behavior is.

In the case of HTTP/1.1 "Connection: close" is present in the response headers so it is fine to not have the "Content-Length" header present

Agree, but the close header is also not sent in this case

The Request would have sent a Connection header (though it may have been keep-alive), but the Server did send "Connection: close" in the Response. In this case the client should read until there is no data and close the connection as normal and not attempt to reuse it. I don't know that the current code is parsing the response headers and checking for this scenario today.

[telliottosceola]

Here's the "code" output from postman(Sensitive information removed):

POST /token HTTP/1.1
Host: oauth2.googleapis.com
Content-Type: application/x-www-form-urlencoded
Cache-Control: no-cache
Postman-Token: dd12e2d3-dfdf-a134-1395-e4d2b158004a

grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Ajwt-bearer&assertion=Base64url encoded header.Base64url encoded claim set.Base64url encoded signature

Postman response headers:

Alt-Svc →quic=":443"; ma=2592000; v="46,43,39"
Cache-Control →private
Content-Encoding →gzip
Content-Type →application/json; charset=utf-8
Date →Tue, 24 Sep 2019 18:24:31 GMT
Server →scaffolding on HTTPServer2
Transfer-Encoding →chunked
Vary →Origin
Vary →X-Origin
Vary →Referer
X-Content-Type-Options →nosniff
X-Frame-Options →SAMEORIGIN
X-XSS-Protection →0

Postman response Body:

{
  "access_token": "******Removed******",
  "expires_in": 3600,
  "token_type": "Bearer"
}

Status was 200. I have no idea why postman is getting the Transfer-Encoding header but the HTTP Client is not. I'll work on getting the raw request from HTTP Client, it appears to send headers and then the payload so give me a minute on that.

[telliottosceola]

Headers sent by HTTPClient:

POST /token HTTP/1.1
Host: oauth2.googleapis.com
User-Agent: ESP32HTTPClient
Connection: close
Accept-Encoding: identity;q=1,chunked;q=0.1,*;q=0
Content-Type: application/x-www-form-urlencoded
Content-Length: 731

I purposely added only one header:
http.addHeader("Content-Type", "application/x-www-form-urlencoded");

I'm going to attempt removing Content-Length from the request headers since that is not included in the Postman request. Just curious at this point. I'll also added Cache-Control just to be thorough.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ lucasssvaz
❌ telliottosceola
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[github-actions]

Warnings

⚠️

Some issues found for the commit messages in this PR: the commit message "Handle HTTP Response with no content-length header": body's lines must not be longer than 100 characters summary looks empty type/action looks empty Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

👋 Hello telliottosceola, we appreciate your contribution to this project!

---

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 44659ac