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

CardDAV: rewrite vCard 4.0 in CardDAV GET #4880

Merged
merged 2 commits into from
Apr 15, 2024
Merged

Conversation

rsto
Copy link
Member

@rsto rsto commented Apr 3, 2024

This works around a bug that had caused Cyrus to write bogus version 4.0 vCards to disk. The issue was that embedded binary data in version 4.0 must be encoded as a data: URI, but it was written as a version 3.0 b-encoded value. The bug is fixed in b8a879c

This patch changes CardDAV GET to always rewrite a version 4.0 vCard before sending the response to the client, even if the vCard already is having version 4.0. As a result, clients get correctly encoded property values for both v3.0 and v4.0.

rsto added 2 commits April 3, 2024 17:33
This works around a bug that had caused Cyrus to write bogus
version 4.0 vCards to disk. The issue was that embedded binary
data in version 4.0 must be encoded as a data: URI, but it
was written as a version 3.0 b-encoded value.
The bug is fixed in b8a879c

This patch changes CardDAV GET to always rewrite a version
4.0 vCard before sending the response to the client, even if
the vCard already is having version 4.0. As a result, clients
get correctly encoded property values for both v3.0 and v4.0.
Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

LGTM assuming that CI passes

@elliefm
Copy link
Contributor

elliefm commented Apr 4, 2024

@rsto I think we should be backporting this one (and the fix in #4877) to the stable branches, yeah? But I'm not sure since you've tagged it do-not-merge. Skimming old release notes, it looks like we've had some kind of vCard 4.0 support since ~3.6, but I'm not sure if the bug dates back that far or was recent.

@rsto
Copy link
Member Author

rsto commented Apr 5, 2024

@elliefm We should backport PR4877. I just created #4881 to do so, the patch didn't apply cleanly so I had to pull in another commit for backporting. The JMAPContacts and Carddav tests now pass for me locally, let's see what CI says about it.

The bug got introduced in 2018 in afc3527

I tagged #4880 as Do-Not-Merge because it is more a workaround than a final solution. A real solution would be to fix the data on-disk, but we haven't come up with tooling for that, yet.
Note that all this only is relevant for installations that already use the JMAP Contacts API, which is experimental and its data model will change significantly when the RFC standards are published at IETF.

@rsto rsto removed the Do Not Merge label Apr 15, 2024
@rsto rsto merged commit 0425439 into master Apr 15, 2024
2 checks passed
@rsto rsto deleted the carddav_rewrite_v4card_on_get branch April 15, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants