lua-xmlrpc icon indicating copy to clipboard operation
lua-xmlrpc copied to clipboard

String is not escaped.

Open nomadalex opened this issue 14 years ago • 5 comments

String within '<' and/or '&' and/or ' ' is not escaped, so if it happend, xml parsing will occur an error, these characters are not allowed in xml without escaped.

And lua table which is an array will be coverted to struct type,I think this is inconvenient for the user, at least I do not want to add "['*type'] = 'array'" to the table each time when it is an array.

Because I met it, I hacked in some quick support that works. The code is here: https://github.com/ifreedom/lua-xmlrpc/commit/4cfb1792233cd1ea061b4492194b661a1116d048

nomadalex avatar Nov 24 '10 06:11 nomadalex

Escaping is a good thing, but in that case it should escape all special characters.

The API change to convert tables to arrays is unacceptable, because it changes the API massively and will break a lot of existing code, structs are more common in XML-RPCs APIs than arrays (from my experience, your mileage may vary), and the "try to be clever" API may be confusing to new users.

I'd take a patch for the escaping if it covered a more complete set of entities. I will not apply the struct/array patch.

timn avatar Nov 24 '10 14:11 timn

Thanks, but in the original, a table like "{ 'aaaaaa', 'bbbbb' }" which is an array will be converted to a struct, but when it decoded, the key will be converted to string type, not number ( { 'aaaaaa', 'bbbbb' } => { ['1']='aaaaaa', ['2']='bbbbb' } ). This is against the habit of thinking of. Please think about it.

Well, putting aside this, base64 type and dateTime type are needed to be supported, because they are basic types of xml-rpc. My advice is here: https://github.com/ifreedom/lua-xmlrpc/commit/76dbc62510b8b73c07ed8298f58e3beb7895914d

nomadalex avatar Nov 25 '10 08:11 nomadalex

Keys are strings because that's the way it's defined for struct member names (cf. http://www.xmlrpc.com/spec/). And since there is a way to encode arrays, and I have used this myself and it is sufficiently easy and without surprises (principle of least surprise), I think we're good here.

The base64/dateTime part is good, I'll add that. You shouldn't remove number though, it is required.

In general, you should make multiple commits for adding multiple features or modifying different files or part of files. This way it is easier to cherry-pick from you. For now, I will add selected parts manually, and will look for a way to encode more entities.

Although you might find the array idea useful, I strongly suggest sticking to the original lua-xmlrpc API (once the subset of your proposed changes has been incorporated) for two reasons: you can easily adapt your code to newer versions, and you can more easily use packages available out-of-the-box (e.g. on a Linux distribution).

timn avatar Nov 25 '10 16:11 timn

Em, thanks, I will do multiple commits in the future.

But, I can not find 'formats.number' in function 'toxml.number', so I think it does not need.

In fact, I use this library before I read 'README', and it made me surprise, so I read sources and made the array idea. And I think this more conforms to the principle of least surprise. An "array" be converted to an array, it is natural, not right? Of course, I know you do not want to change any existing code, so I suggest this is just for reference. I hope you can think more about it.

nomadalex avatar Nov 26 '10 11:11 nomadalex

Hi timn! It not need to covered a more complete set of entities.

On http://www.xmlrpc.com/spec :

What characters are allowed in strings? Non-printable characters? Null characters? Can a "string" be used to hold an arbitrary chunk of binary data?

Any characters are allowed in a string except < and &, which are encoded as < and &. A string can be used to encode binary data.

https://github.com/ifreedom/lua-xmlrpc/commit/a62ef6352b76361278ba19c3ff2cc3147da96c6b

nomadalex avatar Jan 04 '11 12:01 nomadalex