- Notifications
You must be signed in to change notification settings - Fork1.6k
Ensured Netty provider uses CONNECT when proxying ws#657
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The WS standard requires that clients use the CONNECT method whenproxying WebSockets, even if they aren't using SSL. The Grizzlyprovider already supported this, but the Netty didn't.I also implemented a test that tested this case.
This needed to be done both for the Grizzly and the Netty provider.Since this is essentially the same configuration parameter as theuseRelativeURIsWithSSLProxies, I reused that parameter, but I renamed itto useRelativeURIsWithConnectProxies to reflect its true meaning sinceit's no longer used just for SSL, and I deprecated the old parameter.
jroper commentedJul 30, 2014
I've attached a second commit to this pull request, it ensures that when proxying websockets using the CONNECT method, a relative path is used. This slightly changed the meaning of the Let me know if you'd prefer these two commits to be squashed. |
slandelle commentedJul 30, 2014
@jroper Thanks! Could you squash the commits please? I'll then port on 1.9 and master. |
Ensured Netty provider uses CONNECT when proxying ws
jroper commentedJul 30, 2014
I guess you don't want me to squash the commits anymore? |
slandelle commentedJul 30, 2014
Nope, thanks :) I thought you were offline at this time and I wanted to port this ASAP on the other branches. |
slandelle commentedJul 30, 2014
@jroper I guess you need me to release 1.8.13? |
jroper commentedJul 30, 2014
No rush, we fixed this for a customer but that customer can use their own builds until you release it. |
muymoo commentedJul 31, 2014
That customer is very happy. Tested in a corporate environment and ws/wss worked for me. Thanks! |
slandelle commentedJul 31, 2014
Thanks for your feedback, and thanks again to@jroper for the fix! |
* Fix HTTP2StreamChannel leak* Update code comments.
The WS standard requires that clients use the CONNECT method when proxying WebSockets, even if they aren't using SSL. The Grizzly provider already supported this, but the Netty didn't.
I also implemented a test that tested this case.
I've made this change and PR against the 1.8.x branch, master is significantly different but briefly reading the code it looks like the bug is there as well. If you'd prefer it against master first, and then backported, or want me to submit another PR against master/1.9.x, let me know.