Commit 6909d308 authored by Kazuhiko Shiozaki's avatar Kazuhiko Shiozaki

erp5_crm: fix EmailDocument_viewAttachmentListRenderer for renderjs UI.

also open a new window / tab.
parent d220132e
Pipeline #7910 failed with stage
in 0 seconds
<tal:block tal:define="information_list here/getAttachmentInformationList"> <tal:block tal:define="information_list here/getAttachmentInformationList">
<ul> <ul>
<li tal:repeat="information information_list"> <li tal:repeat="information information_list">
<a tal:content="information/filename | information/uid" tal:attributes="href python:'getAttachmentData?index:int=%s' % information['index']"/> <a target="_blank" tal:content="information/filename | information/uid" tal:attributes="href python:'%s/getAttachmentData?index:int=%s' % (context.absolute_url(), information['index'])"/>
(<tal:block tal:content="information/content_type" />) (<tal:block tal:content="information/content_type" />)
</li> </li>
</ul> </ul>
......
  • @kazuhiko what is the goal of the target="_blank" ?

    Please note that this is a security hole and should not be used as is.

  • Maybe you want to use the download attribute?

  • Hmm, I prefer displaying in the browser for displayable attachments such as images. Shall I put rel="noopener" attribute with target="blank" ? (ref. https://developers.google.com/web/tools/lighthouse/audits/noopener)

  • Here the link is not cross-origin, so is it a problem after all ?

    I think we can consider this HTML as trusted as we strip all scripts it in getAttachmentData.

  • Generally, I think we should try to keep the ERP5 UI visible when displaying attachments. This means using *viewers" to show them:

    • show image directly in the mail as img tag
    • use pdfjs inside ERP5 UI for pdf
    • use ckeditor inside ERP5 UI for html
    • If the attachment mime type is unknown, then download it.

    We now have all needed tools to achieve this.

  • I think we can consider this HTML as trusted as we strip all scripts it in getAttachmentData.

    Adding another layer of protection is not a wasted time. Escaping HTML is full of traps.

    See how the Google search page failed from time to time.

  • Thanks @romain you are right. I should have said something like "it's not so bad, because we strip unsafe HTML", but yes, blindly trusting our server side html stripping would be foolish. I think this is code from CMF, maybe there was fixes in CMF/Plone that we did not backport. In other words, we are using some "not really maintained" library for this purpose, so we should definitely not trust it. But still, it removes <script> and <img onerror= so it's better than nothing.

    That video was nice. They recommend sanitizing HTML in the browser, using a <template> element, because javascript is not enabled in <template> element (plus something against <noscript> in templates. I think they also mention https://github.com/cure53/DOMPurify and https://google.github.io/closure-library/api/goog.soy.data.SanitizedHtml.html . The ckeditor gadget does something like this ?

  • I don't know if/how ckeditor sanitize the HTML currently.

    But I like the fact that the HTML is displayed in an iframe, as it reduce the unexpected side effects on the rest of the page.

Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment