Skip to content

templates: Be strict about undefined variables #5201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 30, 2025
Merged

templates: Be strict about undefined variables #5201

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
adad551
templates: fix reliance on lax undefined behaviour
reivilibre Oct 24, 2025
ceaac61
templates: Be strict about undefined variables
reivilibre Oct 24, 2025
647c8e7
For branding: use none instead of undefined
reivilibre Oct 28, 2025
ea7f568
Clarify some more conditions in the templates
reivilibre Oct 28, 2025
eeeec35
Downgrade to SemiStrict in production
reivilibre Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion crates/templates/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -205,6 +205,8 @@ 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(UndefinedBehavior::Strict);
let root = path.canonicalize_utf8()?;
info!(%root, "Loading templates from filesystem");
for entry in walkdir::WalkDir::new(&root)
Expand Down
2 changes: 1 addition & 1 deletion templates/base.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
10 changes: 5 additions & 5 deletions templates/components/footer.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,27 @@
-#}

<footer class="legal-footer">
{%- if branding.policy_uri or branding.tos_uri -%}
{%- if branding.policy_uri is defined or branding.tos_uri is defined -%}
<nav>
{%- if branding.policy_uri -%}
{%- if branding.policy_uri is defined -%}
<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 defined and branding.tos_uri is defined -%}
<div class="separator" aria-hidden="true">•</div>
{%- endif -%}

{%- if branding.tos_uri -%}
{%- if branding.tos_uri is defined -%}
Copy link
Member

@sandhose sandhose Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scares me a little bit, we don't have 100% coverage of templates with the samples, so we might have missed some spots. What about using SemiStrict instead, which allows for if undefined?

Copy link
Contributor Author

@reivilibre reivilibre Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed my mind on this: how about using is none instead? So we never make fields undefined, just none/nullable. I think this would offer even more typo resistance than is_defined

Copy link
Member

@sandhose sandhose Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on using none instead of undefined, but the point that this PR might break other stuff still stands

Copy link
Contributor Author

@reivilibre reivilibre Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah OK, this is going to be gnarly, I've downgraded in 20a13f4

<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 defined -%}
<p class="imprint">{{ branding.imprint }}</p>
{%- endif -%}
</footer>
2 changes: 1 addition & 1 deletion templates/pages/register/password.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 defined %}
{% 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">
Expand Down
2 changes: 1 addition & 1 deletion templates/pages/upstream_oauth2/do_register.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ <h3 class="provider">
</div>
{% endif %}

{% if branding.tos_uri %}
{% if branding.tos_uri is defined %}
{% 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">
Expand Down
Loading