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

Use SSL context for "wss" scheme #869

Merged
merged 6 commits into from
Jan 9, 2024
Merged

Use SSL context for "wss" scheme #869

merged 6 commits into from
Jan 9, 2024

Conversation

MtkN1
Copy link
Contributor

@MtkN1 MtkN1 commented Jan 7, 2024

Summary

This pull request addresses an issue where connecting to a WebSocket server using the "wss" scheme with TLS connection is not possible. (I have not created a discussion, but since it is a small bug fix, I have created a pull request 🙏)

The problem lies in the condition that enables the SSL context, which currently only allows the "https" scheme. This pull request adds the "wss" scheme to the conditions for enabling the SSL context.

Real-world reproduction code: ("echo.websocket.events" is a publicly available WebSocket echo server)

import httpcore

# no secure (success)
with httpcore.ConnectionPool() as pool:
    with pool.stream("GET", "ws://echo.websocket.events") as response:
        print("no secure", response)

print("-" * 80)

# secure (failed)
with httpcore.ConnectionPool() as pool:
    with pool.stream("GET", "wss://echo.websocket.events") as response:
        print("secure", response)

Output:

no secure <Response [200]>
--------------------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/tmp.qSsd6WTpkd.py", line 12, in <module>
    with pool.stream("GET", "wss://echo.websocket.events") as response:
  File "/home/codespace/.python/current/lib/python3.10/contextlib.py", line 135, in __enter__
    return next(self.gen)
  File "/workspaces/httpcore/httpcore/_sync/interfaces.py", line 75, in stream
    response = self.handle_request(request)
  File "/workspaces/httpcore/httpcore/_sync/connection_pool.py", line 268, in handle_request
    raise exc
  File "/workspaces/httpcore/httpcore/_sync/connection_pool.py", line 251, in handle_request
    response = connection.handle_request(request)
  File "/workspaces/httpcore/httpcore/_sync/connection.py", line 103, in handle_request
    return self._connection.handle_request(request)
  File "/workspaces/httpcore/httpcore/_sync/http11.py", line 132, in handle_request
    raise exc
  File "/workspaces/httpcore/httpcore/_sync/http11.py", line 110, in handle_request
    ) = self._receive_response_headers(**kwargs)
  File "/workspaces/httpcore/httpcore/_sync/http11.py", line 175, in _receive_response_headers
    event = self._receive_event(timeout=timeout)
  File "/workspaces/httpcore/httpcore/_sync/http11.py", line 225, in _receive_event
    raise RemoteProtocolError(msg)
httpcore.RemoteProtocolError: Server disconnected without sending a response.

Tests

  1. In the first commit, I modified existing tests to fail. I added a Trace to the test to track the condition for enabling the SSL context. The test fails because the Trace "connection.start_tls.{started,complete}" is missing due to the absence of the "wss" scheme in the conditions for enabling the SSL context.
  2. In the second commit, I added the "wss" scheme to the conditions for enabling the SSL context. This ensures that the initially failing test now succeeds.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@tomchristie
Copy link
Member

😅 Reasonable. Neat test case.
Let's have a CHANGELOG.md entry for it, and we can get this in.

@MtkN1
Copy link
Contributor Author

MtkN1 commented Jan 8, 2024

Thank you for the review!
I have updated a CHANGELOG.md.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks.

@tomchristie tomchristie merged commit b2de19e into encode:master Jan 9, 2024
5 checks passed
@tomchristie tomchristie mentioned this pull request Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants