-
Notifications
You must be signed in to change notification settings - Fork 331
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
fix: close websocket with reason on invalid token #9744
Conversation
Signed-off-by: Matt Krick <matt.krick@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! No comments
if (protocol !== 'trebuchet-ws') { | ||
sendToSentry(new Error(`WebSocket error: invalid protocol: ${protocol}`)) | ||
// WS Error 1002 is roughly HTTP 412 Precondition Failed because we can't support the req header | ||
res.writeStatus('412').end() | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 I think we need to distinguish 2 cases here: The client speaks our language but says the wrong thing (right protocol, wrong auth) and we don't understand it at all (wrong protocol). I think in the latter case, we should not move forward with the upgrade which should also fail the client permanently because it could never connect. Only when the protocol matches, we should upgrade and do the other checks. In that case the client can talk websocket with the server, so there is no need to try other connection methods.
return | ||
} | ||
res.upgrade({ip, authToken}, key, protocol, extensions, context) | ||
const authToken = getQueryToken(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-2 we throw here now and the client tries reconnecting endlessly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoa! could you provide some steps to reproduce? This function has a try/catch inside it so it shouldn't throw, it should just return a {}
, which would then be used to send the ws.close
signal, but I may have missed something!
Description
fix #8370
The server wasn't closing websockets with a
reason
when the session was invalidated.This is important because if a
reason
is included, the client nukes the authToken.Either I missed an API change in uws.js, or it hasn't been working for awhile.
This also rolls back to trebuchet-client 3.0.1 because once
canConnect
is true, it should not turn to false when it encounters an error. It should just try again.The downside to this pattern is that the server has to upgrade the HTTP1.1 request to a websocket before it can close the websocket. This means the server tells the client that the websocket is open (fires
WebSocket.onopen
), the client sends pending messages (e.g. subscription requests) and then the server immediately closes it. It's extra work on the client & server, but sending a 401 during a websocket upgrade does not make it to the client. Per the spec: https://websockets.spec.whatwg.org/#eventdef-websocket-errorTEST