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

Fix unescaping of URIs in VCard4 and mime type of images in converter #602

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mstilkerich
Copy link
Contributor

This PR fixes two issues encountered on a V4 VCard.

  1. RFC6350 allows escaping of semicolons in property values, but vobject did not unescape them in input. data URIs used for inline images in v4 VCard contain both semicolon and comma. When the semicolon is escaped (which is optional for the producer of the VCard), it must be unescaped by the consumer or the resulting URI will still contain the backslash and be malformed.
  2. Mime types are case insensitive (RFC2045). In the vcard converter in conversion from v4, the mime type is checked in a case sensitive manner and would thus fail to convert the mime type if it is not lowercase in the input. Changed the handling to treat this case-insensitive.

Tests adapted / extended accordingly.

This is required for VCard4 (see test sample), as comma and semicolon
may appear in URIs with scheme data.
Also added the mandatory escaping of commas in VCard4 prop values to the
converter test samples.
@codecov
Copy link

codecov bot commented Dec 25, 2022

Codecov Report

Merging #602 (4d89b2e) into master (7b17e07) will increase coverage by 0.19%.
The diff coverage is 91.30%.

@@             Coverage Diff              @@
##             master     #602      +/-   ##
============================================
+ Coverage     98.53%   98.72%   +0.19%     
- Complexity     1865     1869       +4     
============================================
  Files            71       71              
  Lines          4157     5336    +1179     
============================================
+ Hits           4096     5268    +1172     
- Misses           61       68       +7     
Impacted Files Coverage Δ
lib/Property/Uri.php 93.93% <90.00%> (-6.07%) ⬇️
lib/VCardConverter.php 98.48% <100.00%> (+0.11%) ⬆️
lib/Property/Boolean.php 46.66% <0.00%> (-11.67%) ⬇️
lib/UUIDUtil.php 100.00% <0.00%> (ø)
lib/Component/VCard.php 100.00% <0.00%> (ø)
lib/Component/VTodo.php 100.00% <0.00%> (ø)
lib/Component/VAlarm.php 100.00% <0.00%> (ø)
lib/Component/VEvent.php 100.00% <0.00%> (ø)
lib/Component/VJournal.php 100.00% <0.00%> (ø)
lib/Property/UtcOffset.php 100.00% <0.00%> (ø)
... and 35 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

When converting to VCard v4, only X-ADDRESSBOOKSERVER-KIND=GROUP is
converted to KIND=group currently. X-ADDRESSBOOKSERVER-KIND=INDIVIDUAL
is kept in the vcard, although KIND=individual would be correct. Since
this is the default, the property can also be dropped, as is done by
vobject in the opposite direction (converting v4 to v3). Therefore, this
fix will also drop the X-ADDRESSBOOKSERVER-KIND=INDIVIDUAL property.

Test existing test was broken because it used the output of the v4->v3
conversion as input for the v3->v4 conversion, but since the property is
dropped during the first conversion there is no explicit
X-ADDRESSBOOKSERVER-KIND property in the input of the v3->v4 conversion.
Test fixed as well.
@mstilkerich
Copy link
Contributor Author

Added another fix: Remove X-ADDRESSBOOKSERVER-KIND=individual property in converting vcard v3 to v4. The existing test for this was broken because the used input did not contain the corresponding property.

PHOTO:data:image/gif;base64,Zm9v
PHOTO;X-PARAM=FOO:data:image/png;base64,Zm9v
PHOTO;TYPE=HOME:data:image/jpeg;base64\\,Zm9v
PHOTO:data:image/gif;base64\\,Zm9v
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not change the existing test but add a new one so we keep backwards compatibility...?

Copy link
Contributor Author

@mstilkerich mstilkerich Dec 26, 2022

Choose a reason for hiding this comment

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

I changed this because the vcard here is broken. The comma must be escaped in a Vcard v4 property value according to RFC 6350, section 3.4. The test logic does not detect it because vobject not care about the correct escaping when parsing this text, and the resulting parsed vobject is used for the comparison, not the text itself.

Nonetheless, I think the test data should not be malformed data.

Copy link
Member

@staabm staabm Dec 26, 2022

Choose a reason for hiding this comment

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

I don't know whether existing clients might also send malformed requests?

Or existing scripts relied on this example

Lets see what others think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just so there is no confusion: vobject will continue to parse an unescaped comma here as before, this is just a change in the test data. If you prefer to keep it malformed, no problem. I thought it would be better if the actual test samples are properly escaped as to not confuse the reader. Escaping of the commas in the test data I did only for this reason and is not required concerning my code changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, will it help to get this merged if I revert all unnecessary changes in the tests?

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.

2 participants