Fix generating the Origin header value for websocket handshake request
#12941
+401
−51
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@normanmaurer , @chrisvest, @vietj , @slandelle, @ursaj Could you please take a look. |
|
One fixes the NPE by turning origin header generation off? |
|
looks good |
Added a comment to the issue #12933 how they are related. |
…est only if it enabled.
amizurov
force-pushed
the
feature/ws_enable_generate_origin_header
branch
from
efecda2
to
44bd9da
Compare
chrisvest
approved these changes
Nov 2, 2022
|
@normanmaurer, @vietj, @slandelle Could you please review this changes, i would like to know your thoughts. |
normanmaurer
requested changes
Nov 10, 2022
...http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshakerTest.java
Outdated
Show resolved
Hide resolved
...http/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshakerTest.java
Outdated
Show resolved
Hide resolved
|
@amizurov thanks a lot! |
normanmaurer
added a commit
that referenced
this pull request
Nov 10, 2022
…est (#12941) Motivation: We have the old erroneous behavior of generating the `Origin| Sec-WebSocket-Origin` for client websocket handshake request (#9673). In Netty5 this fixed and auto-generation has been deleted at all, only if the client passed the `Origin` header via custom headers. The same we can do for Netty4 but it could potentially break some clients (unlikely), or introduce an additional parameter to disable or enable this behavior. Modification: Introduce new `generateOriginHeader` parameter in client config and generate the `Origin|Sec-WebSocket-Origin` header value only if it enabled. Add additional check for webSocketURI if it contains host or passed through `customHeaders` to prevent NPE in `newHandshakeRequest()`. Result: Fixes #9673 #12933 Co-authored-by: Norman Maurer <norman_maurer@apple.com>
slandelle
reviewed
Nov 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
We have the old erroneous behavior of generating the
Origin| Sec-WebSocket-Originfor client websocket handshake request (#9673). In Netty5 this fixed and auto-generation has been deleted at all, only if the client passed theOriginheader via custom headers. The same we can do for Netty4 but it could potentially break some clients (unlikely), or introduce an additional parameter to disable or enable this behavior.Modification:
Introduce new
generateOriginHeaderparameter in client config and generate theOrigin|Sec-WebSocket-Originheader value only if it enabled. Add additional check for webSocketURI if it contains host or passed throughcustomHeadersto prevent NPE innewHandshakeRequest().Result:
Fixes #9673 #12933