T&A 29648: Adds export and import of units and unit categories of a formula question

[matheuszych]

https://mantis.ilias.de/view.php?id=29648

It seems that a unit export/import was never implemented. This PR adds that "feature".

Serializing the unit categories and units and later base64 encoding them, is needed to preserve special characters, such as the German Umlaute.

[matheuszych]

Already reviewed by @thojou.

[kergomard]

Hi @matheuszych

Thank you very much for this PR, but as far as I can see this would be one huge invitation for an attack. We will need to do this without any serialization. Encoding in base64 is perfectly fine, but you will need to create proper strings that can be checked on import before then building the corresponding objects from scratch ensuring nothing bad gets in. I think ideally you set the values in distinct nodes and/or attributes of the xml-structure.

Maybe I'm missing something?

Thank you very much and best, @kergomard

[matheuszych]

Hello @kergomard

i reviewed the implementation again and was able to improve it. Base64 encoding is no longer required. Serialization is still used, but in a much more controlled way, preventing injections by disallowing class serialization. I hope this approach is more acceptable. Alternatively i could use json encoding?

Please review the changes again and share your feedback.

Best regards @matheuszych

[kergomard]

Hi @matheuszych

Thank you very much for the update! Please remove the serialization completely. This should really not be hard using attributes on the corresponding xml-elements. We should really try to not need any serialization and you have already built everything to make it easy to transform the values into proper xml-elements. If I'm missing something, please let me know.

Best, @kergomard

[matheuszych]

Hello @kergomard thank you for your feedback. I removed the serialization.

Best regards, @matheuszych

[kergomard]

Thank you very much for the update @matheuszych !

...and now we come to the sanitation part ;-).

• I'm all in favor of sanitizing all our outputs and not our inputs, so I would be in favor of keeping this as it is, but adding output sanitizing for all relevant fields (please then make sure, that it also works for everything entered to the interface and no double-encoding or similar issues happen.
• I can now create a xss with "alert();]]>" in all fields I tested (Catgory Title, Unit Title, please also test the reference to the Base-Unit).

Thanks again and best, @kergomard

[matheuszych]

Hello @kergomard, I haven’t forgotten about this PR — I was just waiting for the latest minor releases, which should address your second point.

Regarding your first point: as far as I can see, that’s already the case. Am I overlooking something?

I also came across a small bug in the GlobalUnits tab: if nothing was selected and you clicked Import, the page reloaded and defaulted to Local units instead of Global. I fixed this by adjusting the routing.

Best regards @matheuszych

[kergomard]

Hi @matheuszych

With these changes I can still import and display malicious grades. Please see the corresponding files I've sent you.

Best, @kergomard

[kergomard]

Thank you very much @matheuszych !

We are getting there even if I've one more thing:

• [ ] I do not think we can setThrowErrors here and then catch the errors only in one case. This might break quite a few imports. How about adding an additional parameter bool $throw_errors = false to the constructor? Not beautiful, I know, but we are seeing the end of this broken import/export, so ... What do you think?

Best, @kergomard

[matheuszych]

Hello @kergomard ,

The ilQTIParser is technically only used inside of T&A and in the LearningModule so we would be mostly safe. If it would break any imports, my question would mainly be why. They could only break if the XML structure inside of the to be imported file was corrupt, which in any case, should lead to a Exception and prevent the continuation of the importing process.

But since the import/export is to be discontinued in its current form, i believe your proposed solution is the way to go here for now.

Best regards @matheuszych

[kergomard]

Thank you very much @matheuszych !

Merged and picked to ILIAS 10, ILIAS 11, and trunk.