Don't add default Connection header if it exists

[vkill]

Hi there,

The NIOSSL.NIOSSLError.uncleanShutdown was happened when I post https://accounts.google.com/o/oauth2/token using the following code, because the Connection: close header was automatic added.

// main.swift

import AsyncHTTPClient
import NIO

let eventLoop = MultiThreadedEventLoopGroup(numberOfThreads: 1).next()
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoop))

try httpClient.post(url: "https://accounts.google.com/o/oauth2/token", body: .string("")).flatMap { response -> EventLoopFuture<()> in
    print(response.status)
    return eventLoop.makeSucceededFuture(())
}.flatMapError { error in
    // NIOSSL.NIOSSLError.uncleanShutdown happened
    print(error)
    return eventLoop.makeFailedFuture(error)
}.wait()

So, I think we should not add it if user provide Connection header. In this case, user can make request by try HTTPClient.Request(url: "https://accounts.google.com/o/oauth2/token", method: .POST, headers: .init([("Connection", "keep-alive")]), body: .string("")).

Please review it, thx.

[swift-server-bot]

Can one of the admins verify this patch?

[Lukasa]

I think I disagree with this patch. Right now async-http-client doesn't support keep-alive connections, so Connection: close is the correct header field to send to tell the server we aren't going to re-use this.

What should actually happen is that async-http-client should swallow the uncleanShutdown error and use its own logic to detect whether a response was truncated or not.

[vkill]

Yes, you are right, I will continue resolve it.

[vkill]

@Lukasa This is a google API bug, the Content-Length: n or Transfer-Encoding: chunked did not return in response when I request curl --http1.1 https://accounts.google.com/o/oauth2/token -H 'Connection: close' -d '' -v, so the response state does not switch to .end after connection close.

curl examples:

curl --http1.1 https://accounts.google.com/o/oauth2/token -H 'Connection: close' -d '' -v

< HTTP/1.1 400 Bad Request
< Content-Type: application/json; charset=utf-8
< Vary: X-Origin
< Vary: Referer
< Date: Mon, 05 Aug 2019 03:48:43 GMT
* Server ESF is not blacklisted
< Server: ESF
< 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

curl --http1.1 https://accounts.google.com/o/oauth2/token -d '' -v

< HTTP/1.1 400 Bad Request
< Content-Type: application/json; charset=utf-8
< Vary: X-Origin
< Vary: Referer
< Date: Mon, 05 Aug 2019 05:01:48 GMT
* Server ESF is not blacklisted
< Server: ESF
< 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
< Transfer-Encoding: chunked

curl --http1.1 https://accounts.google.com/o/oauth2/token -H 'Connection: keep-alive' -d '' -v

< HTTP/1.1 400 Bad Request
< Content-Type: application/json; charset=utf-8
< Vary: X-Origin
< Vary: Referer
< Date: Mon, 05 Aug 2019 05:03:34 GMT
* Server ESF is not blacklisted
< Server: ESF
< 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
< Transfer-Encoding: chunked

curl --http1.1 https://accounts.google.com/o/oauth2/token -H 'Connection: close' -H 'Accept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3' -d '' -v

< HTTP/1.1 400 Bad Request
< Content-Type: application/json; charset=utf-8
< Vary: Origin
< Vary: X-Origin
< Vary: Referer
< Content-Encoding: gzip
< Date: Mon, 05 Aug 2019 05:06:41 GMT
* Server ESF is not blacklisted
< Server: ESF
< 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"
< Connection: close
< Transfer-Encoding: chunked

[vkill]

So in this case, we can make request by try HTTPClient.Request(url: "https://accounts.google.com/o/oauth2/token", method: .POST, headers: .init([("Accept-Encoding", "gzip;q=1.0,deflate;q=0.6,identity;q=0.3")]), body: .string("")), then unzip the response body data by GzipSwift.

[Lukasa]

Huh, that's a thoroughly unfortunate bug on Google's part. Do we have any idea where this should be reported?

[vkill]

Do we have any idea where this should be reported?

Do you mean to report it to google? I have reported it on https://issuetracker.google.com/issues .

[vkill]

We can also custom delegate to catch NIOSSLError.uncleanShutdown then build Response to compatible with this non-standard response header.

The example code:

import AsyncHTTPClient
import NIO
import NIOSSL
import NIOHTTP1

class MyResponseAccumulator: HTTPClientResponseDelegate {
    public struct Response {
        /// Remote host of the request.
        public var host: String
        /// Response HTTP status.
        public var status: HTTPResponseStatus
        /// Reponse HTTP headers.
        public var headers: HTTPHeaders
        /// Response body.
        public var body: ByteBuffer?
    }
    
    enum State {
        case idle
        case head(HTTPResponseHead)
        case body(HTTPResponseHead, ByteBuffer)
        case end
        case error(Error)
    }
    
    var state = State.idle
    let request: HTTPClient.Request
    
    var head: HTTPResponseHead? = nil
    var body: ByteBuffer? = nil
    
    init(request: HTTPClient.Request) {
        self.request = request
    }
    
    func didReceiveHead(task: HTTPClient.Task<Response>, _ head: HTTPResponseHead) -> EventLoopFuture<Void> {
        switch self.state {
        case .idle:
            self.state = .head(head)
        case .head:
            preconditionFailure("head already set")
        case .body:
            preconditionFailure("no head received before body")
        case .end:
            preconditionFailure("request already processed")
        case .error:
            break
        }
        return task.eventLoop.makeSucceededFuture(())
    }
    
    func didReceivePart(task: HTTPClient.Task<Response>, _ part: ByteBuffer) -> EventLoopFuture<Void> {
        switch self.state {
        case .idle:
            preconditionFailure("no head received before body")
        case .head(let head):
            self.state = .body(head, part)
            self.head = head
            self.body = part
        case .body(let head, var body):
            var part = part
            body.writeBuffer(&part)
            self.state = .body(head, body)
            self.body = body
        case .end:
            preconditionFailure("request already processed")
        case .error:
            break
        }
        return task.eventLoop.makeSucceededFuture(())
    }
    
    func didReceiveError(task: HTTPClient.Task<Response>, _ error: Error) {
        self.state = .error(error)
    }
    
    func didFinishRequest(task: HTTPClient.Task<Response>) throws -> Response {
        switch self.state {
        case .idle:
            preconditionFailure("no head received before end")
        case .head(let head):
            return Response(host: self.request.host, status: head.status, headers: head.headers, body: nil)
        case .body(let head, let body):
            return Response(host: self.request.host, status: head.status, headers: head.headers, body: body)
        case .end:
            preconditionFailure("request already processed")
        case .error(let error):
            switch error {
            case NIOSSLError.uncleanShutdown:
                if let head = self.head, (!head.headers.contains(name: "Content-Length") && head.headers["Transfer-Encoding"].first != "chunked") {
                    if let body = self.body {
                        return Response(host: self.request.host, status: head.status, headers: head.headers, body: body)
                    } else {
                        return Response(host: self.request.host, status: head.status, headers: head.headers, body: nil)
                    }
                } else {
                    throw error
                }
            default:
                throw error
            }
        }
    }
}

let eventLoop = MultiThreadedEventLoopGroup(numberOfThreads: 1).next()
let httpClient = HTTPClient(eventLoopGroupProvider: .shared(eventLoop))

let request = try HTTPClient.Request(url: "https://accounts.google.com/o/oauth2/token", method: .POST, body: .string(""))

let delegate = MyResponseAccumulator(request: request)
let task = httpClient.execute(request: request, delegate: delegate)
try task.futureResult.flatMap { response in
    return eventLoop.makeSucceededFuture(response)
}.flatMapError { error -> EventLoopFuture<MyResponseAccumulator.Response> in
    do {
        let response = try delegate.didFinishRequest(task: task)
        return eventLoop.makeSucceededFuture(response)
    } catch {
        return eventLoop.makeFailedFuture(error)
    }
}.flatMap { response -> EventLoopFuture<()> in
    print(response.status.code)
    if var responseBody = response.body {
        print(responseBody.readString(length: responseBody.readableBytes) ?? "")
    }
    return eventLoop.makeSucceededFuture(())
}.wait()

The ruby net/http library can compatible with it.

require 'net/http'

uri = URI('https://accounts.google.com/o/oauth2/token')
req = Net::HTTP::Post.new(uri)
req['Connection'] = 'close'
req['Accept-Encoding'] = 'none'

http = Net::HTTP.new(uri.hostname, uri.port)
http.use_ssl = true
http.set_debug_output $stderr

res = http.start() do |http|
  http.request(req)
end

p res.code
p res.body