Paths

Table of Contentst

-
lib/flow-typed/
-
flow-typed/
-
web-crypto-common.js

[Flow202][web][skip-ci] [21/x] Avoid localforage.getItem with unclear type param for web encryption key
ClosedPublic
Actions

Authored by ashoat on Nov 26 2023, 11:11 PM.

Details

Reviewers

marcin
michal
kamil

Commits

rCOMMaf44ff07bbe8: [Flow202][web][skip-ci] [21/x] Avoid localforage.getItem with unclear type…

Summary

After rebasing my Flow upgrade stack, I noticed a bunch of new errors: https://gist.github.com/Ashoat/b83564168fda704b2ce9d8051d0f472e

I played around a bit and it seems like an incompatibility between CryptoKey and SubtleCrypto$JsonWebKey introduced in D9535 and D9661, here and here.

@marcin's proposed this solution, which addresses the issue by avoiding calling localforage.getItem in a situation where the type param is ambiguous.

NOTE: CI will fail on this diff. I considered the possibility of fixing Flow errors BEFORE upgrading Flow, but it wasn't possible... in some cases, the fixes to support the new version of Flow caused errors in the old version. I could have hidden these type errors with $FlowFixMe lines and then later revert those, but that seemed like too much busy work.

Depends on D9786

Test Plan

Confirm the Flow errors go away

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Nov 26 2023, 11:11 PM

Herald added a subscriber: tomek. · View Herald TranscriptNov 26 2023, 11:11 PM

Harbormaster completed remote builds in B24420: Diff 33623.Nov 26 2023, 11:11 PM

ashoat requested review of this revision.Nov 26 2023, 11:11 PM

@marcin – if you suspect my assessment is incorrect, would appreciate it if you could patch this diff locally (yarn arcpatch D9984) and see if you can come up with an alternative way to get the Flow errors in web to go away

After examining CryptoKey and SubtleCrypto$JsonWeKey types I came to a conclusion that they are indeed different types. The CryptoKey type is meant to represent in-memory data structure that is not readable from JS. The SubtleCrypto$JsonWeKey type represents JS-readable form of the former that can be written directly to file, persisted to IndexedDB if the browser does not implement structured clone algorithm. Looks like flow got more restrictive in newer version so we must be more specific in our code. This patch fixes errors you linked without changing CryptoKey typing: https://gist.github.com/marcinwasowicz/3f1478ede2702b8e96d168b5d8a52b2c

This revision now requires changes to proceed.Nov 27 2023, 3:54 AM

Thanks @marcin. I rewrote your code to avoid your inline await (which breaks out conventions), and reversed the condition to reduce indentation. Update coming soon

@marcin's approach (with some changes)

Harbormaster completed remote builds in B24529: Diff 33747.Nov 27 2023, 9:03 AM

ashoat retitled this revision from [Flow202][web][skip-ci] [21/x] Update CryptoKey to be an alias of SubtleCrypto$JsonWebKey to [Flow202][web][skip-ci] [21/x] Avoid localforage.getItem with unclear type param for web encryption key.Nov 27 2023, 9:04 AM

ashoat edited the summary of this revision. (Show Details)

marcin accepted this revision.Nov 27 2023, 9:06 AM

This revision is now accepted and ready to land.Nov 27 2023, 9:06 AM

ashoat added a child revision: D10063: [Flow202][web][skip-ci] [22/x] Add updatesCurrentAsOf to PersistedKeyserverInfo.Nov 27 2023, 2:09 PM

ashoat removed a child revision: D9849: [Flow202][keyserver][skip-ci] [1/x] Fix Flow issues with conditional changeRole passed to promiseAll.Nov 27 2023, 2:10 PM

Closed by commit rCOMMaf44ff07bbe8: [Flow202][web][skip-ci] [21/x] Avoid localforage.getItem with unclear type… (authored by ashoat). · Explain WhyNov 27 2023, 3:26 PM

This revision was automatically updated to reflect the committed changes.

ashoat added a commit: rCOMMaf44ff07bbe8: [Flow202][web][skip-ci] [21/x] Avoid localforage.getItem with unclear type….

ashoat mentioned this in D10065: [Flow202] Fix rebasing mistakes in D9984.Nov 28 2023, 6:35 AM

ashoat mentioned this in rCOMMf4e61ed90d7f: [Flow202] Fix rebasing mistakes in D9984.Nov 28 2023, 7:04 AM

Revision Contents
Changeset List

Path

Size

lib/

flow-typed/

web-crypto-common.js

11 lines

Diff 33842

View Options

lib/flow-typed/web-crypto-common.js

// @flow		// @flow

import olm from '@commapp/olm';		import olm from '@commapp/olm';
import type {		import type {
Account as OlmAccount,		Account as OlmAccount,
Utility as OlmUtility,		Utility as OlmUtility,
		Session as OlmSession,
} from '@commapp/olm';		} from '@commapp/olm';
import uuid from 'uuid';		import uuid from 'uuid';

type PickledOlmAccount = {		type PickledOlmAccount = {
+picklingKey: string,		+picklingKey: string,
+pickledAccount: string,		+pickledAccount: string,
};		};

Show All 22 Lines	): Promise<OlmAccount> {

account.unpickle(		account.unpickle(
pickledOlmAccount.picklingKey,		pickledOlmAccount.picklingKey,
pickledOlmAccount.pickledAccount,		pickledOlmAccount.pickledAccount,
);		);
return account;		return account;
}		}

		async function createPickledOlmSession(
		account: OlmAccount,
		accountPicklingKey: string,
		initialEncryptedMessage: string,
		): Promise<string> {
		await olm.init();
		const session = new olm.Session();
		session.create_inbound(account, initialEncryptedMessage);
		account.remove_one_time_keys(session);
		session.decrypt(0, initialEncryptedMessage);
		ashoatUnsubmitted Not Done Inline Actions Let's avoid passing constants as number literals like this. Instead, please define something like eg. `threadTypes`, so that this code is more "readable" at the callsite (eg. we would see something like `olmMessageTypes.TEXT` or something) ashoat: Let's avoid passing constants as number literals like this. Instead, please define something…
		return session.pickle(accountPicklingKey);
		}

		async function unpickleOlmSession(
		pickledSession: string,
		picklingKey: string,
		): Promise<OlmSession> {
		await olm.init();
		const session = new olm.Session();
		session.unpickle(picklingKey, pickledSession);
		return session;
		}

let cachedOLMUtility: OlmUtility;		let cachedOLMUtility: OlmUtility;
function getOlmUtility(): OlmUtility {		function getOlmUtility(): OlmUtility {
if (cachedOLMUtility) {		if (cachedOLMUtility) {
return cachedOLMUtility;		return cachedOLMUtility;
}		}
cachedOLMUtility = new olm.Utility();		cachedOLMUtility = new olm.Utility();
return cachedOLMUtility;		return cachedOLMUtility;
}		}
Show All 26 Lines	if (
currentDate - lastPrekeyPublishDate >= maxOldPrekeyAge		currentDate - lastPrekeyPublishDate >= maxOldPrekeyAge
) {		) {
account.forget_old_prekey();		account.forget_old_prekey();
}		}
}		}

export {		export {
createPickledOlmAccount,		createPickledOlmAccount,
		createPickledOlmSession,
getOlmUtility,		getOlmUtility,
unpickleOlmAccount,		unpickleOlmAccount,
		unpickleOlmSession,
validateAccountPrekey,		validateAccountPrekey,
};		};

[Flow202][web][skip-ci] [21/x] Avoid localforage.getItem with unclear type param for web encryption keyClosedPublicActions