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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
169e20b
Unescape also semicolon in URI Property
mstilkerich Dec 25, 2022
34e3ddd
Consider image mime types case insensitive in VCardConverter
mstilkerich Dec 25, 2022
4d89b2e
Fix: Remove X-ADDRESSBOOKSERVER-KIND=individual property in convert
mstilkerich Dec 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 28 additions & 18 deletions lib/Property/Uri.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Sabre\VObject\Property;

use Sabre\VObject\Parameter;
use Sabre\VObject\Parser\MimeDir;
use Sabre\VObject\Property;

/**
Expand Down Expand Up @@ -62,32 +63,41 @@ public function parameters(): array
*/
public function setRawMimeDirValue(string $val): void
{
// Normally we don't need to do any type of unescaping for these
// properties, however, we've noticed that Google Contacts
// For VCard4, we need to unescape comma, backslash and semicolon (and newline). (RFC6350, 3.4)
//
// However, we've noticed that Google Contacts
// specifically escapes the colon (:) with a backslash. While I have
// no clue why they thought that was a good idea, I'm unescaping it
// anyway.
//
// Good thing backslashes are not allowed in urls. Makes it easy to
// assume that a backslash is always intended as an escape character.
if ('URL' === $this->name) {
$regex = '# (?: (\\\\ (?: \\\\ | : ) ) ) #x';
$matches = preg_split($regex, $val, -1, PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY);
$newVal = '';
foreach ($matches as $match) {
switch ($match) {
case '\:':
$newVal .= ':';
break;
default:
$newVal .= $match;
break;
}
$escapeColon = ('URL' === $this->name) ? '| : ' : '';

$regex = '# (?: (\\\\ (?: \\\\ ' . $escapeColon . '| N | n | ; | , ) ) ) #x';
$matches = preg_split($regex, $val, -1, PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY);
$newVal = '';
foreach ($matches as $match) {
switch ($match) {
case '\\\\':
$newVal .= '\\';
break;
case '\;':
$newVal .= ';';
break;
case '\,':
$newVal .= ',';
break;
case '\:':
$newVal .= ':';
break;
default:
$newVal .= $match;
break;
}
$this->value = $newVal;
} else {
$this->value = strtr($val, ['\,' => ',']);
}

$this->value = $newVal;
}

/**
Expand Down
6 changes: 5 additions & 1 deletion lib/VCardConverter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,10 @@ protected function convertProperty(Component\VCard $input, Component\VCard $outp
if ('GROUP' === strtoupper($property->getValue())) {
$newProperty = $output->createProperty('KIND', 'GROUP');
}
if ('INDIVIDUAL' === strtoupper($property->getValue())) {
// Individual is implicit, so we skip it.
return;
}
break;
case 'X-ADDRESSBOOKSERVER-MEMBER':
$newProperty = $output->createProperty('MEMBER', $property->getValue());
Expand Down Expand Up @@ -336,7 +340,7 @@ protected function convertUriToBinary(Component\VCard $output, Property\Uri $new
unset($value);

$newProperty['ENCODING'] = 'b';
switch ($mimeType) {
switch (strtolower($mimeType)) {
case 'image/jpeg':
$newProperty['TYPE'] = 'JPEG';
break;
Expand Down
25 changes: 25 additions & 0 deletions tests/VObject/Property/UriTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,29 @@ public function testAlwaysEncodeUriVCalendar(): void
$output = Reader::read($input)->serialize();
$this->assertStringContainsString('URL;VALUE=URI:http://example.org/', $output);
}

public function testUriUnescapedProperly(): void
{
// The colon should normally not be escaped in URL, but Google Contacts does it and
// vobject contains a workaround for it
$input = <<<VCF
BEGIN:VCARD
VERSION:4.0
PRODID:-//Thunderbird.net/NONSGML Thunderbird CardBook V83.6//EN-GB
UID:5cdac90f-cb4c-4c1c-ad66-fe8591bdf3a2
FN:vCard 4 Contact with image
EMAIL:[email protected]
REV:20221222T213003Z
PHOTO:data:image/JPEG\;base64\,/9j/4AAQSkZJRgABAQAAYABgAAD/2wBDAAgGBgcGBQgHB
wcJCQgKDBQNDAsLDBkSEw8UHRofHh0aHBwgJC4nICIsIxwcKDcpLDAxNDQ0Hyc5PTgyPC4zNDL/
wAALCAABAAEBAREA/8QAFAABAAAAAAAAAAAAAAAAAAAAAv/EABQQAQAAAAAAAAAAAAAAAAAAAAD
/2gAIAQEAAD8AL//Z
item1.URL:http\://www.example.com/hello?world
item1.X-ABLabel:
END:VCARD
VCF;
$output = Reader::read($input);
$this->assertSame('http://www.example.com/hello?world', (string) $output->URL);
$this->assertSame('data:image/JPEG;base64,/9j/4AAQSkZJRgABAQAAYABgAAD/2wBDAAgGBgcGBQgHBwcJCQgKDBQNDAsLDBkSEw8UHRofHh0aHBwgJC4nICIsIxwcKDcpLDAxNDQ0Hyc5PTgyPC4zNDL/wAALCAABAAEBAREA/8QAFAABAAAAAAAAAAAAAAAAAAAAAv/EABQQAQAAAAAAAAAAAAAAAAAAAAD/2gAIAQEAAD8AL//Z', (string) $output->PHOTO);
}
}
33 changes: 20 additions & 13 deletions tests/VObject/VCardConverterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ public function testConvert30to40(): void
TEL;PREF=1;TYPE=HOME:+1 555 666 777
ITEM1.TEL:+1 444 555 666
ITEM1.X-ABLABEL:CustomLabel
PHOTO;TYPE=HOME:data:image/jpeg;base64,Zm9v
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 •
edited
Loading

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 •
edited
Loading

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?

PHOTO;X-PARAM=FOO:data:image/png;base64\\,Zm9v
PHOTO:http://example.org/foo.png
KIND:ORG
END:VCARD
Expand All @@ -66,9 +66,9 @@ public function testConvert40to40(): void
VERSION:4.0
FN:Steve
TEL;PREF=1;TYPE=HOME:+1 555 666 777
PHOTO:data:image/jpeg;base64,Zm9v
PHOTO:data:image/gif;base64,Zm9v
PHOTO;X-PARAM=FOO:data:image/png;base64,Zm9v
PHOTO:data:image/jpeg;base64\\,Zm9v
PHOTO:data:image/gif;base64\\,Zm9v
PHOTO;X-PARAM=FOO:data:image/png;base64\\,Zm9v
PHOTO:http://example.org/foo.png
END:VCARD

Expand All @@ -79,9 +79,9 @@ public function testConvert40to40(): void
VERSION:4.0
FN:Steve
TEL;PREF=1;TYPE=HOME:+1 555 666 777
PHOTO:data:image/jpeg;base64,Zm9v
PHOTO:data:image/gif;base64,Zm9v
PHOTO;X-PARAM=FOO:data:image/png;base64,Zm9v
PHOTO:data:image/jpeg;base64\\,Zm9v
PHOTO:data:image/gif;base64\\,Zm9v
PHOTO;X-PARAM=FOO:data:image/png;base64\\,Zm9v
PHOTO:http://example.org/foo.png
END:VCARD

Expand Down Expand Up @@ -193,9 +193,9 @@ public function testConvert40to30(): void
PRODID:foo
FN:Steve
TEL;PREF=1;TYPE=HOME:+1 555 666 777
PHOTO:data:image/jpeg;base64,Zm9v
PHOTO:data:image/gif,foo
PHOTO;X-PARAM=FOO:data:image/png;base64,Zm9v
PHOTO:data:image/JPEG\\;base64\\,Zm9v
PHOTO:data:image/gif\\,foo
PHOTO;X-PARAM=FOO:data:image/png;base64\\,Zm9v
PHOTO:http://example.org/foo.png
KIND:ORG
END:VCARD
Expand Down Expand Up @@ -409,7 +409,14 @@ public function testConvertIndividualCard(): void
$vcard
);

$input = $output;
$input = <<<IN
BEGIN:VCARD
VERSION:3.0
X-ADDRESSBOOKSERVER-KIND:INDIVIDUAL
END:VCARD

IN;

$output = <<<OUT
BEGIN:VCARD
VERSION:4.0
Expand Down