use OpenPGP.js from web content script, simplify #5013

rrrooommmaaa · 2023-03-14T10:36:20Z

This PR use OpenPGP.js from web content script, removing unnecessary calls to background

close #4906
close #2560

Tests (delete all except exactly one):

Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

addresses the issue it closes (if any)
code is readable and understandable
is accompanied with tests, or tests are not needed
is free of vulnerabilities
is documented clearly and usefully, or doesn't need documentation

…in-content-script-live-test

test/source/tests/content-script-test.js

…in-content-script-live-test

rrrooommmaaa · 2023-03-24T16:21:19Z

conf/webpack.config.js

There was really a lot of work done only to be able to call web-stream-tools readToEnd function, with different approaches for unit test code, extension pages and content script. May it be possible to convince OpenPGP.js developers to simply re-export this helper function from their library, @tomholub ?

rrrooommmaaa · 2023-03-24T16:53:57Z

extension/js/common/core/crypto/pgp/openpgp-key.ts

+  private static patch = (opgpKey: OpenPGP.Key) => {
+    if (Catch.browser()?.name === 'firefox') {
+      // need to re-create Uint8Array from globalThis.Uint8Array or window.Uint8Array for Firefox content script
+      OpenPGPKey.fixUint8Arrays(opgpKey);


The main problem with Firefox was that Uint8Array's created by OpenPGP.js were of globalThis.Uint8Array or window.Uint8Array types
and this fix openpgpjs/web-stream-tools#23 didn't help because
calling a method of such an object (e.g. stripped = bin.subarray(i); inside OpenPGP.js) caused an exception Error: Permission denied to access property "constructor"
However, this type of workaround works: new Uint8Array(bin).subarray(i)
So this patch iterates over the tree of OpenPGP.Key object to re-initialize all Uint8Array's with new Uint8Array

Opinions, @sosnovsky, @tomholub?

I imagine this could be fixed purely in OpenPGP.js which would be cleaner? Either that, or monkey-patch the types on our end.

The main problem with Firefox was that Uint8Array's created by OpenPGP.js were of globalThis.Uint8Array or window.Uint8Array types

Does that mean they create sometimes one type and sometimes the other?

Option 1: Can the library be patched to always create the same "good" type?

Or are you saying that both globalThis.Uint8Array and window.Uint8Array are "bad" and we need just Uint8Array? Where does that one come from if not global or window?

Option 2: Could the library be patched to accept a config.Uint8ArrayClass = Uint8Array and then that would be used throughout the codebase? That would be a larger change, presumably.

Option 3: Alternatively, on the "broken" type, could we monkey patch it from outside the library like this? Right in requireOpenpgpjs

globalThis.Uint8Array.prototype.subarray = function (i) { return new Uint8Array(bin).subarray(i); } window.Uint8Array.prototype.subarray = function (i) { return new Uint8Array(bin).subarray(i); }

The main problem with Firefox was that Uint8Array's created by OpenPGP.js were of globalThis.Uint8Array or window.Uint8Array types

Does that mean they create sometimes one type and sometimes the other?

With regard to this particular test if (x instanceof window.Uint8Array) seems to be good enough, while if (x instanceof globalThis.Uint8Array) causes the test to fail.

The window.openpgp (where we take the instance of OpenPGP.js) was undefined when loading the OpenPGP.js bundle in Firefox, so I changed it to globalThis.openpgp. This works in both Chrome and Firefox. So globalThis in Firefox is a sandbox, while in Chrome it's a window.

And the issue (that I dealt with earlier) https://github.com/openpgpjs/web-stream-tools/pull/23/files is that OpenPGP.js doesn't recognize its "own" Uint8Array type when running in a Firefox sandbox.

I just had another look at OpenPGP.js code (and there is native Web Cryptography API involved) and have this hypothesis (not tested yet):
As the "Permission denied" error was encountered in serializeParams function, probably these "bad" window.Uint8Array objects are returned from this Web Cryptography API, while globalThis.Uint8Array (sandboxed) my be created by the OpenPGP.js code itself. And the "constructor" permission denied error actually means that subarray is trying to create window.Uint8Array (same as itself) but as it is called by a sandbox only globalThis.Uint8Array are allowed to be created, hence the cul-de-sac. We can try and file this as a bug to Firefox.

Option 1: Can the library be patched to always create the same "good" type?

The code itself only has simple new Uint8Array calls, so the fact that it creates globalThis.Uint8Array is a consequence that the code is running in a sandbox. So this question should be rephrased as -- can and should we make the code run not in a sandbox in Firefox? If not, we can probably try and update the library to re-initialize all Uint8Array's coming from Web Cryptography API (that is, convert window.Uint8Array to sandboxed globalThis.Uint8Array), but how to unit-test this? We need to handle each place where Uint8Array arrives from the API and make sure we handle each new such place.

Or are you saying that both globalThis.Uint8Array and window.Uint8Array are "bad" and we need just Uint8Array? Where does that one come from if not global or window?

Apparently this is due to the complex Firefox process model (that I don't fully understand yet) https://wiki.mozilla.org/Security/Sandbox/Process_model
Looks like globalThis.Uint8Array (sandbox) is bearable in a sense that it can be dealt with patches like https://github.com/openpgpjs/web-stream-tools/pull/23/files but there may be problems in the code consuming these objects should they try to test their types.

Option 2: Could the library be patched to accept a config.Uint8ArrayClass = Uint8Array and then that would be used throughout the codebase? That would be a larger change, presumably.

It's not an issue of mis-configuration.

Option 3: Alternatively, on the "broken" type, could we monkey patch it from outside the library like this? Right in requireOpenpgpjs

globalThis.Uint8Array.prototype.subarray = function (i) { return new Uint8Array(bin).subarray(i); } window.Uint8Array.prototype.subarray = function (i) { return new Uint8Array(bin).subarray(i); }

I can try this.

This works in both Chrome and Firefox:

if (window !== globalThis) { window.Uint8Array.prototype.subarray = function (i) { return new globalThis.Uint8Array(this).subarray(i); }; }

and this too:

if (window !== globalThis) { window.Uint8Array.prototype.subarray = function (i) { return new Uint8Array(this).subarray(i); }; }

we can also add Catch.browser().name === 'firefox' into the condition.

The following results in an infinite recursion in Chrome:

window.Uint8Array.prototype.subarray = function (i) { return new Uint8Array(this).subarray(i); };

But may this patch possibly affect other scripts on the page? Is window of the content script isolated from the window of the page itself?

we can also add Catch.browser().name === 'firefox' into the condition.

yes. This looks good. If that's the case, we don't have to test Firefox so thoroughly, since it's a global fix, and we also wouldn't have to set up Firefox automated tests at this time.

we don't have to test Firefox so thoroughly

There potentially might be issues with other objects than Uint8Array. Also we might need to patch other methods that return an array, e.g. slice and filter?

I tested slice and filter -- slice failed so I patched it too, filter is working without modification.

I also tested the "bad" Uint8Array as input to a function like readBinary() or openpgp.readKey{ binaryKey: ... }
When unpatched, the permission denied exception happens.
When patched everything is ok.
My concern is whether our prototype patch applies to all potential sources of Uint8Arrays such as network, background page etc.?
Should we pre-emptively re-initialize all Uint8Array arguments of, say, KeyUtil and MsgUtil methods?

Should we pre-emptively re-initialize all Uint8Array arguments of, say, KeyUtil and MsgUtil methods?

I wouldn't. I'd just go with the patches that appear to work, do some manual testing on Firefox, release it, wait for any reports and then fix those (which should be coming from less common workflows).

We only probably have 5k users on Firefox, as opposed to hundreds of thousands on Chrome. Plus I treat Firefox as our beta channel. So I'm not too worried, and wouldn't want to over-focus on this.

extension/js/common/core/crypto/pgp/openpgp-key.ts

rrrooommmaaa · 2023-03-24T17:05:11Z

tooling/build-types-and-manifests.ts

 makeMockBuild(CHROME_ENTERPRISE);
 makeLocalFesBuild(CHROME_ENTERPRISE);
+makeContentScriptTestsBuild('chrome-consumer-mock');
+// makeContentScriptTestsBuild('firefox-consumer'); // for manual testing of content script in Firefox


With this build firefox-consumer-content-script-tests I'm able to manually run the test in Firefox and see the result like this (the script is for some unknown reason called 3 times in Firefox. while only once in Chrome) when opening the gmail page

Should we look into automating firefox tests?

rrrooommmaaa · 2023-03-24T17:12:15Z

tooling/build-types-and-manifests.ts

 makeMockBuild(CHROME_CONSUMER);
 makeMockBuild(CHROME_ENTERPRISE);
 makeLocalFesBuild(CHROME_ENTERPRISE);
+makeContentScriptTestsBuild('chrome-consumer-mock');


The build chrome-consumer-content-script-tests-mock is used in automated testing

rrrooommmaaa · 2023-03-24T17:13:45Z

test/source/tests/content-script.ts

+      await gmailPage.waitAny('@content-script-test-result');
+      const allValues = (await gmailPage.readAll('@content-script-test-result')).map(el => el.innerText);
+      // multiple results appear in Firefox. The test is successful only if all divs have the word 'pass'
+      expect(Value.arr.unique(allValues)).to.eql(['pass']);


The test content script is called 3 times in Firefox for some unknown reason (while only once in Chrome) when opening the gmail page

This is an example of success

Maybe they run 3 times as a retry because they are failing?

Because of these 2 exceptions?

rrrooommmaaa · 2023-03-24T17:15:56Z

test/source/tests/content-script-test.js

Changes in this file are ignored on incremental builds for some reason.

I think it makes sense to test all possible OpenPGP.js use cases from the content script in Firefox

rrrooommmaaa · 2023-03-24T17:19:19Z

tooling/build-types-and-manifests.ts

+    ? sourceBuildType.slice(0, -5) + '-content-script-tests-mock'
+    : sourceBuildType + '-content-script-tests';
+  exec(`cp -r ${buildDir(sourceBuildType)} ${buildDir(testBuildType)}`);
+  edit(`${buildDir(testBuildType)}/js/content_scripts/webmail_bundle.js`, code =>


This basically wipes out the webMail code, only libraries are left and test/source/tests/content-script-test.js
immediately calls OpenPGP.js and web-stream-tools directly

…in-content-script-live-test

tomholub · 2023-03-27T11:34:50Z

extension/js/background_page/background_page.ts

  BrowserMsg.bgAddListener('storeAcctSet', (r: Bm.StoreAcctSet) => AcctStore.set(r.acctEmail, r.values));
-  BrowserMsg.bgAddListener('processAndStoreKeysFromEkmLocally', processAndStoreKeysFromEkmLocally);
-  BrowserMsg.bgAddListener('getLocalKeyExpiration', getLocalKeyExpiration);

  // todo - when https://github.com/FlowCrypt/flowcrypt-browser/issues/2560
  //   is fixed, this can be moved to the gmail content script, and some may be removed


Also BrowserMsg.addPgpListeners(); could be removed from below, and added to content script message listener instead.

Then pgp_block can be updated to call content script instead of background script.

Should I do this in this PR?
I thought after #5022 we may no longer need some of these messages, e.g. pgpMsgDecrypt or pgpMsgVerifyDetached

i see. then it doesn't have to be in this pr.

tomholub

Looks good on a high level, will wait for @sosnovsky to check

sosnovsky

Looks good for me too 👍

Roman Shevchenko added 10 commits March 14, 2023 10:34

use OpenPGP.js from web content script - wip

11d8a76

firefox fix

3346597

use processAndStoreKeysFromEkmLocally() directly

7a7651e

simplify

48fb7f2

Merge remote-tracking branch 'origin/master' into issue-4906-openpgp-…

3dfd68b

…in-content-script-live-test

added link to external PR

6c8da84

Merge remote-tracking branch 'origin/master' into issue-4906-openpgp-…

a2c989f

…in-content-script-live-test

web-stream-tools support in content script (development setup)

fe9bb59

production mode in webpack bundling

7ff645f

testing OpenPGP/Stream operations from content script

6a1af63

github-advanced-security bot found potential problems Mar 20, 2023

View reviewed changes

test/source/tests/content-script-test.js Fixed Show fixed Hide fixed

Roman Shevchenko added 16 commits March 20, 2023 15:26

Merge remote-tracking branch 'origin/master' into issue-4906-openpgp-…

1fa81d9

…in-content-script-live-test

lint fix

7dff2be

build for manual testing of content script in Firefox

4e33580

more debug configurations for VSCode

6d2ec20

removed duplicate

8d4a3ab

Merge remote-tracking branch 'origin/master' into issue-4906-openpgp-…

4298b1d

…in-content-script-live-test

fix

9598bb0

fix

a72e748

allow multiple test result divs

aa5c6ac

Merge remote-tracking branch 'origin/master' into issue-4906-openpgp-…

3fb4257

…in-content-script-live-test

clearer test output

710f27d

niceties

70dc59c

fix Uint8Array problem in Firefox

d7df2bc

Merge remote-tracking branch 'origin/master' into issue-4906-openpgp-…

d971da2

…in-content-script-live-test

updated comment

4d88425

Merge remote-tracking branch 'origin/master' into issue-4906-openpgp-…

3efd2e7

…in-content-script-live-test

rrrooommmaaa commented Mar 24, 2023

View reviewed changes

extension/js/common/core/crypto/pgp/openpgp-key.ts Outdated Show resolved Hide resolved

rrrooommmaaa commented Mar 24, 2023

View reviewed changes

Roman Shevchenko added 6 commits March 25, 2023 15:31

Merge remote-tracking branch 'origin/master' into issue-4906-openpgp-…

e53a17d

…in-content-script-live-test

globally patch Uint8Array.subarray()

2dd0237

fix

18dc1ab

patch Uint8Array.slice

2a0c078

Added a test for KeyUtil.readBinary()

9deb31a

increase ava cli timeout values

97396e1

tomholub reviewed Mar 27, 2023

View reviewed changes

rrrooommmaaa marked this pull request as ready for review March 27, 2023 12:17

rrrooommmaaa enabled auto-merge (squash) March 27, 2023 14:48

rrrooommmaaa requested a review from tomholub March 27, 2023 14:51

tomholub reviewed Mar 27, 2023

View reviewed changes

sosnovsky self-requested a review March 27, 2023 17:58

sosnovsky approved these changes Mar 27, 2023

View reviewed changes

rrrooommmaaa merged commit d92c0b0 into master Mar 27, 2023

rrrooommmaaa deleted the issue-4906-openpgp-in-content-script-live-test branch March 27, 2023 18:00

sosnovsky mentioned this pull request Mar 28, 2023

Build fails on macOS #5043

Closed

rrrooommmaaa mentioned this pull request Mar 29, 2023

build(deps-dev): bump puppeteer from 19.6.3 to 19.8.0 #5041

Closed

rrrooommmaaa mentioned this pull request Jun 2, 2023

Uint8Array - related issues in Firefox content script (sandboxed environment) openpgpjs/openpgpjs#1648

Closed

use OpenPGP.js from web content script, simplify #5013

use OpenPGP.js from web content script, simplify #5013

Uh oh!

Conversation

rrrooommmaaa commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To be filled by reviewers

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rrrooommmaaa Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomholub Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rrrooommmaaa Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rrrooommmaaa Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

sosnovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rrrooommmaaa commented Mar 14, 2023 •

edited

Loading

rrrooommmaaa Mar 24, 2023 •

edited

Loading

tomholub Mar 24, 2023 •

edited

Loading

rrrooommmaaa Mar 24, 2023 •

edited

Loading

rrrooommmaaa Mar 24, 2023 •

edited

Loading