Page MenuHomePhabricator
Files F3547916

D9500.id32034.diff
No OneTemporary
Actions

D9500.id32034.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
@@ -13,8 +13,6 @@
import {
type ServerSessionChange,
cookieLifetime,
- cookieSources,
- type CookieSource,
cookieTypes,
sessionIdentifierTypes,
type SessionIdentifierType,
@@ -59,14 +57,12 @@
| {
type: 'nonexistant',
cookieName: ?string,
- cookieSource: ?CookieSource,
sessionParameterInfo: SessionParameterInfo,
}
| {
type: 'invalidated',
cookieName: string,
cookieID: string,
- cookieSource: CookieSource,
sessionParameterInfo: SessionParameterInfo,
platformDetails: ?PlatformDetails,
deviceToken: ?string,
@@ -74,7 +70,6 @@
async function fetchUserViewer(
cookie: string,
- cookieSource: CookieSource,
sessionParameterInfo: SessionParameterInfo,
): Promise<FetchViewerResult> {
const [cookieID, cookiePassword] = cookie.split(':');
@@ -82,7 +77,6 @@
return {
type: 'nonexistant',
cookieName: cookieTypes.USER,
- cookieSource,
sessionParameterInfo,
};
}
@@ -100,7 +94,6 @@
return {
type: 'nonexistant',
cookieName: cookieTypes.USER,
- cookieSource,
sessionParameterInfo,
};
}
@@ -133,7 +126,6 @@
type: 'invalidated',
cookieName: cookieTypes.USER,
cookieID,
- cookieSource,
sessionParameterInfo,
platformDetails,
deviceToken,
@@ -147,7 +139,6 @@
platformDetails,
deviceToken,
userID,
- cookieSource,
cookieID,
cookiePassword,
sessionIdentifierType: sessionParameterInfo.sessionIdentifierType,
@@ -162,7 +153,6 @@
async function fetchAnonymousViewer(
cookie: string,
- cookieSource: CookieSource,
sessionParameterInfo: SessionParameterInfo,
): Promise<FetchViewerResult> {
const [cookieID, cookiePassword] = cookie.split(':');
@@ -170,7 +160,6 @@
return {
type: 'nonexistant',
cookieName: cookieTypes.ANONYMOUS,
- cookieSource,
sessionParameterInfo,
};
}
@@ -188,7 +177,6 @@
return {
type: 'nonexistant',
cookieName: cookieTypes.ANONYMOUS,
- cookieSource,
sessionParameterInfo,
};
}
@@ -221,7 +209,6 @@
type: 'invalidated',
cookieName: cookieTypes.ANONYMOUS,
cookieID,
- cookieSource,
sessionParameterInfo,
platformDetails,
deviceToken,
@@ -233,7 +220,6 @@
id: cookieID,
platformDetails,
deviceToken,
- cookieSource,
cookieID,
cookiePassword,
sessionIdentifierType: sessionParameterInfo.sessionIdentifierType,
@@ -286,7 +272,6 @@
return {
type: 'nonexistant',
cookieName: null,
- cookieSource: null,
sessionParameterInfo,
};
}
@@ -295,7 +280,6 @@
return {
type: 'nonexistant',
cookieName: null,
- cookieSource: cookieSources.BODY,
sessionParameterInfo,
};
}
@@ -303,28 +287,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,
};
}
@@ -378,7 +352,7 @@
async function fetchViewerForSocket(
req: $Request,
clientMessage: InitialClientSocketMessage,
-): Promise<?Viewer> {
+): Promise<Viewer> {
assertSecureRequest(req);
const { sessionIdentification } = clientMessage.payload;
const { sessionID } = sessionIdentification;
@@ -402,30 +376,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);
}
@@ -455,17 +417,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,
@@ -511,9 +464,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;
}
@@ -529,10 +480,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> {
@@ -589,10 +540,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,
+cookieInsertedThisRequest?: boolean,
+sessionIdentifierType?: SessionIdentifierType,
@@ -38,7 +36,6 @@
+id: string,
+platformDetails: ?PlatformDetails,
+deviceToken: ?string,
- +cookieSource?: CookieSource,
+cookieID: string,
+cookiePassword: ?string,
+cookieInsertedThisRequest?: boolean,
@@ -80,14 +77,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
@@ -182,15 +171,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,7 +73,6 @@
import {
fetchViewerForSocket,
extendCookieLifespan,
- createNewAnonymousCookie,
isCookieMissingSignedIdentityKeysBlob,
isCookieMissingOlmNotificationsSession,
} from '../session/cookies.js';
@@ -209,12 +207,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,61 +276,32 @@
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;
} 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({
- platformDetails: error.platformDetails,
- deviceToken: viewer.deviceToken,
- });
- }
- const { anonymousViewerData } = await promiseAll(promises);
+ await deleteCookie(viewer.cookieID);
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 = {
- 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<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
@@ -336,8 +336,6 @@
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,

File Metadata

Mime Type
text/plain
Expires
Fri, Dec 27, 3:44 PM (2 h, 55 m)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
2716046
Default Alt Text
D9500.id32034.diff (16 KB)

Event Timeline