Always overwrite `Transport-Encoding` and `Content-Length` by dnadoba · Pull Request #479 · swift-server/async-http-client

dnadoba · 2021-11-18T13:06:32Z

Motivation

We want to reuse request validation in the upcoming async/await implementation. During refactoring we noticed some inconsistency in the current implementation on how user defined Transport-Encoding and Content-Length headers are processed. We concluded that a user should not be allowed to set Transport-Encoding or Content-Length headers themselves. A user can only communicate its intend through the body property on a request. We infer the correct and most optimal Transport-Encoding or Content-Length header for them.

Changes

Transport-Encoding and Content-Length or no longer validated and always removed or replaced
rename validate to validateAndSetTransportFraming because it does not only validate but also mutate the HTTPHeaders to include the correct headers for the given request body.
introduce a new enum RequestBodyLength which can be either .dynamic if we do not now the size of a request in advance or .fixed(length: Int) if the user has specified the length of the body.
refactor implementation to no longer expect a HTTPClient.Request.Body? but rather a new type RequestBodyLength. This allows use to reuse the function for async/await because we will not use HTTPClient.Request.Body but rather a new type HTTPRequest.Body.
no request body (where HTTPClient.Request.body is set to nil) and a request body with a length of 0 (e.g where body is .byteBuffer(ByteBuffer()) are now processed the same way.

Alternatives

We could also remove RequestBodyLength and just use and optional Int.

fabianfett · 2021-11-18T15:58:54Z

Sources/AsyncHTTPClient/RequestValidation.swift

+        let encodings = self[canonicalForm: "Transfer-Encoding"]
+
+        // "Transfer-Encoding" and "Content-Length" are not allowed to present at the same time (https://tools.ietf.org/html/rfc7230#section-3.3.1)
+        guard encodings.isEmpty || contentLength == nil else {


Super NIT: I think an if is easier to read here.

fabianfett · 2021-11-18T16:01:29Z

Sources/AsyncHTTPClient/RequestValidation.swift

+    static func validateTransferEncoding<Encodings>(
+        _ encodings: Encodings
+    ) throws where Encodings: Sequence, Encodings.Element: StringProtocol {


Just out of curiosity, why is this generic?

We call this function during validateAndFixTransportFraming with [Substring] but it is more convenience to use [String] in tests.

I think this should be private and all tests should go directly through validateAndFixTransportFraming.

+1 to @fabianfett's note.

fabianfett · 2021-11-18T16:04:54Z

Sources/AsyncHTTPClient/RequestValidation.swift

    }

-    private func validateFieldNames() throws {
+    func validateFieldNames() throws {


why is this internal now?

fabianfett · 2021-11-18T16:09:19Z

Sources/AsyncHTTPClient/RequestValidation.swift

+                self.add(name: "Content-Length", value: String(length))
+                return .init(connectionClose: connectionClose, body: .fixedSize(length))
+            } else {
+                self.add(name: "Transfer-Encoding", value: encodings.joined(separator: ", "))


I think we should make this explicit with a comment.

Even though a fixed length body is going to be sent, the user explicitly set an encoding. We should respect the users wish and use the specified headers.

fabianfett · 2021-11-18T16:10:40Z

Sources/AsyncHTTPClient/RequestValidation.swift

+                throw HTTPClientError.traceRequestWithBody
+            }
+            if encodings.isEmpty {
+                self.add(name: "Content-Length", value: String(length))


There might be an interesting case here: What if the user handed us a fixed size body payload, but also set the Content-Length header to something different?

I once had validation in there but noticed that we actually have tests that check that we always overwrite the Content-Length header. I would also prefer to validate it but this might break user code. WDYT?

async-http-client/Tests/AsyncHTTPClientTests/RequestValidationTests.swift

Lines 43 to 49 in 62ad818

func testContentLengthHeaderIsChangedIfBodyHasDifferentLength() {

var headers = HTTPHeaders([("Content-Length", "0")])

var metadata: RequestFramingMetadata?

XCTAssertNoThrow(metadata = try headers.validateAndFixTransportFraming(method: .PUT, bodyLength: .fixed(length: 200)))

XCTAssertEqual(headers.first(name: "Content-Length"), "200")

XCTAssertEqual(metadata?.body, .fixedSize(200))

}

We need @Lukasa's wisdom here.

We should absolutely always override it. The user is not entitled to set that header: it doesn't belong to them.

fabianfett · 2021-11-18T16:13:18Z

Sources/AsyncHTTPClient/RequestValidation.swift

+                // if a user forgot to specify a Content-Length and Transfer-Encoding, we will set it for them
+                self.add(name: "Transfer-Encoding", value: "chunked")
+            } else {
+                self.add(name: "Transfer-Encoding", value: encodings.joined(separator: ", "))


We also run in this case if the user has set a Content-Length header explicitly. I think in the dynamic case we should trust a user set content length header (though we should validate it is a positive int).

A case might be streaming a file somewhere.

I'm not really sure about this. A user can always specify the length of a stream upfront through our API and doesn't need to set the Content-Length header manually through the length parameter.

async-http-client/Sources/AsyncHTTPClient/HTTPHandler.swift

Lines 61 to 69 in 62ad818

/// Create and stream body using `StreamWriter`.

///

/// - parameters:

/// - length: Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil,

/// unless `Transfer-Encoding: chunked` header is set.

/// - stream: Body chunk provider.

public static func stream(length: Int? = nil, _ stream: @escaping (StreamWriter) -> EventLoopFuture<Void>) -> Body {

return Body(length: length, stream: stream)

}

This would give them a way to do the same thing in to different ways and they may conflict.
I would almost say that we should always ignore/remove "Content-Length" header.
The same is almost true for Transport-Encoding but I'm not fully understanding all use cases for this one.

If I read this code snipped correctly, Transport-Encoding can only be chunked because SwiftNIO always replace the Transport-Encoding with chunked or doesn't do chunked encoding if it is not the only encoding.
https://github.com/apple/swift-nio/blob/addf69cfe60376c325397c8926589415576b1dd1/Sources/NIOHTTP1/HTTPEncoder.swift#L104-L129

we should always ignore/remove "Content-Length" header

Nope, definitely not. I know at least the S3 apis, that require a Content-Length even for large uploads, if you deal with pre-signed links and friends.

Yeah, we don't support other transfer encodings in NIO and so we should probably stop doing so here. However, I don't know if making that change in this PR is worthwhile.

As to supporting Content-Length: yes, we should emit it, but we should do so when users tell us the size of their stream. We shouldn't tolerate them setting that header.

As to supporting Content-Length: yes, we should emit it, but we should do so when users tell us the size of their stream. We shouldn't tolerate them setting that header.

Okay, I'm happy with that!

In that case we need to fix the code comment though here:

async-http-client/Sources/AsyncHTTPClient/HTTPHandler.swift

Lines 61 to 69 in 62ad818

/// Create and stream body using `StreamWriter`.

///

/// - parameters:

/// - length: Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil,

/// unless `Transfer-Encoding: chunked` header is set.

/// - stream: Body chunk provider.

public static func stream(length: Int? = nil, _ stream: @escaping (StreamWriter) -> EventLoopFuture<Void>) -> Body {

return Body(length: length, stream: stream)

}

If no Content-Length header is set, we will just reach for chunked.

Lukasa · 2021-11-22T10:50:35Z

Sources/AsyncHTTPClient/ConnectionPool/RequestBodyLength.swift

+//
+//===----------------------------------------------------------------------===//
+
+enum RequestBodyLength: Equatable {


Nit: As a matter of style I think it's rarely useful to have : Equatable. Whenever you feel like writing that, you may as well also write : Hashable.

Is the binary size increase negligible for the additional synthesised conformance? AFAIK Swift doesn't remove unused synthesised conformances, even for internal types.

Yeah, this is not the most fruitful way of shrinking binary sizes.

Lukasa · 2021-11-22T11:04:40Z

Sources/AsyncHTTPClient/RequestValidation.swift

+        method: HTTPMethod,
+        bodyLength: RequestBodyLength
+    ) throws -> RequestFramingMetadata {
+        let contentLength = self.first(name: "Content-Length")


Why are we getting the value if we don't need it?

Lukasa · 2021-11-22T11:05:12Z

Sources/AsyncHTTPClient/RequestValidation.swift

+                throw HTTPClientError.traceRequestWithBody
+            }
+            if encodings.isEmpty {
+                self.add(name: "Content-Length", value: String(length))


We should absolutely always override it. The user is not entitled to set that header: it doesn't belong to them.

Lukasa · 2021-11-22T11:05:42Z

Sources/AsyncHTTPClient/RequestValidation.swift

+        let contentLength = self.first(name: "Content-Length")
+        let encodings = self[canonicalForm: "Transfer-Encoding"]
+
+        // "Transfer-Encoding" and "Content-Length" are not allowed to present at the same time (https://tools.ietf.org/html/rfc7230#section-3.3.1)


Nit:

Suggested change

// "Transfer-Encoding" and "Content-Length" are not allowed to present at the same time (https://tools.ietf.org/html/rfc7230#section-3.3.1)

// "Transfer-Encoding" and "Content-Length" are not allowed to be present at the same time (https://tools.ietf.org/html/rfc7230#section-3.3.1)

Lukasa · 2021-11-22T11:08:43Z

Sources/AsyncHTTPClient/RequestValidation.swift

+    static func validateTransferEncoding<Encodings>(
+        _ encodings: Encodings
+    ) throws where Encodings: Sequence, Encodings.Element: StringProtocol {


+1 to @fabianfett's note.

Lukasa · 2021-11-22T11:11:05Z

Sources/AsyncHTTPClient/RequestValidation.swift

+                // if a user forgot to specify a Content-Length and Transfer-Encoding, we will set it for them
+                self.add(name: "Transfer-Encoding", value: "chunked")
+            } else {
+                self.add(name: "Transfer-Encoding", value: encodings.joined(separator: ", "))


Yeah, we don't support other transfer encodings in NIO and so we should probably stop doing so here. However, I don't know if making that change in this PR is worthwhile.

As to supporting Content-Length: yes, we should emit it, but we should do so when users tell us the size of their stream. We shouldn't tolerate them setting that header.

fabianfett

Wow, this looks so much better already. Tiny nits.

fabianfett · 2021-11-22T13:47:49Z

Sources/AsyncHTTPClient/HTTPClient.swift

    /// Request contains invalid identity encoding.
    public static let identityCodingIncorrectlyPresent = HTTPClientError(code: .identityCodingIncorrectlyPresent)
    /// Request contains multiple chunks definitions.
+    @available(*, deprecated)


We should add a message here like: "AsyncHTTPClient now silently corrects invalid headers."

fabianfett · 2021-11-22T13:48:04Z

Sources/AsyncHTTPClient/HTTPClient.swift

    /// Body part was written after request was fully sent.
    public static let writeAfterRequestSent = HTTPClientError(code: .writeAfterRequestSent)
    /// Incompatible headers specified, for example `Transfer-Encoding` and `Content-Length`.
+    @available(*, deprecated)


fabianfett · 2021-11-22T13:51:54Z

Tests/AsyncHTTPClientTests/RequestValidationTests.swift

+
+    func testTraceMethodIsNotAllowedToHaveAFixedLengthBody() {
+        var headers = HTTPHeaders()
+        XCTAssertThrowsError(try headers.validateAndSetTransportFraming(method: .TRACE, bodyLength: .fixed(length: 10)))


Since the errors are equatable would you mind checking, we get what we expect?

fabianfett

Huge improvement!

fabianfett requested review from Lukasa and fabianfett November 18, 2021 13:43

fabianfett reviewed Nov 18, 2021

View reviewed changes

dnadoba force-pushed the dn-refactor-request-validation branch from 62ad818 to 539e4fd Compare November 18, 2021 19:57

Lukasa added the 🔨 semver/patch No public API change. label Nov 22, 2021

Lukasa reviewed Nov 22, 2021

View reviewed changes

dnadoba added 2 commits November 22, 2021 13:44

refactor request validation

185dc1d

fix typo

242e392

dnadoba force-pushed the dn-refactor-request-validation branch from 539e4fd to afad35f Compare November 22, 2021 13:41

always overwrite Transport-Encoding and Content-Length

311d927

dnadoba force-pushed the dn-refactor-request-validation branch from afad35f to 311d927 Compare November 22, 2021 13:41

dnadoba changed the title ~~Refactor request validation~~ Always overwrite Transport-Encoding and Content-Length Nov 22, 2021

fabianfett reviewed Nov 22, 2021

View reviewed changes

fix documentation

eaf16eb

fabianfett approved these changes Nov 22, 2021

View reviewed changes

dnadoba added 2 commits November 22, 2021 15:09

add Hashable conformance to RequestBodyLength

2e2140a

remove duplicate be from documentation

5713802

fabianfett requested a review from Lukasa November 22, 2021 17:17

Lukasa approved these changes Nov 23, 2021

View reviewed changes

dnadoba merged commit 0ed00b8 into swift-server:main Nov 23, 2021

dnadoba deleted the dn-refactor-request-validation branch November 23, 2021 09:23

fabianfett added 🆕 semver/minor Adds new public API. and removed 🔨 semver/patch No public API change. labels Nov 23, 2021

	func testContentLengthHeaderIsChangedIfBodyHasDifferentLength() {
	var headers = HTTPHeaders([("Content-Length", "0")])
	var metadata: RequestFramingMetadata?
	XCTAssertNoThrow(metadata = try headers.validateAndFixTransportFraming(method: .PUT, bodyLength: .fixed(length: 200)))
	XCTAssertEqual(headers.first(name: "Content-Length"), "200")
	XCTAssertEqual(metadata?.body, .fixedSize(200))
	}

	/// Create and stream body using `StreamWriter`.
	///
	/// - parameters:
	/// - length: Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil,
	/// unless `Transfer-Encoding: chunked` header is set.
	/// - stream: Body chunk provider.
	public static func stream(length: Int? = nil, _ stream: @escaping (StreamWriter) -> EventLoopFuture<Void>) -> Body {
	return Body(length: length, stream: stream)
	}

	// "Transfer-Encoding" and "Content-Length" are not allowed to present at the same time (https://tools.ietf.org/html/rfc7230#section-3.3.1)
	// "Transfer-Encoding" and "Content-Length" are not allowed to be present at the same time (https://tools.ietf.org/html/rfc7230#section-3.3.1)

Conversation

dnadoba commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Alternatives

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dnadoba commented Nov 18, 2021 •

edited

Loading

fabianfett Nov 18, 2021 •

edited

Loading