From fa6daee4e192083fd22165553b3be86cc5fb2d26 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Fri, 10 Mar 2023 01:47:44 +0000 Subject: [PATCH] markdown: Fix use of pure_markdown for non-pure markdown rendering. `render_markdown_path` renders Markdown, and also (since baff121115a1) runs Jinja2 on the resulting HTML. The `pure_markdown` flag was added in 0a99fa2fd669, and did two things: retried the path directly in the filesystem if it wasn't found by the Jinja2 resolver, and also skipped the subsequent Jinja2 templating step (regardless of where the content was found). In this context, the name `pure_markdown` made some sense. The only two callsites were the TOS and privacy policy renders, which might have had user-supplied arbitrary paths, and we wished to handle absolute paths in addition to ones inside `templates/`. Unfortunately, the follow-up of 01bd55bbcbf7 did not refactor the logic -- it changed it, by making `pure_markdown` only do the former of the two behaviors. Passing `pure_markdown=True` after that commit still caused it to always run Jinja2, but allowed it to look elsewhere in the filesystem. This set the stage for calls, such as the one introduced in dedea237456b, which passed both a context for Jinja2, as well as `pure_markdown=True` implying that Jinja2 was not to be used. Split the two previous behaviors of the `pure_markdown` flag, and use pre-existing data to control them, rather than an explicit flag. For handling policy information which is stored at an absolute path outside of the template root, we switch to using the template search path if and only if the path is relative. This also closes the potential inconsistency based on CWD when `pure_markdown=True` was passed and the path was relative, not absolute. Decide whether to run Jinja2 based on if a context is passed in at all. This restores the behavior in the initial 0a99fa2fd669 where a call to `rendar_markdown_path` could be made to just render markdown, and not some other unmentioned and unrelated templating language as well. --- templates/zerver/documentation_main.html | 12 +++---- zerver/lib/markdown/fenced_code.py | 2 +- zerver/lib/templates.py | 46 +++++++++--------------- zerver/tests/test_email_notifications.py | 34 +++++++++++------- 4 files changed, 46 insertions(+), 48 deletions(-) diff --git a/templates/zerver/documentation_main.html b/templates/zerver/documentation_main.html index 9e5358f8eb..98336a695e 100644 --- a/templates/zerver/documentation_main.html +++ b/templates/zerver/documentation_main.html @@ -16,11 +16,11 @@ {% endif %} {% if page_is_policy_center %} - {{ render_markdown_path(sidebar_index, pure_markdown=True) }} + {{ render_markdown_path(sidebar_index) }} {% elif page_is_help_center %} - {{ render_markdown_path(sidebar_index, pure_markdown=True) }} + {{ render_markdown_path(sidebar_index) }} {% else %} - {{ render_markdown_path(sidebar_index, context=api_uri_context, pure_markdown=True) }} + {{ render_markdown_path(sidebar_index, context=api_uri_context) }} {% endif %} {% if not page_is_policy_center %} @@ -36,11 +36,11 @@
{% if page_is_policy_center %} - {{ render_markdown_path(article, pure_markdown=True) }} + {{ render_markdown_path(article) }} {% elif page_is_help_center %} - {{ render_markdown_path(article, context=api_uri_context, help_center=True, pure_markdown=True) }} + {{ render_markdown_path(article, context=api_uri_context, help_center=True) }} {% else %} - {{ render_markdown_path(article, context=api_uri_context, pure_markdown=True) }} + {{ render_markdown_path(article, context=api_uri_context) }} {% endif %}
diff --git a/zerver/lib/markdown/fenced_code.py b/zerver/lib/markdown/fenced_code.py index bfc236ef26..386b2b2d8f 100644 --- a/zerver/lib/markdown/fenced_code.py +++ b/zerver/lib/markdown/fenced_code.py @@ -544,7 +544,7 @@ class FencedBlockPreprocessor(Preprocessor): return txt -def makeExtension(*args: Any, **kwargs: None) -> FencedCodeExtension: +def makeExtension(*args: Any, **kwargs: Any) -> FencedCodeExtension: return FencedCodeExtension(kwargs) diff --git a/zerver/lib/templates.py b/zerver/lib/templates.py index ed3b53daf6..cc0c4c2921 100644 --- a/zerver/lib/templates.py +++ b/zerver/lib/templates.py @@ -12,7 +12,6 @@ from django.contrib.staticfiles.storage import staticfiles_storage from django.template import Library, engines from django.template.backends.jinja2 import Jinja2 from django.utils.safestring import mark_safe -from jinja2.exceptions import TemplateNotFound import zerver.lib.markdown.api_arguments_table_generator import zerver.lib.markdown.api_return_values_table_generator @@ -87,7 +86,6 @@ docs_without_macros = [ def render_markdown_path( markdown_file_path: str, context: Optional[Dict[str, Any]] = None, - pure_markdown: bool = False, integration_doc: bool = False, help_center: bool = False, ) -> str: @@ -96,16 +94,14 @@ def render_markdown_path( Note that this assumes that any HTML in the Markdown file is trusted; it is intended to be used for documentation, not user data.""" - if context is None: - context = {} # We set this global hackishly from zerver.lib.markdown.help_settings_links import set_relative_settings_links - set_relative_settings_links(bool(context.get("html_settings_links"))) + set_relative_settings_links(bool(context is not None and context.get("html_settings_links"))) from zerver.lib.markdown.help_relative_links import set_relative_help_links - set_relative_help_links(bool(context.get("html_settings_links"))) + set_relative_help_links(bool(context is not None and context.get("html_settings_links"))) global md_extensions global md_macro_extension @@ -119,7 +115,9 @@ def render_markdown_path( guess_lang=False, ), zerver.lib.markdown.fenced_code.makeExtension( - run_content_validators=context.get("run_content_validators", False), + run_content_validators=bool( + context is not None and context.get("run_content_validators", False) + ), ), zerver.lib.markdown.api_arguments_table_generator.makeExtension(), zerver.lib.markdown.api_return_values_table_generator.makeExtension(), @@ -130,7 +128,7 @@ def render_markdown_path( zerver.lib.markdown.help_emoticon_translations_table.makeExtension(), zerver.lib.markdown.static.makeExtension(), ] - if "api_url" in context: + if context is not None and "api_url" in context: # We need to generate the API code examples extension each # time so the `api_url` config parameter can be set dynamically. # @@ -162,31 +160,21 @@ def render_markdown_path( md_engine.reset() jinja = engines["Jinja2"] - try: - # By default, we do both Jinja2 templating and Markdown - # processing on the file, to make it easy to use both Jinja2 - # context variables and markdown includes in the file. - assert isinstance(jinja, Jinja2) + assert isinstance(jinja, Jinja2) + if markdown_file_path.startswith("/"): + with open(markdown_file_path) as fp: + markdown_string = fp.read() + else: markdown_string = jinja.env.loader.get_source(jinja.env, markdown_file_path)[0] - except TemplateNotFound as e: - if pure_markdown: - # For files such as /etc/zulip/terms.md where we don't intend - # to use Jinja2 template variables, we still try to load the - # template using Jinja2 (in case the file path isn't absolute - # and does happen to be in Jinja's recognized template - # directories), and if that fails, we try to load it directly - # from disk. - with open(markdown_file_path) as fp: - markdown_string = fp.read() - else: - raise e - API_ENDPOINT_NAME = context.get("API_ENDPOINT_NAME", "") + API_ENDPOINT_NAME = context.get("API_ENDPOINT_NAME", "") if context is not None else "" markdown_string = markdown_string.replace("API_ENDPOINT_NAME", API_ENDPOINT_NAME) - html = md_engine.convert(markdown_string) - rendered_html = jinja.from_string(html).render(context) - return mark_safe(rendered_html) + html = md_engine.convert(markdown_string) + if context is None: + return mark_safe(html) + + return mark_safe(jinja.from_string(html).render(context)) def webpack_entry(entrypoint: str) -> List[str]: diff --git a/zerver/tests/test_email_notifications.py b/zerver/tests/test_email_notifications.py index 97b297ac33..d845edebeb 100644 --- a/zerver/tests/test_email_notifications.py +++ b/zerver/tests/test_email_notifications.py @@ -1,5 +1,6 @@ import random import re +import tempfile from datetime import datetime, timedelta, timezone from email.headerregistry import Address from typing import List, Optional, Sequence @@ -40,23 +41,31 @@ class TestCustomEmails(ZulipTestCase): email_subject = "subject_test" reply_to = "reply_to_test" from_name = "from_name_test" - markdown_template_path = "templates/zerver/emails/email_base_default.source.html" - send_custom_email( - [hamlet], - options={ - "markdown_template_path": markdown_template_path, - "reply_to": reply_to, - "subject": email_subject, - "from_name": from_name, - "dry_run": False, - }, - ) + + with tempfile.NamedTemporaryFile() as markdown_template: + markdown_template.write(b"# Some heading\n\nSome content\n{{ realm_name }}") + markdown_template.flush() + send_custom_email( + [hamlet], + options={ + "markdown_template_path": markdown_template.name, + "reply_to": reply_to, + "subject": email_subject, + "from_name": from_name, + "dry_run": False, + }, + ) self.assert_length(mail.outbox, 1) msg = mail.outbox[0] self.assertEqual(msg.subject, email_subject) self.assert_length(msg.reply_to, 1) self.assertEqual(msg.reply_to[0], reply_to) self.assertNotIn("{% block content %}", msg.body) + self.assertIn("# Some heading", msg.body) + self.assertIn("Zulip Dev", msg.body) + + assert isinstance(msg, EmailMultiAlternatives) + self.assertIn("Some heading", str(msg.alternatives[0][0])) def test_send_custom_email_remote_server(self) -> None: email_subject = "subject_test" @@ -83,9 +92,10 @@ class TestCustomEmails(ZulipTestCase): self.assertEqual(msg.reply_to[0], reply_to) self.assertNotIn("{% block content %}", msg.body) # Verify that the HTML version contains the footer. + assert isinstance(msg, EmailMultiAlternatives) self.assertIn( "You are receiving this email to update you about important changes to Zulip", - str(msg.message()), + str(msg.alternatives[0][0]), ) def test_send_custom_email_headers(self) -> None: