1212075 - CNs with comma are not getting quoted by validateRecipientList

Status: RESOLVED FIXED

(Calendar :: E-mail based Scheduling (iTIP/iMIP), defect)

Description

[[:MakeMyDay]]

While working on bug 1212072, it turns out that cal.validateRecipientList(...) does not handle entries with a display name containing a comma correctly, because the DN does not get quoted in that case. E.g.

Last, First <[e-mail]>, Last, Second <[e-mail]>

is not converted to

"Last, First" <[e-mail]>, "Last, Second" <[e-mail]>

but ends in 

First <[e-mail]>, Second <[e-mail]>

Comment 1

[[:MakeMyDay]]

Attachment: FixValidateRecipientList-V1.diff

The patch fixes the issue and adds a test for this.

Comment 2

[[:MakeMyDay]]

Eventually, we should still move validateRecipientList from calUtils to ltnInvitationUtils as it's intended and currently only used for preparing email stuff.

Comment 3

[[:MakeMyDay]]

Attachment: FixValidateRecipientList-V2.diff

There was an insufficient handling for semi-colons within common names in the previous patch, as I found when working on bug 1228438. This patch should take care.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8692988 [details] [diff] [review]
FixValidateRecipientList-V2.diff

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

What if the found prefixed contain characters that need to be escaped? Maybe it makes sense to not use regexes here, but a custom character/word based parser.

Comment 5

[[:MakeMyDay]]

If it finds ; or , (escpaed or not) this will end in putting the cn in double quotes. Further characters are currently not covered. We can add more charecters to the pattern if wanted.

As this is currently breaking tests - bug 1230766 - and already an improvement compared to current situation, I suggest to check this in and maybe extend the pattern later on.

Comment 6

[Jorg K (CEST = GMT+2)]

Guys, how are we going with this? I'd like to see the tree green one day, perhaps for Christmas (this year 2015).

Comment 7

[aleth [:aleth]]

(In reply to Philipp Kewisch [:Fallen] from comment #4)
> What if the found prefixed contain characters that need to be escaped? Maybe
> it makes sense to not use regexes here, but a custom character/word based
> parser.

Don't some regexes also have issues with unicode characters?

Comment 8

[aleth [:aleth]]

Note the failing part of the existing test should be reenabled when this lands, cf https://hg.mozilla.org/comm-central/rev/63a4dcada36a

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to aleth [:aleth] from comment #7)
> (In reply to Philipp Kewisch [:Fallen] from comment #4)
> > What if the found prefixed contain characters that need to be escaped? Maybe
> > it makes sense to not use regexes here, but a custom character/word based
> > parser.
> 
> Don't some regexes also have issues with unicode characters?

Yes, but only if you match for them specifically, e.g. you want all characters, then just \w is not enough because that just matches [a-zA-Z_] or something like that.

If you want to take this for now, I'd suggest escaping the regex, e.g. with value.replace(/[\-\[\]{}()*+?.,\\\^$|#\s]/g, "\\$&"); (props to jquery) before passing it to new Regexp().

Comment 10

[[:MakeMyDay]]

Attachment: FixValidateRecipientList-V3.diff

Updated patch with the extended regex pattern and some explaining comments.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8703292 [details] [diff] [review]
FixValidateRecipientList-V3.diff

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

With all the special case workarounds, maybe it makes sense to fix splitRecipients instead of adding more workarounds? I'm giving r+ with comments here in case you decide not to fix splitRecipients, but I'd strongly suggest doing so instead.

::: calendar/base/modules/calUtils.jsm
@@ +267,5 @@
> +                // address) - we still need to identify the original delimiter to append it to the
> +                // prefix
> +                let memberCnPart = member.match(/(.*) <.*>/);
> +                if (memberCnPart) {
> +                    let pattern = new RegExp(prefix + "([;,] *)" + memberCnPart[1]);

Can the prefix contain special regex characters? If so, please escape them.

::: calendar/test/unit/test_calutils.js
@@ +115,5 @@
> +                {input: "first.last.example.net", expected: "first.last.example.net"}];
> +    let i = 0;
> +    for (let test of data) {
> +        i++;
> +        equal(cal.prependMailTo(test.input), test.expected, "(test #" + i + ")");

If you need an index anyway, then I'd suggest just using for (let i = 0, len < data.length; i < len; i++). That aside, maybe instead of an index you can include the input in the description?

@@ +129,2 @@
>      for (let test of data) {
> +        i++;

Same comment about i here.

@@ +273,5 @@
> +    let i = 0;
> +    for (let test of data) {
> +        i++;
> +        equal(cal.validateRecipientList(test.input), test.expected,
> +              "(test #" + i + ")");

Same comment about i and test input here.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Philipp Kewisch [:Fallen] from comment #11)
> ::: calendar/base/modules/calUtils.jsm
> @@ +267,5 @@
> > +                // address) - we still need to identify the original delimiter to append it to the
> > +                // prefix
> > +                let memberCnPart = member.match(/(.*) <.*>/);
> > +                if (memberCnPart) {
> > +                    let pattern = new RegExp(prefix + "([;,] *)" + memberCnPart[1]);
> 
> Can the prefix contain special regex characters? If so, please escape them.
Ah I mentioned this in an earlier comment that I didn't read this time :) Anyway, I still think it should be escaped. MDN also has an escaping function, although it is not built in.

Comment 13

[[:MakeMyDay]]

Attachment: FixValidateRecipientList-V4.diff

Updated patch excluding simple spaces from requiring dquoting.

There's no need for escaping ; or , within [] - only the characters []/^- with a special meaining in that context would need to.

Comment 14

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/686ee5f35b105886d37f3c2ce9f7e727f7d79282
Bug 1212075 - CNs with comma are not getting quoted by validateRecipientList. r=philipp a=aleth

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-aurora changeset a8a7e424d921

Comment 16

[[:MakeMyDay]]

Comment on attachment 8706110 [details] [diff] [review]
FixValidateRecipientList-V4.diff

As you uplifted 1228438 to beta, this should be backported, too.

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8706110 [details] [diff] [review]
FixValidateRecipientList-V4.diff

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

I was wondering why there was bitrot on beta :)

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

This patch doesn't apply cleanly to beta, looks like it depends on yet another patch. Can you either adapt the patch or request more backports?

Comment 19

[[:MakeMyDay]]

Attachment: FixValidateRecipientList-V4-for-beta.diff

Updated beta backport.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Backported to releases/comm-beta changeset ed230b1bf0d8