Bug 68829 - [PATCH] Cannot connect to Facebook
Summary: [PATCH] Cannot connect to Facebook
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-02 00:56 UTC by David Edmundson
Modified: 2013-09-18 17:58 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch for wocky (986 bytes, patch)
2013-09-02 00:56 UTC, David Edmundson
Details | Splinter Review
Updated patch (1.14 KB, patch)
2013-09-04 16:15 UTC, David Edmundson
Details | Splinter Review
Add a regression test for #68829 (2.67 KB, patch)
2013-09-09 12:13 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Edmundson 2013-09-02 00:56:21 UTC
Created attachment 85035 [details] [review]
Patch for wocky

Right now connecting to the new Facebook server we send a disco request to chat.facebook.com. The new server responds omitting the from from in the reply.

This causes gabble to reject the reply and then disconnect because we had no DISCO reply.

Attached patch fixes this.

--

The patch could also be done by using

+stanza_is_from_server(self, should_be_from)

in the if clause below. 
I chose this way to be more explicit, but the other way is maybe simpler.
Comment 1 Simon McVittie 2013-09-02 11:36:36 UTC
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?
Comment 2 David Edmundson 2013-09-04 15:21:35 UTC
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.
Comment 3 David Edmundson 2013-09-04 16:15:15 UTC
Created attachment 85203 [details] [review]
Updated patch
Comment 4 Guillaume Desmottes 2013-09-06 10:12:57 UTC
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.
Comment 5 Guillaume Desmottes 2013-09-06 10:39:58 UTC
I just released telepathy-gabble 0.18.1 including this fix.
Comment 6 Simon McVittie 2013-09-06 16:15:39 UTC
(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"...
Comment 7 David Edmundson 2013-09-06 16:18:37 UTC
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.
Comment 8 Simon McVittie 2013-09-09 12:02:14 UTC
Reopening to fix in older branches - I need a backport for Debian anyway.
Comment 9 Simon McVittie 2013-09-09 12:13:04 UTC
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.
Comment 10 Simon McVittie 2013-09-09 12:25:50 UTC
(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 11 Guillaume Desmottes 2013-09-09 13:04:06 UTC
Comment on attachment 85494 [details] [review]
Add a regression test for #68829

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

++
Comment 12 Simon McVittie 2013-09-18 17:58:28 UTC
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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.