inspircd / inspircd

A modular C++ IRC server (ircd).
https://www.inspircd.org
1.14k stars 265 forks source link

Websocket connection attempt from Node.js and Deno fails with 400 response #2066

Closed aronson closed 1 month ago

aronson commented 8 months ago

Description

Hello, I wrote websocket protocol support for deno-irc, an IRC client library.

I found that InspIRCd does not negotiate the websocket connection attempt properly in the Node.js and Deno environments. It does work in browser environments, specifically tested with Google Chrome and Safari. We also tested against Ergo and UnrealIRCd.

Here is the wireshark output of a failed negotiation from the testnet InspIRCd server followed by an example successful connection from a local Ergo server.

The shape of the HTTP upgrade attempt sent by the framework looks as such:

Hypertext Transfer Protocol
    GET / HTTP/1.1\r\n
        [Expert Info (Chat/Sequence): GET / HTTP/1.1\r\n]
        Request Method: GET
        Request URI: /
        Request Version: HTTP/1.1
    user-agent: Deno/1.37.2\r\n
    host: testnet.inspircd.org:8067\r\n
    upgrade: websocket\r\n
    connection: Upgrade\r\n
    sec-websocket-key: hzrVmFpj9t85Wv8/sNcGeg==\r\n
    sec-websocket-version: 13\r\n
    sec-websocket-protocol: binary.ircv3.net, text.ircv3.net\r\n
    \r\n
    [Full request URI: http://testnet.inspircd.org:8067/]
    [HTTP request 1/1]
    [Response in frame: 63044]

The server returns a 400 bad request response and the connection fails.

This currently blocks InspIRCd websocket support in deno-irc. I have also tested against a build of the master branch.

Steps to reproduce the issue: Both Node.js and Deno can reproduce this issue as clients with a minimal example borrowed from this working browser IRC websocket client. I've provided instructions for both.

Node:

  1. Install Node.js if not present
  2. Download attached node-tester.zip
  3. Extract folder and cd into it
  4. npm install to install websocket module
  5. node test.js to execute test script. Edit websocket URL string appropriately for local testing
  6. Program will execute and print out failed websocket connection attempt

Deno:

  1. Install Deno if not present
  2. Download attached test.ts.zip
  3. Extract to test.ts
  4. deno run -A test.ts in the same directory. Edit websocket URL string appropriately for local testing
  5. Program will execute and print out failed websocket connection attempt

Describe the results you received: A 400 Bad Request response on a websocket upgrade attempt from two standard JS/TS runtimes.

Describe the results you expected: A 101 Switching Protocols response on a websocket upgrade attempt.

Additional information you deem important (e.g. issue happens only occasionally): EDIT: I uploaded files with my personal testing URLs by mistake originally and quickly fixed them to the testnet ws://testnet.inspircd.org:8067, apologies for any confusion.

Output of ./bin/inspircd --version: InspIRCd version: 4.0.0-82b5826d0

SadieCat commented 8 months ago

I'm not at a computer right now so I can't check your code but according to the testnet logs you are not sending an origin header. This is required by InspIRCd as it used for checking whether the origin host is whitelisted (testnet allows all origins but this behaviour is not recommended for typical servers as it puts them at risk of bot attacks).

aronson commented 8 months ago

What origin header might a client send when it's not from a web browser? The hostname?

We indeed don't send an origin header. That must be the issue. Surprising the other two server implementations allow this. Is this documented somewhere?

aronson commented 8 months ago

Seems there is a problem, Node.js and Deno don't allow the user to modify the headers of the upgrade attempt.

I was unable to find a custom library that offers functionality to modify the Origin header while also allowing for subprotocol negotiation.

I suggest enabling behavior to allow this optional header to be optional when clients do not support it in the case where the allowed origins is set to "*". Otherwise a categorical class of clients will not be able to connect to your server implementation.

SadieCat commented 8 months ago

Adding special cases for some origins is problematic as we would then need to handle a bunch of extra cases that are effectively the same like https://*. Whitelisting such origins is also massively discouraged as it exposes servers to spambot attacks (like this similar non-websocket attack).

Websocket support is intended for web applications (which aways send an origin header) controlled by the IRC network admins. If you're making a desktop application there is zero benefit for using websockets over normal sockets so you can just use them instead.

aronson commented 8 months ago

Deno also operates in the browser environment where it should send the origin header. You make a great point, servers aren't going to only provide a websocket endpoint without offering a TCP endpoint. If they do and it's an issue I think it's on the operator to support their clients.

I'm going to close this issue out as not valid. Thank you for explaining what's going on here and why my understanding was incorrect, I appreciate it 🙂

SadieCat commented 1 month ago

I've been thinking about this some more and I've decided that it doesn't hurt for us to support this as long as admins can opt-out of it.