websockets icon indicating copy to clipboard operation
websockets copied to clipboard

Don't follow redirects when path, sock, host or port are overridden

Open aaugustin opened this issue 6 years ago • 5 comments

(from https://github.com/aaugustin/websockets/pull/544#issuecomment-505837262)

Since #528 websockets follows redirects.

I believe that this feature is incompatible with connection overrides (either with host + port or with sock). If you need to control exactly where you're connecting, I assume you aren't expecting to be redirected somewhere else.

This deserves documentation at a minimum, perhaps a warning or even an exception.

aaugustin avatar Jun 26 '19 11:06 aaugustin

Here's what I think is possible:

  • [ ] path is set (Unix socket connection): only same-origin redirects
  • [ ] sock is set (already connected socket): if the server responds with connection: keep-alive, only same-origin redirects, else, no redirects — not sure I'll want to get into this level of detail for an edge case, I think it's fine to reject all redirects in that case
  • [ ] host or port is set (network address override): only same-origin redirects
  • [x] only uri is set: all redirects

aaugustin avatar Jun 29 '19 11:06 aaugustin

Patch should look like:

diff --git a/src/websockets/client.py b/src/websockets/client.py
index 10435c1..a2a9f32 100644
--- a/src/websockets/client.py
+++ b/src/websockets/client.py
@@ -497,6 +497,16 @@ class Connect:
             old_wsuri.host == new_wsuri.host and old_wsuri.port == new_wsuri.port
         )

+        # Redirect isn't compatible with an already connected socket.
+        # (Same-origin redirect would be possible if the server keeps
+        # the connection alive, but let's keep things simple.)
+        if self._create_connection.keywords.get("sock") is not None:
+            raise InvalidHandshake(f"cannot redirect when sock is set")
+
+        # TODO - prevent cross-origin redirects when host and port are
+        # overridden or when path is set. Probably this requires keeping
+        # track of the initial situation in __init__...
+
         # Rewrite the host and port arguments for cross-origin redirects.
         # This preserves connection overrides with the host and port
         # arguments if the redirect points to the same host and port.

Writing this code and associated tests is a slightly boring, so I'll wait to see if someone actually hits this issue. Please add a comment if you do!

aaugustin avatar Jun 29 '19 15:06 aaugustin

Well, it looks like demand for this feature is low...

aaugustin avatar Jun 05 '21 21:06 aaugustin

Probably the pragmatic solution is to disable all redirects when any of path, sock, host, or port is provided and wait until someone asks for allowing same-origin redirects.

aaugustin avatar Jun 05 '21 21:06 aaugustin