templates: Be strict about undefined variables (#5201)

Author: reivilibre

crates/cli/src/commands/server.rs

@@ -160,8 +160,14 @@ impl Options {
         )?;
 
         // Load and compile the templates
-        let templates =
-            templates_from_config(&config.templates, &site_config, &url_builder).await?;
+        let templates = templates_from_config(
+            &config.templates,
+            &site_config,
+            &url_builder,
+            // Don't use strict mode in production yet
+            false,
+        )
+        .await?;
         shutdown.register_reloadable(&templates);
 
         let http_client = mas_http::reqwest_client();

crates/cli/src/commands/templates.rs

@@ -65,8 +65,14 @@ impl Options {
                     &account_config,
                     &captcha_config,
                 )?;
-                let templates =
-                    templates_from_config(&template_config, &site_config, &url_builder).await?;
+                let templates = templates_from_config(
+                    &template_config,
+                    &site_config,
+                    &url_builder,
+                    // Use strict mode in template checks
+                    true,
+                )
+                .await?;
                 templates.check_render(clock.now(), &mut rng)?;
 
                 Ok(ExitCode::SUCCESS)

crates/cli/src/commands/worker.rs

@@ -52,8 +52,14 @@ impl Options {
         )?;
 
         // Load and compile the templates
-        let templates =
-            templates_from_config(&config.templates, &site_config, &url_builder).await?;
+        let templates = templates_from_config(
+            &config.templates,
+            &site_config,
+            &url_builder,
+            // Don't use strict mode on task workers for now
+            false,
+        )
+        .await?;
 
         let mailer = mailer_from_config(&config.email, &templates)?;
         test_mailer_in_background(&mailer, Duration::from_secs(30));

crates/cli/src/util.rs

@@ -232,6 +232,7 @@ pub async fn templates_from_config(
     config: &TemplatesConfig,
     site_config: &SiteConfig,
     url_builder: &UrlBuilder,
+    strict: bool,
 ) -> Result<Templates, anyhow::Error> {
     Templates::load(
         config.path.clone(),
@@ -240,6 +241,7 @@ pub async fn templates_from_config(
         config.translations_path.clone(),
         site_config.templates_branding(),
         site_config.templates_features(),
+        strict,
     )
     .await
     .with_context(|| format!("Failed to load the templates at {}", config.path))

crates/handlers/src/test_utils.rs

@@ -176,6 +176,8 @@ impl TestState {
             workspace_root.join("translations"),
             site_config.templates_branding(),
             site_config.templates_features(),
+            // Strict mode in testing
+            true,
         )
         .await?;
 

crates/templates/src/context/branding.rs

@@ -58,9 +58,9 @@ impl Object for SiteBranding {
     fn get_value(self: &Arc<Self>, name: &Value) -> Option<Value> {
         match name.as_str()? {
             "server_name" => Some(self.server_name.clone().into()),
-            "policy_uri" => self.policy_uri.clone().map(Value::from),
-            "tos_uri" => self.tos_uri.clone().map(Value::from),
-            "imprint" => self.imprint.clone().map(Value::from),
+            "policy_uri" => Some(Value::from(self.policy_uri.clone())),
+            "tos_uri" => Some(Value::from(self.tos_uri.clone())),
+            "imprint" => Some(Value::from(self.imprint.clone())),
             _ => None,
         }
     }

crates/templates/src/lib.rs

@@ -17,7 +17,7 @@ use camino::{Utf8Path, Utf8PathBuf};
 use mas_i18n::Translator;
 use mas_router::UrlBuilder;
 use mas_spa::ViteManifest;
-use minijinja::Value;
+use minijinja::{UndefinedBehavior, Value};
 use rand::Rng;
 use serde::Serialize;
 use thiserror::Error;
@@ -71,6 +71,9 @@ pub struct Templates {
     vite_manifest_path: Utf8PathBuf,
     translations_path: Utf8PathBuf,
     path: Utf8PathBuf,
+    /// Whether template rendering is in strict mode (for testing,
+    /// until this can be rolled out in production.)
+    strict: bool,
 }
 
 /// There was an issue while loading the templates
@@ -151,6 +154,7 @@ impl Templates {
         translations_path: Utf8PathBuf,
         branding: SiteBranding,
         features: SiteFeatures,
+        strict: bool,
     ) -> Result<Self, TemplateLoadingError> {
         let (translator, environment) = Self::load_(
             &path,
@@ -159,6 +163,7 @@ impl Templates {
             &translations_path,
             branding.clone(),
             features,
+            strict,
         )
         .await?;
         Ok(Self {
@@ -170,6 +175,7 @@ impl Templates {
             translations_path,
             branding,
             features,
+            strict,
         })
     }
 
@@ -180,6 +186,7 @@ impl Templates {
         translations_path: &Utf8Path,
         branding: SiteBranding,
         features: SiteFeatures,
+        strict: bool,
     ) -> Result<(Arc<Translator>, Arc<minijinja::Environment<'static>>), TemplateLoadingError> {
         let path = path.to_owned();
         let span = tracing::Span::current();
@@ -205,6 +212,15 @@ impl Templates {
             span.in_scope(move || {
                 let mut loaded: HashSet<_> = HashSet::new();
                 let mut env = minijinja::Environment::new();
+                // Don't allow use of undefined variables
+                env.set_undefined_behavior(if strict {
+                    UndefinedBehavior::Strict
+                } else {
+                    // For now, allow semi-strict, because we don't have total test coverage of
+                    // tests and some tests rely on if conditions against sometimes-undefined
+                    // variables
+                    UndefinedBehavior::SemiStrict
+                });
                 let root = path.canonicalize_utf8()?;
                 info!(%root, "Loading templates from filesystem");
                 for entry in walkdir::WalkDir::new(&root)
@@ -275,6 +291,7 @@ impl Templates {
             &self.translations_path,
             self.branding.clone(),
             self.features,
+            self.strict,
         )
         .await?;
 
@@ -524,6 +541,8 @@ mod tests {
             translations_path,
             branding,
             features,
+            // Use strict mode in tests
+            true,
         )
         .await
         .unwrap();

templates/base.html

@@ -28,7 +28,7 @@
     {{ captcha.head() }}
   </head>
   <body>
-    <div class="layout-container{% if consent_page %} consent{% endif %}">
+    <div class="layout-container{% if consent_page is defined %} consent{% endif %}">
       {% block content %}{% endblock content %}
       {% include "components/footer.html" %}
     </div>

templates/components/footer.html

@@ -7,27 +7,27 @@
 -#}
 
 <footer class="legal-footer">
-  {%- if branding.policy_uri or branding.tos_uri -%}
+  {%- if branding.policy_uri is not none or branding.tos_uri is not none -%}
     <nav>
-      {%- if branding.policy_uri -%}
+      {%- if branding.policy_uri is not none -%}
         <a href="{{ branding.policy_uri }}" referrerpolicy="no-referrer" title="{{ _('branding.privacy_policy.alt') }}" class="cpd-link" data-kind="primary">
           {{- _("branding.privacy_policy.link") -}}
         </a>
       {%- endif -%}
 
-      {%- if branding.policy_uri and branding.tos_uri -%}
+      {%- if branding.policy_uri is not none and branding.tos_uri is not none -%}
         <div class="separator" aria-hidden="true">•</div>
       {%- endif -%}
 
-      {%- if branding.tos_uri -%}
+      {%- if branding.tos_uri is not none -%}
         <a href="{{ branding.tos_uri }}" referrerpolicy="no-referrer" title="{{ _('branding.terms_and_conditions.alt') }}" class="cpd-link" data-kind="primary">
           {{- _("branding.terms_and_conditions.link") -}}
         </a>
       {%- endif -%}
     </nav>
   {%- endif -%}
 
-  {%- if branding.imprint -%}
+  {%- if branding.imprint is not none -%}
     <p class="imprint">{{ branding.imprint }}</p>
   {%- endif -%}
 </footer>

templates/form_post.html

@@ -15,7 +15,7 @@ <h1 class="title">{{ _("common.loading") }}</h1>
     </div>
   </header>
 
-  <form method="post" class="flex flex-col"{% if redirect_uri %} action="{{ redirect_uri }}"{% endif %}>
+  <form method="post" class="flex flex-col"{% if redirect_uri is not none %} action="{{ redirect_uri }}"{% endif %}>
     {% for key, value in params|items %}
       <input type="hidden" name="{{ key }}" value="{{ value }}" />
     {% endfor %}