maranda / metronome

Metronome IM, lightweight xmpp server with advanced microblogging features.
https://metronome.im
Other
165 stars 40 forks source link

Lowercase vcard element name in vcard-temp #526

Closed woj-tek closed 4 years ago

woj-tek commented 4 years ago

Metronome responds with <vcard xmlns='vcard-temp'> (vcard being lowercase) to the request

Example:

<!--   2020-10-02T15:19:35Z   >>>>   -->
<iq type='get' to='conversations@conference.siacs.eu/gabriel' id='5682A0CF-17E4-4295-9347-AA328B665EEB'>
<vCard xmlns='vcard-temp'/>
</iq>

<!--   2020-10-02T15:19:37Z   <<<<   -->
<iq xmlns='jabber:client' type='result' to='wojtek@…' from='conversations@conference.siacs.eu/gabriel' id='5682A0CF-17E4-4295-9347-AA328B665EEB'>
<vcard xmlns='vcard-temp'>
<PHOTO>
<TYPE>image/jpeg</TYPE>
<BINVAL>/9j/4AAQSkZJRgABAQAAAQABAAD/4gIoSUNDX1BST0ZJTEUAAQEAAAIYAAAAAAIQAABtbnRyUkdC
IFhZWiAAAAAAAAAAAAAAAABhY3NwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAA9tYAAQAA
AADTLQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
…

There was a related fix: https://github.com/maranda/metronome/commit/24d18f738983b7b6eeb3e545efddd56e109128a6 but it seems that most likely there is an old <vcard/> element (in lowercase) in the database and in that case you are only replacing <PHOTO/> element without updating the vcard element name.

The user is using Metronome 3.14.1.

maranda commented 4 years ago

@woj-tek the commit you referenced infact addresses Metronome itself creating a malformed vcard element when doing PEP Avatar to vCard format conversion, should the user have a previously corrupted vcard, the only way to rectify that is wiping the old vcard and resubmitting it.

woj-tek commented 4 years ago

Shouldn't Metronome correct that on behave of the user? Or not reply with incorrect payload?

maranda commented 4 years ago

@woj-tek user input "is sanitized" into https://github.com/maranda/metronome/blob/master/plugins/mod_vcard.lua#L211 and https://github.com/maranda/metronome/blob/master/plugins/mod_vcard.lua#L212 alas if a "vcard" element is sent it will throw an error.

There won't be a correction in this case because the bug was introduced by a mistake in conversion's code, which was rectified very soon, and it's pointless to put additional code to correct a just a few malformed vcards created by Metronome during conversion. As Metronome won't accept a user submitted vcard which element is "vcard" and not "vCard".

Hope it's clear.

hantu85 commented 4 years ago

While the solution is good for vCard publication I think that there is an issue with this solution, as if user will use only PEP User Avatar for updating it's avatar, it will not fix vCard element name and will keep sending vCard with invalid element name vcard when queried for vCard. Due to that vcard may be there and may be sent for a long time. (How often do you update your vCard?)

maranda commented 4 years ago

@hantu85 I was clear enough, about what caused the invalid element name. It was a mistake in the conversion code, and that case happened only when a user didn't have legacy vcard already. Adding additional code to correct a few corrupted vcards doesn't make any sense I'm not sure why are you guys insisting.

The users who have a corrupted vcards just need to wipe the vcard in the store or request to, to their admin, there's nothing to solve as the offending code was corrected months ago.

hantu85 commented 4 years ago

I'm not insisting. I'm just pointing out that the issue, while fixed, requires user's action to actually fix entries in the database - action which is not done very often.

It is up to you to decide if you consider that a thing to fix or not.

maranda commented 4 years ago

No it's not because the amount of corrupted vcards can't justify such a change :)