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

add native websocket support #2350

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Conversation

ksshen0000
Copy link
Contributor

@ksshen0000 ksshen0000 commented Feb 18, 2024

Add native support for websocket.

this file are derived from pwntools-tube-websocket by frankli0324 under the MIT License.
https://gist.github.com/frankli0324/795162a14be988a01e0efa0531f7ac5a

@peace-maker peace-maker linked an issue Feb 20, 2024 that may be closed by this pull request
Copy link
Member

@peace-maker peace-maker left a comment

Choose a reason for hiding this comment

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

Thank you for bringing this in! There are several things we need to consider before merging which I've mentioned inline.

Hi @frankli0324


Examples:

>>> ws = wstube('wss://echo.websocket.events')
Copy link
Member

Choose a reason for hiding this comment

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

Can we host a basic echo server ourselves in ci instead of relying on an external service? Those tend to be flakey and disturb the testing flow from time to time.

@@ -69,7 +69,7 @@ The table below shows which release corresponds to each branch, and what date th
| [2.2.0](#220) | | Jan 5, 2015

## 4.13.0 (`dev`)

- [#2350][2350] add native websocket support
Copy link
Member

Choose a reason for hiding this comment

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

Please add new entries to the bottom of the list

.. testsetup:: *

from pwn import *
from websocket import WebSocket, ABNF, WebSocketException, WebSocketTimeoutException
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary since it's not used in the doc tests themselves


Arguments:
url (str): The websocket server's URL to connect to.
headers (dict): The same headers as the websocket protocol.
Copy link
Member

Choose a reason for hiding this comment

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

Aren't those additional headers to pass around?
How could we test this?

self.closed = False
self.sock = WebSocket()
self.url = url
self.sock.connect(url, header=headers)
Copy link
Member

Choose a reason for hiding this comment

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

I think it is worth it to forward other useful arguments to the constructor like host, origin, cookie, and subprotocols. As well as the other ones.

Copy link
Member

Choose a reason for hiding this comment

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

Handling the proxy configured in context.proxy would be great too!

self.url = url
self.sock.connect(url, header=headers)


Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a magic __call__ handler to forward calls to the underlying web socket instance like ping()

Some __getattr__ forward to e.g. get the optional close reason of the remote after the server closed the connection would be cool too.

except:
return False

def close(self):
Copy link
Member

Choose a reason for hiding this comment

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

It should be possible to specify the web socket close reason

self.sock.connect(url, header=headers)


def recv_raw(self, numb):
Copy link
Member

Choose a reason for hiding this comment

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

can_recv_raw needs to be implemented too to handle timeouts in the tube model.

@minpeter
Copy link

Is anyone working on this? I hope it will be merged by the third 2024 Q3 at the latest.

If there are no contributors working on it, I will try it.

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.

Native websocket support in pwntools
3 participants