Skip to content

HTML/Markdown components: Script sanitization settings should be separate from Liquid rendering #18768

@BenedekFarkas

Description

@BenedekFarkas

Describe the bug

The current SanitizeHtml settings' hints don't tell the whole story and enabling/disabling it causes potentially unwanted behavior.

  1. The hints only refer to preventing custom scripts, which is true, but it does much more by default. This, in itself is trivial to fix. However:
  2. This setting also governs whether Liquid markup is rendered: SanitizeHtml = false means Liquid is rendered. This prevents a simple use-case where (script) sanitization is wanted, but Liquid should be rendered too.
  3. Regardless of the setting, Liquid code is always validated. This is problematic when Liquid code is included as a code sample (in a <code> block) and that code is invalid, e.g. because it references a Liquid tag, which is not available in the current system.

This affects HtmlBodyPart, HtmlField, MarkdownPart, and MarkdownField.

Orchard Core version

2.2.1 and below (where SanitizeHtml is available).

To Reproduce

  1. Set up site with Blog recipe.
  2. Optional: Add HtmlBodyPart instead of MarkdownBodyPart to the BlogPost content type and modify Content-BlogPost.liquid the render it.
  3. Add any Liquid code in the Markdown/HTML body, e.g. <h2>{{ "Orchard Core uses Liquid!" }}</h2>. It's only rendered as Liquid when HtmlSanitize is false.
  4. Also add {% href "https://orchardcore.net" %} anywhere in the body. It will prevent saving the item, regardless of the SanitizeHtml settings's value.

Expected behavior

  1. SanitizeHtml fields' hints are worded more clearly regarding their behavior.
  2. Having a RenderLiquid setting, which should work independently from SanitizeHtml.
  3. Liquid code should be only be validated, if RenderLiquid is enabled.

Nice to haves later

  1. Liquid code inside a <code> tag should not be rendered/validated (just displayed raw), even if RenderLiquid is enabled. Not sure how easy that is to achieve (in terms of separation of concerns); might be out of scope for this issue.
  2. HtmlMenuItemPart could also support Liquid syntax/rendering, same as HtmlBodyPart and HtmlField.

Design questions

  1. MarkdownBodyPartDisplayDriver (same for Field) states that "The liquid rendering is for backwards compatibility and can be removed in a future version.".
    I removed these notices; there's nothing wrong with MarkdownField/Part being able to render Liquid, especially now that it's not competing with sanitization.
  2. Seeing that MarkdownBodyPart and MarkdownField are both found in the OrchardCore.Markdown module, I suggest moving HtmlField (from OrchardCore.ContentFields) and HtmlMenuItemPart (from OrchardCore.Menu) to OrchardCore.Html. This would allow some DRYing and make it easier to maintain these components (i.e. keep their changes in sync).
    This seems too much hassle for little gain.

Metadata

Metadata

Assignees

Type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions