Origin header is always sent from WebSocket client #9673

ursaj · 2019-10-15T19:07:17Z

Expected behavior

No 'Origin' header sent by default.
A way to set custom 'Origin' header when it is needed.

Actual behavior

'Origin' header is set.
'Origin' header is auto-resolved into meaningless value. In my case - IP of the intermediate proxy between client and server sides.
Enforced 'Origin' header leads to cors check failure on server side and denial of service for legitim client.

Steps to reproduce

Open WebSocket connection with v13 handshaker

WebSocketClientHandshaker13.newHandshakeRequest:

if (!headers.contains(HttpHeaderNames.ORIGIN)) {
    headers.set(HttpHeaderNames.ORIGIN, websocketOriginValue(wsURL));
}

Netty version

4.1.39

Related issues: PR #9312 and PR #9435.

JVM version (e.g. `java -version`)

1.8.0_223-b27

OS version (e.g. `uname -a`)

Windows 7 Enterprise
Linux gmafxu42 3.0.101-108.98-default Build stability fix + .gitignore update #1 SMP Mon Jul 15 13:58:06 UTC 2019 (262a94d) x86_64 x86_64 x86_64 GNU/Linux

The text was updated successfully, but these errors were encountered:

amizurov · 2019-10-16T09:56:03Z

Hi, @ursaj. I agree that Origin and Sec-WebSocket-Origin is not required according the spec https://tools.ietf.org/html/rfc6455#section-1.3 Origin/Sec-WebSocket-Origin header field is sent by browser clients; for non-browser clients, this header field may be sent if it makes sense in the context of those clients. But i'm not sure can we just delete them. WDYT @normanmaurer @slandelle

slandelle · 2019-10-16T11:04:28Z

I agree there's no way for an HTTP client to be able to infer Origin header and it can only be provided reliably by the user. IMHO, this is a bug, but fixing it would be a breaking change.
I say we should have an option to disable and deprecate sending Origin header for removal in Netty 5.

ursaj · 2019-10-17T00:15:49Z

Actually it is already a breaking change between 4.1.31 and 4.1.39 - when 'Origin' header was forcibly injected. I'd spend quite a lot of effort to sort it out, when Singapore clients failed to connect to our backends during their upgrade. For me it was 4 a.m. on Sunday - quite an inconvenient time to solve such issues.

I'll better interpret it like a "temporary bug" and revert this behaviour to the one of 4.1.31.

normanmaurer · 2019-10-17T02:27:04Z

@slandelle I agree with @ursaj ... we should fix it. @ursaj can you do a pr ?

### Motivation: Those who need 'Origin' or 'Sec-WebSocket-Origin' headers should provide them explicitly, like it is stated in WebSocket specs. E.g. through custom headers: HttpHeaders customHeaders = new DefaultHttpHeaders() .add(HttpHeaderNames.ORIGIN, "http://localhost:8080"); new WebSocketClientProtocolHandler( new URI("ws://localhost:1234/test"), WebSocketVersion.V13, subprotocol, allowExtensions, customHeaders, maxFramePayloadLength, handshakeTimeoutMillis) ### Modification: * Remove enforced origin headers. * Update tests ### Result: Fixes #9673: Origin header is always sent from WebSocket client

Those who need 'Origin' or 'Sec-WebSocket-Origin' headers should provide them explicitly, like it is stated in WebSocket specs. E.g. through custom headers: HttpHeaders customHeaders = new DefaultHttpHeaders() .add(HttpHeaderNames.ORIGIN, "http://localhost:8080"); new WebSocketClientProtocolHandler( new URI("ws://localhost:1234/test"), WebSocketVersion.V13, subprotocol, allowExtensions, customHeaders, maxFramePayloadLength, handshakeTimeoutMillis) * Remove enforced origin headers. * Update tests Fixes #9673: Origin header is always sent from WebSocket client

normanmaurer · 2019-12-18T07:31:07Z

Sorry but I have to revert this as this needs more thoughts.

…ent (#9692)" This reverts commit f48d9fa as this needs more thoughts.

…ent (#9692)" This reverts commit f48d9fa as it needs more thoughts

tmsimont · 2020-07-15T19:07:59Z

Out of curiosity, what motivated the revert for this? I see "more thoughts" are cited but why? To me it seemed like the argument in this commit message was pretty compelling 19a4633

For context, I ran into this bug when performing a minor upgrade, too.

tmsimont · 2020-07-16T15:31:39Z

This feels more wrong the more I look into it.

The current state, in which the bug fix is reverted, sets the Origin header to the target webSocketURL

That is flat out wrong.

HTTP Origin
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Origin

The Origin request header indicates where a fetch originates from

webSocketURL

URL for web socket communications. e.g "ws://myhost.com/mypath".
Subsequent web socket frames will be sent to this URL.

It seems that the bug was introduced to hack in a fix for CORS requests. The hack was a breaking change that truly makes no sense. If a user wants to spoof the origin by setting it to the target URL, then there's an API in place to do this.

Why would you break the library by making this the default behavior?

ursaj · 2020-07-17T20:55:53Z

I agree with @tmsimont .Reasoning like "this needs more thoughts" feels really disappointing. To such extend that I've just lost my passion to discuss or contribute anything.

Adding something or removing something to project is ok. Even if its buggy. But double standards in change management feel disrespectful to contributors.

Just a thought.

normanmaurer · 2020-07-17T21:10:44Z

I fully understand and I am really sorry about that. I am currently OOO but will try to pick this up again once back.

…

Am 17.07.2020 um 22:56 schrieb ursa ***@***.***>: I agree with @tmsimont .Reasoning like "this needs more thoughts" feels really disappointing. To such extend that I've just lost my passion to discuss or contribute anything. Adding something or removing something to project is ok. Even if its buggy. But double standards in change management feel disrespectful to contributors. Just a thought. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

tmsimont · 2020-07-17T22:59:38Z

Since everyone is busy allow me to review the history here...

It looks like this Origin header has had a rough history:

The ORIGIN used to be totally absent from all but v0 handshake... see 16be36a#diff-ffbbdb8352c6101b19ebd7c08d125bca

Initially, the only version of the handshake (v0) was allowing custom headers to override the Origin if set:

netty/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00.java

Lines 139 to 150 in 16be36a

    
                  .add(HttpHeaderNames.ORIGIN, websocketOriginValue(host, wsPort)) 
        
                  .add(HttpHeaderNames.SEC_WEBSOCKET_KEY1, key1) 
        
                  .add(HttpHeaderNames.SEC_WEBSOCKET_KEY2, key2); 
        
           String expectedSubprotocol = expectedSubprotocol(); 
        
           if (expectedSubprotocol != null && !expectedSubprotocol.isEmpty()) { 
        
               headers.add(HttpHeaderNames.SEC_WEBSOCKET_PROTOCOL, expectedSubprotocol); 
        
           } 
        
           if (customHeaders != null) { 
        
               headers.add(customHeaders); 
        
           }

)

Then a bug was introduced (still only v0)... ORIGIN always overrode custom headers (May 30, 2018) 48911e0#diff-ffbbdb8352c6101b19ebd7c08d125bca
Then (July 12, 2019), the bug grew beyond v0 into v13 in a misguided attempt to fix a broken handshake in WebSocketClientHandshaker13 Invalid Handshake #9134
Before this, the Origin field was totally absent from requests unless set in custom headers, but Sec-WebSocket-Origin was present and for some odd reason was being set to the target, not the origin. This is wrong.

From https://tools.ietf.org/id/draft-ietf-hybi-thewebsocketprotocol-06.html#:~:text=The%20%7CSec%2DWebSocket%2DOrigin,API%20in%20a%20Web%20browser.&text=If%20the%20server%20does%20not,choose%20to%20abort%20the%20connection.

The |Sec-WebSocket-Origin| header is used to protect against unauthorized cross-origin use of a WebSocket server by scripts using the |WebSocket| API in a Web browser. The server is informed of the script origin generating the WebSocket connection request. If the server does not wish to accept connections from this origin, it can choose to abort the connection. This header is sent by browser clients, for non-browser clients this header may be sent if it makes sense in the context of those clients.

So, before WebSocketClientHandshaker13 Invalid Handshake #9134, we already had a bug in Sec-WebSocket-Origin and then the fix for WebSocketClientHandshaker13 Invalid Handshake #9134 extends that bug into Origin, which then breaks a lot more users, which leads to...
Set the ORIGIN header from a custom headers if present #9435
The previous "fix" has extended the bug into Origin from Sec-WebSocket-Origin. Not only is Origin now set incorrectly, but it also overrides any explicitly set Origin fields in custom headers. The fix for this is to allow Origin to be set in custom headers, and we only set the buggy and incorrect value if Origin is not already set. This is the fix in Set the ORIGIN header from a custom headers if present #9435 (August 10, 2019)
Now we finally arrive at Origin header is always sent from WebSocket client #9673 -- this issue... As we pull in the Origin-forcing function, we break because the value being set makes no sense. @ursaj has a fix, which is don't set Origin incorrectly at all. This fixes the bug introduced in WebSocketClientHandshaker13 Invalid Handshake #9134, but now the problem is that WebSocketClientHandshaker13 Invalid Handshake #9134 is basically reopened because Origin is not going to be present automatically without being explicitly set into a custom header.

so to review...

Origin used to be missing from v13 handshaker and has a bumpy history in v0
Origin and Sec-WebSocket-Origin have always been set to the "Target" which is wrong...
Origin is now set incorrectly in v13, too, in addition to v0 but is not set in v7 nor v8
We can now at least set Origin into custom headers that won't be overridden which is nice
Users that depend on a value in Origin probably are not setting it explicitly and allowing the default behavior, which is backwards.

I'm totally lost as to why it was ever thought to be correct to set Origin = targetUrl

Can anyone explain that one?

…t client (netty#9692)" This reverts commit f48d9fa as it needs more thoughts

vietj · 2021-10-06T08:10:55Z

I believe there should be an option to disable sending the origin header by the handshaker.

Currently it is possible to work around this by subclassing handshaker classes and override the newHandshakeRequest request to clean up the request header.

amizurov · 2021-10-06T08:58:58Z

I think we can just remove ORIGIN and provide utility method HttpUtil.websocketOriginValue() for setting it via custom headers if it needed. The most of part was done in #9692. I believe we can close this problem for 4.1.* @normanmaurer WDYT ?

…2293) Motivation: In Netty 5 we only left the latest 13 version of websocket, so we can clean up the code a bit. Modification: - Remove `origin` header from handshake request, it can be set with `customHeaders`, see #9673 - Calculate expected value of `sec-websocket-accept` only when we received a response - Remove unused code - Add more tests for clientHandshaker13 Result: Removed not used code and fixed wrong behaviour with origin header #9673.

…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>

ursaj mentioned this issue Oct 18, 2019

Bugfix #9673: Origin header is always sent from WebSocket client #9692

Merged

normanmaurer closed this as completed in #9692 Nov 27, 2019

normanmaurer added this to the 4.1.44.Final milestone Nov 27, 2019

normanmaurer reopened this Dec 18, 2019

normanmaurer modified the milestones: 4.1.44.Final, 4.1.45.Final Dec 18, 2019

normanmaurer added a commit that referenced this issue Dec 18, 2019

Revert "Bugfix #9673: Origin header is always sent from WebSocket cli…

d24dfb1

…ent (#9692)" This reverts commit f48d9fa as this needs more thoughts.

normanmaurer added a commit that referenced this issue Dec 18, 2019

Revert "Bugfix #9673: Origin header is always sent from WebSocket cli…

607cf80

…ent (#9692)" This reverts commit f48d9fa as it needs more thoughts

normanmaurer modified the milestones: 4.1.45.Final, 4.1.46.Final Jan 13, 2020

normanmaurer modified the milestones: 4.1.46.Final, 4.1.47.Final Feb 28, 2020

normanmaurer modified the milestones: 4.1.47.Final, 4.1.48.Final Mar 9, 2020

normanmaurer modified the milestones: 4.1.48.Final, 4.1.49.Final Mar 17, 2020

normanmaurer modified the milestones: 4.1.49.Final, 5.0.0.Final Apr 22, 2020

ihanyong pushed a commit to ihanyong/netty that referenced this issue Jul 31, 2020

Revert "Bugfix netty#9673: Origin header is always sent from WebSocke…

9ff697a

…t client (netty#9692)" This reverts commit f48d9fa as it needs more thoughts

titusfortner mentioned this issue Feb 9, 2022

[🐛 Bug]: DevTools Functionality is Broken for Firefox 97 in Java SeleniumHQ/selenium#10348

Closed

amizurov mentioned this issue Apr 7, 2022

Remove ORIGIN header from ws opening handshake v13, clean up code #12293

Merged

SquidDev mentioned this issue Oct 31, 2022

Cannot connect to Discord via websocket cc-tweaked/CC-Tweaked#1197

Closed

This was referenced Oct 31, 2022

Fix generating the Origin header value for websocket handshake request #12941

Merged

WebSocketClientHandshaker java.lang.NullPointerException #12933

Closed

normanmaurer closed this as completed in #12941 Nov 10, 2022

normanmaurer modified the milestones: 5.0.0.Final, 4.1.86.Final Nov 10, 2022

OrKoN mentioned this issue Mar 8, 2023

[🐛 Bug]: Chrome 111 is not compatible with default HTTP Client SeleniumHQ/selenium#11750

Closed

dadza mentioned this issue Mar 12, 2023

Origin header should not be sent by client in handshake eclipse-ee4j/tyrus#615

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Origin header is always sent from WebSocket client #9673

Origin header is always sent from WebSocket client #9673

ursaj commented Oct 15, 2019

amizurov commented Oct 16, 2019

slandelle commented Oct 16, 2019

ursaj commented Oct 17, 2019

normanmaurer commented Oct 17, 2019

normanmaurer commented Dec 18, 2019 •

edited

tmsimont commented Jul 15, 2020

tmsimont commented Jul 16, 2020

ursaj commented Jul 17, 2020

normanmaurer commented Jul 17, 2020 via email

tmsimont commented Jul 17, 2020

vietj commented Oct 6, 2021

amizurov commented Oct 6, 2021

Origin header is always sent from WebSocket client #9673

Origin header is always sent from WebSocket client #9673

Comments

ursaj commented Oct 15, 2019

Expected behavior

Actual behavior

Steps to reproduce

Netty version

JVM version (e.g. java -version)

OS version (e.g. uname -a)

amizurov commented Oct 16, 2019

slandelle commented Oct 16, 2019

ursaj commented Oct 17, 2019

normanmaurer commented Oct 17, 2019

normanmaurer commented Dec 18, 2019 • edited

tmsimont commented Jul 15, 2020

tmsimont commented Jul 16, 2020

ursaj commented Jul 17, 2020

normanmaurer commented Jul 17, 2020 via email

tmsimont commented Jul 17, 2020

vietj commented Oct 6, 2021

amizurov commented Oct 6, 2021

JVM version (e.g. `java -version`)

OS version (e.g. `uname -a`)

normanmaurer commented Dec 18, 2019 •

edited