Closed Bug 1212075 Opened 4 years ago Closed 4 years ago

CNs with comma are not getting quoted by validateRecipientList

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

References

Details

Attachments

(2 files, 3 obsolete files)

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 <first.last@example.net>, Last, Second <second.last@example.net>

is not converted to

"Last, First" <first.last@example.net>, "Last, Second" <second.last@example.net>

but ends in 

First <first.last@example.net>, Second <second.last@example.net>
Attached patch FixValidateRecipientList-V1.diff (obsolete) — Splinter Review
The patch fixes the issue and adds a test for this.
Attachment #8670481 - Flags: review?(philipp)
Eventually, we should still move validateRecipientList from calUtils to ltnInvitationUtils as it's intended and currently only used for preparing email stuff.
Blocks: ltn47
Summary: DNs with comma are not getting quoted by validateRecipientList → CNs with comma are not getting quoted by validateRecipientList
Attached patch FixValidateRecipientList-V2.diff (obsolete) — Splinter Review
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.
Attachment #8670481 - Attachment is obsolete: true
Attachment #8670481 - Flags: review?(philipp)
Attachment #8692988 - Flags: review?(philipp)
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.
Attachment #8692988 - Flags: review?(philipp) → feedback+
Blocks: 1230766
Blocks: 1212072
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.
Guys, how are we going with this? I'd like to see the tree green one day, perhaps for Christmas (this year 2015).
Flags: needinfo?(philipp)
(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?
Note the failing part of the existing test should be reenabled when this lands, cf https://hg.mozilla.org/comm-central/rev/63a4dcada36a
Flags: needinfo?(philipp)
(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().
Attached patch FixValidateRecipientList-V3.diff (obsolete) — Splinter Review
Updated patch with the extended regex pattern and some explaining comments.
Attachment #8692988 - Attachment is obsolete: true
Attachment #8703292 - Flags: review?(philipp)
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.
Attachment #8703292 - Flags: review?(philipp) → review+
(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.
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.
Attachment #8703292 - Attachment is obsolete: true
Attachment #8706110 - Flags: review?(philipp)
Attachment #8706110 - Flags: approval-calendar-aurora?(philipp)
Attachment #8706110 - Flags: review?(philipp)
Attachment #8706110 - Flags: review+
Attachment #8706110 - Flags: approval-calendar-aurora?(philipp)
Attachment #8706110 - Flags: approval-calendar-aurora+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/686ee5f35b105886d37f3c2ce9f7e727f7d79282
Bug 1212075 - CNs with comma are not getting quoted by validateRecipientList. r=philipp a=aleth
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.8
Whiteboard: [checkin-needed comm-aurora]
Keywords: checkin-needed
Backported to releases/comm-aurora changeset a8a7e424d921
Keywords: checkin-needed
Target Milestone: 4.8 → 4.7
Whiteboard: [checkin-needed comm-aurora]
Comment on attachment 8706110 [details] [diff] [review]
FixValidateRecipientList-V4.diff

As you uplifted 1228438 to beta, this should be backported, too.
Attachment #8706110 - Flags: approval-calendar-beta?(philipp)
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 :)
Attachment #8706110 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
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?
Flags: needinfo?(makemyday)
Updated beta backport.
Flags: needinfo?(makemyday)
Attachment #8706560 - Flags: review?(philipp)
Attachment #8706560 - Flags: approval-calendar-beta?(philipp)
Attachment #8706560 - Flags: review?(philipp)
Attachment #8706560 - Flags: review+
Attachment #8706560 - Flags: approval-calendar-beta?(philipp)
Attachment #8706560 - Flags: approval-calendar-beta+
Backported to releases/comm-beta changeset ed230b1bf0d8
Target Milestone: 4.7 → 4.6
You need to log in before you can comment on or make changes to this bug.