mailto: parsing tries to stick in-reply-to into references, but fails to account for escaping

RESOLVED FIXED in Thunderbird 3.0b3

Status

MailNews Core
Composition
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: philor, Assigned: Magnus Melin)

Tracking

Trunk
Thunderbird 3.0b3

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

4.28 KB, patch
standard8
: review+
standard8
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
There's a cute section in nsMailtoUrl::ParseMailtoUrl which attempts to take what's passed in in the |in-reply-to=<foo@bar.com>| and insert it at the end of |references=<bar@bar.com> <baz@bar.com>| without duplicating it if it's already properly at the end of the references, but unfortunately it only works if you fail to escape your mailto URI, because it finds the last reference by looking for "<", which at that point really should still be "%3C" instead.
Product: Core → MailNews Core
(Assignee)

Updated

9 years ago
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 3.0b3
(Assignee)

Comment 1

9 years ago
Created attachment 366376 [details] [diff] [review]
proposed fix

To be applied on top of the patch for bug 443851.
Attachment #366376 - Flags: superreview?(bugzilla)
Attachment #366376 - Flags: review?(bugzilla)
Attachment #366376 - Flags: superreview?(bugzilla)
Attachment #366376 - Flags: review?(bugzilla)
Attachment #366376 - Flags: review-
Comment on attachment 366376 [details] [diff] [review]
proposed fix

>+    // If not References is set, set it to be the In-Reply-To.

nit: "If References is not set" (reads better)

>+    if (m_referencePart.IsEmpty())
>+    {
>+      m_referencePart = inReplyToPart;
>+    }
>+    // References is set. Add the In-Reply-To as last header unless it's
>+    // set as last header already.
>+    else
>+    {

nit: please put the comments inside the brackets.

>+      const char * lastRef = strrchr(m_referencePart.get(), '<');
>+      nsCString lastReference;
>+      lastReference = (lastRef) ? lastRef : m_referencePart.get();

I think you should use m_referencePart.RFindChar() and StringTail.

The last line should also be merged onto the definition line for lastReference so that allocations can take place at construction time.
(Assignee)

Comment 3

9 years ago
Created attachment 368675 [details] [diff] [review]
proposed fix, v2

Addressing review comments. 
The compiler won't let me put the definition on the the same line (and use ternary...).
Attachment #366376 - Attachment is obsolete: true
Attachment #368675 - Flags: superreview?(bugzilla)
Attachment #368675 - Flags: review?(bugzilla)
Attachment #368675 - Flags: superreview?(bugzilla)
Attachment #368675 - Flags: superreview+
Attachment #368675 - Flags: review?(bugzilla)
Attachment #368675 - Flags: review+
(Assignee)

Comment 4

9 years ago
changeset:   2323:233c1c6f6853
http://hg.mozilla.org/comm-central/rev/233c1c6f6853

->FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.