search

matrix-org / matrix-bifrost

General purpose bridging with a variety of backends including libpurple and xmpp.js
Apache License 2.0
162 stars 31 forks source link

Stanza attribute escaping looks like a foot gun #99

Open Zash opened 4 years ago

Zash commented 4 years ago

https://github.com/matrix-org/matrix-bifrost/blob/f2f28bb7f7b069b8491c822e54ad0eeab6d8d114/src/xmppjs/Stanzas.ts#L23-L27

This, escaping fields in a setter instead of during serialization, scares me.

Half-Shot commented 4 years ago

Hmm, suspect this came about as a result of dodgy matrix displaynames. This should definitely be applied at the point of constructing a JID rather than setting it on the Stanza.

Zash commented 4 years ago

I see now some fields are encoded when the XML strings are composed, while others, like from as show above, are not. Being consistent with these things seems like the safest thing. Probably too easy to make mistakes otherwise, opening for injection attacks or having things accidentally double escaped.