1680843 - Message header display shows stale `Sender: ...` header for all messages viewed after displaying S/MIME message which shows `Sender: ...` if `From:` != signing address (with mailnews.headers.showSender = false)

Status: VERIFIED FIXED

(Thunderbird :: Security, defect, P1)

Description

[Andreas Schamanek]

Attachment: Testcase 1: sMimeSenderHeaderTest.eml with `From` != `signer address`

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

Starting with Thunderbird 78, running Windows 10 or Linux x64, we observe the erroneous displaying of a "Sender:" header if a particular type of message was previewed. The "Sender:" header of the triggering message is then display on all subsequently viewed messages.

Steps to reproduce:
Thunderbird 78.x, tested up to 78.5.1;
add the attached minimal example S/MIME signed message smimesendertest.eml to a folder (IMAP or local);
preview the message (or open it in a tab).

Actual results:

The example message includes the header Sender: FALSE SENDER <[e-mail]> which is correctly displayed in the preview. After this, the same header will be shown on all subsequently previewed messages (or ones open in tabs) which is of course incorrect.

Testing showed that a crucial condition of messages that trigger this bug is the fact that the RFC5322-From of the S/MIME signed message does not match the signers address. We observed this on a local mailing list that changes the "From:".

Comment 1

[Andreas Schamanek]

Attachment: Screenshot 1

Screenshot 1 shows preview of triggering example smimesendertest.eml featuring the correct display of the "Sender:" header.

Comment 2

[Andreas Schamanek]

Attachment: Screenshot 2

Screenshot 2 shows a preview of a message sent by Bugzilla opened after previewing smimesendertest.eml. The screenshot shows the erroneous display of a Sender: header which is not part of the Bugzilla message.

Comment 4

[Thomas D. (:thomas8)]

Comment on attachment 9191420 [details]
Testcase 1: sMimeSenderHeaderTest.eml with From != signer address

...

Comment 5

[Thomas D. (:thomas8)]

Magnus, you've set this as P1 due to the very exposed wrong UI state (9 duplicates). Now that it's actionable, could you assign someone?
I'll add a possible starting point in code in my next comment.

Thank you very much Andreas Sch. for this crystal-clear bug report in perfect shape!
Let's reopen this one for a clean start to work on bug 1670243, which doesn't have a good description and is currently already at comment 29 due to a worthwhile effort of trying to figure out bit by bit what's going on...

Confirmed for Release 78.12.0 (32-bit), Beta 91.0b2 (64-bit), and Daily 91.0b2 (64-bit).

• Happens exactly as described after previewing the testcase message of attachment 9191420 [details].
• Obviously highly irritating that TB keeps showing a stale Sender: header on any unrelated messages.
• Priority P1 set by mkmelin on original bug 1670243, which has 8 more duplicates.

STR

1. Copy message of minimal testcase 1 (attachment 9191420 [details], sMimeSenderHeaderTest.eml) into any TB folder (IMAP or local).

• S/MIME signed message
• From: != signing address, which triggers display of Sender: header (important) even when...
• mailnews.headers.showSender = false (important; bug may not happen otherwise)

2. First view other messages before viewing the testcase (for comparison).
3. View testcase 1 (in preview pane or in a tab).
4. Then view any unrelated messages in any folder of any account after viewing the testcase.

Actual result

• Before viewing testcase, all messages will display with correct headers.
• After viewing testcase, all messages will henceforth display with the stale Sender: header of testcase 1.

Expected result

• Sender: header must be hidden from message header display for any message which does not have its own Sender: header which we need to show; while still respecting mailnews.headers.showSender pref.
• See comment 7 for the current code which seems to attempt just that.

Comment 6

[Thomas D. (:thomas8)]

Alice, could you kindly find the regression range?
It's a high-value bug ;-)

Comment 7

[Thomas D. (:thomas8)]

Possible starting point in code

(updated from Gerald's Bug 1670243 Comment 24 - thanks for the pointer!)

The function onSMIMEBeforeShowHeaderPane() looks as if it should do exactly what is needed to prevent this bug.
So probably for some reason, either it's not running at all or it fails to achieve.
No errors in console afsics.

https://searchfox.org/comm-central/rev/aa91ead1bc230db8903fc6b0fef946c2535fbf0b/mail/extensions/smime/content/msgHdrViewSMIMEOverlay.js#360-371

function onSMIMEBeforeShowHeaderPane() {
  // For signed messages with differing Sender as signer we force showing Sender.
  // If we're now in a different message, hide the (old) sender row and remove
  // it from the header view, so that Sender normally isn't shown.
  if (
    "sender" in gExpandedHeaderView &&
    !Services.prefs.getBoolPref("mailnews.headers.showSender")
  ) {
    gExpandedHeaderView.sender.enclosingRow.collapsed = true;
    delete gExpandedHeaderView.sender;
  }
}

https://searchfox.org/comm-central/search?q=onSMIMEBeforeShowHeaderPane&path=

Comment 8

[Thomas D. (:thomas8)]

Kai, any insights?

Comment 10

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=3d544985b7296fd45fcd15123e607acf58bc1057&tochange=284f2743822fb496e79f927afcfa23dc4ee7c5cb
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=79b08a0a1c3fa2834e668b205fd69ff79c8630d1&tochange=c31591e0b66f277398bee74da03c49e8f8a0ede0

Comment 11

[Kai Engert (:KaiE:)]

Alice, thanks a lot.

Thomas, thanks for pointing us to the related code.

This was regressed by bug 1537729 which missed the code that Thomas quoted in comment 8.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Bug 1680843 - Fix regression that causes a Sender: header to remain shown after switching message. r=khushil324

Comment 13

[Thomas D. (:thomas8)]

Awesome, thanks Kai for the patch, which indeed seems to fix this from my local test.

Comment 14

[Thomas D. (:thomas8)]

Attachment: 1680843_fixSenderHeaderDisplay.diff

To speed up landing this one-liner patch, I've fixed the nit mentioned my mkmelin.
Has r=mkmelin.

# HG changeset patch
# User Kai Engert <[e-mail]>
# Parent  7401d1f46d1b9d335d9fcfd954f0b87c92b96188
Bug 1680843 - Fix regression that causes a Sender: header to remain shown after switching message. r=mkmelin,ThomasD

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9c82d0b39103
Fix regression that causes a Sender: header to remain shown after switching message. r=ThomasD

Comment 16

[Rob Lemley [:rjl]]

Comment on attachment 9233064 [details] [diff] [review]
1680843_fixSenderHeaderDisplay.diff

[Approval Request Comment]
Regression caused by (bug #): 1537729
User impact if declined: Duplicate "Sender" header visible
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
Low, this is a small change to the display style.

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9233064 [details] [diff] [review]
1680843_fixSenderHeaderDisplay.diff

[Triage Comment]
Approved for beta

Comment 18

[Rob Lemley [:rjl]]

Thunderbird 91.0b5:
https://hg.mozilla.org/releases/comm-beta/rev/373bfbdc06d4

Comment 19

[Thomas D. (:thomas8)]

Great teamwork!

Verified FIXED on 91.0b5 (64-bit), Win10