Comment on attachment 85035 [details] [review]
Patch for wocky

Review of attachment 85035 [details] [review]:
-----------------------------------------------------------------

What's the worst that can happen?

RFC 6120 §8.1.2.1 says:

   2.  When the server generates a stanza on its own behalf for delivery
       to the client from the server itself, the stanza MUST include a
       'from' attribute whose value is the bare JID

so this is not RFC-compliant XMPP. (Interop > RFC pedantry in situations where it doesn't harm security, though.)

Also,

   4.  A server MUST NOT send to the client a stanza without a 'from'
       attribute if the stanza was not generated by the server on its
       own behalf

so if the server obeys §8.1.2.1.4, the worst that can happen with this change is that we see a stanza generated by the server on behalf of our account and we wrongly accept it as a valid stanza generated by the server itself, which is what we need to do to fix this interop bug.

So, I'm OK with this change in principle.

::: wocky/wocky-c2s-porter.c
@@ +859,5 @@
>          goto finally;
>      }
>  
> +  /** If we sent an IQ to the server allow the server to omit 'to'
> +   * in the reply. */

omit 'from', surely?

/** is for doc-comments consumed by gtk-doc, not for ordinary comments.

Please do mention that the Facebook server now does this (preferably with "see fd.o #68829" or the URL of the bug) and that we would normally interpret this as coming from "the server on behalf of our account". Something like this:

  /* If we sent an IQ to the server itself, allow it to
   * omit 'from' in its reply, which is normally used
   * for messages from the server on behalf of our own
   * account (as of 2013-09-02, the Facebook server does
   * this). See fd.o #68829 */

The server is still returning valid disco information about itself, not about our account, I hope?

Thanks will update patch. 


Disco requests/responses attached for future reference.

Facebook's beta server for me returns the following on a disco request

<!-- sent -->
<iq id="327894361669" to="beta.chat.facebook.com" type="get" xmlns="jabber:client">
	<query xmlns="http://jabber.org/protocol/disco#info"/>
</iq>

<!-- received -->
<iq id="327894361669" type="result" xmlns="jabber:client">
	<query xmlns="http://jabber.org/protocol/disco#info">
		<identity category="server" type="Facebook XMPP"/>
		<feature var="http://jabber.org/protocol/commands"/>
		<feature var="http://jabber.org/protocol/chatstates"/>
		<feature var="vcard-temp"/>
	</query>
</iq>

requesting disco info chat.facebook.com is the same

<!-- sent -->
<iq id="67671632942" to="chat.facebook.com" type="get" xmlns="jabber:client">
	<query xmlns="http://jabber.org/protocol/disco#info"/>
</iq>

<!-- received -->
<iq id="67671632942" type="result" xmlns="jabber:client">
	<query xmlns="http://jabber.org/protocol/disco#info">
		<identity category="server" type="Facebook XMPP"/>
		<feature var="http://jabber.org/protocol/commands"/>
		<feature var="http://jabber.org/protocol/chatstates"/>
		<feature var="vcard-temp"/>
	</query>
</iq>



I know Facebook rolls out services gradually, and some of my user's are hitting the same error that I am currently getting with the beta service so I suspect this will hit most user's soon.

Created attachment 85203 [details] [review]
Updated patch

Thanks, I merged your patch to the Wocky 'master' and 'gabble-0.18' branches.

Should be in telepathy-gabble 0.18.1 and 0.19.0.

I just released telepathy-gabble 0.18.1 including this fix.

(In reply to comment #2)
> Facebook's beta server for me returns the following on a disco request
> 
> <!-- sent -->
> <iq id="327894361669" to="beta.chat.facebook.com" type="get"

Are you explicitly connecting to beta.chat.facebook.com, or getting directed there via resource binding? I notice that Gabble's special cases for Facebook are all based on the stream server being "chat.facebook.com"...

I was explicitly using beta.chat.facebook.com in order to reproduce this issue and for the the IQ stanzas I posted.

Other reporters on KDE forums who appeared to have this issue were using standard chat.facebook.com. One user verified this patch solved their problem.

Reopening to fix in older branches - I need a backport for Debian anyway.

Created attachment 85494 [details] [review]
Add a regression test for #68829

---

This passes on 0.18 and master. I haven't tried 0.16 yet.

(In reply to comment #9)
> Created attachment 85494 [details] [review]
> Add a regression test for #68829
> 
> ---
> 
> This passes on 0.18 and master. I haven't tried 0.16 yet.

Also works fine on 0.16 when I cherry-pick David's Wocky patch. I'd like to do a 0.16.7 release with this: hopefully the Debian release team will let me ship that, rather than messing about with a patch series (which is annoying because Wocky is a submodule).

Comment on attachment 85494 [details] [review]
Add a regression test for #68829

Review of attachment 85494 [details] [review]:
-----------------------------------------------------------------

++

This was merged in the 0.16.x "old-stable" branch too, it's just waiting for a release (0.16.7).

The same code might need similar modifications for Bug #39057.