XSS injection vectors from editor-controlled forms
Closed this issue · 1 comments
kbayliss commented
From @thibaudcolas :
The templates make a lot of use of
|safe, which makes me feel wary at the very least, and will be a source of XSS vulnerabilities when forms are defined via user input (the Wagtail form builder in particular).Testing the current implementation of BaseWagtailFormBuilder and the templates,
- This makes it possible do an XSS injection via the fields’ label
- The buttons label also allows XSS injection (
input.value|safein button.html)- The injection via help_text is prevented by WAGTAILFORMS_HELP_TEXT_ALLOW_HTML – so we’re good, but it’d be nice if similarly usage of
|safefor help text was configurable within the template pack (rather than trusting it’s done upstream).- For sites that make it possible to do fieldsets and legends (with implementations like wagtailstreamforms), I’m pretty sure there would be more injection opportunities.
So – I’d suggest:
- Removing the
|safefor anything that doesn’t strictly need it- For things that do need it, checking whether there might be more bespoke sanitisation we could introduce
- Documenting which things we’d expect implementers of the template pack to sanitise themselves
The current implementation matches how Crispy Forms work out of the box (https://github.com/django-crispy-forms/django-crispy-forms/search?q=%7Csafe), though Crispy Forms doesn't assume the forms are editable via a CMS whereas ours could be.
thibaudcolas commented
I think we should do this for the first release at least for the fields’ labels and button labels. I can see two options:
- Remove the
|safefrom templates in the right spots - Add extra escaping with relevant logic in Python (FormBuilder implementation)