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

feat: Support WebSocket API routes, Upgrade requests #58704

Open
wants to merge 2 commits into
base: canary
Choose a base branch
from

Conversation

AaronFriel
Copy link

Enables route handlers to receive and act on Connection: Upgrade requests, such as WebSockets, when using the Node runtime. To enable this, the base http server has the on('upgrade') handler removed. In this author's opinion, that handler is an anti-pattern as it makes it much more difficult to handle middleware and other request lifecycle behavior.

By passing the raw request to the route handler and implementing a NextResponse.upgrade() response value to opt out of additional processing that would write to the socket, the route handler can handle an upgrade request itself.

Fixes #58698 (feature request)
Fixes #56368 (caused by next-ws / websocket middleware)

@AaronFriel
Copy link
Author

Apologies. I meant to create this as a draft PR, pending discussion with @ijjk and @ztanner.


const { matchedOutput, parsedUrl } = await resolveRoutes({
req,
res: socket as any,

Choose a reason for hiding this comment

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

зачем удалил?

Copy link
Author

Choose a reason for hiding this comment

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

This is not necessary, the regular request handling event can handle upgrade requests.

@AaronFriel AaronFriel force-pushed the friel/upgrade-request-handler branch from 546ebdc to 6bbe950 Compare December 1, 2023 02:25
Enables route handlers to receive and act on `Connection: Upgrade` requests, such as WebSockets,
when using the Node runtime. To enable this, the base http server has the `on('upgrade')` handler
removed. In this author's opinion, that handler is an anti-pattern as it makes it much more
difficult to handle middleware and other request lifecycle behavior.

By passing the raw request to the route handler and implementing a `NextResponse.upgrade()` response
value to opt out of additional processing that would write to the socket, the route handler can
handle an upgrade request itself.

Fixes vercel#58698 (feature request)
Fixes vercel#56368 (caused by next-ws / websocket middleware)
@AaronFriel AaronFriel force-pushed the friel/upgrade-request-handler branch from 6bbe950 to 89bc37c Compare December 1, 2023 02:30
When the request is upgraded as part of a request listener, one of three things
must happen in order to prevent Node.js from closing the connection:

1. Disable request timeouts.

2. The request handler may mark the `OutgoingMessage` as having the response
   headers sent before the timeout expires. This happens as part of
   `flushHeaders()` calling `Socket#_send()` in
   `node.js/lib/_http_outgoing.js`[^1].

3. Add an event listener on the `http.Server` for `clientError` events to handle
   the HTTP request timeout error. In `node.js/lib/_http_server.js`[^2], the
   presence of an event listener bypasses error handling.

We rule out the first two options as:

1. This would re-introduce Slowloris and other vulnerabilities that are the
   reason these timeouts were added.

2. The most popular Websocket library, `ws`, writes to the raw socket and does
   not use `OutgoingMessage` methods which would mark the headers as sent.

This leaves the third option, and we implement a workaround as seen in
[koa-easy-ws#36](b3nsn0w/koa-easy-ws#36).

[^1]: https://github.com/nodejs/node/blob/v20.10.0/lib/_http_outgoing.js#L378
[^2]: https://github.com/nodejs/node/blob/a2206640f366cc145b17a2ece66d36bfa75720ee/lib/_http_server.js#L883
@avarayr
Copy link

avarayr commented Dec 3, 2023

Looks like I'm dealing with the same exact server timeout issue that was addressed by your recent commit. I must say great job on the documentation and research!

Overriding the clientError handler seems to be the only feasible solution. Meanwhile my workaround was a very awkward kludge that proxied req.socket.write and req.socket.destroy to prevent Node from killing my socket.

Hopefully we get some more eyes on this feature and it gets merged soon!

@feedthejim
Copy link
Contributor

@AaronFriel

In this author's opinion, that handler is an anti-pattern as it makes it much more difficult to handle middleware and other request lifecycle behavior.

can you expand on that?

@ijjk
Copy link
Member

ijjk commented Dec 5, 2023

Allow CI Workflow Run

  • approve CI run for commit: 486b362

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@AaronFriel
Copy link
Author

@feedthejim - sure.

on('upgrade') is an anti-pattern that's very likely to surprise users, at best because necessary middleware won't populate a header on the request and it will fail, at worst it introduces security concerns because it's no longer possible to write authz/authn code once, every upgrade handler must be aware of every middleware.

Upgrade handlers don't match user routing expectations

To participate in advanced routing behavior, one has to reimplement it:
https://github.com/apteryxxyz/next-ws/blob/main/packages/core/src/server/utilities/next.ts#L29-L41

There are bugs with this approach: it doesn't handle basePath, it doesn't pass along path params like [id], etc. The upgrade handler needs to be deeply tied to the regular request router.

Upgrade handlers need to re-implement middleware

To implement SOCKET route handlers in the manner of next-ws, Next.js would need to contort itself substantially - and implement middleware (auth, caching, etc.) twice or abstract middleware even further, as upgrade handlers do not receive a ServerResponse object.

It doesn't make sense to me, but you can try this yourself below. So if a Next.js user installs a global middleware to perform authn/authz, then installs next-ws, it's incumbent on them to also implement the middleware behavior they expect. Try it yourself:

import * as http from 'node:http';
import assert, { deepStrictEqual } from 'node:assert';
import { WebSocket, WebSocketServer } from 'ws';

let logOutput: string[] = [];
let log = (message: string) => {
  logOutput.push(message);
  console.log(message);
}

let globalMiddleware = (req, res) => {
  // Imagine this is global auth middleware!
  log('global-middleware');
};

let pathMiddleware = (req, res) => {
  log('path-middleware');
};

const server = http.createServer((req, res) => {
  globalMiddleware(req, res);
  if (req.url === '/middleware') {
   pathMiddleware(req, res);
  }
  log('request');
  res.statusCode = 200;
  res.end('Hello World');
});

server.on('upgrade', (req, socket, head) => {
  log('upgrade');
  socket.end();
  // Must manually implement the response, write to the socket, etc. Middleware is ignored.
});


const port = Math.floor(Math.random() * 20_000 + 20_000);
console.log(`Starting server on port ${port}`);
await new Promise<void>((resolve) => {
  server.listen(port, () => {
    resolve();
  });
});
console.log(`Server started on port ${port}`);

console.log('\n\nTesting /');
logOutput = [];
await fetch(`http://localhost:${port}/`);
assert.deepStrictEqual(logOutput, [  'global-middleware', 'request' ]);

console.log('\n\nTesting /middleware');
logOutput = [];
await fetch(`http://localhost:${port}/middleware`);
assert.deepStrictEqual(logOutput, [  'global-middleware', 'path-middleware', 'request', ]);

logOutput = [];
console.log('\n\nTesting /websocket');
await new Promise<void>((resolve) => {
  const ws = new WebSocket(`ws://localhost:${port}/websocket`);
  ws.on('error', () => {
    // Ignore the error
    resolve();
  });
});
assert.deepStrictEqual(logOutput, [ 'upgrade' ]);

server.close();

The alternative

Implement upgrades as part of the request handler and main router. We get parameter parsing "for free", middleware compliance "for free", niceties in Next such as useSearchParams() work, and the Next router has full control over the socket - we can either write a response to it or upgrade it, and that decision can be made in the request handler.

@bbigras
Copy link

bbigras commented Mar 4, 2024

Any progress on this?

@AaronFriel
Copy link
Author

@feedthejim curious if you've had a chance to look more deeply in this?

@bladerunner2020
Copy link

Implement upgrades as part of the request handler and main router. We get parameter parsing "for free", middleware compliance "for free", niceties in Next such as useSearchParams() work, and the Next router has full control over the socket - we can either write a response to it or upgrade it, and that decision can be made in the request handler.

Could you come with example please how to implement upgrades without on.upgrade?

@AaronFriel
Copy link
Author

AaronFriel commented Mar 23, 2024

@bladerunner2020 upgrades involve "hijacking" the socket (in Go terminology), wherein the request handler for the websocket route takes full control of the duplex socket.

The on.upgrade handler is deficient in that implementing it correctly in the presence of middleware which performs authn/authz, cookie parsing, or other operations is difficult because the shapes of on.upgrade and on.request are so different. When a client requests an upgrade, on.upgrade is fired but any middleware implemented for on.request is not. This means websocket routes could skip authn, authz, redirect/URL rewriting, and more.

To implement upgrades in a request, we must do a few things:

First, ensure that the Node runtime does not close or drop our socket. That's implemented here:

export function handleClientError(
e: NodeJS.ErrnoException,
socket: stream.Duplex
) {
if (isSocketUpgraded(socket)) {
return
}
// Begin Node.js implementation:
if (
socket.writable &&
(!socket?._httpMessage || !socket?._httpMessage?._headerSent)
) {
let response
switch (e.code) {
case 'HPE_HEADER_OVERFLOW':
response = requestHeaderFieldsTooLargeResponse
break
case 'ERR_HTTP_REQUEST_TIMEOUT':
response = requestTimeoutResponse
break
default:
response = badRequestResponse
break
}
socket.write(response)
}
socket.destroy(e)
}

Second, we add a method on the request object to hijack or "upgrade" the request from within an ordinary request handler:

public upgrade(
handler: (request: IncomingMessage, socket: Duplex) => void
): never {
const rawRequest = this[INTERNALS].rawRequest
if (!rawRequest) {
throw new Error(
'Cannot upgrade to websocket, this feature is not compatible with the edge runtime.'
)
}
markSocketUpgraded(rawRequest.socket)
scheduleOnNextTick(() => handler(rawRequest, rawRequest.socket))
throw new RequestUpgraded('upgrade')
}
}

Third, from within a regular request handler, we call req.upgrade() to obtain a raw duplex socket and pass that to any other protocol, e.g.: a websocket server.

export const GET = async (req: NextRequest): Promise<Response> => {
req.upgrade((request, socket) => {
const server = new ws.WebSocketServer({ noServer: true })
server.handleUpgrade(request, socket, Buffer.alloc(0), (ws) => {

The Websocket library parses the request headers and completes the handshake, writing the HTTP/1.1 101 Switching Protocols response with Connection: Upgrade and websocket specific headers.

@Chidocs
Copy link

Chidocs commented Mar 26, 2024

@ztanner @ijjk Any chance this could get looked at? #56368 Would also get addressed by this PR. For anyone using WebSockets (popular with LLM projects), this is potentially impeding progress.

@AaronFriel There appear to be a few conflicts on this PR. Is this still a viable solution in your opinion?

@AaronFriel
Copy link
Author

@Chidocs I think it's still viable, and I expect I'll update this is this is something Vercel will review. @ijjk @feedthejim et al., this is one of the highest upvoted PRs on the repo! I'd love to get a review.

@mushan0x0
Copy link

I need this feature, it's great, it's even a must-have feature for Next.js! @huozhi @ijjk @ztanner

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

Successfully merging this pull request may close these issues.

Next.js TypeError: Cannot read properties of undefined (reading 'bind') - caused by middleware.ts