Closed Bug 1677508 Opened 2 months ago Closed 2 months ago

Add some xpcshell tests for RNP.encryptAndOrSign()

Categories

(MailNews Core :: Security: OpenPGP, task)

Tracking

(thunderbird_esr78+ fixed, thunderbird84 fixed)

RESOLVED FIXED
85 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird84 --- fixed

People

(Reporter: lasana, Assigned: lasana)

References

Details

Attachments

(1 file, 2 obsolete files)

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

Status: NEW → ASSIGNED
Attached patch bug1677508.patch (obsolete) — Splinter Review

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.

Attachment #9188050 - Flags: review?(mkmelin+mozilla)
See Also: → 1669107
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
Attachment #9188050 - Flags: review?(mkmelin+mozilla) → review-
Attached patch bug1677508v2.patch (obsolete) — Splinter Review

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.

Attachment #9188050 - Attachment is obsolete: true
Attachment #9188423 - Flags: review?(mkmelin+mozilla)
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
Attachment #9188423 - Flags: review?(mkmelin+mozilla) → review+

Test description changed.

Attachment #9188423 - Attachment is obsolete: true
Attachment #9188545 - Flags: review?(mkmelin+mozilla)
Attachment #9188545 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 85 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/8aded86879ee
Add xpcshell tests for RNP.encryptAndOrSign(). r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/26cb0bdf028c
followup - black linting. rs=black-list DONTBUILD
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/7f5f0b2d45a9
follow-up - Test manifest linting. rs=linting
Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/c5cdb8b1ac4d
Follow-up: Exclude the test directory from l10n linter's configuration. rs=bustage-fix

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

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):

Attachment #9188545 - Flags: approval-comm-esr78?

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

Attachment #9188545 - Flags: approval-comm-beta?

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

[Triage Comment]
Approved for beta

Attachment #9188545 - Flags: approval-comm-beta? → approval-comm-beta+

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

[Triage Comment]
Approved for esr78

Attachment #9188545 - Flags: approval-comm-esr78? → approval-comm-esr78+

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.

Flags: needinfo?(lasana)
Depends on: 1676887

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

Flags: needinfo?(lasana) → needinfo?(justdave)

(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.

Flags: needinfo?(justdave)

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

(Follow-ups are folded into the commit above)

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.

Flags: needinfo?(lasana)

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?

Flags: needinfo?(mkmelin+mozilla)

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.

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.

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?

Flags: needinfo?(mkmelin+mozilla)

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.

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.

Flags: needinfo?(lasana)

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

(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.

Depends on: 1675325

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

[Approval Request Comment]
Resetting the approval status for now

Attachment #9188545 - Flags: approval-comm-esr78+ → approval-comm-esr78?

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

[Triage Comment]
Approved for esr78

Bug 1675325 need uplifting first

Attachment #9188545 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.