Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quote (or don't quote) property values individually before joining #555

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

darktrojan
Copy link
Collaborator

Instead of adding quotes to the delimiting comma and around the outside, quote the individual members separately if necessary. This prevents the whole string of multiple values being quoted because of the presence of a delimiting comma.

When quotes are required (multiValueSeparateDQuote is true) only URIs are valid, which will get quoted due to the colon. When not required only specific strings (WORK, VOICE, PARCEL, etc.) are valid, none of which need quotes.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3418295304

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0007%) to 98.137%

Totals Coverage Status
Change from base Build 3275454147: -0.0007%
Covered Lines: 9196
Relevant Lines: 9355

💛 - Coveralls

@coveralls
Copy link

coveralls commented Nov 8, 2022

Pull Request Test Coverage Report for Build 8427565049

Details

  • 16 of 16 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.093%

Totals Coverage Status
Change from base Build 8419308194: 0.0%
Covered Lines: 9290
Relevant Lines: 9457

💛 - Coveralls

@darktrojan
Copy link
Collaborator Author

Aha, I thought I had asked about this before. I did, in #532.

@darktrojan darktrojan requested a review from kewisch November 8, 2022 10:05
@dilyanpalauzov
Copy link
Contributor

dilyanpalauzov commented Nov 9, 2022

In the provided test cases:

ADR;TYPE=dom,home,postal,parcel:;;123 Main Street;Any Town;CA;91921-1234
TEL;TYPE=home,voice:1234567

the property values are ;;123 Main Street;Any Town;CA;91921-1234 and 1234567. The property parameter values are dom,home,postal,parcel and home,voice. Just for clarification, can you state again, if the current case is about property values or property parameter values, as these terms can be easily confused.

Concerning the stringification of property parameter values and avoiding unnecessary quotes, I have a different approach — #535, for a different test-case. In A;X=Y:V it does the stringification of the property value parameters (Y) independent of the property-value (V) and thus irrespective on whether the value-type is multi-valued. I think this is the right approach.

I understand this is an open-source voluntary run project and I get what I paid for. Different users of ical.js submit corrections for inclusion upstream. Including the corrections upstream takes sometimes very, very long time. This results that different solutions to the same problem are submitted, while uploaded corrections are not merged upstream.

I will appreciate if some process is suggested, that makes merging submitted changes, which include test cases, upstream more fluent. This will be in the common interest.

I value the time invested in maintaining the project and personally I am not willing to be its maintainer. The only point I am raising is that it takes very long time between patch submission and its inclusion. So any speedup will be more than welcomed.

@dilyanpalauzov
Copy link
Contributor

Ah, this is not about multivalued property values, but about multivalued parameter values. Can you add a test/example, which produces this stringification result:

TEL;TYPE=home,voice,x-a,"x-b,x-c":1234567

@darktrojan
Copy link
Collaborator Author

Yes, the names are confusing. I'd have changed the commit message with my recent push if only Github had notified me about your comments.

Ah, this is not about multivalued property values, but about multivalued parameter values. Can you add a test/example, which produces this stringification result:

TEL;TYPE=home,voice,x-a,"x-b,x-c":1234567

I'm not sure if that's valid. It's a question I've been wrestling with since I first tried to fix this bug. It looks like it should be valid, but I'm yet to find any concrete answer one way or the other. Maybe I'll just assume it is valid and move on.

@darktrojan
Copy link
Collaborator Author

There. Added the extra bit to the test, which unsurprisingly was already working. I also renamed the function in question to be less confusing.

@dilyanpalauzov
Copy link
Contributor

https://www.rfc-editor.org/rfc/rfc6350#section-3.3 defines

any-param = (iana-token / x-name) "=" param-value *("," param-value)
param-value = *SAFE-CHAR / DQUOTE *QSAFE-CHAR DQUOTE
type-param = "TYPE=" type-value ("," type-value)
type-value = "work" / "home" / type-param-tel / type-param-related / iana-token / x-name
x-name = "x-" 1
(ALPHA / DIGIT / "-")
; Names that begin with "x-" or "X-" are
; reserved for experimental use, not intended for released
; products, or for use in bilateral agreements.

QSAFE-CHAR = WSP / "!" / %x23-7E / NON-ASCII
; Any character except CTLs, DQUOTE

SAFE-CHAR = WSP / "!" / %x23-39 / %x3C-7E / NON-ASCII
; Any character except CTLs, DQUOTE, ";", ":"
x22 x3A x3B

Comma (ascii code dec 44, %x2C) is SAFE-CHAR and QSAFE-CHARE, as it is within %x23-39 and within %x23-7E.

I came now the conclusion that x-b,x-c as in TEL;TYPE=home,voice,x-a,\"x-b,x-c\":1234567\r\n is not permitted in TYPE as separate value: x-b,x-c is not an x-name, since it contains comma. The ABNF of type-parameter (TYPE=) does not permit a comma or a quote in its value, when the comma is not separator, it permits x-name.

I have created one more example:

diff --git a/test/stringify_test.js b/test/stringify_test.js
index bc375b6..1ebd7c4 100644
--- a/test/stringify_test.js
+++ b/test/stringify_test.js
@@ -193,6 +193,14 @@ suite('ICAL.stringify', function() {
             },
             "phone-number",
             "1234567"
+          ],
+          [
+            "x-a",
+            {
+                "x-B": "x-c^d,"
+            },
+            "text",
+            "Brum"
           ]
         ]
       ];
@@ -200,6 +208,7 @@ suite('ICAL.stringify', function() {
         "BEGIN:VCARD\r\n" +
         "ADR;TYPE=dom,home,postal,parcel:;;123 Main Street;Any Town;CA;91921-1234\r\n" +
         "TEL;TYPE=home,voice,x-a,\"x-b,x-c\":1234567\r\n" +
+        "X-A;X-B=x-c^^d,;VALUE=TEXT:Brum\r\n" +
         "END:VCARD";
       assert.equal(ICAL.stringify.component(subject), expected);
     });

This fails. The produced output is X-A;X-B="x-c^^d,";VALUE=TEXT:Brum but there is no need to quote, since x-c^^d, are all SAFE-CHAR*.

  931 passing (964ms)
  1 failing

  1) ICAL.stringify
       stringify component
         multiple types unquoted:

      AssertionError: expected 'BEGIN:VCARD\r\nADR;TYPE=dom,home,post…' to equal 'BEGIN:VCARD\r\nADR;TYPE=dom,home,post…'
      + expected - actual

       BEGIN:VCARD
       ADR;TYPE=dom,home,postal,parcel:;;123 Main Street;Any Town;CA;91921-1234
       TEL;TYPE=home,voice,x-a,"x-b,x-c":1234567
      -X-A;X-B="x-c^^d,";VALUE=TEXT:Brum
      +X-A;X-B=x-c^^d,;VALUE=TEXT:Brum
       END:VCARD
      
      at assert.equal (node_modules/chai/lib/chai/interface/assert.js:139:10)

@dilyanpalauzov
Copy link
Contributor

#460 is a different approach to the same problem.

@darktrojan
Copy link
Collaborator Author

I took the tests from #460 since they're more comprehensive than mine, and fixed a failure they caught.

let multiValue = (paramName in designSet.param) && designSet.param[paramName].multiValue;

let paramDesign = designSet.param[paramName];
let multiValue = paramDesign && paramDesign.multiValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const multiValue = paramDesign?.multiValue;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it is unclear, why the above suggestion was skipped.

@dilyanpalauzov
Copy link
Contributor

The backslash is not escaping character in property parameters. When I modify one of the tests as:

diff --git a/test/stringify_test.js b/test/stringify_test.js
index 40337b8..800fe19 100644
--- a/test/stringify_test.js
+++ b/test/stringify_test.js
@@ -84,9 +84,9 @@ suite('ICAL.stringify', function() {
       ICAL.design.defaultSet.property.custom = { defaultType: 'text' };
       ICAL.design.defaultSet.param.type = { multiValue: ',' };
       let subject = new ICAL.Property('custom');
-      subject.setParameter('type', ['ABC', 'XYZ']);
+      subject.setParameter('type', ['\\:ABC', 'XYZ']);
       subject.setValue('some value');
-      assert.equal(subject.toICALString(), 'CUSTOM;TYPE=ABC,XYZ:some value');
+      assert.equal(subject.toICALString(), 'CUSTOM;TYPE="\\:ABC",XYZ:some value');
       delete ICAL.design.defaultSet.property.custom;
       delete ICAL.design.defaultSet.param.type;
     });

it fails:

  1) ICAL.stringify                                                                                                                     
       stringify property                                                                                                               
         property with multiple parameter values:                                                                                       
                                                                                                                                        
      AssertionError: expected 'CUSTOM;TYPE=\:ABC,XYZ:some value' to equal 'CUSTOM;TYPE="\:ABC",XYZ:some value'                         
      + expected - actual                                                                                                               
                                                                                                                                        
      -CUSTOM;TYPE=\:ABC,XYZ:some value                                                                                                 
      +CUSTOM;TYPE="\:ABC",XYZ:some value                                                                                               

\:ABC must be quoted, since \ is not an escaping character and the property parameter value contains colon :.

#535 suggests to use value.indexOf() in stringify.propertyParameterValue instead of helpers.unescapedIndexOf().

Verified

This commit was signed with the committer’s verified signature.
darktrojan Geoff Lankow
…joining

Instead of adding quotes to the delimiting comma and around the outside, quote the individual members separately if necessary.
This prevents the whole string of multiple values being quoted because of the presence of a delimiting comma.

When quotes are required (multiValueSeparateDQuote is true) only URIs are valid, which will get quoted due to the colon.
When not required only specific strings (WORK, VOICE, PARCEL, etc.) are valid, none of which need quotes.
@darktrojan
Copy link
Collaborator Author

Rebased with no further changes.

FWIW, we've been shipping these changes in Thunderbird since November 2022. It may not be perfect but I think we should merge as-is and open follow-up issues for outstanding problems.

@kewisch kewisch merged commit 1fe330f into kewisch:main Mar 26, 2024
7 checks passed
@kewisch
Copy link
Owner

kewisch commented Mar 26, 2024

Sounds fine to me, thank you!

@dilyanpalauzov
Copy link
Contributor

As I mentioned above, this change is suboptimal. #658 does further improve encoding (quoting) of property parameter values.

dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Apr 13, 2024
… as \ is no escape character there.  When the propery parameter
contains :, then it must be quoted, the colon cannot be escaped.

As the function stringify.propertyValue in fact stringifies
property parameter values, it is renamed accordingly.

kewisch#658

This supersedes kewisch#535
and updates kewisch#555 .

Without these changes, the herein added test fails:

1) ICAL.stringify
     stringify property
            stringify property value containing "escaped" ; , ::

    AssertionError: expected 'ATTENDEE;CN=X\::mailto:id' to equal 'ATTENDEE;CN="X\:":mailto:id'
        + expected - actual

    -ATTENDEE;CN=X\::mailto:id
        +ATTENDEE;CN="X\:":mailto:id
@darktrojan darktrojan deleted the noquote-tel-type branch April 18, 2024 00:58
dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Apr 20, 2024
… as \ is no escape character there.  When the propery parameter
contains :, then it must be quoted, the colon cannot be escaped.

As the function stringify.propertyValue in fact stringifies
property parameter values, it is renamed accordingly.

kewisch#658

This supersedes kewisch#535
and updates kewisch#555 .

Without these changes, the herein added test fails:

1) ICAL.stringify
     stringify property
            stringify property value containing "escaped" ; , ::

    AssertionError: expected 'ATTENDEE;CN=X\::mailto:id' to equal 'ATTENDEE;CN="X\:":mailto:id'
        + expected - actual

    -ATTENDEE;CN=X\::mailto:id
        +ATTENDEE;CN="X\:":mailto:id
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants