Load avatar data over HTTP if available in vCard EXTVAL #3782
Conversation
| @@ -12,6 +12,7 @@ export async function parseVCardResultStanza(iq) { | |||
| fullname: iq.querySelector(':scope > vCard FN')?.textContent, | |||
| image: iq.querySelector(':scope > vCard PHOTO BINVAL')?.textContent, | |||
| image_type: iq.querySelector(':scope > vCard PHOTO TYPE')?.textContent, | |||
| image_url: iq.querySelector(':scope > vCard PHOTO EXTVAL')?.textContent, | |||
There was a problem hiding this comment.
XEP-0054 and XEP-0292 both support PHOTO EXTVAL but funnily enough XEP-0153 VCard-based Avatars discourages it. I'm not sure why.
The <PHOTO/> element SHOULD NOT contain an <EXTVAL/> that points to a URI for the image file.
In any case, the changes look OK to me, but I think we should put result.image first in the if statement below, and then check for result.image_url.
There was a problem hiding this comment.
XEP-0054 and XEP-0292 both support
PHOTO EXTVALbut funnily enough XEP-0153 VCard-based Avatars discourages it. I'm not sure why.
I guess it is to prevent IP leaking. An attacker could use this field to point to their server, and get other users IP. Which could be an issue when you have high privacy expectation.
There was a problem hiding this comment.
Perhaps, but this is the same with HTTP file upload.
My guess is it's because that XEP deals with creating hashes of BINVAL values and advertising those in order to let clients know when they need to update the VCard.
When you're using a URL, presumably you'd change the URL when there's a new avatar so the usage of a hash is not necessary (but then you'd ideally advertise the new image URL similarly to how the hash is advertised).
There was a problem hiding this comment.
Perhaps, but this is the same with HTTP file upload.
yes, and that's why some clients don't display images automatically.
My guess is it's because that XEP deals with creating hashes of BINVAL values and advertising those in order to let clients know when they need to update the VCard.
When you're using a URL, presumably you'd change the URL when there's a new avatar so the usage of a hash is not necessary (but then you'd ideally advertise the new image URL similarly to how the hash is advertised)
Could be because of this too, indeed.
There was a problem hiding this comment.
yes, and that's why some clients don't display images automatically.
This is actually a good feature which I'd like to have in Converse as well.
There was a problem hiding this comment.
I didn't see an issue with IP leaks since many clients already block untrusted domains and i'd assume deployments of Converse would set up content security policy rules. I'm not sure if the same applies with Converse Desktop so that would have to be checked first.
If CSP isn't enough then creating some configuration to allow trusted domains for avatars can be created.
| const vcards = []; | ||
| if (occupant.get('jid')) { | ||
| vcards.push(_converse.state.vcards.get(occupant.get('jid'))); | ||
| } | ||
| vcards.push(_converse.state.vcards.get(occupant.get('from'))); | ||
| vcards.forEach((v) => hash && v && v?.get('image_hash') !== hash && api.vcard.update(v, true)); | ||
| vcards.forEach((v) => (hash || url) && v && (v?.get('image_hash') !== hash || v?.get('image_url') !== url) && api.vcard.update(v, true)); |
There was a problem hiding this comment.
This still doesn't look right, because of the (hash || url) the hash value could be undefined, but then the v?.get('image_hash') !== hash check could still execute (because url is defined), potentially resulting in an unnecessary call to api.vcard.update.
I think it's better to write this logic out.
| vcards.forEach((v) => (hash || url) && v && (v?.get('image_hash') !== hash || v?.get('image_url') !== url) && api.vcard.update(v, true)); | |
| vcards.filter((v) => v).forEach((v) => { | |
| if (hash && v.get('image_hash') !== hash || url && v.get('image_url') !== url) { | |
| api.vcard.update(v, true); | |
| } | |
| }); |
| } | ||
| else if (image_url) { |
There was a problem hiding this comment.
Nitpick: Please put the else on the same line as the curly bracket.
| } | |
| else if (image_url) { | |
| } else if (image_url) { |
| } | ||
| else { |
There was a problem hiding this comment.
Nitpick: Please put the else on the same line as the curly bracket.
| } | |
| else { | |
| } else { |
Its possible to point the user avatar to a URL address and store it in PEP. It then becomes accessible from vcard-temp in the EXTVAL tag.
The same can be done with MUC avatars by setting the EXTVAL tag with the url, but this is not oficially supported by XEP-0486.
Edit: I think handleVCardUpdatePresence() in src/headless/plugins/vcard/utils.js should be modified, but i'm not sure if its only needed for the binary avatars or not since i'm not too familiar with Javascript or Sizzle. Seems to function fine without any changes to it however.