uNetworking / uWebSockets.js

μWebSockets for Node.js back-ends :metal:
Apache License 2.0
8.08k stars 574 forks source link

app.any('/') is handling also websocket connections #740

Closed a-tonchev closed 2 years ago

a-tonchev commented 2 years ago

If someone try to create a websocket connection to url that has method any, the connection enters the handler but breaks the server, because req and res have no methods:

Example Code on Server:

 app.any('/', (req, res) => {
    const origin = req.getHeader('origin');
    setupCors(res, origin);

    res.writeStatus('200').end('Welcome To The ShopSuy API (<a href="https://JUST-SELL.online">JUST-SELL.online!</a>');
});

Call in the client:

let socket = new WebSocket("ws://localhost:1234");

If there is no websocket registered at this link '/' the server crashes.

I tried also to make some workaround:

 app.any('/', (req, res) => {
   try {
    const origin = req.getHeader('origin');
    setupCors(res, origin);

    res.writeStatus('200').end('Welcome To The ShopSuy API (<a href="https://JUST-SELL.online">JUST-SELL.online!</a>');
    } catch(e) {
      res.close();
    }
});

But res.close is not recognized as an function. If we remove the res.close, then we get error

Error: Returning from a request handler without responding or attaching an abort handler is forbidden!
terminate called without an active exception
Aborted (core dumped)`

I think best would be if the websocket completely ignores any('/')

Insterestingly the any('/*') is not reached by the websocket if I try to connect to something like '/asdasdasd'

Edit:

The only working workaround if you don't want to use the root as websocket endpoint, or to avoid someone to crash your server if you don't use websockets:

app.ws('/', {
  open: ws => ws.close(),
});
a-tonchev commented 2 years ago

Just tried app.get('/') -> the websocket connection is also handled by it... or it tries to.

e3dio commented 2 years ago

It is (res, req) not (req, res). Res is more frequently used than req so is helpful to have as first argument

ghost commented 2 years ago

The equivalent of saying "the server crashes" is like going to a mechanic and saying "the car doesn't work". It gives me no information whatsoever and is an example of how not to post reports.

a-tonchev commented 2 years ago

It is (res, req) not (req, res). Res is more frequently used than req so is helpful to have as first argument

Well okay, that was the reason for crashing - i mistakenly changed them on this route. But anyway - the websocket should not call the app.any('/') handler IMHO :)

e3dio commented 2 years ago

But anyway - the websocket should not call the app.any('/') handler IMHO :)

It does not call the get('/') or any('/') handler if you have the ws('/') route set up. Technically websocket request is an http request, what if I want to respond with 404 or redirect etc, the get handler can do it if ws route is not declared

app.get('/*', res => res.writeStatus('404').end())
a-tonchev commented 2 years ago

But anyway - the websocket should not call the app.any('/') handler IMHO :)

It does not call the get('/') or any('/') handler if you have the ws('/') route set up. Technically websocket request is an http request, what if I want to respond with 404 or redirect etc, the get handler can do it if ws route is not declared

If that is the case, then get('/somethingElse') should also respond to a websocket call to this route. Is it a little bit inconsistency here, or am I missing something?

Anyway - it is not a big deal, we can close the issue if this is the expected behavior.

P.S. @alexhultman great great server, I am migrating right now all our servers from KOA to uWS.js

e3dio commented 2 years ago

If that is the case, then get('/somethingElse') should also respond to a websocket call to this route. Is it a little bit inconsistency here, or am I missing something?

No inconsistency, if the websocket route is declared it will handle websocket route, otherwise if get or any is declared it will handle it, priority is: ws > get > any