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 @@ -12,8 +12,6 @@ import { type ServerSessionChange, cookieLifetime, - cookieSources, - type CookieSource, cookieTypes, sessionIdentifierTypes, type SessionIdentifierType, @@ -56,29 +54,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(':'); @@ -86,7 +81,6 @@ return { type: 'nonexistant', cookieName: cookieTypes.USER, - cookieSource, sessionParameterInfo, }; } @@ -104,7 +98,6 @@ return { type: 'nonexistant', cookieName: cookieTypes.USER, - cookieSource, sessionParameterInfo, }; } @@ -138,7 +131,6 @@ type: 'invalidated', cookieName: cookieTypes.USER, cookieID, - cookieSource, sessionParameterInfo, platformDetails, deviceToken, @@ -152,7 +144,6 @@ platformDetails, deviceToken, userID, - cookieSource, cookieID, cookiePassword, cookieHash, @@ -168,7 +159,6 @@ async function fetchAnonymousViewer( cookie: string, - cookieSource: CookieSource, sessionParameterInfo: SessionParameterInfo, ): Promise { const [cookieID, cookiePassword] = cookie.split(':'); @@ -176,7 +166,6 @@ return { type: 'nonexistant', cookieName: cookieTypes.ANONYMOUS, - cookieSource, sessionParameterInfo, }; } @@ -194,7 +183,6 @@ return { type: 'nonexistant', cookieName: cookieTypes.ANONYMOUS, - cookieSource, sessionParameterInfo, }; } @@ -228,7 +216,6 @@ type: 'invalidated', cookieName: cookieTypes.ANONYMOUS, cookieID, - cookieSource, sessionParameterInfo, platformDetails, deviceToken, @@ -240,7 +227,6 @@ id: cookieID, platformDetails, deviceToken, - cookieSource, cookieID, cookiePassword, cookieHash, @@ -294,7 +280,6 @@ return { type: 'nonexistant', cookieName: null, - cookieSource: null, sessionParameterInfo, }; } @@ -303,7 +288,6 @@ return { type: 'nonexistant', cookieName: null, - cookieSource: cookieSources.BODY, sessionParameterInfo, }; } @@ -311,28 +295,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, }; } @@ -386,7 +360,7 @@ async function fetchViewerForSocket( req: $Request, clientMessage: InitialClientSocketMessage, -): Promise { +): Promise { assertSecureRequest(req); const { sessionIdentification } = clientMessage.payload; const { sessionID } = sessionIdentification; @@ -410,30 +384,18 @@ } 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, - }); - } + 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; - } - return createViewerForInvalidFetchViewerResult(result, anonymousViewerData); } @@ -463,17 +425,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, @@ -519,9 +472,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; } @@ -537,10 +488,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 { @@ -598,10 +549,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,9 +73,9 @@ import { fetchViewerForSocket, updateCookie, - createNewAnonymousCookie, isCookieMissingSignedIdentityKeysBlob, isCookieMissingOlmNotificationsSession, + createNewAnonymousCookie, } from '../session/cookies.js'; import { Viewer } from '../session/viewer.js'; import { serverStateSyncSpecs } from '../shared/state-sync/state-sync-specs.js'; @@ -209,12 +208,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,24 +277,20 @@ 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: { id: this.viewer.cookieID, anonymous: true, }, - }; - } + }, + }; + await this.sendMessage(authErrorMessage); this.ws.close(4100, error.message); return; @@ -310,35 +299,31 @@ invariant(viewer, 'should be set'); const promises = {}; promises.deleteCookie = deleteCookie(viewer.cookieID); - if (viewer.cookieSource !== cookieSources.BODY) { - promises.anonymousViewerData = createNewAnonymousCookie({ - platformDetails: error.platformDetails, - deviceToken: viewer.deviceToken, - }); - } + promises.anonymousViewerData = createNewAnonymousCookie({ + platformDetails: error.platformDetails, + deviceToken: viewer.deviceToken, + }); const { anonymousViewerData } = await promiseAll(promises); + // 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: { id: anonViewer.cookieID, 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 @@ -487,8 +487,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 @@ -333,14 +333,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 =