Skip to content

add response decompression support#86

Merged
weissi merged 25 commits into
masterfrom
support_response_decompression
Oct 23, 2019
Merged

add response decompression support#86
weissi merged 25 commits into
masterfrom
support_response_decompression

Conversation

@artemredkin

Copy link
Copy Markdown
Collaborator

fixes #44

Comment thread Sources/AsyncHTTPClient/HTTPDecompressor.swift Outdated
@artemredkin

Copy link
Copy Markdown
Collaborator Author

@tomerd build fails with error: 'zlib.h' file not found, could you help with that?

@weissi weissi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs a limit on the decompression threshold so denial of service attacks are harder. Just chatted to @Lukasa who actually has experience writing HTTP clients. We should either have a (configurable) limit of say a maximum of 32 MB or something or alternatively a maximum decompression threshold so for example: The decompressed data must not be more than 100x the size of the compressed data.

@weissi weissi added the ⚠️ semver/major Breaks existing public API. label Sep 5, 2019
@weissi weissi added this to the 1.0.0 milestone Sep 5, 2019
@artemredkin

Copy link
Copy Markdown
Collaborator Author

@weissi @Lukasa added configurable limit and more tests

@artemredkin artemredkin requested a review from Lukasa September 17, 2019 12:23
Comment thread Sources/AsyncHTTPClient/HTTPDecompressor.swift Outdated
}
}

public mutating func inflatePart(to buffer: inout ByteBuffer) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API must not be public, as it is extremely unsafe. Please make it private.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

Comment thread Sources/AsyncHTTPClient/HTTPDecompressor.swift Outdated
Comment thread Sources/AsyncHTTPClient/HTTPDecompressor.swift Outdated
Comment thread Sources/AsyncHTTPClient/HTTPDecompressor.swift Outdated
Comment thread Sources/AsyncHTTPClient/HTTPDecompressor.swift Outdated
}

let rc = CNIOExtrasZlib_inflateInit2(&self.stream, window)
precondition(rc == Z_OK, "Unexpected return from zlib init: \(rc)")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably throw.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks!

Comment thread Sources/AsyncHTTPClient/HTTPClient.swift Outdated
@tomerd

tomerd commented Sep 17, 2019

Copy link
Copy Markdown
Contributor

@tomerd build fails with error: 'zlib.h' file not found, could you help with that?

@artemredkin add apt-get install libz-dev to docker file

Comment thread Sources/AsyncHTTPClient/HTTPDecompressor.swift Outdated
@artemredkin

Copy link
Copy Markdown
Collaborator Author

@tomerd @Lukasa @weissi I've updated the PR with the latests changes from swift-nio-extras, we'll be able to merge this after 1.3.0 is tagged (waiting for apple/swift-nio-extras#59).

Comment thread Package.swift Outdated

@Lukasa Lukasa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM.

@artemredkin

Copy link
Copy Markdown
Collaborator Author

@weissi are you happy with the latest changes?

Comment thread Sources/AsyncHTTPClient/HTTPClient.swift
@artemredkin

Copy link
Copy Markdown
Collaborator Author

@weissi ready for review

@weissi weissi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@artemredkin sorry for the delay! LGTM

@weissi weissi merged commit 191c4ba into master Oct 23, 2019
@weissi weissi deleted the support_response_decompression branch October 23, 2019 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ semver/major Breaks existing public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support gzip/deflate decompression

5 participants