Remove raw from various template

[rioug]

What? Why?

• Closes remove usage of raw in Order Cycle Report email template #11801

Remove raw from various template, as they are introducing security issue. It potentially allows user to insert dangerous code in the html pages.
It looks like a lot of it is left over from Spree, so I don't really have any context as to why they used raw, so I removed it where it didn't make sense to me why it was used. Hopefully I didn't break anything.

Other instance of raw usage that reviewers might want to check:

• https://github.com/openfoodfoundation/openfoodnetwork/blame/master/app/views/spree/admin/adjustments/_new_form.html.haml
• https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/views/spree/shared/_line_item_name.html.haml

• openfoodnetwork/app/helpers/spree/admin/navigation_helper.rb

  Line 101 in b2c6551

  text = options[:no_text] ? '' : raw("<span class='text'>#{text}</span>")

What should we test?

• order cycle report email :
  • Create/update supplier name, product name, customer first and last name, with html tag and check they are properly escaped in the order cycle report email.
• home page alert:
  • Create/update a home page alert with content including tags from this list: https://github.com/openfoodfoundation/openfoodnetwork/blob/a30c0008a1d8a271358175de5452b6b7054e0dd4/app/services/trix_scrubber.rb#L4C1-L5C1
    and check they are properly rendered,
  • check any other html tag are properly escaped.
• Printed invoices:
  • Create/update product name, variant name, supplier name, shipping method name with html tag and check they are properly escaped in the printed invoices
• Order summary email:
  • Create/update supplier name, variant name_to_display with and check they are properly escaped in the order summary email

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes
• API changes (V0, V1, DFC or Webhook)
• Technical changes only
• Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

this PR will need to be merge first #11798

[dacook]

Thanks for cleaning this up.
I'm not sure but may have spotted a problem.
Also a couple of minor suggestions.

[dacook]

Should @comment.body be ContentConfig.home_page_alert_html ?

Also, if we have a standard set of allowed tags and attributes, I'd suggest defining these in a constant.

[rioug]

Indeed, I must have been tired. Fixed it.

[dacook]

I think we can use = instead of interpolation here, and below ;)

[rioug]

Nice pick, I cleaned up the file

[dacook]

Great!

But we are still awaiting merge of the dependent PR, so I will move to In Dev until then.

[mkllnk]

Thank you!

[drummer83]

Hi @rioug,
Thanks for this one and the detailed test instructions!

Test cases

I can see the following parts of the app have been modified, so I tried to test them:

• navigation_helper.rb
  • I added some tags to the locale file and they were displayed as text:
    
  • Here is the source:
    
• Order cycle report email
  • I added some tags to supplier name, hub name, product name, SKU, customer first and last name and they were displayed as text in the shopfront as well as in the order cycle report email:
    
    
    
• Home page alert
  • I added some tags to the alert content and only the allowed tags from the TrixScrubber were interpreted as HTML - even though not all of them make sense in this context because they are not within the view port anymore, e.g. <h1>. The others were removed (button in this case):
    
  • Here is the result:
    
  • Here is the source:
    
• Invoices
  • With tags added to supplier name, hub name, product name, SKU, customer first and last name and an adjustment I created all three types of invoices and the tags were all displayed as text. Here is an example (adjustment with tag is not shown here):
    
• Order summary email
  • With tags added to supplier name, hub name, product name, SKU, customer first and last name, adjustment and display name (!= display as) I sent the confirmation mail and the tags were all displayed as text:
    
    

Questions

• Is the definition of text_for_button_link still required? Using a simple search I couldn't find any other calls than the one you deleted in this PR.
• Use TrixScrubber for other super admin created contents as well? These are currently using .html_safe (e.g. ContentConfig.group_signup_detail_html.html_safe). Isn't that a potential risk as well? Like
  • Producer signup pricing table, case studies and detail html
  • Hub signup pricing table, case studies and detail html
  • Group signup pricing table, case studies and detail html

Other findings (out of scope)

• SKU is only displayed if set on variant level. But it can still be set on product level.
• Display as is not shown in order confirmation etc.

Conclusion

• No regressions found. 👍
• Everything working as expected. 👍
• Ready for merging! 🚀
• Would be interesting to hear your thoughts on my questions - thanks! 🙏

Thanks again! Also I have learnt a bit while testing this PR!