Skip to content

[Issue-10320] More values other than chunked defined in Transfer-Encoding header leads to decode failure#10321

Merged
normanmaurer merged 1 commit into
netty:4.1from
gaol:check_chunked_request_header
Jun 2, 2020
Merged

[Issue-10320] More values other than chunked defined in Transfer-Encoding header leads to decode failure#10321
normanmaurer merged 1 commit into
netty:4.1from
gaol:check_chunked_request_header

Conversation

@gaol
Copy link
Copy Markdown
Contributor

@gaol gaol commented May 27, 2020

Motivation:

containsValue() will check if there are multiple values defined in the specific header name, we need to use this method instead of contains() for the Transfer-Encoding header to cover the case that multiple values defined, like: Transfer-Encoding: gzip, chunked

Modification:

Change from contains() to containsValue() in HttpUtil.isTransferEncodingChunked() method.

Fixes #10320

@netty-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

3 similar comments
@netty-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@netty-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@netty-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

Comment thread codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java Outdated
Comment thread codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java Outdated
@gaol gaol force-pushed the check_chunked_request_header branch from 399a391 to acb2313 Compare May 27, 2020 08:10
@gaol
Copy link
Copy Markdown
Contributor Author

gaol commented May 27, 2020

@normanmaurer Done by addressing the above comments, thanks :)

Copy link
Copy Markdown
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

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

Good catch!

Comment thread codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java Outdated
@gaol gaol force-pushed the check_chunked_request_header branch from acb2313 to 7e98c1a Compare May 29, 2020 05:07
@gaol
Copy link
Copy Markdown
Contributor Author

gaol commented May 29, 2020

@idelpivnitskiy After trying to use the full gzipped http request and checked the desired decoded request content, I found it did not decode the gzip content if we have Transfer-Encoding: gzip set, I updated the PR to address it as well. Please review again. Thanks :)

Comment thread codec-http/src/main/java/io/netty/handler/codec/http/HttpContentDecoder.java Outdated
Copy link
Copy Markdown
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

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

LGTM after Norman's comment is addressed

Comment thread codec-http/src/test/java/io/netty/handler/codec/http/HttpContentDecoderTest.java Outdated
Comment thread codec-http/src/test/java/io/netty/handler/codec/http/HttpContentDecoderTest.java Outdated
Comment thread codec-http/src/test/java/io/netty/handler/codec/http/HttpContentDecoderTest.java Outdated
@gaol gaol force-pushed the check_chunked_request_header branch from 7e98c1a to 7cadd57 Compare May 30, 2020 07:52
@gaol
Copy link
Copy Markdown
Contributor Author

gaol commented May 30, 2020

Updated PR to address review comments. Thanks :)

@normanmaurer normanmaurer added this to the 4.1.51.Final milestone Jun 2, 2020
@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer normanmaurer merged commit de134da into netty:4.1 Jun 2, 2020
@normanmaurer
Copy link
Copy Markdown
Member

@gaol thanks a lot

normanmaurer pushed a commit that referenced this pull request Jun 2, 2020
…ads to decode failure (#10321)

Motivation:

`containsValue()` will check if there are multiple values defined in the specific header name, we need to use this method instead of `contains()` for the `Transfer-Encoding` header to cover the case that multiple values defined, like: `Transfer-Encoding: gzip, chunked`

Modification:

Change from `contains()` to `containsValue()` in `HttpUtil.isTransferEncodingChunked()` method.

Result:

Fixes #10320
Kvicii pushed a commit to Kvicii/netty that referenced this pull request Jun 6, 2020
* '4.1' of github.com:netty/netty:
  Motivation: (netty#10326)
  Fix netty#10261 stomp can be chunked, so implement StompWebSocketFrameEncoder (netty#10274)
  Update test to directly check for SslHandshakeTimeoutException (netty#10339)
  Fix possible StackOverflowError when try to resolve authorative names… (netty#10337)
  Fix a test case problem: testSwallowedReadComplete(...) may fail with an AssertionError sometimes. (netty#10313)
  More values other than chunked defined in Transfer-Encoding header leads to decode failure (netty#10321)
  Set (and override) reserved websocket handshake response headers after custom to avoid duplication (netty#10319)
  Update GraalVM with JDK 8 and add GraalVM with JDK 11 (netty#10333)
  Include more details if we throw an IllegalArgumentException because of overflow (netty#10330)
  SslHandler#wrap to preserve exception if SSLEngine is closed (netty#10327)
ihanyong pushed a commit to ihanyong/netty that referenced this pull request Jul 31, 2020
…ads to decode failure (netty#10321)

Motivation:

`containsValue()` will check if there are multiple values defined in the specific header name, we need to use this method instead of `contains()` for the `Transfer-Encoding` header to cover the case that multiple values defined, like: `Transfer-Encoding: gzip, chunked`

Modification:

Change from `contains()` to `containsValue()` in `HttpUtil.isTransferEncodingChunked()` method.

Result:

Fixes netty#10320
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

More values other than chunked defined in Transfer-Encoding header leads to decode failure

4 participants