1806630 - XMPP implementation does not respond to all IQ queries

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[guus.der.kinderen]

Steps to reproduce:

The XMPP specifications, in RFC 6120 (section 8.2.3), dictate:

An entity that receives an IQ request of type "get" or "set" MUST reply with an IQ response of type "result" or "error".

The instant messaging client that is part of Thunderbird only responds to IQ requests that it recognizes.

As some servers use (the absence of) IQ responses to detect network connectivity issues, failing to respond to an IQ request can cause the client to be disconnected from a server, or service. An example of such an issue was reported as a bug against the Openfire XMPP server implementation, in https://discourse.igniterealtime.org/t/users-are-kicked-out-from-room-since-openfire-4-7-4-and-monitoring-2-4-0/

To reproduce, manually crafted IQ requests were sent from the server to a user connected to a server called example.org, that also joined a chatroom named test.

The following are four IQ request that were sent. The first and third requests are "IQ Ping" requests, which is a feature that is implemented in Thunderbird. The second and fourth request are non-standard requests that I've made up on the spot. They are not implemented in Thunderbird. The first and second requests are sent by the server to the user directly. The third and fourth request are sent by the server to the occupant-representation of the user in a chatroom (which affects addressing).

<iq from='example.org' to='[e-mail]/73ba7e8e-9cd3-4792-ae00-50dc5acc4cfd' id='stanza1' type='get'>
  <ping xmlns='urn:xmpp:ping'/>
</iq>

<iq from='example.org' to='[e-mail]/73ba7e8e-9cd3-4792-ae00-50dc5acc4cfd' id='stanza2' type='get'>
  <foo xmlns='foobar'/>
</iq>

<iq from='[e-mail]' to='[e-mail]/73ba7e8e-9cd3-4792-ae00-50dc5acc4cfd' id='stanza3' type='get'>
  <ping xmlns='urn:xmpp:ping'/>
</iq>

<iq from='[e-mail]' to='[e-mail]/73ba7e8e-9cd3-4792-ae00-50dc5acc4cfd' id='stanza4' type='get'>
  <foo xmlns='foobar'/>
</iq>

Actual results:

Only the first IQ request was responded to:

<iq type="result" id="stanza1" to="example.org" from="[e-mail]/73ba7e8e-9cd3-4792-ae00-50dc5acc4cfd"/>

For some requests, this was logged:

prpl-jabber: Unhandled IQ get stanza.

Expected results:

Each IQ request should have been responded to, either with a result (as shown in the actual results), or with an error, such as:

<iq type="error" id="stanza3" to="example.org" from="[e-mail]/73ba7e8e-9cd3-4792-ae00-50dc5acc4cfd">
  <error type="cancel">
    <service-unavailable xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" type="cancel"></service-unavailable>
  </error>
</iq>

Note that it was expected that the third request (an IQ Ping request to a chat room occupant representation) was responded to by a 'normal' response, as Thunderbird implements the IQ Ping feature. This did not happen for reasons that I will make the subject of a different ticket.

Comment 1

[guus.der.kinderen]

This issue is related to, but not a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1806631

Comment 2

[guus.der.kinderen]

Attachment: Patch with suggested fix for bug 1806630

This patch ensures that all IQ requests are responded to, if code leading up to the 'unhandled IQ stanza' log message did not yet process the request.

Comment 3

[Patrick Cloke [:clokep]]

I think this is essentially bug 1365044, although you've included much more info here (and a patch!).

Comment 4

[Patrick Cloke [:clokep]]

Looks reasonable overall, I'll try to test it out tomorrow. Thanks!

Comment 5

[guus.der.kinderen]

This indeed seems to be a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1365044

Comment 6

[Patrick Cloke [:clokep]]

(In reply to guus.der.kinderen from comment #5)

This indeed seems to be a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1365044

Thanks for the confirmation. I'll duplicate that into this bug since you've provided more steps to reproduce and provided a patch.

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 9309152 [details] [diff] [review]
Patch with suggested fix for bug 1806630

Review of attachment 9309152 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks! I'm going to upload a new patch with the proper meta-data attached to it (and after running `eslint --fix` for reformatting).

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/92c50dd98b6b
Fix XMPP protocol to respond to unhandled IQ queries. r=clokep

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 9309703 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: Users may be kicked from rooms by servers running openfire servers.
Testing completed (on c-c, etc.): This has been on c-c for a few weeks without any issues (if this caused a bug I'd expect to see it immediately).
Risk to taking this patch (and alternatives if risky): Low, this is responding to some additional commands from the server. It doesn't have unit tests, however.

(This is about to go onto beta so only requesting comm-esr102 uplift. Please shout if I should request approval-comm-beta too.)

Comment 12

[Patrick Cloke [:clokep]]

Also -- thank you so much for the patch Guus (and congrats on your first Thunderbird change! :) ).

Comment 13

[Patrick Cloke [:clokep]]

Comment on attachment 9309703 [details] [diff] [review]
Patch v2

Copied from comment 11:

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: Users may be kicked from rooms by servers running openfire servers.
Testing completed (on c-c, etc.): This has been on c-c for a few weeks without any issues (if this caused a bug I'd expect to see it immediately).
Risk to taking this patch (and alternatives if risky): Low, this is responding to some additional commands from the server. It doesn't have unit tests, however.

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9309703 [details] [diff] [review]
Patch v2

[Triage Comment]
Approved for beta

Comment 15

[Daniel Darnell [:dandarnell]]

Thunderbird 109.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/3108e8b63958

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9309703 [details] [diff] [review]
Patch v2

[Triage Comment]
approved for esr102

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 102.7.1:
https://hg.mozilla.org/releases/comm-esr102/rev/c2ef38526e97