templates: Be strict about undefined variables

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 %}

templates/pages/index.html

@@ -19,7 +19,7 @@ <h1 class="title">{{ _("app.human_name") }}</h1>
       </div>
     </header>
 
-    {% if current_session %}
+    {% if current_session is not none %}
       <p class="cpd-text-body-md-regular">
         {{ _("mas.navbar.signed_in_as", username=current_session.user.username) }}
       </p>

templates/pages/register/password.html

@@ -49,7 +49,7 @@ <h1 class="title">{{ _("mas.register.create_account.heading") }}</h1>
       <input {{ field.attributes(f) }} class="cpd-text-control" type="password" autocomplete="new-password" required />
     {% endcall %}
 
-    {% if branding.tos_uri %}
+    {% if branding.tos_uri is not none %}
       {% call(f) field.field(label=_("mas.register.terms_of_service", tos_uri=branding.tos_uri), name="accept_terms", form_state=form, inline=true, class="my-4") %}
         <div class="cpd-form-inline-field-control">
           <div class="cpd-checkbox-container">

templates/pages/upstream_oauth2/do_register.html

@@ -26,7 +26,7 @@ <h1 class="title">
         </p>
       </div>
     </header>
-  {% elif upstream_oauth_provider.human_name %}
+  {% elif upstream_oauth_provider.human_name is not none %}
     <header class="page-heading">
       <div class="icon">
         {{ icon.user_profile_solid() }}
@@ -55,9 +55,9 @@ <h1 class="title">
     </header>
   {% endif %}
 
-  {% if upstream_oauth_provider.human_name %}
+  {% if upstream_oauth_provider.human_name is not none %}
     <section class="upstream-oauth2-provider-account">
-      {% if upstream_oauth_provider.brand_name %}
+      {% if upstream_oauth_provider.brand_name is not none %}
         {{ logo(brand=upstream_oauth_provider.brand_name, class="brand") }}
       {% else %}
         {{ icon.user_profile() }}
@@ -67,7 +67,7 @@ <h1 class="title">
         <h3 class="provider">
           {{- _("mas.upstream_oauth2.register.provider_name", human_name=upstream_oauth_provider.human_name) -}}
         </h3>
-        {% if upstream_oauth_link.human_account_name %}
+        {% if upstream_oauth_link.human_account_name is not none %}
           <p class="account">
             {{- upstream_oauth_link.human_account_name -}}
           </p>
@@ -147,7 +147,7 @@ <h3 class="provider">
           <input {{ field.attributes(f) }} class="cpd-text-control" type="text" value="{{ imported_display_name }}" readonly />
 
           <div class="cpd-form-message cpd-form-help-message">
-            {% if upstream_oauth_provider.human_name %}
+            {% if upstream_oauth_provider.human_name is not none %}
               {{- _("mas.upstream_oauth2.register.imported_from_upstream_with_name", human_name=upstream_oauth_provider.human_name) -}}
             {% else %}
               {{- _("mas.upstream_oauth2.register.imported_from_upstream") -}}
@@ -175,7 +175,7 @@ <h3 class="provider">
       </div>
     {% endif %}
 
-    {% if branding.tos_uri %}
+    {% if branding.tos_uri is not none %}
       {% call(f) field.field(label=_("mas.register.terms_of_service", tos_uri=branding.tos_uri), name="accept_terms", form_state=form_state, inline=true, class="my-4") %}
         <div class="cpd-form-inline-field-control">
           <div class="cpd-checkbox-container">