powered by Jive Software

Smack WebSocket implementation does not convert <open> element into <stream> element unless it self-closes

Smack version: 4.5.0-alpha1-SNAPSHOT (4.3.4-856-g0a6c21982-master 2020-09-07)

XMPP message text received in phase openFrameSent:

<open xmlns="urn:ietf:params:xml:ns:xmpp-framing" xmlns:stream="http://etherx.jabber.org/streams" version="1.0" from="REMOVED" id="REMOVED" xml:lang="en"></open>

Relevant code from OkHttpWebSocket:

                case openFrameSent:
                    if (isOpenElement(text)) {
                        // Converts the <open> element received into <stream> element.
                        openStreamHeader = getStreamFromOpenElement(text);

Relevant code for AbstractWebSocket.getStreamFromOpenElement(String):

    protected static String getStreamFromOpenElement(String openElement) {
        String streamElement = openElement.replaceFirst("\\A<open ", "<stream ")
                                          .replace("urn:ietf:params:xml:ns:xmpp-framing", "jabber:client")
                                          .replaceFirst("/>\\s*\\z", ">");
        return streamElement;
    }

This method returns:

<stream xmlns="jabber:client" xmlns:stream="http://etherx.jabber.org/streams" version="1.0" from="REMOVED" id="REMOVED" xml:lang="en"></open>

This results in a parse error, which causes the connection to fail.

The relevant specification from RFC 7395:

3.3.1. Framed XML Stream

The start of a framed XML stream is marked by the use of an opening “stream header”, which is an <open/> element with the appropriate attributes and namespace declarations (see Section 3.3.2). The attributes of the <open/> element are the same as those of the <stream/> element defined for the ‘http://etherx.jabber.org/streams’ namespace RFC6120 and with the same semantics and restrictions.

Although the RFC references the <open/> element in self-closing style, it also references the <stream/> element in self-closing style. It would not make much sense to have a self-closing <stream/> element, therefore I don’t think the specification implies a requirement that the <open/> element must self-close, because it doesn’t explicitly specify it as self-closing.

It seems AbstractWebSocket.getStreamFromOpenElement(String) has a bug because it does not handle this scenario.

1 Like

Hi @carlton.whitehead,

Thanks for pointing this out :grinning:
While formulating the AbstractWebsocket.getStreamOpen(String) method, only self-closing open elements were considered.
The reason to discard <open...></open> kind of structure is that it adds unnecessary bytes to the wire which is not a great idea as far as I understand. Normally, XMPP servers would want to avoid that.
This motivates me to ask you, could you tell us, which XMPP server served you with the described open element?

XMPP servers like Openfire and Prosody do stick to RFC 7395 references by generating self-enclosing open elements.

Support for your websocket element can still be made available in smack-websocket but since there are very few servers providing websocket open element in that form, I am not sure if the reason is enough to convince Smack’s maintainer @Flow to incorporate changes here.

2 Likes

Smack wouldn’t be standards compliant if it does not support <open></open>. I have a private branch with some changes for the websocket code. I guess I will take care of this too (provided I don’t forget about it). But this definelty will need fixing before this code can be released.

2 Likes

I worked around this in a local build with the following patch applied:

diff --git a/smack-websocket/src/main/java/org/jivesoftware/smack/websocket/impl/AbstractWebSocket.java b/smack-websocket/src/main/java/org/jivesoftware/smack/websocket/impl/AbstractWebSocket.java
index d7e76e230..e623639de 100644
diff --git a/smack-websocket/src/main/java/org/jivesoftware/smack/websocket/imp
l/AbstractWebSocket.java b/smack-websocket/src/main/java/org/jivesoftware/smack
/websocket/impl/AbstractWebSocket.java
index d7e76e230..e623639de 100644
--- a/smack-websocket/src/main/java/org/jivesoftware/smack/websocket/impl/AbstractWebSocket.java
+++ b/smack-websocket/src/main/java/org/jivesoftware/smack/websocket/impl/AbstractWebSocket.java
@@ -31,7 +31,8 @@ public abstract class AbstractWebSocket {
     protected static String getStreamFromOpenElement(String openElement) {
         String streamElement = openElement.replaceFirst("\\A<open ", "<stream ")
                                           .replace("urn:ietf:params:xml:ns:xmpp-framing", "jabber:client")
-                                          .replaceFirst("/>\\s*\\z", ">");
+                                          .replaceFirst("/>\\s*\\z", ">")
+                                          .replaceFirst("</open>\\z", "");
         return streamElement;
     }
 

It removes a trailing </open> tag. I found that by replacing the </open> tag with a </stream> tag that it triggered the usual behavior of closing the stream. This is admittedly a hack so I could proceed to experiment with other parts of the websocket implementation.

I wonder if using an XML parser instead of string manipulation would be a better choice here for robustness’ sake.

1 Like

That is totally to be expected :wink:.

1 Like

Perhaps it’s time to file a Jira ticket?

1 Like