diff --git a/keyserver/src/session/cookies.js b/keyserver/src/session/cookies.js --- a/keyserver/src/session/cookies.js +++ b/keyserver/src/session/cookies.js @@ -14,8 +14,6 @@ import { type ServerSessionChange, cookieLifetime, - cookieSources, - type CookieSource, cookieTypes, sessionIdentifierTypes, type SessionIdentifierType, @@ -25,7 +23,6 @@ import type { UserInfo } from 'lib/types/user-types.js'; import { isDev } from 'lib/utils/dev-utils.js'; import { values } from 'lib/utils/objects.js'; -import { promiseAll } from 'lib/utils/promises.js'; import { isBcryptHash, @@ -59,29 +56,26 @@ }; type FetchViewerResult = - | { type: 'valid', viewer: Viewer } + | { +type: 'valid', +viewer: Viewer } | InvalidFetchViewerResult; type InvalidFetchViewerResult = | { - type: 'nonexistant', - cookieName: ?string, - cookieSource: ?CookieSource, - sessionParameterInfo: SessionParameterInfo, + +type: 'nonexistant', + +cookieName: ?string, + +sessionParameterInfo: SessionParameterInfo, } | { - type: 'invalidated', - cookieName: string, - cookieID: string, - cookieSource: CookieSource, - sessionParameterInfo: SessionParameterInfo, - platformDetails: ?PlatformDetails, - deviceToken: ?string, + +type: 'invalidated', + +cookieName: string, + +cookieID: string, + +sessionParameterInfo: SessionParameterInfo, + +platformDetails: ?PlatformDetails, + +deviceToken: ?string, }; async function fetchUserViewer( cookie: string, - cookieSource: CookieSource, sessionParameterInfo: SessionParameterInfo, ): Promise { const [cookieID, cookiePassword] = cookie.split(':'); @@ -89,7 +83,6 @@ return { type: 'nonexistant', cookieName: cookieTypes.USER, - cookieSource, sessionParameterInfo, }; } @@ -107,7 +100,6 @@ return { type: 'nonexistant', cookieName: cookieTypes.USER, - cookieSource, sessionParameterInfo, }; } @@ -141,7 +133,6 @@ type: 'invalidated', cookieName: cookieTypes.USER, cookieID, - cookieSource, sessionParameterInfo, platformDetails, deviceToken, @@ -155,7 +146,6 @@ platformDetails, deviceToken, userID, - cookieSource, cookieID, cookiePassword, cookieHash, @@ -171,7 +161,6 @@ async function fetchAnonymousViewer( cookie: string, - cookieSource: CookieSource, sessionParameterInfo: SessionParameterInfo, ): Promise { const [cookieID, cookiePassword] = cookie.split(':'); @@ -179,7 +168,6 @@ return { type: 'nonexistant', cookieName: cookieTypes.ANONYMOUS, - cookieSource, sessionParameterInfo, }; } @@ -197,7 +185,6 @@ return { type: 'nonexistant', cookieName: cookieTypes.ANONYMOUS, - cookieSource, sessionParameterInfo, }; } @@ -231,7 +218,6 @@ type: 'invalidated', cookieName: cookieTypes.ANONYMOUS, cookieID, - cookieSource, sessionParameterInfo, platformDetails, deviceToken, @@ -243,7 +229,6 @@ id: cookieID, platformDetails, deviceToken, - cookieSource, cookieID, cookiePassword, cookieHash, @@ -297,7 +282,6 @@ return { type: 'nonexistant', cookieName: null, - cookieSource: null, sessionParameterInfo, }; } @@ -306,7 +290,6 @@ return { type: 'nonexistant', cookieName: null, - cookieSource: cookieSources.BODY, sessionParameterInfo, }; } @@ -314,28 +297,18 @@ return { type: 'nonexistant', cookieName: null, - cookieSource: null, sessionParameterInfo, }; } const [type, cookie] = cookiePair.split('='); if (type === cookieTypes.USER && cookie) { - return await fetchUserViewer( - cookie, - cookieSources.BODY, - sessionParameterInfo, - ); + return await fetchUserViewer(cookie, sessionParameterInfo); } else if (type === cookieTypes.ANONYMOUS && cookie) { - return await fetchAnonymousViewer( - cookie, - cookieSources.BODY, - sessionParameterInfo, - ); + return await fetchAnonymousViewer(cookie, sessionParameterInfo); } return { type: 'nonexistant', cookieName: null, - cookieSource: null, sessionParameterInfo, }; } @@ -389,7 +362,7 @@ async function fetchViewerForSocket( req: $Request, clientMessage: InitialClientSocketMessage, -): Promise { +): Promise { assertSecureRequest(req); const { sessionIdentification } = clientMessage.payload; const { sessionID } = sessionIdentification; @@ -412,30 +385,26 @@ return result.viewer; } - const promises = {}; - if (result.cookieSource === cookieSources.BODY) { - // We initialize a socket's Viewer after the WebSocket handshake, since to - // properly initialize the Viewer we need a bunch of data, but that data - // can't be sent until after the handshake. Consequently, by the time we - // know that a cookie may be invalid, we are no longer communicating via - // HTTP, and have no way to set a new cookie for HEADER (web) clients. - const platformDetails = - result.type === 'invalidated' ? result.platformDetails : null; - const deviceToken = - result.type === 'invalidated' ? result.deviceToken : null; - promises.anonymousViewerData = createNewAnonymousCookie({ - platformDetails, - deviceToken, - }); - } - if (result.type === 'invalidated') { - promises.deleteCookie = deleteCookie(result.cookieID); - } - const { anonymousViewerData } = await promiseAll(promises); - - if (!anonymousViewerData) { - return null; - } + const anonymousViewerDataPromise: Promise = + (async () => { + const platformDetails = + result.type === 'invalidated' ? result.platformDetails : null; + const deviceToken = + result.type === 'invalidated' ? result.deviceToken : null; + return await createNewAnonymousCookie({ + platformDetails, + deviceToken, + }); + })(); + const deleteCookiePromise = (async () => { + if (result.type === 'invalidated') { + await deleteCookie(result.cookieID); + } + })(); + const [anonymousViewerData] = await Promise.all([ + anonymousViewerDataPromise, + deleteCookiePromise, + ]); return createViewerForInvalidFetchViewerResult(result, anonymousViewerData); } @@ -466,17 +435,8 @@ result: InvalidFetchViewerResult, anonymousViewerData: AnonymousViewerData, ): Viewer { - // If a null cookie was specified in the request body, result.cookieSource - // will still be BODY here. The only way it would be null or undefined here - // is if there was no cookie specified in either the body or the header, in - // which case we default to returning the new cookie in the response header. - const cookieSource = - result.cookieSource !== null && result.cookieSource !== undefined - ? result.cookieSource - : cookieSources.HEADER; const viewer = new Viewer({ ...anonymousViewerData, - cookieSource, sessionIdentifierType: result.sessionParameterInfo.sessionIdentifierType, isSocket: result.sessionParameterInfo.isSocket, ipAddress: result.sessionParameterInfo.ipAddress, @@ -521,9 +481,7 @@ userInfos: (values(userInfos).map(a => a): UserInfo[]), }: ServerSessionChange); } - if (viewer.cookieSource === cookieSources.BODY) { - sessionChange.cookie = viewer.cookiePairString; - } + sessionChange.cookie = viewer.cookiePairString; if (viewer.sessionIdentifierType === sessionIdentifierTypes.BODY_SESSION_ID) { sessionChange.sessionID = viewer.sessionID ? viewer.sessionID : null; } @@ -539,10 +497,10 @@ // The result of this function should not be passed directly to the Viewer // constructor. Instead, it should be passed to viewer.setNewCookie. There are // several fields on AnonymousViewerData that are not set by this function: -// sessionIdentifierType, cookieSource, ipAddress, and userAgent. These -// parameters all depend on the initial request. If the result of this function -// is passed to the Viewer constructor directly, the resultant Viewer object -// will throw whenever anybody attempts to access the relevant properties. +// sessionIdentifierType, ipAddress, and userAgent. These parameters all depend +// on the initial request. If the result of this function is passed to the +// Viewer constructor directly, the resultant Viewer object will throw whenever +// anybody attempts to access the relevant properties. async function createNewAnonymousCookie( params: AnonymousCookieCreationParams, ): Promise { @@ -600,10 +558,10 @@ // The result of this function should never be passed directly to the Viewer // constructor. Instead, it should be passed to viewer.setNewCookie. There are // several fields on UserViewerData that are not set by this function: -// sessionID, sessionIdentifierType, cookieSource, and ipAddress. These -// parameters all depend on the initial request. If the result of this function -// is passed to the Viewer constructor directly, the resultant Viewer object -// will throw whenever anybody attempts to access the relevant properties. +// sessionID, sessionIdentifierType, and ipAddress. These parameters all depend +// on the initial request. If the result of this function is passed to the +// Viewer constructor directly, the resultant Viewer object will throw whenever +// anybody attempts to access the relevant properties. async function createNewUserCookie( userID: string, params: UserCookieCreationParams, diff --git a/keyserver/src/session/viewer.js b/keyserver/src/session/viewer.js --- a/keyserver/src/session/viewer.js +++ b/keyserver/src/session/viewer.js @@ -6,7 +6,6 @@ import type { Platform, PlatformDetails } from 'lib/types/device-types.js'; import type { CalendarQuery } from 'lib/types/entry-types.js'; import { - type CookieSource, type SessionIdentifierType, cookieTypes, type CookieType, @@ -21,7 +20,6 @@ +deviceToken: ?string, +userID: string, +cookieID: ?string, - +cookieSource?: CookieSource, +cookiePassword: ?string, +cookieHash: ?string, +cookieInsertedThisRequest?: boolean, @@ -39,7 +37,6 @@ +id: string, +platformDetails: ?PlatformDetails, +deviceToken: ?string, - +cookieSource?: CookieSource, +cookieID: string, +cookiePassword: ?string, +cookieHash: ?string, @@ -82,14 +79,6 @@ } setNewCookie(data: ViewerData) { - if (data.cookieSource === null || data.cookieSource === undefined) { - if (data.loggedIn) { - data = { ...data, cookieSource: this.cookieSource }; - } else { - // This is a separate condition because of Flow - data = { ...data, cookieSource: this.cookieSource }; - } - } if ( data.sessionIdentifierType === null || data.sessionIdentifierType === undefined @@ -184,15 +173,6 @@ return this.data.loggedIn; } - get cookieSource(): CookieSource { - const { cookieSource } = this.data; - invariant( - cookieSource !== null && cookieSource !== undefined, - 'Viewer.cookieSource should be set', - ); - return cookieSource; - } - get cookieID(): string { const { cookieID } = this.data; invariant( diff --git a/keyserver/src/socket/socket.js b/keyserver/src/socket/socket.js --- a/keyserver/src/socket/socket.js +++ b/keyserver/src/socket/socket.js @@ -22,7 +22,6 @@ import { redisMessageTypes, type RedisMessage } from 'lib/types/redis-types.js'; import { serverRequestTypes } from 'lib/types/request-types.js'; import { - cookieSources, sessionCheckFrequency, stateCheckInactivityActivationInterval, } from 'lib/types/session-types.js'; @@ -74,11 +73,12 @@ import { fetchViewerForSocket, updateCookie, - createNewAnonymousCookie, isCookieMissingSignedIdentityKeysBlob, isCookieMissingOlmNotificationsSession, + createNewAnonymousCookie, } from '../session/cookies.js'; import { Viewer } from '../session/viewer.js'; +import type { AnonymousViewerData } from '../session/viewer.js'; import { serverStateSyncSpecs } from '../shared/state-sync/state-sync-specs.js'; import { commitSessionUpdate } from '../updaters/session-updaters.js'; import { compressMessage } from '../utils/compress.js'; @@ -209,12 +209,6 @@ this.httpRequest, clientSocketMessage, ); - if (!this.viewer) { - // This indicates that the cookie was invalid, but the client is using - // cookieSources.HEADER and thus can't accept a new cookie over - // WebSockets. See comment under catch block for socket_deauthorized. - throw new ServerError('socket_deauthorized'); - } } const { viewer } = this; if (!viewer) { @@ -284,59 +278,56 @@ invariant(clientSocketMessage, 'should be set'); const responseTo = clientSocketMessage.id; if (error.message === 'socket_deauthorized') { + invariant(this.viewer, 'should be set'); const authErrorMessage: AuthErrorServerSocketMessage = { type: serverSocketMessageTypes.AUTH_ERROR, responseTo, message: error.message, - }; - if (this.viewer) { - // viewer should only be falsey for cookieSources.HEADER (web) - // clients. Usually if the cookie is invalid we construct a new - // anonymous Viewer with a new cookie, and then pass the cookie down - // in the error. But we can't pass HTTP cookies in WebSocket messages. - authErrorMessage.sessionChange = { + sessionChange: { cookie: this.viewer.cookiePairString, currentUserInfo: { anonymous: true, }, - }; - } + }, + }; + await this.sendMessage(authErrorMessage); this.ws.close(4100, error.message); return; } else if (error.message === 'client_version_unsupported') { const { viewer } = this; invariant(viewer, 'should be set'); - const promises = {}; - promises.deleteCookie = deleteCookie(viewer.cookieID); - if (viewer.cookieSource !== cookieSources.BODY) { - promises.anonymousViewerData = createNewAnonymousCookie({ + + const anonymousViewerDataPromise: Promise = + createNewAnonymousCookie({ platformDetails: error.platformDetails, deviceToken: viewer.deviceToken, }); - } - const { anonymousViewerData } = await promiseAll(promises); + const deleteCookiePromise = deleteCookie(viewer.cookieID); + const [anonymousViewerData] = await Promise.all([ + anonymousViewerDataPromise, + deleteCookiePromise, + ]); + + // It is normally not safe to pass the result of + // createNewAnonymousCookie to the Viewer constructor. That is because + // createNewAnonymousCookie leaves several fields of + // AnonymousViewerData unset, and consequently Viewer will throw when + // access is attempted. It is only safe here because we can guarantee + // that only cookiePairString and cookieID are accessed on anonViewer + // below. + const anonViewer = new Viewer(anonymousViewerData); const authErrorMessage: AuthErrorServerSocketMessage = { type: serverSocketMessageTypes.AUTH_ERROR, responseTo, message: error.message, - }; - if (anonymousViewerData) { - // It is normally not safe to pass the result of - // createNewAnonymousCookie to the Viewer constructor. That is because - // createNewAnonymousCookie leaves several fields of - // AnonymousViewerData unset, and consequently Viewer will throw when - // access is attempted. It is only safe here because we can guarantee - // that only cookiePairString and cookieID are accessed on anonViewer - // below. - const anonViewer = new Viewer(anonymousViewerData); - authErrorMessage.sessionChange = { + sessionChange: { cookie: anonViewer.cookiePairString, currentUserInfo: { anonymous: true, }, - }; - } + }, + }; await this.sendMessage(authErrorMessage); this.ws.close(4101, error.message); return; diff --git a/lib/socket/socket.react.js b/lib/socket/socket.react.js --- a/lib/socket/socket.react.js +++ b/lib/socket/socket.react.js @@ -493,8 +493,6 @@ ); if (!recoverySessionChange && sessionChange) { - // This should only happen in the cookieSources.BODY (native) case when - // the resolution attempt failed const { cookie: newerCookie, currentUserInfo } = sessionChange; this.props.dispatch({ type: setNewSessionActionType, diff --git a/lib/types/session-types.js b/lib/types/session-types.js --- a/lib/types/session-types.js +++ b/lib/types/session-types.js @@ -19,21 +19,6 @@ // How long the server debounces after activity before initiating a state check export const stateCheckInactivityActivationInterval = 3 * 1000; // in milliseconds -// On native, we specify the cookie directly in the request and response body. -// We do this because: -// (1) We don't have the same XSS risks as we do on web, so there is no need to -// prevent JavaScript from knowing the cookie password. -// (2) In the past the internal cookie logic on Android has been buggy. -// https://github.com/facebook/react-native/issues/12956 is an example -// issue. By specifying the cookie in the body we retain full control of how -// that data is passed, without necessitating any native modules like -// react-native-cookies. -export const cookieSources = Object.freeze({ - BODY: 0, - HEADER: 1, -}); -export type CookieSource = $Values; - // On native, we use the cookieID as a unique session identifier. This is // because there is no way to have two instances of an app running. On the other // hand, on web it is possible to have two sessions open using the same cookie, diff --git a/lib/types/socket-types.js b/lib/types/socket-types.js --- a/lib/types/socket-types.js +++ b/lib/types/socket-types.js @@ -335,14 +335,12 @@ }); export type AuthErrorServerSocketMessage = { - type: 3, - responseTo: number, - message: string, - // If unspecified, it is because the client is using cookieSources.HEADER, - // which means the server can't update the cookie from a socket message. - sessionChange?: { - cookie: string, - currentUserInfo: LoggedOutUserInfo, + +type: 3, + +responseTo: number, + +message: string, + +sessionChange: { + +cookie: string, + +currentUserInfo: LoggedOutUserInfo, }, }; export const authErrorServerSocketMessageValidator: TInterface =