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

Fixed poor implementations of vCard 3.0 being rejected by server #415

Closed
wants to merge 1 commit into from
Closed

Conversation

GOinfo-Ltd
Copy link
Contributor

Let's take an example of the BlackBerry 10 OS.

It shares the vCards in the v3 format, but uses the "BASE64" encoding instead of the "B" one. This results in really odd behaviour in the communication between the sabre server and the BB client :

  • contact disappearing from both the server and client
  • contact being duplicated in the client at each and every synchronisation
  • contact losing its name, phone number and/or profile picture
  • ...

This PR effectively fixes that issue for each and every OS10 based BlackBerry device, and I assume will fix every client implementation that uses the "BASE64" encoding instead of "B".

I have tested this patch successfully in both sabre standalone and in Nextcloud 12 and 13.

@evert
Copy link
Member

evert commented Jun 24, 2018

ENCODING=BASE64 is not valid in vCard 3. Supporting broken vCard implementations is kind of a bad idea, because it means that sabre/dav will also serve broken vCards to other clients.

I think you should report the bug first to blackberry and see if they can fix it. If they refuse or don't respond, we can reopen this discussion.

@evert evert closed this Jun 24, 2018
@GOinfo-Ltd
Copy link
Contributor Author

BlackBerry dropped support for OS10 last year.

Any chance in accepting the reception of BASE64 vCards and doing the stripping of the extra 5 chars on the fly?

@evert
Copy link
Member

evert commented Jun 25, 2018

I didn't know!

In that case, I would propose the following:

Change your PR to not remove the error, but instead 'repair' the problem (with the REPAIR option) . Repairing in this case is simply changing the BASE64 into B and setting the warning level to 1 instead of 3 (but only if REPAIR was passed).

sabre/dav will call validate with the repair option by default, so this should fix your problem in a more correct way

@evert evert reopened this Jun 25, 2018
@GOinfo-Ltd
Copy link
Contributor Author

GOinfo-Ltd commented Jun 25, 2018

Will try that tonight.

EDIT:
After having given up last time, I'm trying this again and I have to admit that I have no idea where to actually repair the card properly.

Moreover, this just seems to be more of a trick that would need to be used only by people affected by that problem, and doesn't seem to be PR worthy.

I don't actually know what you, @evert, wanted me to do with that information... Add the REPAIR node if the card came from BB phones ? Add the REPAIR node every time it's V3 and BASE64 ?
Also, I don't know what the repair actually does, and don't want to install a full PHP IDE just to be able to navigate easily through method calls and everything... Lastly, I really don't have a clue how to detect whether the card is V3 with the BASE64 header. I have only a limited knowledge of VCards, PHP, and the sabre library.

@evert evert closed this Sep 4, 2018
@GOinfo-Ltd
Copy link
Contributor Author

I see this has been closed.

Has a fix been made available anywhere ? Or an alternative ?

@msberends
Copy link

Please reopen; there is no fix yet! ENCODING=BASE64 is still not supported! @evert

@evert
Copy link
Member

evert commented Dec 13, 2018

@msberends if you want to provide a good PR for this, it would get accepted.

@GOinfo-Ltd
Copy link
Contributor Author

GOinfo-Ltd commented Dec 24, 2018

image

(edited)

@evert ...Would this work properly ? Do I need to call $this->offsetUnset('ENCODING') ?
Also, for the pull request to work, I'm assuming I have to alter the test file as well, right ?

Like so ?
image
(I don't have a clue where those test functions are called, I'm assuming in your CI script, so I can't make a separate "check repair" function)

EDIT2 : definitely sorry about the PR spam, I didn't know how else to avoid polluting the commit log.

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

3 participants