Skip to content

Upgrading from polling to webtransport will report engine invalid WebTransport handshake. #688

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

Closed
zishang520 opened this issue Jul 21, 2023 · 8 comments
Labels
bug Something isn't working

Comments

@zishang520
Copy link

Describe the bug
Upgrading from polling to webtransport will report engine invalid WebTransport handshake.

To Reproduce

Please fill the following code example:

Engine.IO server version: 6.5.1

Server

import { readFileSync } from "fs";
import { createServer } from "https";
import { Server } from "engine.io";
import { Http3Server } from "@fails-components/webtransport";

// WARNING: the total length of the validity period MUST NOT exceed two weeks (https://w3c.github.io/webtransport/#custom-certificate-requirements)
const cert = readFileSync("/path/to/my/cert.pem");
const key = readFileSync("/path/to/my/key.pem");

const httpsServer = createServer({
  key,
  cert
});

httpsServer.listen(3000);

const engine = new Server({
  transports: ["polling", "websocket", "webtransport"] // WebTransport is not enabled by default
});

engine.attach(httpsServer);

const h3Server = new Http3Server({
  port: 3000,
  host: "0.0.0.0",
  secret: "changeit",
  cert,
  privKey: key,
});

(async () => {
  const stream = await h3Server.sessionStream("/engine.io/");
  const sessionReader = stream.getReader();

  while (true) {
    const { done, value } = await sessionReader.read();
    if (done) {
      break;
    }
    engine.onWebTransportSession(value);
  }
})();

h3Server.startServer();

Engine.IO client version: 6.5.1

Client

const socket = require("engine.io-client")("wss://localhost:3000", {
    transports: ["polling", "webtransport"]
});

function send() {
    socket.send('ping');
}

socket.on('open', () => {
    send();
});

socket.on('close', () => {
    console.log("closed.")
});

socket.on('message', () => {
    // setTimeout(send, 100);
});

Expected behavior

  engine:socket executing batch send callback +4ms
  engine intercepting request for path "/engine.io/" +14ms
  engine handling "POST" http request "/engine.io/?EIO=4&transport=polling&t=Obthcpl&sid=tEe1aTFiB5e8y_T8AAAA" +0ms
  engine applying middleware n°1 +0ms
  engine setting new request for existing client +1ms
  engine:polling received "4ping" +14ms
  engine:socket received packet message +11ms
4
  engine:socket sending packet "message" (pong) +1ms
  engine intercepting request for path "/engine.io/" +7ms
  engine handling "GET" http request "/engine.io/?EIO=4&transport=polling&t=Obthcpm&sid=tEe1aTFiB5e8y_T8AAAA" +0ms
  engine applying middleware n°1 +0ms
  engine setting new request for existing client +1ms
  engine:polling setting request +7ms
  engine:socket flushing buffer to transport +6ms
  engine:polling writing "4pong" +0ms
  engine:socket executing batch send callback +0ms
handshake data >>>>>> 0{"sid":"tEe1aTFiB5e8y_T8AAAA"}2probe
  engine invalid WebTransport handshake +7ms
  engine intercepting request for path "/engine.io/" +31ms
  engine handling "GET" http request "/engine.io/?EIO=4&transport=polling&t=Obthcq1&sid=tEe1aTFiB5e8y_T8AAAA" +0ms
  engine applying middleware n°1 +0ms
  engine setting new request for existing client +1ms
  engine:polling setting request +39ms
        console.log(handshake);
        const sid = parseSessionId(handshake);
        if (!sid) {
            debug("invalid WebTransport handshake");
            return session.close();
        }

Platform:

  • Device: [e.g. Samsung S8]
  • OS: [e.g. Android 9.2]

Additional context
Upgrading from polling to webtransport will report engine invalid WebTransport handshake, because the original message reported is: 0{"sid":"xxx"}2probe, parseSessionId cannot correctly parse the sid

@zishang520 zishang520 added the bug Something isn't working label Jul 21, 2023
@zishang520
Copy link
Author

I used Firefox to test some message sending and receiving, and there will be sticky packets. This feature also needs improvement.

@darrachequesne
Copy link
Member

That's weird, it seems two messages are concatenated: 0{"sid":"tEe1aTFiB5e8y_T8AAAA"}2probe

Which version of Firefox are you using? Which OS?

@zishang520
Copy link
Author

That's weird, it seems two messages are concatenated: 0{"sid":"tEe1aTFiB5e8y_T8AAAA"}2probe

Which version of Firefox are you using? Which OS?

The OS is the latest version of Windows 11, and the Firefox browser is also the latest version.

darrachequesne added a commit that referenced this issue Aug 1, 2023
WebTransport being a stream-based protocol, the chunking boundaries are
not necessarily preserved. That's why we need a header indicating the
type of the payload (plain text or binary) and its length.

We will use a format inspired by the WebSocket frame:

- first bit indicates whether the payload is binary
- the next 7 bits are either:
  - 125 or less: that's the length of the payload
  - 126: the next 2 bytes represent the length of the payload
  - 127: the next 8 bytes represent the length of the payload

Reference: https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_servers#decoding_payload_length

Related:

- #687
- #688
@darrachequesne
Copy link
Member

@zishang520 this should be fixed by a306db0, included in version 6.5.2.

Could you please check?

@zishang520
Copy link
Author

ok i will get back to you after testing.

@zishang520
Copy link
Author

@zishang520 this should be fixed by a306db0, included in version 6.5.2.

Could you please check?

I have tested sending and receiving messages, and there are no issues; it functions properly. However, there is another problem that needs to be addressed. Here are the reproduction steps:

On a Windows system, using the Firefox browser (version 115.0.2 (64-bit)), after starting the engine.io server, connecting to the server using the browser client, sending a message to the server in the 'open' event. Then, pressing F5 to refresh the page and reconnect to the server leads to a server crash.

  send accept ranges +0ms
  send cache-control public, max-age=0 +0ms
  send modified Fri, 04 Aug 2023 06:56:42 GMT +0ms
  send etag W/"5f3-189bf558e3d" +0ms
  send content-type application/javascript +0ms
  send not modified +0ms
  engine intercepting request for path "/engine.io/" +10m
  engine handling "GET" http request "/engine.io/?EIO=4&transport=polling&t=Oc_Oe50" +0ms
  engine applying middleware n°1 +0ms
  engine handshaking client "MKuE7QAV04RAXQT5AAAB" +1ms
  engine:transport readyState updated from undefined to open (polling) +10m
  engine:socket readyState updated from undefined to opening +3s
  engine:socket readyState updated from opening to open +1ms
  engine:socket sending packet "open" ({"sid":"MKuE7QAV04RAXQT5AAAB","upgrades":["webtransport"],"pingInterval":25000,"pingTimeout":20000,"maxPayload":1000000}) +0ms
  engine:polling setting request +10m
  engine:socket flushing buffer to transport +0ms
  engine:polling writing "0{"sid":"MKuE7QAV04RAXQT5AAAB","upgrades":["webtransport"],"pingInterval":25000,"pingTimeout":20000,"maxPayload":1000000}" +0ms
  engine:socket executing batch send callback +1ms
  engine:transport readyState updated from open to closed (webtransport) +16ms
  engine:socket readyState updated from open to closed +15ms
  engine:webtransport error while reading: undefined +3s
node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "0".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

Node.js v18.17.0

darrachequesne added a commit that referenced this issue Oct 5, 2023
Refreshing the page with a client connected with WebTransport would
trigger the following exception:

> node:internal/process/promises:288
>            triggerUncaughtException(err, true /* fromPromise */);
>            ^
>
> [UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "0".] {
>   code: 'ERR_UNHANDLED_REJECTION'
> }

Related: #688
@darrachequesne
Copy link
Member

@zishang520 this should finally be fixed by ff1c861, included in version 6.5.3.

Could you please confirm?

@zishang520
Copy link
Author

@zishang520 this should finally be fixed by ff1c861, included in version 6.5.3.

Could you please confirm?

I have completed the test, and the issue has been fixed. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants