christopher-ramirez / secretary

Take the power of Jinja2 templates to OpenOffice and LibreOffice.
Other
190 stars 48 forks source link

Rendering may fail if autoescape is not enabled in user-provided jinja environment #41

Open liberforce opened 7 years ago

liberforce commented 7 years ago

To render XML, you need a jinja environment with autoescape that evaluates to True, otherwise _render_xml with fail with an expat error if you feed it strings like "m&m's": ExpatError: ExpatError "not well-formed (invalid token)" at line 1, column 3210

You currently force autoescape when no environment is passed, which is good. However, you don't force autoescape when the user provides its own environment.

Now, the problem is that the environment passed by the user may be shared, to produce output for templates of multiple types. For that, the user can provide (since jinja 2.4) in the Environment() constructor an autoescape a function that will return a boolean to tell if escaping is needed, based on the template filename extension. This means you can't just blindly force the autoescape to 'True', as that would break the environment for other (non-XML, non-HTML) template types.

I think the best way to fix this is: in Renderer.render, check the self.environment.autoescape value. If the value is True, or if it's a function that evaluates to True, all is fine. If it's False, or a function that evaluates to False, then raise an error with sensible message

christopher-ramirez commented 7 years ago

Thanks for reporting this. Will take a look into it.

christopher-ramirez commented 7 years ago

Fixed in a134d33 .

liberforce commented 7 years ago

Hi, thanks for your reactivity. I have one problem with your fix though. I'm in a corporate environment, using Ubuntu 14.04, which ships jinja 2.7.2, meaning the autoescape trick won't work :-( .

christopher-ramirez commented 7 years ago

With a134d33 I wrote a workaround for Jinja < 2.9. Please give me your comments about.

I'm thinking that another solution could be to load the XML string into a file like object with its name attribute ending with .xml. That (I think) would force Jinja to autoescape values.

liberforce commented 7 years ago

Hi, I've tried but can't get your development branch installed. I think you forgot to update the setup.py to reflect the current source tree. The MANIFEST.in also contains a reference to the simple_template.odt file that doesn't exist anymore. Is there a way to test the package without installing it with pip ?

liberforce commented 7 years ago

Ok, just found a way to test.

liberforce commented 7 years ago

Ok, I think I understand a bit better how that autoescape feature works in jinja.

The auto-escape overrides was handled by an extension in jinja 2.4, and became a builtin in jinja 2.9. However the autoescape mecanism was already functional in 2.4, with no extension. Only the autoescape overrides needed an extension.

The problem with your last patch is that you check the autoescape function exists for XML, but you create the template from a string, not a file. That means jinja will never check the filetypes marked for escaping. It will instead pass 'None' to the autoescape function, which returns the value of the 'default_for_string' parameter of jinja2.select_autoescape function since 2.9. If the user didn't request auto escaping for strings, the rendering will fail.

So if you want to respect the escaping values set in the environment, you're right, using file-like objects and calling get_template instead of from_string is an option. Jinja would call the autoescape function and guess its corresponding filename, reflecting the user environment. As a result, you wouldn't need to rely on the autoescape overrides mecanism.

christopher-ramirez commented 7 years ago

Thank you for pointing this. You are right! The env.autoescape('.xml') part does nothing at the end. Will have a look for a better method.

christopher-ramirez commented 7 years ago

Just reopening issue

christopher-ramirez commented 7 years ago

A change introduced in the current official branch 0.2.x could be an acceptable solution to the problem addressed here.

See 76cb15e2