From 3bcdd21b539dc46a70e5f13195d921bb0d41eee6 Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Mon, 15 Feb 2021 06:48:17 -0500 Subject: [PATCH 1/5] refactored dearmor and added tests #3407 --- .../js/common/core/crypto/pgp/pgp-armor.ts | 10 +++++ extension/js/common/core/stream.ts | 10 +++++ extension/js/common/core/types/openpgp.d.ts | 4 +- package-lock.json | 18 ++++++++ package.json | 12 ++--- test/source/buf.ts | 2 +- test/source/mock/wkd/wkd-endpoints.ts | 20 ++++----- test/source/tests/unit-node.ts | 44 ++++++++++++++++++- 8 files changed, 101 insertions(+), 19 deletions(-) create mode 100644 extension/js/common/core/stream.ts diff --git a/extension/js/common/core/crypto/pgp/pgp-armor.ts b/extension/js/common/core/crypto/pgp/pgp-armor.ts index 6d385829047..94ff3f27152 100644 --- a/extension/js/common/core/crypto/pgp/pgp-armor.ts +++ b/extension/js/common/core/crypto/pgp/pgp-armor.ts @@ -6,6 +6,7 @@ import { Buf } from '../../buf.js'; import { ReplaceableMsgBlockType } from '../../msg-block.js'; import { Str } from '../../common.js'; import { opgp } from './openpgpjs-custom.js'; +import { Stream } from '../../stream.js'; export type PreparedForDecrypt = { isArmored: boolean, isCleartext: true, message: OpenPGP.cleartext.CleartextMessage | OpenPGP.message.Message } | { isArmored: boolean, isCleartext: false, message: OpenPGP.message.Message }; @@ -94,4 +95,13 @@ export class PgpArmor { throw new Error('Message does not have armor headers'); } + public static dearmor = async (text: string): Promise<{ type: number, data: Uint8Array }> => { + const decoded = await opgp.armor.decode(text); + const data = await Stream.readToEnd(decoded.data); + return { type: decoded.type, data }; + } + + public static armor = (messagetype: OpenPGP.enums.armor, body: object, partindex?: number, parttotal?: number, customComment?: string): string => { + return opgp.armor.encode(messagetype, body, partindex, parttotal, customComment); + } } diff --git a/extension/js/common/core/stream.ts b/extension/js/common/core/stream.ts new file mode 100644 index 00000000000..2e9f425a78c --- /dev/null +++ b/extension/js/common/core/stream.ts @@ -0,0 +1,10 @@ +/* ©️ 2016 - present FlowCrypt a.s. Limitations apply. Contact human@flowcrypt.com */ + +export class Stream { + public static readToEnd = async (data: ReadableStream) => { + let buffer = new Uint8Array(); + const ws = new WritableStream({ write: chunk => { buffer = new Uint8Array([...buffer, ...chunk]); } }); + await data.pipeTo(ws); + return buffer; + } +} \ No newline at end of file diff --git a/extension/js/common/core/types/openpgp.d.ts b/extension/js/common/core/types/openpgp.d.ts index 71609e95e73..1d991d7960c 100644 --- a/extension/js/common/core/types/openpgp.d.ts +++ b/extension/js/common/core/types/openpgp.d.ts @@ -506,13 +506,13 @@ declare namespace OpenPGP { * @param partindex * @param parttotal */ - function encode(messagetype: enums.armor, body: object, partindex: number, parttotal: number): string; + function encode(messagetype: enums.armor, body: object, partindex?: number, parttotal?: number, customComment?: string): string; /** DeArmor an OpenPGP armored message; verify the checksum and return the encoded bytes * * @param text OpenPGP armored message */ - function decode(text: string): Promise<{ type: number, data: ReadableStream }>; + function decode(text: string): Promise<{ type: enums.armor, data: ReadableStream }>; } export namespace cleartext { diff --git a/package-lock.json b/package-lock.json index 1e5d042735e..53914b82d6a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -469,6 +469,15 @@ "integrity": "sha512-rYff6FI+ZTKAPkJUoyz7Udq3GaoDZnxYDEvdEdFZASiA7PoErltHezDishqQiSDWrGxvxmplH304jyzQmjp0AQ==", "dev": true }, + "@types/chai-as-promised": { + "version": "7.1.3", + "resolved": "https://registry.npmjs.org/@types/chai-as-promised/-/chai-as-promised-7.1.3.tgz", + "integrity": "sha512-FQnh1ohPXJELpKhzjuDkPLR2BZCAqed+a6xV4MI/T3XzHfd2FlarfUGUdZYgqYe8oxkYn0fchHEeHfHqdZ96sg==", + "dev": true, + "requires": { + "@types/chai": "*" + } + }, "@types/chrome": { "version": "0.0.129", "resolved": "https://registry.npmjs.org/@types/chrome/-/chrome-0.0.129.tgz", @@ -2018,6 +2027,15 @@ "type-detect": "^4.0.5" } }, + "chai-as-promised": { + "version": "7.1.1", + "resolved": "https://registry.npmjs.org/chai-as-promised/-/chai-as-promised-7.1.1.tgz", + "integrity": "sha512-azL6xMoi+uxu6z4rhWQ1jbdUhOMhis2PvscD/xjLqNMkv3BPPp2JyyuTHOrf9BOosGpNQ11v6BKv/g57RXbiaA==", + "dev": true, + "requires": { + "check-error": "^1.0.2" + } + }, "chalk": { "version": "2.4.2", "resolved": "https://registry.npmjs.org/chalk/-/chalk-2.4.2.tgz", diff --git a/package.json b/package.json index b34ab78c773..5cc595ff903 100644 --- a/package.json +++ b/package.json @@ -6,18 +6,20 @@ "graceful-fs": "4.1.13" }, "devDependencies": { - "@types/chrome": "0.0.129", "@types/chai": "4.2.15", + "@types/chai-as-promised": "^7.1.3", + "@types/chrome": "0.0.129", "@types/dompurify": "2.2.1", "@types/jquery": "3.5.5", "@types/mailparser": "3.0.1", - "@types/puppeteer": "5.4.3", "@types/mkdirp": "^1.0.1", - "@typescript-eslint/eslint-plugin": "^4.15.0", + "@types/puppeteer": "5.4.3", "@types/request": "2.48.5", - "ava": "3.15.0", + "@typescript-eslint/eslint-plugin": "^4.15.0", "@typescript-eslint/parser": "^4.15.0", + "ava": "3.15.0", "chai": "4.3.0", + "chai-as-promised": "^7.1.1", "del": "6.0.0", "eslint": "^7.19.0", "eslint-plugin-header": "^3.1.1", @@ -90,4 +92,4 @@ "url": "https://github.com/FlowCrypt/flowcrypt-browser/issues" }, "homepage": "https://flowcrypt.com" -} \ No newline at end of file +} diff --git a/test/source/buf.ts b/test/source/buf.ts index 055a768012b..f02c9bce0bf 100644 --- a/test/source/buf.ts +++ b/test/source/buf.ts @@ -28,7 +28,7 @@ const UTF8 = `გამარჯობა.\nこんにちは。\nЗдравств const UTF8_AS_BYTES = Buffer.from(UTF8); const UTF8_AS_RAW_STRING = Buffer.from(UTF8).toString('binary'); -const equals = (a: string | Uint8Array, b: string | Uint8Array) => { +export const equals = (a: string | Uint8Array, b: string | Uint8Array) => { expect(typeof a).to.equal(typeof b, `types dont match`); if (typeof a === 'string' && typeof b === 'string') { expect(a).to.equal(b, 'string result mismatch'); diff --git a/test/source/mock/wkd/wkd-endpoints.ts b/test/source/mock/wkd/wkd-endpoints.ts index 6e7cc354bc1..7884be09b8b 100644 --- a/test/source/mock/wkd/wkd-endpoints.ts +++ b/test/source/mock/wkd/wkd-endpoints.ts @@ -1,6 +1,6 @@ /* ©️ 2016 - present FlowCrypt a.s. Limitations apply. Contact human@flowcrypt.com */ -import { KeyUtil } from '../../core/crypto/key.js'; +import { PgpArmor } from '../../core/crypto/pgp/pgp-armor.js'; import { HandlersDefinition } from '../all-apis-mock'; const alice = `-----BEGIN PGP PUBLIC KEY BLOCK----- @@ -191,28 +191,28 @@ ctnWuBzRDeI0n6XDaPv5TpKpS7uqy/fTlJLGE9vZTFUKzeGkQFomBoXNVWs= export const mockWkdEndpoints: HandlersDefinition = { '/.well-known/openpgpkey/hu/ihyath4noz8dsckzjbuyqnh4kbup6h4i?l=john.doe': async () => { - return Buffer.from((await KeyUtil.dearmor(johnDoe1)).data); // direct for john.doe@localhost + return Buffer.from((await PgpArmor.dearmor(johnDoe1)).data); // direct for john.doe@localhost }, '/.well-known/openpgpkey/hu/ihyath4noz8dsckzjbuyqnh4kbup6h4i?l=John.Doe': async () => { - return Buffer.from((await KeyUtil.dearmor(johnDoe1)).data); // direct for John.Doe@localhost + return Buffer.from((await PgpArmor.dearmor(johnDoe1)).data); // direct for John.Doe@localhost }, '/.well-known/openpgpkey/hu/cb53pfqmbzc8mm3ecbjxyen65fdxos56?l=jack.advanced': async () => { - return Buffer.from((await KeyUtil.dearmor(jackAdvanced)).data); // direct for jack.advanced@localhost + return Buffer.from((await PgpArmor.dearmor(jackAdvanced)).data); // direct for jack.advanced@localhost }, '/.well-known/openpgpkey/localhost/hu/ihyath4noz8dsckzjbuyqnh4kbup6h4i?l=john.doe': async () => { - return Buffer.from((await KeyUtil.dearmor(johnDoe)).data); // advanced for john.doe@localhost + return Buffer.from((await PgpArmor.dearmor(johnDoe)).data); // advanced for john.doe@localhost }, '/.well-known/openpgpkey/localhost/hu/ihyath4noz8dsckzjbuyqnh4kbup6h4i?l=John.Doe': async () => { - return Buffer.from((await KeyUtil.dearmor(johnDoe)).data); // advanced for John.Doe@localhost + return Buffer.from((await PgpArmor.dearmor(johnDoe)).data); // advanced for John.Doe@localhost }, '/.well-known/openpgpkey/localhost/hu/pob4adi8roqdsmtmxikx68pi6ij35oca?l=incorrect': async () => { - return Buffer.from((await KeyUtil.dearmor(alice)).data); // advanced for incorrect@localhost + return Buffer.from((await PgpArmor.dearmor(alice)).data); // advanced for incorrect@localhost }, '/.well-known/openpgpkey/localhost/hu/66iu18j7mk6hod4wqzf6qd37u6wejx4y?l=some.revoked': async () => { return Buffer.from([ - ...(await KeyUtil.dearmor(validAmongRevokedRevoked1)).data, - ...(await KeyUtil.dearmor(validAmongRevokedValid)).data, - ...(await KeyUtil.dearmor(validAmongRevokedRevoked2)).data, + ...(await PgpArmor.dearmor(validAmongRevokedRevoked1)).data, + ...(await PgpArmor.dearmor(validAmongRevokedValid)).data, + ...(await PgpArmor.dearmor(validAmongRevokedRevoked2)).data, ]); }, '/.well-known/openpgpkey/localhost/policy': async () => { diff --git a/test/source/tests/unit-node.ts b/test/source/tests/unit-node.ts index 915bcb84768..6fcc02f3d32 100644 --- a/test/source/tests/unit-node.ts +++ b/test/source/tests/unit-node.ts @@ -6,7 +6,8 @@ import { MsgBlock } from '../core/msg-block'; import { MsgBlockParser } from '../core/msg-block-parser'; import { PgpHash } from '../core/crypto/pgp/pgp-hash'; import { TestVariant } from '../util'; -import { expect } from 'chai'; +import chai = require('chai'); +import chaiAsPromised = require('chai-as-promised'); import { KeyUtil, KeyInfoWithOptionalPp } from '../core/crypto/key'; import { UnreportableError } from '../platform/catch.js'; import { Buf } from '../core/buf'; @@ -17,7 +18,12 @@ import { Attachment } from '../core/attachment.js'; import { ContactStore } from '../platform/store/contact-store.js'; import { GoogleData, GmailParser, GmailMsg } from '../mock/google/google-data'; import { pubkey2864E326A5BE488A, rsa1024subkeyOnly, rsa1024subkeyOnlyEncrypted } from './tooling/consts'; +import { PgpArmor } from '../core/crypto/pgp/pgp-armor'; +import { equals } from '../buf.js'; +import { Stream } from '../core/stream'; +chai.use(chaiAsPromised); +const expect = chai.expect; // tslint:disable:no-blank-lines-func /* eslint-disable max-len */ // tslint:disable:no-unused-expression @@ -1526,5 +1532,41 @@ kBXo expect(key.usableForSigningButExpired).to.equal(false); t.pass(); }); + + ava.default(`[unit][PgpArmor.dearmor] throws on incorrect sequence`, async t => { + expect(PgpArmor.dearmor(`-----BEGIN PGP MESSAGE----- + +AAAAAAAAAAAAAAAAzzzzzzzzzzzzzzzzzzzzzzzzzzzz..... +-----END PGP MESSAGE-----`)).to.eventually.be.rejectedWith('Misformed armored text'); + t.pass(); + }); + + ava.default(`[unit][PgpArmor.dearmor] correctly handles long string`, async t => { + const source = Buffer.from('The test string concatenated many times to produce large output'.repeat(100000)); + const type = 3; + const armored = PgpArmor.armor(type, source); + const dearmored = await PgpArmor.dearmor(armored); + expect(dearmored.type).to.equal(type); + equals( + dearmored.data, + source + ); + t.pass(); + }); + + ava.default(`[unit][Stream.readToEnd] efficiently handles multiple chunks`, async t => { + const stream = new ReadableStream({ + start(controller) { + for (let i = 0; i < 100; i++) { + controller.enqueue(Buffer.from('test'.repeat(1000000))); + } + controller.close(); + } + }); + const result = await Stream.readToEnd(stream); + expect(result.length).to.equal(400000000); + t.pass(); + }); + } }; From 6ef7998e808784d2d13979e1ad09d87fc55f74f3 Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Mon, 15 Feb 2021 07:02:04 -0500 Subject: [PATCH 2/5] decrease size of test data --- test/source/tests/unit-node.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/source/tests/unit-node.ts b/test/source/tests/unit-node.ts index 6fcc02f3d32..0b58ab58256 100644 --- a/test/source/tests/unit-node.ts +++ b/test/source/tests/unit-node.ts @@ -1557,14 +1557,14 @@ AAAAAAAAAAAAAAAAzzzzzzzzzzzzzzzzzzzzzzzzzzzz..... ava.default(`[unit][Stream.readToEnd] efficiently handles multiple chunks`, async t => { const stream = new ReadableStream({ start(controller) { - for (let i = 0; i < 100; i++) { + for (let i = 0; i < 10; i++) { controller.enqueue(Buffer.from('test'.repeat(1000000))); } controller.close(); } }); const result = await Stream.readToEnd(stream); - expect(result.length).to.equal(400000000); + expect(result.length).to.equal(40000000); t.pass(); }); From 12774f3e0a555da38f8866c38488ece2b6f6df80 Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Mon, 15 Feb 2021 09:15:26 -0500 Subject: [PATCH 3/5] fix tests --- test/source/tests/flaky.ts | 15 +++++++++++++++ test/source/tests/unit-node.ts | 20 ++------------------ 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/test/source/tests/flaky.ts b/test/source/tests/flaky.ts index e401e19e630..a60d22dede3 100644 --- a/test/source/tests/flaky.ts +++ b/test/source/tests/flaky.ts @@ -12,6 +12,7 @@ import { SettingsPageRecipe } from './page-recipe/settings-page-recipe'; import { SetupPageRecipe } from './page-recipe/setup-page-recipe'; import { TestWithBrowser } from './../test'; import { GoogleData } from './../mock/google/google-data'; +import { Stream } from '../core/stream'; // tslint:disable:no-blank-lines-func @@ -119,6 +120,20 @@ export const defineFlakyTests = (testVariant: TestVariant, testWithBrowser: Test expect(fileText.toString()).to.equal(`small text file\nnot much here\nthis worked\n`); })); + ava.default(`[unit][Stream.readToEnd] efficiently handles multiple chunks`, async t => { + const stream = new ReadableStream({ + start(controller) { + for (let i = 0; i < 10; i++) { + controller.enqueue(Buffer.from('test'.repeat(1000000))); + } + controller.close(); + } + }); + const result = await Stream.readToEnd(stream); + expect(result.length).to.equal(40000000); + t.pass(); + }); + } }; diff --git a/test/source/tests/unit-node.ts b/test/source/tests/unit-node.ts index 0b58ab58256..17f8c468621 100644 --- a/test/source/tests/unit-node.ts +++ b/test/source/tests/unit-node.ts @@ -20,7 +20,6 @@ import { GoogleData, GmailParser, GmailMsg } from '../mock/google/google-data'; import { pubkey2864E326A5BE488A, rsa1024subkeyOnly, rsa1024subkeyOnlyEncrypted } from './tooling/consts'; import { PgpArmor } from '../core/crypto/pgp/pgp-armor'; import { equals } from '../buf.js'; -import { Stream } from '../core/stream'; chai.use(chaiAsPromised); const expect = chai.expect; @@ -1534,10 +1533,9 @@ kBXo }); ava.default(`[unit][PgpArmor.dearmor] throws on incorrect sequence`, async t => { - expect(PgpArmor.dearmor(`-----BEGIN PGP MESSAGE----- + await expect(PgpArmor.dearmor(`-----BEGIN PGP MESSAGE----- -AAAAAAAAAAAAAAAAzzzzzzzzzzzzzzzzzzzzzzzzzzzz..... ------END PGP MESSAGE-----`)).to.eventually.be.rejectedWith('Misformed armored text'); +AAAAAAAAAAAAAAAAzzzzzzzzzzzzzzzzzzzzzzzzzzzz.....`)).to.eventually.be.rejectedWith('Misformed armored text'); t.pass(); }); @@ -1554,19 +1552,5 @@ AAAAAAAAAAAAAAAAzzzzzzzzzzzzzzzzzzzzzzzzzzzz..... t.pass(); }); - ava.default(`[unit][Stream.readToEnd] efficiently handles multiple chunks`, async t => { - const stream = new ReadableStream({ - start(controller) { - for (let i = 0; i < 10; i++) { - controller.enqueue(Buffer.from('test'.repeat(1000000))); - } - controller.close(); - } - }); - const result = await Stream.readToEnd(stream); - expect(result.length).to.equal(40000000); - t.pass(); - }); - } }; From 34b1b269458f4fbcbe3d02bda416f26103e00d59 Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Mon, 15 Feb 2021 09:29:22 -0500 Subject: [PATCH 4/5] fixed type --- extension/js/common/core/crypto/pgp/pgp-armor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extension/js/common/core/crypto/pgp/pgp-armor.ts b/extension/js/common/core/crypto/pgp/pgp-armor.ts index 94ff3f27152..081b9fba95a 100644 --- a/extension/js/common/core/crypto/pgp/pgp-armor.ts +++ b/extension/js/common/core/crypto/pgp/pgp-armor.ts @@ -95,7 +95,7 @@ export class PgpArmor { throw new Error('Message does not have armor headers'); } - public static dearmor = async (text: string): Promise<{ type: number, data: Uint8Array }> => { + public static dearmor = async (text: string): Promise<{ type: OpenPGP.enums.armor, data: Uint8Array }> => { const decoded = await opgp.armor.decode(text); const data = await Stream.readToEnd(decoded.data); return { type: decoded.type, data }; From 38dcdc542a509b564d99c66fd19e17c474d65e8b Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Mon, 15 Feb 2021 10:44:22 -0500 Subject: [PATCH 5/5] Removed dearmor() from KeyUtil --- extension/js/common/core/crypto/key.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/extension/js/common/core/crypto/key.ts b/extension/js/common/core/crypto/key.ts index b6dfafce17c..6ca0c16644a 100644 --- a/extension/js/common/core/crypto/key.ts +++ b/extension/js/common/core/crypto/key.ts @@ -130,14 +130,6 @@ export class KeyUtil { return (await KeyUtil.parseMany(text))[0]; } - public static dearmor = async (text: string): Promise<{ type: number, data: Uint8Array }> => { - const decoded = await opgp.armor.decode(text); - let buffer = new Uint8Array(); - const ws = new WritableStream({ write: chunk => { buffer = new Uint8Array([...buffer, ...chunk]); } }); - await decoded.data.pipeTo(ws); - return { type: decoded.type, data: buffer }; - } - public static parseMany = async (text: string): Promise => { const keyType = KeyUtil.getKeyType(text); if (keyType === 'openpgp') {