Page MenuHomePhabricator
Files F3534933

D9500.id33739.diff
No OneTemporary
Actions

D9500.id33739.diff
View Options

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<FetchViewerResult> {
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<FetchViewerResult> {
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<?Viewer> {
+): Promise<Viewer> {
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<AnonymousViewerData> =
+ (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<AnonymousViewerData> {
@@ -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<AnonymousViewerData> =
+ 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<typeof cookieSources>;
-
// 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<AuthErrorServerSocketMessage> =

File Metadata

Mime Type
text/plain
Expires
Thu, Dec 26, 1:42 PM (7 h, 16 m)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
2707081
Default Alt Text
D9500.id33739.diff (18 KB)

Event Timeline