candy icon indicating copy to clipboard operation
candy copied to clipboard

JID escaping/unescaping

Open benlangfeld opened this issue 9 years ago • 4 comments

There's a whole bunch of calls to Candy.Util.unescapeJid() scattered across the Candy code-base and it's difficult to follow precisely what they're each for.

There are the following options:

  1. Rely on Strophe JS to properly escape/unescape JIDs. This seems the most obvious, but I'm not sure Strophe actually does so, especially since it's difficult to know generally exactly which attributes will contain a JID except to/from.
  2. Use unescaped JIDs within Candy until such a point as we hit the wire. This would mean unescaping JIDs for inclusion in events, and escaping them again in actions. Plugin authors would be responsible for escaping JIDs when using Strophe directly to send stanzas.
  3. Use escaped JIDs within Candy until we display. This eases some of the effort plugin authors have to do to interact with Strophe, but increases their effort if they ever display a JID (or part of one). It's also semantically wrong, since escaping is purely a transport problem.
  4. Use a JID model (perhaps this one) instead of strings to make it abundantly obvious that what we have is a JID and only allow it to be converted to a string using unescapedJid() or escapedJid() functions so that it's always clear what kind of string representation we're dealing with (and never escape/unescape multiple times).

@mweibel / @pstadler My preference is for 2 or 4. Thoughts?

benlangfeld avatar Aug 08 '14 17:08 benlangfeld

I actually worked on this on the refactoring branch, maybe we could find the responsible commit. I did it like you said on (2). I think this is the lightest option, but probably the cleanest would be (4).

Relying on Strophe is not an option as it doesn't do it for us. Maybe some plugins do it but yeah.. not all of them are particularly great IMO.

mweibel avatar Aug 08 '14 19:08 mweibel

The relevant commit is 68eeb5cb9b0aeca1042db5fd39f544a51fed1d64

benlangfeld avatar Aug 08 '14 19:08 benlangfeld

I think the best thing to do would be to ensure all tests:

  • Event: provide an escaped JID in incoming stanzas and assert that events include the unescaped version
  • Action: provide an unescaped JID as an argument and assert the wire receives the escaped version

Then we can apply your changes from that commit and fix any issues that show up as a result.

benlangfeld avatar Aug 08 '14 20:08 benlangfeld

Yep, that sounds reasonable (thanks for finding the commit btw)

mweibel avatar Aug 12 '14 06:08 mweibel