1677508 - Add some xpcshell tests for RNP.encryptAndOrSign()

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, task)

Description

[Lasana Murray]

This adds tests for RNP.encryptAndOrSign() so we can ensure it works properly.

Comment 1

[Lasana Murray]

Attachment: bug1677508.patch

I use some files I found in this test:
https://searchfox.org/comm-central/source/mailnews/base/test/unit/test_mailstoreConverter.js#16

The ones that fail are currently skipped.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9188050 [details] [diff] [review]
bug1677508.patch

Review of attachment 9188050 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/extensions/openpgp/test/unit/rnp/test_encryptAndOrSign.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

It's nice to for new files give place a file level comment telling what's it about. That's shown in directory listings for some systems.

/**
  * Tests OpenPGP encryption and signing functionality.
  */

@@ +167,5 @@
> +    info(`Running test with input from: ${filename}`);
> +
> +    let buffer = await OS.File.read(do_get_file(test.filename).path, {
> +      encoding: "utf8",
> +    });

"utf-8", but, you can't read in the non-utf8 files and expect them to work later

Comment 3

[Lasana Murray]

Attachment: bug1677508v2.patch

Take encoding into consideration for some of the file loads, added a test for bug 1669107 (skipped).

What these tests reveal to me is that RNP.encryptAndSign() seems to struggle with unicode characters.

It does not show up when composing because we base64 encode the message body when these characters appear. However I'm not sure we do that for .eml attachments otherwise bug 1669107 would not happen.

The failing tests are skipped for ow.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9188423 [details] [diff] [review]
bug1677508v2.patch

Review of attachment 9188423 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/extensions/openpgp/test/unit/rnp/test_encryptAndOrSign.js
@@ +238,5 @@
> +
> +    Assert.equal(
> +      sourceText,
> +      decryptedData,
> +      `${filename}: source text is same as decrypted text`

If this test fails the assertion will be confusing. Maybe instead

${filename}: source text and decrypted text should be the same

Comment 5

[Lasana Murray]

Attachment: bug1677508v3.patch

Test description changed.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8aded86879ee
Add xpcshell tests for RNP.encryptAndOrSign(). r=mkmelin

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/26cb0bdf028c
followup - black linting. rs=black-list DONTBUILD

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7f5f0b2d45a9
follow-up - Test manifest linting. rs=linting

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c5cdb8b1ac4d
Follow-up: Exclude the test directory from l10n linter's configuration. rs=bustage-fix

Comment 10

[Kai Engert (:KaiE:)]

If this targets 78, it should get uplifted to beta 84, can you please request beta approval?

Comment 11

[Lasana Murray]

Comment on attachment 9188545 [details] [diff] [review]
bug1677508v3.patch

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined: None.
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): Little risk. Patch has disabled tests for issue related to bug 1669107

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):

Comment 12

[Lasana Murray]

Comment on attachment 9188545 [details] [diff] [review]
bug1677508v3.patch

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined: None.
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): Little risk. Patch has disabled tests for issue related to bug 1669107

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9188545 [details] [diff] [review]
bug1677508v3.patch

[Triage Comment]
Approved for beta

Comment 14

[Dave Miller [:justdave]]

Thunderbird 84.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/2c7024dfc814
https://hg.mozilla.org/releases/comm-beta/rev/8aae141e1c9a
https://hg.mozilla.org/releases/comm-beta/rev/aa68237e0d56
https://hg.mozilla.org/releases/comm-beta/rev/ec3e1cbf8c2d

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9188545 [details] [diff] [review]
bug1677508v3.patch

[Triage Comment]
Approved for esr78

Comment 16

[Kai Engert (:KaiE:)]

This code here appears to depend on bug 1676887,
because it calls OpenPGPTestUtils.initOpenPGP which was added in bug 1676887.

Because bug 1676887 hasn't yet been approved for esr78, it cannot yet be uplifted to esr78.
Please clarify what you want to do (uplift bug 1676887 too?).

Also, it isn't obvious what to do about the 4th commit
https://hg.mozilla.org/releases/comm-beta/rev/ec3e1cbf8c2d
because file tools/lint/l10n.yml doesn't exist on the esr78 branch.
Please clarify.

Comment 17

[Lasana Murray]

I requested uplift for bug 1676887. Maybe https://hg.mozilla.org/releases/comm-beta/rev/ec3e1cbf8c2d can be ignored for 78?

Comment 18

[Rob Lemley [:rjl]]

(In reply to Kai Engert (:KaiE:) from comment #16)

Also, it isn't obvious what to do about the 4th commit
https://hg.mozilla.org/releases/comm-beta/rev/ec3e1cbf8c2d
because file tools/lint/l10n.yml doesn't exist on the esr78 branch.
Please clarify.

We can safely skip this commit when uplifting to esr78. Holding off on this until the dependent bug is approved for uplift.

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 78.6.0:
https://hg.mozilla.org/releases/comm-esr78/rev/a4d9a53f5ec3

(Follow-ups are folded into the commit above)

Comment 20

[Kai Engert (:KaiE:)]

The uplift isn't working on esr78.

We get
WARNING - TEST-UNEXPECTED-FAIL | comm/mail/extensions/openpgp/test/unit/rnp/test_encryptAndOrSign.js | xpcshell return code: 0

for several builds, but I cannot see the real failure reason in the logfile.

Comment 21

[Kai Engert (:KaiE:)]

Found the likely cause:

INFO - PID 3099 | console.debug: (new Error("Error(s) encountered during statement execution: no such table: acceptance_decision", "resource://gre/modules/Sqlite.jsm", 887))
INFO - Unexpected exception Error: configured sender key 0xFBFCC82A015E7330 isn't accepted as a personal key at chrome://openpgp/content/modules/RNP.jsm:2309

So apparently init hasn't passed.

Mark Banner had made changes to the OpenPGP code on trunk only, cleanup, optimization, delaying, etc.
As a result, you probably need different init code in your tests on esr78.

Should we back out on esr78 until we have a fix?

Comment 22

[Kai Engert (:KaiE:)]

openpgp core.jsm startup is a sync function on esr78.
It had been changed to async in bug 1675325.

That means the test starts init asynchronously, but the sqlite init is not yet done when other code attempts to use the tables.
Lasana, as part of requesting uplift to a stable branch, it should be good practice to test that it works on that branch.

Comment 23

[Kai Engert (:KaiE:)]

I think unless the test code can ensure that OpenPGP has been completely initialized, the tests aren't testing the same runtime environment as the regular Thunderbird process. I suggest to back out until we have a branch specific patch that is known to correctly initialize on esr78.

Comment 24

[Magnus Melin [:mkmelin]]

Probably best to back out on 78, since it's test only and the test is failing.
Unless Lasana has quick ideas of how to get it working. Maybe uplifting bug 1675325 fixes it?

Comment 25

[Kai Engert (:KaiE:)]

There have been several changes to openpgp init logic on comm-central.
Difficult to say if cherry-picking this single change to the init code works independently without other side effects.
At the very least, I suggest that Lasana compares the openpgp code on esr78 and comm-central, looking for differences and potential problems, to investigate which changes to the init logic are required on esr78 - plus some interactive testing using a local esr78 build.

Comment 26

[Kai Engert (:KaiE:)]

Backed out from 78.x
https://hg.mozilla.org/releases/comm-esr78/rev/ed6ff392ec09ad15c1fd1c73f5611693b44a528f

Comment 27

[Lasana Murray]

Sorry, I'm not sure what the strategy is here. If the init code is only async on trunk then it's probably better to leave these tests our of the esr. I only requested uplift as I was asked to, otherwise it's a bit difficult to keep up with the changes.

Comment 28

[Lasana Murray]

Actually, Bug 1675325 needs to be uplifted before this. The console message Kai posted in comment 21 is why I did that patch.

Comment 29

[Kai Engert (:KaiE:)]

(In reply to Lasana Murray from comment #28)

Actually, Bug 1675325 needs to be uplifted before this. The console message Kai posted in comment 21 is why I did that patch.

Please also set the dependency attribute of bugs, if you notice such dependencies.

Comment 30

[Rob Lemley [:rjl]]

Comment on attachment 9188545 [details] [diff] [review]
bug1677508v3.patch

[Approval Request Comment]
Resetting the approval status for now

Comment 31

[Wayne Mery (:wsmwk)]

Comment on attachment 9188545 [details] [diff] [review]
bug1677508v3.patch

[Triage Comment]
Approved for esr78

Bug 1675325 need uplifting first

Comment 32

[Dave Miller [:justdave]]

Thunderbird 78.7.0:
https://hg.mozilla.org/releases/comm-esr78/rev/9a4cebb46eb7

Comment 33

[Kai Engert (:KaiE:)]

Relanding on esr78 introduced a permanent orange for test_encryptAndOrSign.js

The reason is you didn't reland dependency bug 1676887.

You could reland it by reverting the second commit from bug 1676887 comment 10.
https://hg.mozilla.org/releases/comm-esr78/rev/068dfd26c5d334bac8473c110934be67e2351b7d

Let me verify locally prior to doing that.

Comment 34

[Kai Engert (:KaiE:)]

I locally confirmed comment 33 on Linux esr78

Comment 35

[Rob Lemley [:rjl]]

Thanks, I'm handling 78.7.1 so I will make sure it gets taken care of.

Comment 36

[Rob Lemley [:rjl]]

1676887 comment 12.