Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix WebSocketClientHandshaker not generating correct handshake request when path is empty #10095

Merged
merged 1 commit into from Mar 10, 2020

Conversation

@slandelle
Copy link
Contributor

slandelle commented Mar 10, 2020

Motivation:

WebSocketClientHandshaker#upgradeUrl doesn't comperly compute relative url when path is empty and produces url such as ?access_token=foo instead of /?access_token=foo.

Modifications:

  • fix WebSocketClientHandshaker#upgradeUrl
  • add tests for urls without path, with and without query

Result:

WebSocketClientHandshaker properly connects to url without path.

…t when path is empty

Motivation:

WebSocketClientHandshaker#upgradeUrl doesn't comperly compute relative url when path is empty and produces url such as `?access_token=foo` instead of `/?access_token=foo`.

Modifications:

* fix WebSocketClientHandshaker#upgradeUrl
* add tests for urls without path, with and without query

Result:

WebSocketClientHandshaker properly connects to url without path.
@slandelle slandelle requested a review from normanmaurer Mar 10, 2020
@netty-bot

This comment has been minimized.

Copy link

netty-bot commented Mar 10, 2020

Can one of the admins verify this patch?

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 10, 2020

@netty-bot test this please

@normanmaurer normanmaurer merged commit 2576a2d into 4.1 Mar 10, 2020
3 checks passed
3 checks passed
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 10, 2020

@slandelle thanks a lot!

@normanmaurer normanmaurer deleted the fix-wshandshaker-upgradeurl-nopath branch Mar 10, 2020
@normanmaurer normanmaurer added this to the 4.1.48.Final milestone Mar 10, 2020
normanmaurer added a commit that referenced this pull request Mar 10, 2020
…t when path is empty (#10095)

Motivation:

WebSocketClientHandshaker#upgradeUrl doesn't comperly compute relative url when path is empty and produces url such as `?access_token=foo` instead of `/?access_token=foo`.

Modifications:

* fix WebSocketClientHandshaker#upgradeUrl
* add tests for urls without path, with and without query

Result:

WebSocketClientHandshaker properly connects to url without path.
gnehcgnaw pushed a commit to gnehcgnaw/netty that referenced this pull request Mar 13, 2020
* '4.1' of https://github.com/netty/netty: (614 commits)
  Use WebSocketVersion.toAsciiString() as header value when possible (netty#10105)
  Don't override HOST header if provided by user already in WebSocketClientHandshaker (netty#10104)
  Added support for the SameSite attribute in Cookies (netty#10050)
  Let LzfEncoder support length aware ability. (netty#10082)
  Fix WebSocketClientHandshaker not generating correct handshake request when path is empty (netty#10095)
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release netty-4.1.47.Final
  Replace several magic numbers. (netty#10094)
  Add a HTTP/2 client example using the newer frames approach the current HTTP/2 client example uses the older 'HTTP/1 <--> HTTP/2' translation approach. (netty#10081)
  Add a log level check simply before logging. (netty#10093)
  Use MacOSDnsServerAddressStreamProvider when on the classpath and we … (netty#10079)
  http multipart decode with chinese chars should work (netty#10089)
  Add log level check simply before logging. (netty#10080)
  Update ProtocolDetectionResult to fix typo (netty#10086)
  Make sure we always flush window update frames in AbstractHttp2StreamChannel (netty#10075)
  Fix AssertionError in ScheduledFutureTask (netty#10073)
  Add test cases for StringUtil. (netty#10074)
  Preserve order when using alternate event loops (netty#10069)
  Add test cases for MathUtil. (netty#10071)
  [maven-release-plugin] prepare for next development iteration
  ...

# Conflicts:
#	pom.xml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.