mapeditor / tiled

Flexible level editor
https://www.mapeditor.org/
Other
11.15k stars 1.75k forks source link

Option for Tiled to use CData for more complex Property Texts #3644

Open Hanmac opened 1 year ago

Hanmac commented 1 year ago

Is your feature request related to a problem? Please describe. Currently the data inside the Property is escaped for proper XML with " But this isn't very readable (or friendly for systems like git ?)

Describe the solution you'd like I would like to use CDATA for these sections to make them more readable.

Describe alternatives you've considered The files are generated by Tiled, so and i don't know if tiled even would support CDATA while reading?

Before:

<property name="reward">[{&quot;type&quot;: &quot;randomCard&quot;,&quot;colors&quot;: [&quot;Blue&quot;], &quot;count&quot;: 5 }, {&quot;type&quot;: &quot;item&quot;,&quot;count&quot;: 1,&quot;probability&quot;:0.2,&quot;itemName&quot;: &quot;Iron Armor&quot;},
{
&quot;type&quot;: &quot;card&quot;,
&quot;probability&quot;: 1,
&quot;count&quot;: 1,
&quot;cardName&quot;: &quot;Staff of the Mind Magus&quot;}]</property>

After:

<property name="reward">
<![CDATA[
  [
     {"type": "randomCard","colors": ["Blue"], "count": 5 },
     {"type": "item","count": 1,"probability":0.2,"itemName": "Iron Armor"},
     {"type": "card","probability": 1,"count": 1,"cardName": "Staff of the Mind Magus"}
  ]
]]>
</property>
cafeTechne commented 1 year ago

This would open it to a buffer overflow exploit which could lead to privelege escalation, no?

See here:

https://forums.oracle.com/ords/apexds/post/comment-cdata-pi-buffer-overflow-maximum-size-is-4096-bytes-9053

Best practices:

https://www.eccouncil.org/cybersecurity-exchange/penetration-testing/buffer-overflow-attack-types/

eishiya commented 1 year ago

This would be a breaking format change (edit: for existing parsers, not necessarily for Tiled), as some XML libraries (such as RapidXML) treat CDATA as an additional node, requiring changes to parsers that read in string properties. Given how well the "class" change went over with users and parser devs despite that being ostensibly a small change and similarly optional, I wonder if this might not be best saved for 2.0, if it's implemented at all.

I personally think that extensive JSON should be stored in external files, though. Tiled is not a tool for editing JSON or other significant data strings IMO.

Edit: Given that git also can't preview Tiled map/tileset data in a meaningful way anyway, I also don't think git readability should be a major concern. Despite being XML, TMX files aren't all that human-readable xP And if someone does write a git differ plugin for Tiled XML files, they can parse the XML string data too.

bjorn commented 1 year ago

In case there are compatibility concerns, I think it would be nice to support this as an opt-in. Going over the QXmlStreamReader API is looks to me like Tiled should already support reading properties saved as such, so at least for Tiled it's likely a backwards compatible change.

@cafeTechne I doubt this would open a buffer overflow exploit, since that would mean Qt's XML reader has a serious security issue already.