Page MenuHomePhabricator
Files F3353794

D9500.id32615.diff
No OneTemporary
Actions

D9500.id32615.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
@@ -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<FetchViewerResult> {
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<FetchViewerResult> {
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<?Viewer> {
+): Promise<Viewer> {
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<AnonymousViewerData> {
@@ -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<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
@@ -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<AuthErrorServerSocketMessage> =

File Metadata

Mime Type
text/plain
Expires
Sun, Nov 24, 11:26 AM (21 h, 3 m)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
2575176
Default Alt Text
D9500.id32615.diff (17 KB)

Event Timeline