Repository navigation
Generated XEP-0388 encoders serialize child elements in schema-invalid order #4606
Description
The generated XEP-0388 codec serializes several child elements in an order that does not match the
XEP-0388 schema. The underlying causes are a reference-ordering problem in the fast_xml generator
and an incorrect sasl2_continue declaration order in xmpp/specs/xmpp_codec.spec.
thx
Metadata
Metadata
Assignees
Labels
No labels
Activity
badlop commentedon Aug 24, 2026
Nice finding! Can you mention what's the ordering problem in fast_xml and the incorrect declaration order in the xmpp library?
rallep71 commentedon Aug 24, 2026
Hi, if I manage to get round to it today, I’ll put together a more detailed explanation. It’s also linked to the bug I’ve reported – though it isn’t public yet; prefix is looking into it. You could always enquire about it. It should get the go-ahead from you lot first.
rallep71 commentedon Aug 24, 2026
Here are the details.
The first problem is in fxml_gen:group_refs/1. It groups references in a dict
and returns them through dict:to_list/1, so the declaration order from
#elem.refs is lost. make_elem_enc_fun/5 then folds over that result and reverses
the accumulator. The generated sibling order therefore depends on the dict
order instead of the order declared in the codec specification.
With the current generated xep0388.erl this results in, for example:
authentication: inline before mechanism
success: authorization-identifier before additional-data
continue: text, tasks, additional-data
XEP-0388 requires:
mechanism(s), then inline
additional-data, then authorization-identifier
additional-data, tasks, text
The second issue is independent of that generator problem:
sasl2_continue in specs/xmpp_codec.spec currently declares additional-data,
text, tasks. It should declare additional-data, tasks, text.
The source fix should be made in fast_xml and xmpp_codec.spec, followed by
regeneration. Editing generated xep0388.erl alone would only hide the problem.
processone/fast_xml#53 processone/xmpp#112 thx
prefiks commentedon Aug 25, 2026
Looking at those linked pull requests, those changes make sense. This will likely change order inside other stanzas as well, which i don't think this will change handling in ejabberd, but it may possibly be issue in test suite if some tests aren't using xmpp decoder, and are comparing to raw data that could be changed by this.
rallep71 commentedon Aug 25, 2026
Hello and thank you for your reply,
I only realised this whilst integrating and debugging SASL2 and Bind2 into the XMPP client.
I hadn’t considered that this might cause problems with other dependencies in the test suite.
What do you suggest? Right, I’ll look into it a bit more deeply to see how these proposed changes might affect other parts of the system.
prefiks commentedon Aug 25, 2026
Don't worry about this, we will just fix tests when this happen, i just left that as a note to myself or badlop when this will be merged
badlop commentedon Aug 25, 2026
As the fixes are already available in rallep library forks, I'll use them in my ejabberd fork to run the tests, and check if ejabberd tests need some update before merging :)
badlop commentedon Aug 27, 2026
Right, the fixes in fast_xml produce several changes in xmpp when running "make spec xdata".
The good news: ejabberd passes all tests after applying fast_xml and xmpp patches
Just a small note:
It would be great if you can split your fast_xml commit into two:
rallep71 commentedon Aug 27, 2026
Good to hear that ejabberd passes the full test suite with both patches applied.
Yes, that split makes sense. I’ll update processone/fast_xml#53 so the first commit contains the hand-written generator and regression-test changes, while the second contains only the regenerated src/fxmlrpc_codec.erl. I’ll note REBAR=rebar3 make spec as the regeneration command.
rallep71 commentedon Aug 27, 2026
It’s sorted, but my status is still showing as ‘pending’,
Update fast_xml and xmpp with fix to preserve reference order in XML (#…
badlop commentedon Aug 28, 2026
All is merged and tests are passed... I mean the tests that cover 35% of ejabberd source code. The next ejabberd release notes should mention this fix, in case there is somewhere some code that depends on the previous order.
Something like this, does it look correct?
Fixed the ordering of some XML child elements
There was a reference-ordering problem in the
fast_xmlgenerator, it generated XML encoders that could reorder child elements in a different order from the one declared in the codec specification. From now, the order is the one declared in the codec specification. See details in processone/fast_xml#53Additionally there was an incorrect sasl2_continue declaration order in the
xmpperlang library: it declared "additional-data, text, tasks". Now it follows the ordering defined in XEP-0388 schema: "additional-data, tasks, text". See details in processone/xmpp#112Acknowledgments
fast_xmlandxmppXML child orderingrallep71 commentedon Aug 28, 2026
Yes, that looks good to me. It describes both parts of the fix correctly, and mentioning it in the release notes makes sense
in case anything relied on the old generated order. The acknowledgement is fine too. Thanks for merging both changes and
running the ejabberd tests.
THX
dwd commentedon Aug 29, 2026
Erm... I think I probably wrote the XEP-0388 schema, and I'm not entirely sure why I made it a sequence and not xs:all. Quite possibly I just used xs:sequence because I knew about it.
Is there any reason, we think, that the ordering actually makes a difference here?
Put another way, if we changed the schema to remove the ordering requirement, would this be a problem?
(I'll raise this to the XSF's Standards SIG too)
rallep71 commentedon Aug 29, 2026
From my side, I found that the generated encoders did not preserve the order declared in the codec specification. I did not
establish that SASL2 itself depends on that order, or that clients reject a different order.
If the Standards SIG concludes that the child order is not significant, I would have no objection to relaxing the schema. I
would still keep the fast_xml change, as the generator should behave deterministically and follow the codec specification it
is given.
Thanks for taking this to the Standards SIG.
badlop commentedon Sep 24, 2026
What's the status of the XEP modification?
Is the change in processone/xmpp#112 relevant, or maybe it should be reverted before next xmpp/ejabberd releases?
rallep71 commentedon Sep 25, 2026
Hmm, there haven’t been any changes to the standard yet: “Version
1.0.4 (25 August 2025)”. I’ll stick to my proposed changes; whether they’ll be incorporated into the next version of ejabberd is up to processone to decide, and as long as there are no changes to the standard, it simply remains the standard. Anyway.
For my part, I’ve put the implementation on hold for the time being until this has been clarified; this also applies to another fix I’ve submitted, which I won’t go into here. On top of that, it’s not just ejabberd that’s affected – Prosody has similar issues – but I won’t submit the fixes there until everything here has been sorted out.
dwd commentedon Sep 25, 2026
Yeah, sorry, I've not made a PR yet. But I'll try to get that sorted today, and through council next week?
tmolitor-stud-tu commentedon Oct 1, 2026
@dwd no, I don't think that should be a problem and in fact it could also be that the ordering in the schema comes from me :D