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.

Sign up for GitHub

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

Jump to bottom

fix: close websocket with reason on invalid token #9744

Merged
merged 1 commit into from
May 9, 2024
Merged

fix: close websocket with reason on invalid token #9744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/client/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
"@emotion/styled": "^10.0.27",
"@mattkrick/graphql-trebuchet-client": "^2.2.1",
"@mattkrick/sanitize-svg": "0.4.0",
"@mattkrick/trebuchet-client": "^3.0.2",
"@mattkrick/trebuchet-client": "3.0.1",
"@mui/icons-material": "^5.8.4",
"@mui/material": "^5.9.2",
"@mui/x-date-pickers": "^6.3.1",
Expand Down
27 changes: 26 additions & 1 deletion packages/server/socketHandlers/handleOpen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,46 @@
import {WebSocketBehavior} from 'uWebSockets.js'
import {TrebuchetCloseReason} from '../../client/types/constEnums'
import activeClients from '../activeClients'
import AuthToken from '../database/types/AuthToken'
import ConnectionContext from '../socketHelpers/ConnectionContext'
import keepAlive from '../socketHelpers/keepAlive'
import {sendEncodedMessage} from '../socketHelpers/sendEncodedMessage'
import {isAuthenticated} from '../utils/authorization'
import checkBlacklistJWT from '../utils/checkBlacklistJWT'
import sendToSentry from '../utils/sendToSentry'
import handleConnect from './handleConnect'

const APP_VERSION = process.env.npm_package_version
export type SocketUserData = {
connectionContext: ConnectionContext
authToken: AuthToken
ip: string
protocol: string
done?: true
}

const handleOpen: WebSocketBehavior<SocketUserData>['open'] = async (socket) => {
const {authToken, ip} = socket.getUserData()
const {authToken, ip, protocol} = socket.getUserData()
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
socket.end(412, 'Invalid protocol')
return
}

if (!isAuthenticated(authToken)) {
socket.end(401, TrebuchetCloseReason.EXPIRED_SESSION)
return
}

// ALL async calls must come after the message listener, or we'll skip out on messages (e.g. resub after server restart)
const {sub: userId, iat} = authToken
const isBlacklistedJWT = await checkBlacklistJWT(userId, iat)
if (isBlacklistedJWT) {
socket.end(401, TrebuchetCloseReason.EXPIRED_SESSION)
return
}

const connectionContext = (socket.getUserData().connectionContext = new ConnectionContext(
socket,
authToken,
Expand Down
32 changes: 4 additions & 28 deletions packages/server/socketHandlers/handleUpgrade.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,14 @@
import {WebSocketBehavior} from 'uWebSockets.js'
import {TrebuchetCloseReason} from '../../client/types/constEnums'
import safetyPatchRes from '../safetyPatchRes'
import {isAuthenticated} from '../utils/authorization'
import checkBlacklistJWT from '../utils/checkBlacklistJWT'
import getQueryToken from '../utils/getQueryToken'
import sendToSentry from '../utils/sendToSentry'
import uwsGetIP from '../utils/uwsGetIP'

const handleUpgrade: WebSocketBehavior<void>['upgrade'] = async (res, req, context) => {
safetyPatchRes(res)
const protocol = req.getHeader('sec-websocket-protocol')
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
}
Comment on lines -13 to -18
Copy link
Contributor

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.

const authToken = getQueryToken(req)
if (!isAuthenticated(authToken)) {
res.writeStatus('401').end()
return
}

const handleUpgrade: WebSocketBehavior<void>['upgrade'] = (res, req, context) => {
const key = req.getHeader('sec-websocket-key')
const protocol = req.getHeader('sec-websocket-protocol')
const extensions = req.getHeader('sec-websocket-extensions')
const ip = uwsGetIP(res, req)
const {sub: userId, iat} = authToken
// ALL async calls must come after the message listener, or we'll skip out on messages (e.g. resub after server restart)
const isBlacklistedJWT = await checkBlacklistJWT(userId, iat)
if (isBlacklistedJWT) {
res.writeStatus('401').end(TrebuchetCloseReason.EXPIRED_SESSION)
return
}
res.upgrade({ip, authToken}, key, protocol, extensions, context)
const authToken = getQueryToken(req)
Copy link
Contributor

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

Copy link
Member Author

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!

res.upgrade({ip, authToken, protocol}, key, protocol, extensions, context)
}

export default handleUpgrade
39 changes: 7 additions & 32 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4980,10 +4980,10 @@
resolved "https://registry.yarnpkg.com/@mattkrick/sanitize-svg/-/sanitize-svg-0.4.0.tgz#388c29614cf72aa0dd9803c77c9c9d070bd3cd2d"
integrity sha512-TnPI97WVAxo8SQcPy8aV3OF9/2WjXB5/+pRNVudIWR7Bhi5ZjtR/ur162So08GkvsvB914AXCW2sxFh1x6KhHA==

"@mattkrick/trebuchet-client@^3.0.2":
version "3.0.2"
resolved "https://registry.yarnpkg.com/@mattkrick/trebuchet-client/-/trebuchet-client-3.0.2.tgz#336d687256fb9ac7bb407f656bf2f69f35f817e1"
integrity sha512-JLOx8gd+cGkDeHhdnns0oor2UNv5uRZ8A/Jfw3NhQcVqQe6R+x9xIHW29M2T78TDHUWqItTgntX+cdDLqVjVvg==
"@mattkrick/trebuchet-client@3.0.1":
version "3.0.1"
resolved "https://registry.yarnpkg.com/@mattkrick/trebuchet-client/-/trebuchet-client-3.0.1.tgz#3fc49f7858652a55dca92cb0c14c0313df931619"
integrity sha512-5uHCldCqmVntoyujTzRfmGtjld8k4JuBdFN0SvhvRFf83FPaNeoit6Mh8VgrcxxxKujKeYCQDMQH6LWwpsshgQ==
dependencies:
"@mattkrick/fast-rtc-peer" "^0.4.1"
eventemitter3 "^4.0.7"
Expand Down Expand Up @@ -20311,7 +20311,7 @@ string-similarity@^3.0.0:
resolved "https://registry.yarnpkg.com/string-similarity/-/string-similarity-3.0.0.tgz#07b0bc69fae200ad88ceef4983878d03793847c7"
integrity sha512-7kS7LyTp56OqOI2BDWQNVnLX/rCxIQn+/5M0op1WV6P8Xx6TZNdajpuqQdiJ7Xx+p1C5CsWMvdiBp9ApMhxzEQ==

"string-width-cjs@npm:string-width@^4.2.0":
"string-width-cjs@npm:string-width@^4.2.0", "string-width@^1.0.2 || 2 || 3 || 4", string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
version "4.2.3"
resolved "https://registry.yarnpkg.com/string-width/-/string-width-4.2.3.tgz#269c7117d27b05ad2e536830a8ec895ef9c6d010"
integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==
Expand All @@ -20329,15 +20329,6 @@ string-width@^1.0.1:
is-fullwidth-code-point "^1.0.0"
strip-ansi "^3.0.0"

"string-width@^1.0.2 || 2 || 3 || 4", string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
version "4.2.3"
resolved "https://registry.yarnpkg.com/string-width/-/string-width-4.2.3.tgz#269c7117d27b05ad2e536830a8ec895ef9c6d010"
integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==
dependencies:
emoji-regex "^8.0.0"
is-fullwidth-code-point "^3.0.0"
strip-ansi "^6.0.1"

string-width@^5.0.0:
version "5.1.0"
resolved "https://registry.yarnpkg.com/string-width/-/string-width-5.1.0.tgz#5ab00980cfb29f43e736b113a120a73a0fb569d3"
Expand Down Expand Up @@ -20414,7 +20405,7 @@ stringify-object@^3.3.0:
is-obj "^1.0.1"
is-regexp "^1.0.0"

"strip-ansi-cjs@npm:strip-ansi@^6.0.1":
"strip-ansi-cjs@npm:strip-ansi@^6.0.1", strip-ansi@^6.0.0, strip-ansi@^6.0.1:
version "6.0.1"
resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9"
integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
Expand All @@ -20428,13 +20419,6 @@ strip-ansi@^3.0.0, strip-ansi@^3.0.1:
dependencies:
ansi-regex "^2.0.0"

strip-ansi@^6.0.0, strip-ansi@^6.0.1:
version "6.0.1"
resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9"
integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
dependencies:
ansi-regex "^5.0.1"

strip-ansi@^7.0.1:
version "7.0.1"
resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-7.0.1.tgz#61740a08ce36b61e50e65653f07060d000975fb2"
Expand Down Expand Up @@ -22287,7 +22271,7 @@ workbox-window@6.5.4:
"@types/trusted-types" "^2.0.2"
workbox-core "6.5.4"

"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0":
"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0", wrap-ansi@^7.0.0:
version "7.0.0"
resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-7.0.0.tgz#67e145cff510a6a6984bdf1152911d69d2eb9e43"
integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==
Expand All @@ -22305,15 +22289,6 @@ wrap-ansi@^6.0.1, wrap-ansi@^6.2.0:
string-width "^4.1.0"
strip-ansi "^6.0.0"

wrap-ansi@^7.0.0:
version "7.0.0"
resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-7.0.0.tgz#67e145cff510a6a6984bdf1152911d69d2eb9e43"
integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==
dependencies:
ansi-styles "^4.0.0"
string-width "^4.1.0"
strip-ansi "^6.0.0"

wrap-ansi@^8.1.0:
version "8.1.0"
resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-8.1.0.tgz#56dc22368ee570face1b49819975d9b9a5ead214"
Expand Down
Loading