wow-rp-addons / LibMSP

The “Mary Sue Protocol” (“MSP” for short) is a simple challenge/response protocol for RP UI add-ons within WoW to communicate with each other and publish text information to other clients (such as RP character names and descriptions).
Creative Commons Zero v1.0 Universal
2 stars 1 forks source link

Use SendAddonMessageLogged to report ToS breaking RP profiles #2

Closed Ellypse closed 6 years ago

Ellypse commented 6 years ago

Patch 8.0 will introduce the SendAddonMessageLogged function, in part to address issues with harassment made using add-on data sent by users, like RP profiles. This function is documented as "Intended for plain text payloads; logged and throttled".

[Edited: Removed original proposal of keeping SendAddonMessage for data transfer as I have now learned we can use SendAddonMessageLogged for all transfers.]

Ellypse commented 6 years ago

For reference, here's the documentation on Wowpedia for SendAddonMessageLogged that I wrote https://wow.gamepedia.com/API_C_ChatInfo.SendAddonMessageLogged

Ellypse commented 6 years ago

Here are the latest information I have on how we can use C_ChatInfo.SendAddonMessageLogged:

Ellypse commented 6 years ago

After having a first try at this, and as expected and documented, SendAddonMessageLogged only accepts printable characters. This means we have to change the separator from \1 and the prefixes like MSP\1 to something printable. I picked ~ as probably being the less used character for the separator. For now my changes doesn't escape this character and causes issues, but we will of course have to escape this character. For the prefixes, I simply removed the \ to have prefixes as MSP1 with no impact.

Also, currently in the beta and PTR client, the CHAT_MSG_ADDON_LOGGED event is not implemented and regular CHAT_MSG_ADDON are fired. Once the new event is implemented we will have to switch to using to appropriate event.

To improve the system and the readability of the logs on Blizzard's end, it might be better to use regular SendAddonMessage when probing the users on mouseover. Only the actual profile data should transit through logs.

Excepted those small issues from that, everything is pretty much working right of the bat 😄

Ellypse commented 6 years ago

Small note: I have added a modified version of ChatThrottleLib to this project that supports sending using SendAddonMessageLogged. Without knowing if the throttling is shared between logged and non-logged functions or if each get their individual throttle, I went with the first one to be sure. Hopefully ChatThrottleLib get's an official update. The changes I have done are completely backward compatible, but if we have to ship this modified version we might want to rename it or something, to make sure we are not impacting other add-ons.

Ellypse commented 6 years ago

The development branch now has a fully working version using the SendAddonMessageLogged methods. The new separator for the field is ~~ and any ~ inside the body of the profile is escaped as !~ before being sent and then un-escaped when received.

The request for informations go through regular SendAddonMessage, so that we are not logging irrelevant data, and only actual profile data goes through SendAddonMessageLogged.

image

Now I'm gonna have to wait for the clients to get the new CHAT_MSG_ADDON_LOGGED event to accept only profile data coming from SendAddonMessageLogged on the receiver's end.

Ellypse commented 6 years ago

Note: Incremented ChatThrotteLib is probably not the brightest idea. Renaming the global reference as this is a very custom modification would be more future proof in case of future updates of the lib.

Ellypse commented 6 years ago

The CHAT_MSG_ADDON_LOGGED event was added in the latest PTR/Beta build. I have updated the changes to support this new event, now fired when receiving messages sent via SendAddonMessageLogged. The library now only accepts profile data if it was received from the CHAT_MSG_ADDON_LOGGED event (requests are still sent via non-logged messages, to avoid spamming logs), ignoring any profile that is not sent through the logged pipes.

I have renamed the global name of our modified ChatThrotteLib to ChatThrotteLibMSP to avoid issues and collisions with the regular versions of the lib, and I have also incremented a few versions number for the protocol, and update the LibMSP_Debug file for the latest changes.

This means this new version of the library is now ready for implementation and testing. I have been running it inside MyRolePlay during the development of the changes, and will no look into implementing it inside Total RP 3.

ghost commented 6 years ago

(Context: This is Bor of XRP, addressing a brief e-mail discussion I've had with Ellypse recently.)

Using ~~ for a separator character is not especially viable, as people tend to use that for profile decoration or their own separator (separating profile sections in plain text, not a protocol separator). Things like "**Fairy Princess**" for example, or "~~~~~~~~" between paragraphs. Either of these would, with ~~ as a separator, get fields cut up in unpleasant and buggy ways.

There are two viable choices that I see: (1) encode (quoted-printable, probably) the parts of the message, as necessary (see Chomp for an example implementation) and keep using \001 as the separator to limit the changes as much as possible, or (2) adopt \127 (or, less viably, \009) as the separator byte instead. These are not filtered by Blizzard for unknown reasons, but we can use that if we wanted.

Per a discussion over Discord, I'll look at throwing together a development version from Ellypse's work with Chomp capabilities.

Ellypse commented 6 years ago

I'm all in for using a different separator, but just for info, I had the ~ character be escaped from the content, so that ~~ in a text is escaped as !~!~ (etc for more occurrences) and then unescaped on the other end, to avoid the issue :)

Of course I'd rather not escape things and use a character that the users cannot input themselves.

ghost commented 6 years ago

I found that when I started actually looking at the code. You were definitely on the right track, but escape characters like that are really tricksy things. The following is mainly for learning purposes, since I have some different ideas in mind to handle it all.

Imagine what happens, for example, if a user ends a field with ...and that's how he got his sword!. MSP would construct a bit of field data like: FF1=[...]sword!~~NF12=More content[...], and in your decoding you'd end up with [...]sword~NF12=More content[...] as part of the FF field, and no content for the NF field.

So let's say you escape the escape character, too. How do you do it? If your first thought was "Well, I'll replace ! by !!, and then ~ by !~.", you're probably thinking two gsubs and that'll break in the same way with certain sequences of escaped characters. You need to do it in one string.gsub, like string.gsub(text, "([%!%~])", "!%1") and in reverse: string.gsub(text, "(%![%!%~])", "%1"). So you do it in a single gsub call, everything's fine now, right?

Unfortunately, this will still fail without further changes to message splitting, as you're running it on every incoming chunk as they come in. What happens if you have an escaped character split over a message boundary? "[...]blah blah !", "~!~ blah blah[...]"? When you decode each message, you'll end up with "[...]blah blah !", "~~ blah blah[...]", which you then reassemble to "[...]blah blah !~~ blah blah[...]" and, whoops, you just truncated the user's field by accident when you parse that.

It's a really nasty business, encoding characters. This is why I think using quoted-printable, where encoding is necessary, is the way to go. It's an ancient encoding method and is rather well-tested. As long as the implementation is sane and we're careful to not split encoded characters (which Chomp does when it's the one to handle message splitting), quoted-printable should be able to handle just about anything that gets thrown at it.

Ellypse commented 6 years ago

Oh wow, yeah that's bad 😅 I'm ashamed I did not see the !~ issue 😖