Cancel message in newsgroups not possible for messages with 2 lines From: header

RESOLVED FIXED in Thunderbird 65.0

Status

defect
RESOLVED FIXED
12 years ago
8 months ago

People

(Reporter: juergen.nagler-ihlein, Unassigned)

Tracking

1.8 Branch
Thunderbird 65.0
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: 2.0.0.6 (20070728)

In my name Jürgen (ü) a German Umlaut (special character) is included. Using it in my Thunderbird News Account Setting within "Your Name" results in From: header lines in all my news postings like the following:

Date: Fri, 03 Aug 2007 00:15:32 +0200
From: =?ISO-8859-15?Q?J=FCrgen_Nagler-Ihlein?=
 <juergen.nagler-ihlein@uni-ulm.de>
User-Agent: Thunderbird 2.0.0.6 (Windows/20070728)

Following RFC 822 this is correct and is called a folded header. But trying to cancel such a message after sending from the same account results in an error complaining "This message does not appear to be from you..."

If I use the international replacement for ü (&uuml;) which is ue only a 1 lined From header is created for my messages and I don't have any problems canceling these messages.

Experimenting around I wasn't able to create a folded From header line without using an iso-escaped character, even when the resulting string was much longer, e.g. "From: Juergen Nagler-Ihlein trying to debug we need a very very very very very very very very very very long name <juergen.nagler-ihlein@uni-ulm.de>"

So one question is why an iso-escaped header has to be folded.

But the real bug is Thunderbird not recognizing the From address out of a folded From header.

Reproducible: Always

Steps to Reproduce:
1. Setup or change an existing newsgroup account and use as name "Jürgen Nagler-Ihlein"
2. Send a message to a test newsgroup
3. Check, whether the header was folded
4. Try to cancel this message

Actual Results:  
Thunderbird complains that it was not me who sent the message and that's why I'm not allowed to cancel it.

Expected Results:  
Should accept canceling the message and confirm the successful cancellation.
Version: unspecified → 2.0
Probably related to bug 383955?
Don't think so, that's trunk only (this is tb 2.0).
Problem was re-created with Seamonkey 1.1.3(Release build,Win-XP SP2).

News Server = news.mozilla.org  NewsGroup = mozilla.test
My mail address = m-wada@japan.com.
"This message does not appear to be from you..." was issued when I tried to "Cancel Message" for following my post.
(encoded name part of From: = 日本語 日本語 ... 日本語 日本語)
> Path: border1.nntp.dca.giganews.com!nntp.giganews.com!local01.nntp.dca.giganews.com!nntp.mozilla.org!news.mozilla.org.POSTED!not-for-mail
> NNTP-Posting-Date: Mon, 13 Aug 2007 06:25:37 -0500
> Date: Mon, 13 Aug 2007 20:25:05 +0900
> From: =?ISO-2022-JP?B?GyRCRnxLXDhsGyhCIBskQkZ8S1w4bBsoQiAbJEJGfEtcOGwbKEIg?=
> =?ISO-2022-JP?B?GyRCRnxLXDhsGyhCIBskQkZ8S1w4bBsoQiAbJEJGfEtcOGwbKEIgGyRCRnwbKEI=?=
> =?ISO-2022-JP?B?GyRCS1w4bBsoQiAbJEJGfEtcOGwbKEIgGyRCRnxLXDhsGyhCIBskQkZ8GyhC?=
> =?ISO-2022-JP?B?GyRCS1w4bBsoQiAbJEJGfEtcOGwbKEIgGyRCRnxLXDhsGyhCIBskQkZ8GyhC?=
> =?ISO-2022-JP?B?GyRCS1w4bBsoQiAbJEJGfEtcOGwbKEIgGyRCRnxLXDhsGyhCIBskQkZ8GyhC?=
> =?ISO-2022-JP?B?GyRCS1w4bBsoQg==?= <m-wada@japan.com>
> User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070716 SeaMonkey/1.1.3
> Newsgroups: mozilla.test
> Subject: Test of Bug 390721
> Message-ID: <JJGdndu-sJ4sol3bnZ2dnUVZ_u2mnZ2d@mozilla.org>

Confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comparing of mail address in From: header and identity setting is executed in CheckIfAuthor routine.
http://lxr.mozilla.org/seamonkey/source/mailnews/news/src/nsNNTPProtocol.cpp#4066
And mail post data for check(cancelInfo) is passed as *data.
> PRBool nsNNTPProtocol::CheckIfAuthor(nsISupports *aElement, void *data)
> cancelInfoEntry *cancelInfo = (cancelInfoEntry*) data;
And From: header data is put in *data before calling of CheckIfAuthor, but it looks that data in the first line of From: header only is saved in *data.
i.e. multi-line From: header is not supported yet in canceling of news post.
Component: General → Networking: News
Product: Thunderbird → Core
QA Contact: general → networking.news
Version: 2.0 → 1.8 Branch
Product: Core → MailNews Core
does this reproduce in version 3.1?
Whiteboard: [needs retest v3.1]
Duplicate of this bug: 217892
Cancel code hasn't changed, it's still in 3.1.

What we should do is just stream the message through libmime and ask that for the From (and other) headers.
Whiteboard: [needs retest v3.1]
When loading an article, we read the data line by line from the StreamBuffer. When we cancel, we save the necessary headers for later use. Because of the linewise reading, some trickery is needed to map the folded lines to the correct header.
Attachment #9029213 - Flags: review?(jorgk)
Comment on attachment 9029213 [details] [diff] [review]
When parsing headers to cancel an article also use folded lines.

This looks OK but the string manipulations are pretty terrible.

Coincidentally I found this comment:
// mscott: we can probably replace this stuff with nsString
M. Scott has long left the building, but we should do that.

Change those four member variables to nsCString and adapt the code that uses them. You will see that stuff like 
  m_cancelFromHdr = nullptr;
  m_cancelNewsgroups = nullptr;
  m_cancelDistribution = nullptr;
  m_cancelID = nullptr;
can just be deleted and all the clumsy freeing goes, too. Inserting something into an nsCString is also very easy.

Let me know if you don't have time and I can do it. I was told not to interfere too much with peoples work, so I'm asking.
Attachment #9029213 - Flags: review?(jorgk)
Posted patch Bug390721.patch (obsolete) — Splinter Review
OK, I got permission via PM to go though with the a steal brush here and toss a lot of ugly code.

I think this looks easier. I think when combining a folded header you need to add a space between the two bits, no?
Attachment #9029213 - Attachment is obsolete: true
Attachment #9029224 - Flags: feedback?(infofrommozilla)
Posted patch Bug390721.patch (obsolete) — Splinter Review
Oops, first test white-space then strip it. BTW, does that |static int lastHeader| need to be reset somewhere? In the CTOR/DTOR? Should that variable move to the top of the file ... with a comment?
Attachment #9029224 - Attachment is obsolete: true
Attachment #9029224 - Flags: feedback?(infofrommozilla)
Attachment #9029252 - Flags: feedback?(infofrommozilla)
(In reply to Jorg K (GMT+1) from comment #10)

>  I think when combining a folded header you need
> to add a space between the two bits, no?

Actually no. StripWhitespace () really removes all spaces, tabs
and even linebreakes from the string.
Later only the email address is used from that string.
(In reply to Jorg K (GMT+1) from comment #11)
> Created attachment 9029252 [details] [diff] [review]
> Bug390721.patch
> 
> O BTW, does that |static int
> lastHeader| need to be reset somewhere? In the CTOR/DTOR?

It would be very unusual if the header would begin with
a folded line. As soon as we hit the first valid header,
lastHeader will be reset.

>  Should that
> variable move to the top of the file ... with a comment?

A matter of taste. Since we use the variable only in this
function, I find the local declaration more logical.
(In reply to Alfred Peters from comment #12)
> (In reply to Jorg K (GMT+1) from comment #10)
> >  I think when combining a folded header you need
> > to add a space between the two bits, no?
> Actually no. StripWhitespace () really removes all spaces, tabs
> and even linebreakes from the string.
> Later only the email address is used from that string.
But don't you want to combine
XXX: this is the first line
 and here the second
into
XXX: this is the first line and here the second
and not:
XXX: this is the first lineand here the second
?

Anyway, I think this should be good to go, just need to decide on the
  m_cancelFromHdr += NS_LITERAL_CSTRING(" ") + header;
with or without the space.
Comment on attachment 9029252 [details] [diff] [review]
Bug390721.patch

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

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +2254,5 @@
> +        header.StripWhitespace();
> +        // Add folded line to header if needed.
> +        switch (lastHeader) {
> +        case 1:
> +            m_cancelFromHdr += NS_LITERAL_CSTRING(" ") + header;

See Comment 12

@@ +3716,2 @@
>                         "." CRLF, /* trailing message terminator "." */
> +                       from.get(), newsgroups, id.get(), id.get(),

==> newsgroups.get()

::: mailnews/news/src/nsNNTPProtocol.h
@@ +187,5 @@
>    nsresult LoadUrlInternal(nsIProxyInfo* aProxyInfo);
>    nsresult CleanupAfterRunningUrl();
>    void Cleanup(); //free char* member variables
>  
> +  void nsNNTPProtocol::extendCancelHeader(nsCString &header, char **p_m_header);

The function has been dropped.
Attachment #9029252 - Flags: feedback?(infofrommozilla) → feedback+
(In reply to Jorg K (GMT+1) from comment #14)
> (In reply to Alfred Peters from comment #12)
> > (In reply to Jorg K (GMT+1) from comment #10)
> > >  I think when combining a folded header you need
> > > to add a space between the two bits, no?
> > Actually no. StripWhitespace () really removes all spaces, tabs
> > and even linebreakes from the string.
> > Later only the email address is used from that string.
> But don't you want to combine
> XXX: this is the first line

StripWhitespace() makes "thisisthefirstline" out of it.

>  and here the second

StripWhitespace() => "andherethesecond"

> into
> XXX: this is the first line and here the second
> and not:
> XXX: this is the first lineand here the second

> with or without the space.

"thisisthefirstline andherethesecond"
or
"thisisthefirstlineandherethesecond"

I don't really know why this was done that way.
May be parsing is more fail-safe afterwards.
OK, fixed the issue. Please confirm via interdiff that everything is good to go now.
Attachment #9029252 - Attachment is obsolete: true
Attachment #9029322 - Flags: review+
Attachment #9029322 - Flags: feedback?(infofrommozilla)
Attachment #9029322 - Flags: feedback?(infofrommozilla) → feedback+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7cdd9ac6221e
When parsing headers to cancel an article, also use folded lines. r=jorgk
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
You need to log in before you can comment on or make changes to this bug.