From 39e12cef65d8cc35de18eda7d194f184cd38cd29 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 15 Jul 2023 16:56:32 -0400 Subject: [PATCH] chore: Use `httpOnly` authentication cookie (#5552) --- app/components/Authenticated.tsx | 10 +-- app/components/WebsocketProvider.tsx | 13 +--- app/hooks/useCurrentToken.ts | 8 --- .../Document/components/MultiplayerEditor.tsx | 7 +- app/stores/AuthStore.ts | 68 +++++++++++-------- app/utils/ApiClient.ts | 23 ++----- package.json | 1 + .../collaboration/AuthenticationExtension.ts | 2 +- server/models/User.ts | 18 ++++- server/routes/api/auth/auth.ts | 10 ++- server/routes/auth/index.ts | 1 - server/services/websockets.ts | 56 ++++++++------- server/utils/authentication.ts | 3 +- server/utils/jwt.ts | 7 +- server/utils/passport.ts | 2 - yarn.lock | 5 ++ 16 files changed, 114 insertions(+), 120 deletions(-) delete mode 100644 app/hooks/useCurrentToken.ts diff --git a/app/components/Authenticated.tsx b/app/components/Authenticated.tsx index 2f148ec9e8..f22f43bfa5 100644 --- a/app/components/Authenticated.tsx +++ b/app/components/Authenticated.tsx @@ -1,7 +1,6 @@ import { observer } from "mobx-react"; import * as React from "react"; import { useTranslation } from "react-i18next"; -import { Redirect } from "react-router-dom"; import LoadingIndicator from "~/components/LoadingIndicator"; import useStores from "~/hooks/useStores"; import { changeLanguage } from "~/utils/language"; @@ -22,17 +21,10 @@ const Authenticated = ({ children }: Props) => { }, [i18n, language]); if (auth.authenticated) { - const { user, team } = auth; - - if (!team || !user) { - return ; - } - return children; } - void auth.logout(true); - return ; + return ; }; export default observer(Authenticated); diff --git a/app/components/WebsocketProvider.tsx b/app/components/WebsocketProvider.tsx index abd606d6dc..cac3201653 100644 --- a/app/components/WebsocketProvider.tsx +++ b/app/components/WebsocketProvider.tsx @@ -70,6 +70,7 @@ class WebsocketProvider extends React.Component { transports: ["websocket"], reconnectionDelay: 1000, reconnectionDelayMax: 30000, + withCredentials: true, }); invariant(this.socket, "Socket should be defined"); @@ -89,18 +90,6 @@ class WebsocketProvider extends React.Component { fileOperations, notifications, } = this.props; - if (!auth.token) { - return; - } - - this.socket.on("connect", () => { - // immediately send current users token to the websocket backend where it - // is verified, if all goes well an 'authenticated' message will be - // received in response - this.socket?.emit("authentication", { - token: auth.token, - }); - }); // on reconnection, reset the transports option, as the Websocket // connection may have failed (caused by proxy, firewall, browser, ...) diff --git a/app/hooks/useCurrentToken.ts b/app/hooks/useCurrentToken.ts deleted file mode 100644 index b3f46c4610..0000000000 --- a/app/hooks/useCurrentToken.ts +++ /dev/null @@ -1,8 +0,0 @@ -import invariant from "invariant"; -import useStores from "./useStores"; - -export default function useCurrentToken() { - const { auth } = useStores(); - invariant(auth.token, "token is required"); - return auth.token; -} diff --git a/app/scenes/Document/components/MultiplayerEditor.tsx b/app/scenes/Document/components/MultiplayerEditor.tsx index 13993c1277..8cbd8e8ddd 100644 --- a/app/scenes/Document/components/MultiplayerEditor.tsx +++ b/app/scenes/Document/components/MultiplayerEditor.tsx @@ -8,7 +8,6 @@ import * as Y from "yjs"; import MultiplayerExtension from "@shared/editor/extensions/Multiplayer"; import Editor, { Props as EditorProps } from "~/components/Editor"; import env from "~/env"; -import useCurrentToken from "~/hooks/useCurrentToken"; import useCurrentUser from "~/hooks/useCurrentUser"; import useIdle from "~/hooks/useIdle"; import useIsMounted from "~/hooks/useIsMounted"; @@ -45,8 +44,7 @@ function MultiplayerEditor({ onSynced, ...props }: Props, ref: any) { const history = useHistory(); const { t } = useTranslation(); const currentUser = useCurrentUser(); - const { presence, ui } = useStores(); - const token = useCurrentToken(); + const { presence, auth, ui } = useStores(); const [showCursorNames, setShowCursorNames] = React.useState(false); const [remoteProvider, setRemoteProvider] = React.useState(null); @@ -54,6 +52,7 @@ function MultiplayerEditor({ onSynced, ...props }: Props, ref: any) { const [isRemoteSynced, setRemoteSynced] = React.useState(false); const [ydoc] = React.useState(() => new Y.Doc()); const { showToast } = useToasts(); + const token = auth.collaborationToken; const isIdle = useIdle(); const isVisible = usePageVisibility(); const isMounted = useIsMounted(); @@ -188,8 +187,8 @@ function MultiplayerEditor({ onSynced, ...props }: Props, ref: any) { documentId, ui, presence, - token, ydoc, + token, currentUser.id, isMounted, ]); diff --git a/app/stores/AuthStore.ts b/app/stores/AuthStore.ts index ac6467aa00..4581fa85ad 100644 --- a/app/stores/AuthStore.ts +++ b/app/stores/AuthStore.ts @@ -5,6 +5,7 @@ import { getCookie, setCookie, removeCookie } from "tiny-cookie"; import { CustomTheme, TeamPreferences, UserPreferences } from "@shared/types"; import Storage from "@shared/utils/Storage"; import { getCookieDomain, parseDomain } from "@shared/utils/domains"; +import { Hour } from "@shared/utils/time"; import RootStore from "~/stores/RootStore"; import Policy from "~/models/Policy"; import Team from "~/models/Team"; @@ -13,6 +14,7 @@ import env from "~/env"; import { client } from "~/utils/ApiClient"; import Desktop from "~/utils/Desktop"; import Logger from "~/utils/Logger"; +import history from "~/utils/history"; const AUTH_STORE = "AUTH_STORE"; const NO_REDIRECT_PATHS = ["/", "/create", "/home", "/logout"]; @@ -20,6 +22,7 @@ const NO_REDIRECT_PATHS = ["/", "/create", "/home", "/logout"]; type PersistedData = { user?: User; team?: Team; + collaborationToken?: string; availableTeams?: { id: string; name: string; @@ -45,12 +48,19 @@ export type Config = { }; export default class AuthStore { + /* The user that is currently signed in. */ @observable user?: User | null; + /* The team that the current user is signed into. */ @observable team?: Team | null; + /* A short-lived token to be used to authenticate with the collaboration server. */ + @observable + collaborationToken?: string | null; + + /* A list of teams that the current user has access to. */ @observable availableTeams?: { id: string; @@ -60,24 +70,27 @@ export default class AuthStore { isSignedIn: boolean; }[]; - @observable - token?: string | null; - + /* A list of cancan policies for the current user. */ @observable policies: Policy[] = []; + /* The authentication provider the user signed in with. */ @observable lastSignedIn?: string | null; + /* Whether the user is currently saving their profile or team settings. */ @observable isSaving = false; + /* Whether the user is currently suspended. */ @observable isSuspended = false; + /* The email address to contact if the user is suspended. */ @observable suspendedContactEmail?: string | null; + /* The auth configuration for the current domain. */ @observable config: Config | null | undefined; @@ -90,6 +103,9 @@ export default class AuthStore { this.rehydrate(data); + // Refresh the auth store every 12 hours that the window is open + setInterval(this.fetch, 12 * Hour); + // persists this entire store to localstorage whenever any keys are changed autorun(() => { Storage.set(AUTH_STORE, this.asJson); @@ -124,13 +140,10 @@ export default class AuthStore { rehydrate(data: PersistedData) { this.user = data.user ? new User(data.user, this) : undefined; this.team = data.team ? new Team(data.team, this) : undefined; - this.token = getCookie("accessToken"); + this.collaborationToken = data.collaborationToken; this.lastSignedIn = getCookie("lastSignedIn"); this.addPolicies(data.policies); - - if (this.token) { - setTimeout(() => this.fetch(), 0); - } + void this.fetch(); } addPolicies(policies?: Policy[]) { @@ -143,7 +156,7 @@ export default class AuthStore { @computed get authenticated(): boolean { - return !!this.token; + return !!this.user && !!this.team; } @computed @@ -151,6 +164,7 @@ export default class AuthStore { return { user: this.user, team: this.team, + collaborationToken: this.collaborationToken, availableTeams: this.availableTeams, policies: this.policies, }; @@ -176,6 +190,7 @@ export default class AuthStore { this.user = new User(user, this); this.team = new Team(team, this); this.availableTeams = res.data.availableTeams; + this.collaborationToken = res.data.collaborationToken; if (env.SENTRY_DSN) { Sentry.configureScope(function (scope) { @@ -232,11 +247,11 @@ export default class AuthStore { runInAction("AuthStore#updateUser", () => { this.user = null; this.team = null; + this.collaborationToken = null; this.availableTeams = this.availableTeams?.filter( (team) => team.id !== this.team?.id ); this.policies = []; - this.token = null; }); }; @@ -306,7 +321,12 @@ export default class AuthStore { }; @action - logout = async (savePath = false) => { + logout = async ( + /** Whether the current path should be saved and returned to after login */ + savePath = false, + /** Whether the auth token is already expired. */ + tokenIsExpired = false + ) => { // if this logout was forced from an authenticated route then // save the current path so we can go back there once signed in if (savePath) { @@ -317,19 +337,15 @@ export default class AuthStore { } } - // If there is no auth token stored there is nothing else to do - if (!this.token) { - return; + // invalidate authentication token on server and unset auth cookie + if (!tokenIsExpired) { + try { + await client.post(`/auth.delete`); + } catch (err) { + Logger.error("Failed to delete authentication", err); + } } - // invalidate authentication token on server - const promise = client.post(`/auth.delete`); - - // remove authentication token itself - removeCookie("accessToken", { - path: "/", - }); - // remove session record on apex cookie const team = this.team; @@ -344,17 +360,13 @@ export default class AuthStore { // clear all credentials from cache (and local storage via autorun) this.user = null; this.team = null; + this.collaborationToken = null; this.policies = []; - this.token = null; // Tell the host application we logged out, if any – allows window cleanup. void Desktop.bridge?.onLogout?.(); this.rootStore.logout(); - try { - await promise; - } catch (err) { - Logger.error("Failed to delete authentication", err); - } + history.replace("/"); }; } diff --git a/app/utils/ApiClient.ts b/app/utils/ApiClient.ts index 500252ce89..f3efc9cf4b 100644 --- a/app/utils/ApiClient.ts +++ b/app/utils/ApiClient.ts @@ -1,10 +1,8 @@ import retry from "fetch-retry"; -import invariant from "invariant"; import { trim } from "lodash"; import queryString from "query-string"; import EDITOR_VERSION from "@shared/editor/version"; import stores from "~/stores"; -import isCloudHosted from "~/utils/isCloudHosted"; import Logger from "./Logger"; import download from "./download"; import { @@ -95,14 +93,8 @@ class ApiClient { } const headers = new Headers(headerOptions); - - if (stores.auth.authenticated) { - invariant(stores.auth.token, "JWT token not set properly"); - headers.set("Authorization", `Bearer ${stores.auth.token}`); - } - - let response; const timeStart = window.performance.now(); + let response; try { response = await fetchWithRetry(urlToFetch, { @@ -110,15 +102,7 @@ class ApiClient { body, headers, redirect: "follow", - // For the hosted deployment we omit cookies on API requests as they are - // not needed for authentication this offers a performance increase. - // For self-hosted we include them to support a wide variety of - // authenticated proxies, e.g. Pomerium, Cloudflare Access etc. - credentials: options.credentials - ? options.credentials - : isCloudHosted - ? "omit" - : "same-origin", + credentials: "same-origin", cache: "no-cache", }); } catch (err) { @@ -147,7 +131,8 @@ class ApiClient { // Handle 401, log out user if (response.status === 401) { - await stores.auth.logout(); + const tokenIsExpired = true; + await stores.auth.logout(false, tokenIsExpired); return; } diff --git a/package.json b/package.json index 9dab50a048..d857dba9bb 100644 --- a/package.json +++ b/package.json @@ -85,6 +85,7 @@ "class-validator": "^0.14.0", "command-score": "^0.1.2", "compressorjs": "^1.2.1", + "cookie": "^0.5.0", "copy-to-clipboard": "^3.3.3", "core-js": "^3.30.2", "crypto-js": "^4.1.1", diff --git a/server/collaboration/AuthenticationExtension.ts b/server/collaboration/AuthenticationExtension.ts index cfe6be9a8b..b913eafe77 100644 --- a/server/collaboration/AuthenticationExtension.ts +++ b/server/collaboration/AuthenticationExtension.ts @@ -19,7 +19,7 @@ export default class AuthenticationExtension implements Extension { throw AuthenticationError("Authentication required"); } - const user = await getUserForJWT(token); + const user = await getUserForJWT(token, ["session", "collaboration"]); if (user.isSuspended) { throw AuthenticationError("Account suspended"); diff --git a/server/models/User.ts b/server/models/User.ts index f2b4b6bf5f..719cd03c35 100644 --- a/server/models/User.ts +++ b/server/models/User.ts @@ -1,5 +1,5 @@ import crypto from "crypto"; -import { addMinutes, subMinutes } from "date-fns"; +import { addHours, addMinutes, subMinutes } from "date-fns"; import JWT from "jsonwebtoken"; import { Context } from "koa"; import { Transaction, QueryTypes, SaveOptions, Op } from "sequelize"; @@ -453,6 +453,22 @@ class User extends ParanoidModel { this.jwtSecret ); + /** + * Returns a session token that is used to make collaboration requests and is + * stored in the client memory. + * + * @returns The session token + */ + getCollaborationToken = () => + JWT.sign( + { + id: this.id, + expiresAt: addHours(new Date(), 24).toISOString(), + type: "collaboration", + }, + this.jwtSecret + ); + /** * Returns a temporary token that is only used for transferring a session * between subdomains or domains. It has a short expiry and can only be used diff --git a/server/routes/api/auth/auth.ts b/server/routes/api/auth/auth.ts index 4a8b1c3c3e..4244d3b802 100644 --- a/server/routes/api/auth/auth.ts +++ b/server/routes/api/auth/auth.ts @@ -1,8 +1,8 @@ -import { subHours } from "date-fns"; +import { subHours, subMinutes } from "date-fns"; import Router from "koa-router"; import { uniqBy } from "lodash"; import { TeamPreference } from "@shared/types"; -import { parseDomain } from "@shared/utils/domains"; +import { getCookieDomain, parseDomain } from "@shared/utils/domains"; import env from "@server/env"; import auth from "@server/middlewares/authentication"; import { transaction } from "@server/middlewares/transaction"; @@ -139,6 +139,7 @@ router.post("auth.info", auth(), async (ctx: APIContext) => { includeDetails: true, }), team: presentTeam(team), + collaborationToken: user.getCollaborationToken(), availableTeams: uniqBy([...signedInTeams, ...availableTeams], "id").map( (team) => presentAvailableTeam( @@ -176,6 +177,11 @@ router.post( } ); + ctx.cookies.set("accessToken", "", { + expires: subMinutes(new Date(), 1), + domain: getCookieDomain(ctx.hostname), + }); + ctx.body = { success: true, }; diff --git a/server/routes/auth/index.ts b/server/routes/auth/index.ts index 4e3a54ca2b..dcd903dda6 100644 --- a/server/routes/auth/index.ts +++ b/server/routes/auth/index.ts @@ -32,7 +32,6 @@ router.get("/redirect", auth(), async (ctx: APIContext) => { await user.updateActiveAt(ctx, true); ctx.cookies.set("accessToken", jwtToken, { - httpOnly: false, sameSite: "lax", expires: addMonths(new Date(), 3), }); diff --git a/server/services/websockets.ts b/server/services/websockets.ts index e47d0a2cab..6d627a71ef 100644 --- a/server/services/websockets.ts +++ b/server/services/websockets.ts @@ -1,6 +1,6 @@ import http, { IncomingMessage } from "http"; import { Duplex } from "stream"; -import invariant from "invariant"; +import cookie from "cookie"; import Koa from "koa"; import IO from "socket.io"; import { createAdapter } from "socket.io-redis"; @@ -56,8 +56,7 @@ export default function init( server.on( "upgrade", function (req: IncomingMessage, socket: Duplex, head: Buffer) { - if (req.url?.startsWith(path)) { - invariant(ioHandleUpgrade, "Existing upgrade handler must exist"); + if (req.url?.startsWith(path) && ioHandleUpgrade) { ioHandleUpgrade(req, socket, head); return; } @@ -92,29 +91,13 @@ export default function init( } }); - io.on("connection", (socket: SocketWithAuth) => { + io.on("connection", async (socket: SocketWithAuth) => { Metrics.increment("websockets.connected"); Metrics.gaugePerInstance("websockets.count", io.engine.clientsCount); - socket.on("authentication", async function (data) { - try { - await authenticate(socket, data); - Logger.debug("websockets", `Authenticated socket ${socket.id}`); - - socket.emit("authenticated", true); - void authenticated(io, socket); - } catch (err) { - Logger.error(`Authentication error socket ${socket.id}`, err); - socket.emit("unauthorized", { message: err.message }, function () { - socket.disconnect(); - }); - } - }); - socket.on("disconnect", async () => { Metrics.increment("websockets.disconnected"); Metrics.gaugePerInstance("websockets.count", io.engine.clientsCount); - await Redis.defaultClient.hdel(socket.id, "userId"); }); setTimeout(function () { @@ -126,6 +109,19 @@ export default function init( socket.disconnect("unauthorized"); } }, 1000); + + try { + await authenticate(socket); + Logger.debug("websockets", `Authenticated socket ${socket.id}`); + + socket.emit("authenticated", true); + void authenticated(io, socket); + } catch (err) { + Logger.error(`Authentication error socket ${socket.id}`, err); + socket.emit("unauthorized", { message: err.message }, function () { + socket.disconnect(); + }); + } }); // Handle events from event queue that should be sent to the clients down ws @@ -204,15 +200,17 @@ async function authenticated(io: IO.Server, socket: SocketWithAuth) { * Authenticate the socket with the given token, attach the user model for the * duration of the session. */ -async function authenticate(socket: SocketWithAuth, data: { token: string }) { - const { token } = data; +async function authenticate(socket: SocketWithAuth) { + const cookies = socket.request.headers.cookie + ? cookie.parse(socket.request.headers.cookie) + : {}; + const { accessToken } = cookies; - const user = await getUserForJWT(token); + if (!accessToken) { + throw new Error("No access token"); + } + + const user = await getUserForJWT(accessToken); socket.client.user = user; - - // store the mapping between socket id and user id in redis so that it is - // accessible across multiple websocket servers. Lasts 24 hours, if they have - // a websocket connection that lasts this long then well done. - await Redis.defaultClient.hset(socket.id, "userId", user.id); - await Redis.defaultClient.expire(socket.id, 3600 * 24); + return user; } diff --git a/server/utils/authentication.ts b/server/utils/authentication.ts index f8d6a49f6d..ad3b4a0a76 100644 --- a/server/utils/authentication.ts +++ b/server/utils/authentication.ts @@ -118,9 +118,8 @@ export async function signIn( ); } } else { - ctx.cookies.set("accessToken", user.getJwtToken(), { + ctx.cookies.set("accessToken", user.getJwtToken(expires), { sameSite: "lax", - httpOnly: false, expires, }); diff --git a/server/utils/jwt.ts b/server/utils/jwt.ts index 7e4293616b..e3ccdb949b 100644 --- a/server/utils/jwt.ts +++ b/server/utils/jwt.ts @@ -20,10 +20,13 @@ function getJWTPayload(token: string) { } } -export async function getUserForJWT(token: string): Promise { +export async function getUserForJWT( + token: string, + allowedTypes = ["session", "transfer"] +): Promise { const payload = getJWTPayload(token); - if (payload.type === "email-signin") { + if (!allowedTypes.includes(payload.type)) { throw AuthenticationError("Invalid token"); } diff --git a/server/utils/passport.ts b/server/utils/passport.ts index a1b2ffd275..dbf0c1593d 100644 --- a/server/utils/passport.ts +++ b/server/utils/passport.ts @@ -27,7 +27,6 @@ export class StateStore { const state = buildState(host, token, client); ctx.cookies.set(this.key, state, { - httpOnly: false, expires: addMinutes(new Date(), 10), domain: getCookieDomain(ctx.hostname), }); @@ -54,7 +53,6 @@ export class StateStore { // Destroy the one-time pad token and ensure it matches ctx.cookies.set(this.key, "", { - httpOnly: false, expires: subMinutes(new Date(), 1), domain: getCookieDomain(ctx.hostname), }); diff --git a/yarn.lock b/yarn.lock index f6fe616e7f..2deff08236 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4981,6 +4981,11 @@ cookie@^0.4.1, cookie@~0.4.1: resolved "https://registry.yarnpkg.com/cookie/-/cookie-0.4.1.tgz#afd713fe26ebd21ba95ceb61f9a8116e50a537d1" integrity sha512-ZwrFkGJxUR3EIoXtO+yVE69Eb7KlixbaeAWfBQB9vVsNn/o+Yw69gBWSSDK825hQNdN+wF8zELf3dFNl/kxkUA== +cookie@^0.5.0: + version "0.5.0" + resolved "https://registry.yarnpkg.com/cookie/-/cookie-0.5.0.tgz#d1f5d71adec6558c58f389987c366aa47e994f8b" + integrity sha512-YZ3GUyn/o8gfKJlnlX7g7xq4gyO6OSuhGPKaaGssGB2qgDUS0gPgtTvoyZLTt9Ab6dC4hfc9dV5arkvc/OCmrw== + cookiejar@^2.1.4: version "2.1.4" resolved "https://registry.yarnpkg.com/cookiejar/-/cookiejar-2.1.4.tgz#ee669c1fea2cf42dc31585469d193fef0d65771b"