MUC: Return better error on illegal nick changes

lovetox commented 2 months ago

Environment

ejabberd version: 24.7

Errors from error.log/crash.log

<presence xmlns="jabber:client" id="2e61e2e0-b181-45a8-bca1-fa8b70803ffb" from="batiliho@conference.movim.eu/lovetox2🔫" type="error">
  <c xmlns="http://jabber.org/protocol/caps" hash="sha-1" node="https://gajim.org" ver="oHb81TBTQlH4TRHlgbyUyVWVAIo=" />
  <error type="modify">
    <bad-request xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" />
    <text xml:lang="en" xmlns="urn:ietf:params:xml:ns:xmpp-stanzas">Bad value of attribute 'to' in tag &lt;presence/&gt; qualified by namespace 'jabber:client'</text>
</error>
</presence>

Bug description

the <text> element should give a error message that the client can show to the user. This error will not be useful to the average user.

You could argue that the client should validate the resource before sending it, and in fact the client here in question does it. The problem is that with precis and stringprep, two conflicting standards are in use, and the client cannot find out what the server supports.

badlop commented 2 months ago

This error message seems to be generated in xmpp library, rfc6120.erl and the actual text is in xmpp_codec.erl. All that source code is automatically generated from xmpp_codec.spec, which precisely describes how the presence stanza must be, and complains precisely about the exact problem when decoding it.

If the error message is so technically precise that it is almost useless to the potential destination human at the end of the line... then maybe the corresponding ejabberd module could detect that error before returning it to the client, and replace the text with a more suitable one?

lovetox commented 2 months ago

im asking myself if its even feasible to change this on the server.

i guess you have some base stanza parsing before the stanza hits any module, and there it discovers the to attribute is invalid. seems complicated to still route the stanza through various modules, and discover its a nick change.

Not sure if this is something you normally do.

of course the other solution is here, as the client triggers the nick change, it could track also the answer, and trying to interpret the error.

badlop commented 1 month ago

Right, the stanza is parsed long before reaching mod_muc, so the error message cannot explain explicitly anything about the room nickname.

Furthermore: even if that were possible and done for this specific case you mentioned, then there would be many other cases where the user can introduce invalid data, that clients may not verify, and the server would also report with a generic error message...

I guess we can close this issue as "not planned: won't fix"

processone / ejabberd