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

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

Closed
aaugustin opened this issue Jun 26, 2019 · 6 comments · Fixed by #1495
Closed

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

aaugustin opened this issue Jun 26, 2019 · 6 comments · Fixed by #1495

Comments

@aaugustin
Copy link
Member

(from #544 (comment))

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 aaugustin added this to the 8.0 milestone Jun 26, 2019
@aaugustin
Copy link
Member Author

aaugustin commented Jun 29, 2019

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 EDIT: I don't want to get into this level of detail
  • host or port is set (network address override): only same-origin redirects
  • only uri is set: all redirects

@aaugustin
Copy link
Member Author

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 aaugustin removed this from the 8.0 milestone Jun 29, 2019
@aaugustin aaugustin changed the title Following redirects is incompatible with passing a sock argument. Don't follow redirects when path, sock, host or port are overridden Jun 29, 2019
@aaugustin
Copy link
Member Author

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

@aaugustin
Copy link
Member Author

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.

SLiV9 added a commit to SLiV9/websockets that referenced this issue Feb 21, 2024
SLiV9 added a commit to SLiV9/websockets that referenced this issue Feb 22, 2024
@SLiV9
Copy link

SLiV9 commented Feb 22, 2024

I ran into problems with unexpected cross-origin redirects, so I have created a PR that implements this: #1444

My use case is disabling cross-origin redirects for security reasons. I agree with your assessment that if you really care about the host you are connecting to, it makes sense to pass the host as an argument, and that avoids having a separate "disable redirects" parameter. Perhaps this needs to be mentioned in the documentation as well.

Thanks for the extensive issue description, it really simplified writing the PR. Please let me know if this needs anything else to be merged; I'll probably use my fork until then.

@aaugustin
Copy link
Member Author

Supporting keep-alive connections (which is the default in HTTP/1.1 unless the server sends Connection: close) gets complicated quickly.

Probably, we could simply reuse the socket (passing it back with sock=...) in the not-TLS case. In the TLS case, if we keep ssl=..., then asyncio will redo the TLS handshake; if we remove it, then websockets won't realize that it's already a TLS socket and complain.

On top of that, we have to handle cases where host/port are set and unix connections. While it has to be possible to handle systematically all cases, this is a lot of work and complexity for a little to no benefit. No one complained that the current implementation doesn't reuse connections when it could.

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