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 the issue of redirecting from ws to wss. #1454

Closed
wants to merge 1 commit into from

Conversation

xianglin1998
Copy link

I found that when configuring our company's Apache server, redirecting ws requests to wss and using this websocket library for connection would result in the error "The plain HTTP request was sent to HTTPS port". After using Wireshark for packet capture, I found that this library did not properly configure SSL. After DEBUG, I confirmed that the problem occurred in the 'handle_redirect' function: 'create_connection' requires the correct use of the SSL keyword.

@aaugustin aaugustin added the bug label Mar 27, 2024
@owl-with-beard
Copy link

owl-with-beard commented Jul 15, 2024

Push for visibility. I spent a day to figure out the exact same thing as @xianglin1998 .
If the server requires SSL on the redirected address, the redirection fails as the connection to the new address is still made using {'ssl': False}.

The proposed commit fixes the problem, as it adjusts the handle_redirect function to account for WS --> WSS redirects.
However, I think you have to update the protocol factory too (the lines right above the proposed commit)

factory = self._create_connection.args[0]
factory = functools.partial(
   factory.func,
   *factory.args,
   **dict(factory.keywords, host=new_wsuri.host, port=new_wsuri.port, secure=new_wsuri.secure)
)

(The other way, WSS --> WS is already correctly addressed by raising an error.)

Is there something I can do to make this pull request merge-able @aaugustin ?

Greetings

@aaugustin
Copy link
Member

This patch breaks redirects from a wss:// to another wss:// URI when the ssl argument of connect is set to a value other than True because it replaces the custom SSLContext by a default context. If you run the tests, you get an error for this reason.

@aaugustin aaugustin closed this Jul 21, 2024
@xianglin1998
Copy link
Author

So does the latest release fix the ws to wss redirection bug?

@aaugustin
Copy link
Member

It doesn't because it predates this pull request.

I'm working on a correct fix for this issue as we speak.

aaugustin added a commit that referenced this pull request Jul 21, 2024
@aaugustin
Copy link
Member

Now fixed in the main branch. The fix will be included in the next release.

@aaugustin
Copy link
Member

Thank you for raising this issue!

@xianglin1998
Copy link
Author

Good!!!Thank you for your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants