You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Niklas Gustavsson <ni...@protocol7.com> on 2009/07/09 23:48:55 UTC
[Vysper] Error stanzas return too much of the original
Hi
I'm having lots of fun hacking on Vysper :-)
In investigating to troubles with my integration tests for XMPP Ping,
I generated the following traffic between the client and the server:
Client:
<iq id="BuvmS-2" to="vysper.org" from="test@vysper.org"
type="get"><ping xmlns="urn:xmpp:ping"></ping></iq>
Server:
<iq to="test@vysper.org" from="vysper.org" type="error" id="BuvmS-2">
<iq id="BuvmS-2" to="vysper.org" from="test@vysper.org" type="get">
<ping xmlns="urn:xmpp:ping"></ping>
</iq>
<error type="cancel">
<service-unavailable
xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"></service-unavailable>
</error>
</iq>
Looking at the examples from XEP-0199, we should not include the
original <iq> element in the error response, but merely the <ping>
element, so something like:
<iq to="test@vysper.org" from="vysper.org" type="error" id="BuvmS-2">
<ping xmlns="urn:xmpp:ping"></ping>
<error type="cancel">
<service-unavailable
xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"></service-unavailable>
</error>
</iq>
This also seems to agree with some examples from RFC 3920, for example here:
http://xmpp.org/rfcs/rfc3920.html#bind
Should we change this?
/niklas
Re: [Vysper] Error stanzas return too much of the original
Posted by Bernd Fondermann <bf...@brainlounge.de>.
Niklas Gustavsson wrote:
> On Mon, Jul 13, 2009 at 6:19 PM, Bernd Fondermann<bf...@brainlounge.de> wrote:
>>> Yeah, I read this as well as also makes the interpretation that we
>>> should include the complete stanza, but examples seems to contradict
>>> that.
>> What kind of contradiction is it?
>> (I must admit, my mental svn up failed when I wanted to get me to HEAD
>> on this discussion.)
>
> :-)
>
>> All examples I see are completely without error echos.
>> I didn't find a single example for an error stanza containing the whole
>> or only a part of the original. Can you point me to one?
>
> The examples I refer to are those in the RFC and XMPP Ping specs. For
> example, in http://www.ietf.org/rfc/rfc3920.txt page 39 you will find
> some example. And in http://xmpp.org/extensions/xep-0199.html, you can
> find examples in each of the use cases (4.X). All of these include the
> original stanza, without the outermost iq element.
>
>>> For one, Smack can not handle our current error stanzas as showed by
>>> the integration tests I've written for XMPP Ping (not yet committed
>>> since they do not currently pass). In these, when sending an error,
>>> Smack will not be able to locate the <error> element.
>> Why would Smack try and locate it? What happens if it can't? Does it
>
> Smack provides a IQ.getError() method for retrieving details on the
> error. In interacting with the current Vysper, this will return null.
> With the proposed patch, it will be correctly populated.
>
>> At xsf.org, there are two lists,
>> JDev@jabber.org and
>> Standards@xmpp.org
>> which coul prove helpful if we are in doubt.
>
> Yeah, that's a good suggestion. At least I think the phrasing in 9.3.1
> should be clarified. I'll ask over there. Given the spec examples
> however, I think we should change the Vysper implementation for now.
Now I get it. You are right. Sorry for the delay in comprehension.
It makes sense to not echo the outer stanza frame because the reference
is already clear from the answer. However, if the malicious part is in
the 'frame', why the error is returned is not clear without knowing the
original. But maybe these kinds of errors would better result in a
stream error, I don't know.
+1 to apply the proposed patch.
Thanks for your patience,
Bernd
Re: [Vysper] Error stanzas return too much of the original
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Mon, Jul 13, 2009 at 6:19 PM, Bernd Fondermann<bf...@brainlounge.de> wrote:
>> Yeah, I read this as well as also makes the interpretation that we
>> should include the complete stanza, but examples seems to contradict
>> that.
>
> What kind of contradiction is it?
> (I must admit, my mental svn up failed when I wanted to get me to HEAD
> on this discussion.)
:-)
> All examples I see are completely without error echos.
> I didn't find a single example for an error stanza containing the whole
> or only a part of the original. Can you point me to one?
The examples I refer to are those in the RFC and XMPP Ping specs. For
example, in http://www.ietf.org/rfc/rfc3920.txt page 39 you will find
some example. And in http://xmpp.org/extensions/xep-0199.html, you can
find examples in each of the use cases (4.X). All of these include the
original stanza, without the outermost iq element.
>> For one, Smack can not handle our current error stanzas as showed by
>> the integration tests I've written for XMPP Ping (not yet committed
>> since they do not currently pass). In these, when sending an error,
>> Smack will not be able to locate the <error> element.
>
> Why would Smack try and locate it? What happens if it can't? Does it
Smack provides a IQ.getError() method for retrieving details on the
error. In interacting with the current Vysper, this will return null.
With the proposed patch, it will be correctly populated.
> At xsf.org, there are two lists,
> JDev@jabber.org and
> Standards@xmpp.org
> which coul prove helpful if we are in doubt.
Yeah, that's a good suggestion. At least I think the phrasing in 9.3.1
should be clarified. I'll ask over there. Given the spec examples
however, I think we should change the Vysper implementation for now.
/niklas
Re: [Vysper] Error stanzas return too much of the original
Posted by Bernd Fondermann <bf...@brainlounge.de>.
Niklas Gustavsson wrote:
>> I cannot emphasize enough that the examples are not normative. In fact,
> they often contain errors.
>
> Agreed, but when other hints are missing, it's the best we got I guess.
>
>
>> The entity that generates an error stanza SHOULD include the
>> original XML sent so that the sender can inspect and, if
>> necessary, correct the XML before attempting to resend.
>
> Yeah, I read this as well as also makes the interpretation that we
> should include the complete stanza, but examples seems to contradict
> that.
What kind of contradiction is it?
(I must admit, my mental svn up failed when I wanted to get me to HEAD
on this discussion.)
All examples I see are completely without error echos.
I didn't find a single example for an error stanza containing the whole
or only a part of the original. Can you point me to one?
>
>> What would be our motivation to change this behaviour?
>
> For one, Smack can not handle our current error stanzas as showed by
> the integration tests I've written for XMPP Ping (not yet committed
> since they do not currently pass). In these, when sending an error,
> Smack will not be able to locate the <error> element.
Why would Smack try and locate it? What happens if it can't? Does it
> One can probably
> argue that this is a bug in Smack, but given the examples in both the
> RFC and the XMPP Ping spec, I would lean on the problem being with us
> :-)
Probably. :-)
At xsf.org, there are two lists,
JDev@jabber.org and
Standards@xmpp.org
which coul prove helpful if we are in doubt.
Bernd
Re: [Vysper] Error stanzas return too much of the original
Posted by Niklas Gustavsson <ni...@protocol7.com>.
> I cannot emphasize enough that the examples are not normative. In fact,
they often contain errors.
Agreed, but when other hints are missing, it's the best we got I guess.
> The entity that generates an error stanza SHOULD include the
> original XML sent so that the sender can inspect and, if
> necessary, correct the XML before attempting to resend.
Yeah, I read this as well as also makes the interpretation that we
should include the complete stanza, but examples seems to contradict
that.
> What would be our motivation to change this behaviour?
For one, Smack can not handle our current error stanzas as showed by
the integration tests I've written for XMPP Ping (not yet committed
since they do not currently pass). In these, when sending an error,
Smack will not be able to locate the <error> element. One can probably
argue that this is a bug in Smack, but given the examples in both the
RFC and the XMPP Ping spec, I would lean on the problem being with us
:-)
/niklas
Re: [Vysper] Error stanzas return too much of the original
Posted by Bernd Fondermann <bf...@brainlounge.de>.
Niklas Gustavsson wrote:
> On Thu, Jul 9, 2009 at 11:48 PM, Niklas Gustavsson<ni...@protocol7.com> wrote:
>> This also seems to agree with some examples from RFC 3920, for example here:
>> http://xmpp.org/rfcs/rfc3920.html#bind
I cannot emphasize enough that the examples are not normative. In fact,
they often contain errors.
Nevertheless, I agree that I cannot find an example where the error
stanza contains the original.
That's the related section in RFC3920 which is implemented currently:
>>>>
9.3.1. Rules
The following rules apply to stanza-related errors:
o <ommitted/>
o The entity that generates an error stanza SHOULD include the
original XML sent so that the sender can inspect and, if
necessary, correct the XML before attempting to resend.
<<<<
I interpret "the original XML" as "the original stanza", because
everything else would making this inspection difficult.
I don't think what we return right now is too much or inconsistent, but
I don't want to be a bonehead. ;-)
What would be our motivation to change this behaviour?
Bernd
>
> Here's a proposed patch for this:
> Index: src/main/java/org/apache/vysper/xmpp/server/response/ServerErrorResponses.java
> ===================================================================
> --- src/main/java/org/apache/vysper/xmpp/server/response/ServerErrorResponses.java (revision
> 791956)
> +++ src/main/java/org/apache/vysper/xmpp/server/response/ServerErrorResponses.java (working
> copy)
> @@ -104,7 +104,6 @@
> }
>
> StanzaBuilder responseBuilder =
> StanzaBuilder.createDirectReply(stanza, true, "error");
> -
> fillErrorStanza(stanza, type, errorCondition, errorText,
> errorLang, errorConditionElement, responseBuilder);
>
> return responseBuilder.getFinalStanza();
> @@ -112,7 +111,9 @@
>
> private void fillErrorStanza(XMPPCoreStanza stanza,
> StanzaErrorType type, StanzaErrorCondition errorCondition, String
> errorText, String errorLang, XMLElement errorConditionElement,
> StanzaBuilder responseBuilder) {
> // inline incoming stanza as of RFC 3920 9.3.1
> - responseBuilder.addPreparedElement(stanza);
> + for(XMLElement innerElement : stanza.getInnerElements()) {
> + responseBuilder.addPreparedElement(innerElement);
> + }
>
> // error element
> responseBuilder.startInnerElement("error")
>
>
> /niklas
>
Re: [Vysper] Error stanzas return too much of the original
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Thu, Jul 9, 2009 at 11:48 PM, Niklas Gustavsson<ni...@protocol7.com> wrote:
> This also seems to agree with some examples from RFC 3920, for example here:
> http://xmpp.org/rfcs/rfc3920.html#bind
Here's a proposed patch for this:
Index: src/main/java/org/apache/vysper/xmpp/server/response/ServerErrorResponses.java
===================================================================
--- src/main/java/org/apache/vysper/xmpp/server/response/ServerErrorResponses.java (revision
791956)
+++ src/main/java/org/apache/vysper/xmpp/server/response/ServerErrorResponses.java (working
copy)
@@ -104,7 +104,6 @@
}
StanzaBuilder responseBuilder =
StanzaBuilder.createDirectReply(stanza, true, "error");
-
fillErrorStanza(stanza, type, errorCondition, errorText,
errorLang, errorConditionElement, responseBuilder);
return responseBuilder.getFinalStanza();
@@ -112,7 +111,9 @@
private void fillErrorStanza(XMPPCoreStanza stanza,
StanzaErrorType type, StanzaErrorCondition errorCondition, String
errorText, String errorLang, XMLElement errorConditionElement,
StanzaBuilder responseBuilder) {
// inline incoming stanza as of RFC 3920 9.3.1
- responseBuilder.addPreparedElement(stanza);
+ for(XMLElement innerElement : stanza.getInnerElements()) {
+ responseBuilder.addPreparedElement(innerElement);
+ }
// error element
responseBuilder.startInnerElement("error")
/niklas