Surface websocket protocol errors to user applications #804

smerritt · 2023-06-22T19:54:54Z

Currently, if a websocket client sends bad data[1] to an application, the application just receives a generic error message with no indication of what went wrong. Also, the client just gets an aborted[2] websocket connection without any clue as to what they did wrong.

This commit changes two things: first, the application now gets an error event describing what was wrong with the received data. This gives the application's owner some clue what's going wrong.

Second, we now send a Close frame to the client with an appropriate error code, as we SHOULD do[3]. In an ideal world, this will let the client's owner figure out what they're doing wrong and fix it.

[1] Invalid according to RFC 6455, for example sending a continuation frame without a preceding start frame or sending a frame with reserved bits (RSV1, RSV2, and RSV3; see
https://datatracker.ietf.org/doc/html/rfc6455#section-5.2) set.

[2] The underlying TCP connection is closed without first sending a websocket "Close" frame.

[3] https://datatracker.ietf.org/doc/html/rfc6455#section-7.1.7

github-actions · 2023-06-22T19:55:09Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

smerritt · 2023-06-23T15:35:32Z

I have read the CLA Document and I hereby sign the CLA

src/workerd/api/web-socket.c++

src/workerd/api/web-socket.h

src/workerd/io/io-context.h

src/workerd/server/server-test.c++

src/workerd/api/web-socket.h

src/workerd/api/web-socket.c++

MellowYarker · 2023-07-21T02:15:27Z

src/workerd/api/web-socket.c++

-  auto upstream = other->pumpTo(*self);
-  auto downstream = self->pumpTo(*other);
+  auto upstream = other->pumpTo(*self).catch_([](kj::Exception&& e) {
+    KJ_UNWRAP_OR(WebSocketProtocolError::fromException(e), {


Might be misreading this, but I suspect:

if (WebSocketProtocolError::fromException(e) == nullptr) { throw e; }

may have clearer intent. KJ_UNWRAP_OR returns the inner value on success, but we don't seem to be doing anything with that value here so I'm wondering if something was supposed to be done with it?

I'm actually not sure if it matters all that since this is pretty much passing through the error, but you might also want to use kj::throwFatalException(e) instead. I believe that it basically extends our async stack a bit?

btw, can this be written to use coroutine syntax rather than using catch_?

And yes, this should use kj::throwFatalException(e) instead.

Ah, that's how you say "this Maybe is empty". I'd found KJ_IF_MAYBE(_, m){} else { ... }, but that's ugly.

(In my ideal world, kj::Maybe would provide operator bool() for "has stuff in it", or maybe a .empty() method, but this is not that world.)

I fixed everything but the coroutine syntax. I can convert each individual pumpTo call, but currently the two promises get joined down below and I'm not sure what to do with that. Suggestions welcome.

@jasnell I've coroutine-ified things as much as I can. There's still a call to kj::joinPromises that I don't see how to get rid of, but there's no more catch_. Does this look good to you?

src/workerd/api/web-socket.c++

Needs updating and rebasing

MellowYarker

Thanks Sam!

MellowYarker · 2023-07-31T14:01:25Z

🙃 it just occurred to me that the hibernatable websocket readloop is outside the regular Workerd web-socket code. Could you also update workerd/io/hibernation-manager.c++'s readLoop so we surface the error there too?

Needs updates for hibernation

Currently, if a websocket client sends bad data[1] to an application, the application just receives a generic error message with no indication of what went wrong. Also, the client just gets an aborted[2] websocket connection without any clue as to what they did wrong. This commit changes two things: first, the application now gets an error event describing what was wrong with the received data. This gives the application's owner some clue what's going wrong. Second, we now send a Close frame to the client with an appropriate error code, as we SHOULD do[3]. In an ideal world, this will let the client's owner figure out what they're doing wrong and fix it. [1] Invalid according to RFC 6455, for example sending a continuation frame without a preceding start frame or sending a frame with reserved bits (RSV1, RSV2, and RSV3; see https://datatracker.ietf.org/doc/html/rfc6455#section-5.2) set. [2] The underlying TCP connection is closed without first sending a websocket "Close" frame. [3] https://datatracker.ietf.org/doc/html/rfc6455#section-7.1.7

smerritt · 2023-11-28T20:59:40Z

I'm dusting this change off after months of ignoring it. It still needs to include hibernating websockets, so I've marked as draft for the time being.

smerritt · 2024-01-29T18:36:14Z

Obsoleted by capnproto/capnproto#1896, which handles the error much closer to its cause

smerritt force-pushed the smerritt/websocket-protocol-errors branch from 8c33a48 to 6ff569a Compare June 22, 2023 23:05

jasnell reviewed Jun 23, 2023

View reviewed changes

src/workerd/api/web-socket.c++ Outdated Show resolved Hide resolved

github-actions bot added a commit that referenced this pull request Jun 28, 2023

@smerritt has signed the CLA from Pull Request #804

d188f44

smerritt force-pushed the smerritt/websocket-protocol-errors branch from 26e1402 to 013463a Compare July 10, 2023 23:03

smerritt changed the title ~~WIP: surface websocket protocol errors to user applications~~ Surface websocket protocol errors to user applications Jul 10, 2023

smerritt marked this pull request as ready for review July 10, 2023 23:05

smerritt force-pushed the smerritt/websocket-protocol-errors branch from 013463a to df9776c Compare July 11, 2023 21:57