From 4792af56823a051547c0249843eb08f16a26c588 Mon Sep 17 00:00:00 2001
From: Steve Howell <showell@zulip.com>
Date: Sun, 21 Nov 2021 15:17:01 +0000
Subject: [PATCH] templates: Prevent dangling end tags.

In cases where an opening tag is so long that we stretch
it to 2+ lines of code, we should try to use block-style
formatting in the template code.

Unfortunately, we have lots of legacy code that violates
this concept, so this is a timid fix.

There are also legit use cases like textarea where we
probably need to keep the ugly template syntax for things
to render properly.
---
 static/templates/invite_user.hbs              |  4 +-
 static/templates/navbar.hbs                   |  3 +-
 static/templates/right_sidebar.hbs            |  4 +-
 .../stream_settings/stream_members.hbs        |  3 +-
 static/templates/user_profile_modal.hbs       |  3 +-
 templates/two_factor/_wizard_forms.html       |  4 +-
 templates/zerver/development/dev_login.html   | 36 +++++++++-----
 templates/zerver/for-open-source.html         |  3 +-
 tools/lib/template_parser.py                  | 47 +++++++++++++++++++
 9 files changed, 87 insertions(+), 20 deletions(-)

diff --git a/static/templates/invite_user.hbs b/static/templates/invite_user.hbs
index e7a952985c..ead3b9ad9e 100644
--- a/static/templates/invite_user.hbs
+++ b/static/templates/invite_user.hbs
@@ -68,7 +68,9 @@
             <div class="modal-footer">
                 <button class="button exit small rounded" data-dismiss="modal">{{t "Cancel" }}</button>
                 <button id="submit-invitation" class="button small rounded sea-green" type="button"
-                  data-loading-text="{{t 'Inviting...' }}">{{t "Invite" }}</button>
+                  data-loading-text="{{t 'Inviting...' }}">
+                    {{t "Invite" }}
+                </button>
                 <div class="alert" id="multiuse_invite_status"></div>
             </div>
         </form>
diff --git a/static/templates/navbar.hbs b/static/templates/navbar.hbs
index 8cf1ad2eba..1a7eb12d0c 100644
--- a/static/templates/navbar.hbs
+++ b/static/templates/navbar.hbs
@@ -21,7 +21,8 @@
                             <div id="search_arrows" class="pill-container input-append">
                                 <span class="search_icon search_open" ><i class="fa fa-search" aria-hidden="true"></i></span>
                                 <div contenteditable="true" class="input search-query input-block-level" id="search_query" type="text" placeholder="{{t 'Search' }}"
-                                  autocomplete="off" aria-label="{{t 'Search' }}" title="{{t 'Search' }} (/)"> </div>
+                                  autocomplete="off" aria-label="{{t 'Search' }}" title="{{t 'Search' }} (/)">
+                                </div>
                                 <button class="btn search_button" type="button" id="search_exit" aria-label="{{t 'Exit search' }}"><i class="fa fa-remove" aria-hidden="true"></i></button>
                             </div>
                         </form>
diff --git a/static/templates/right_sidebar.hbs b/static/templates/right_sidebar.hbs
index 93d8e901e9..d9aa6e7b3f 100644
--- a/static/templates/right_sidebar.hbs
+++ b/static/templates/right_sidebar.hbs
@@ -3,7 +3,9 @@
         <div id="user-list">
             <div id="userlist-header">
                 <h4 class='sidebar-title'
-                  id='userlist-title' data-tippy-content="{{t 'Search people' }} (w)">{{t 'USERS' }}</h4>
+                  id='userlist-title' data-tippy-content="{{t 'Search people' }} (w)">
+                    {{t 'USERS' }}
+                </h4>
                 <i id="user_filter_icon" class="fa fa-search"
                   aria-hidden="true" aria-label="{{t 'Search people' }}"
                   data-tippy-content="{{t 'Search people' }} (w)">
diff --git a/static/templates/stream_settings/stream_members.hbs b/static/templates/stream_settings/stream_members.hbs
index e36898b146..62e7e929ee 100644
--- a/static/templates/stream_settings/stream_members.hbs
+++ b/static/templates/stream_settings/stream_members.hbs
@@ -12,7 +12,8 @@
                 <div class="add_subscribers_container">
                     <div class="pill-container person_picker">
                         <div class="input" contenteditable="true"
-                          data-placeholder="{{t 'Add subscribers. Use @usergroup or #streamname to bulk add subscribers.' }}"></div>
+                          data-placeholder="{{t 'Add subscribers. Use @usergroup or #streamname to bulk add subscribers.' }}">
+                        </div>
                     </div>
                     <div class="add_subscriber_btn_wrapper inline-block">
                         <button type="submit" name="add_subscriber" class="button add-subscriber-button small rounded" tabindex="0">
diff --git a/static/templates/user_profile_modal.hbs b/static/templates/user_profile_modal.hbs
index 975e36252a..4f6bb25a28 100644
--- a/static/templates/user_profile_modal.hbs
+++ b/static/templates/user_profile_modal.hbs
@@ -15,7 +15,8 @@
                 <div class="tab-data">
                     <div class="tabcontent active" id="profile-tab">
                         <div id="avatar" {{#if user_is_guest}} class="guest-avatar" {{/if}}
-                          style="background-image: url('{{user_avatar}}');"></div>
+                          style="background-image: url('{{user_avatar}}');">
+                        </div>
                         <div id="default-section">
                             {{#if show_email}}
                             <div id="email" class="default-field">
diff --git a/templates/two_factor/_wizard_forms.html b/templates/two_factor/_wizard_forms.html
index 20917cd8a8..159536ae3b 100644
--- a/templates/two_factor/_wizard_forms.html
+++ b/templates/two_factor/_wizard_forms.html
@@ -37,6 +37,8 @@
 <p>{{ _("As a last resort, you can use a backup token:") }}</p>
 <p>
     <button name="wizard_goto_step" type="submit" value="backup"
-      class="btn btn-default btn-block">{{ _("Use backup token") }}</button>
+      class="btn btn-default btn-block">
+        {{ _("Use backup token") }}
+    </button>
 </p>
 {% endif %}
diff --git a/templates/zerver/development/dev_login.html b/templates/zerver/development/dev_login.html
index cdef2b6d61..c91498655d 100644
--- a/templates/zerver/development/dev_login.html
+++ b/templates/zerver/development/dev_login.html
@@ -21,14 +21,18 @@ page can be easily identified in it's respective JavaScript file -->
                     <div class="group">
                         {% if realm_web_public_access_enabled %}
                         <h2>{{_('Anonymous user') }}</h2>
-                        <p><input type="submit" formaction="{{ current_realm.uri }}{{ url('login-local') }}"
-                            name="prefers_web_public_view" class="btn-direct btn-admin" value="Anonymous login" /></p>
+                        <p>
+                            <input type="submit" formaction="{{ current_realm.uri }}{{ url('login-local') }}"
+                              name="prefers_web_public_view" class="btn-direct btn-admin" value="Anonymous login" />
+                        </p>
                         {% endif %}
                         <h2>{{_('Owners') }}</h2>
                         {% if direct_owners %}
                             {% for direct_owner in direct_owners %}
-                            <p><input type="submit" formaction="{{ direct_owner.realm.uri }}{{ url('login-local') }}"
-                                      name="direct_email" class="btn-direct btn-admin" value="{{ direct_owner.delivery_email }}" /></p>
+                            <p>
+                                <input type="submit" formaction="{{ direct_owner.realm.uri }}{{ url('login-local') }}"
+                                  name="direct_email" class="btn-direct btn-admin" value="{{ direct_owner.delivery_email }}" />
+                            </p>
                             {% endfor %}
                         {% else %}
                             <p>No owners found in this realm</p>
@@ -36,8 +40,10 @@ page can be easily identified in it's respective JavaScript file -->
                         <h2>{{ _('Administrators') }}</h2>
                         {% if direct_admins %}
                             {% for direct_admin in direct_admins %}
-                            <p><input type="submit" formaction="{{ direct_admin.realm.uri }}{{ url('login-local') }}"
-                                      name="direct_email" class="btn-direct btn-admin" value="{{ direct_admin.delivery_email }}" /></p>
+                            <p>
+                                <input type="submit" formaction="{{ direct_admin.realm.uri }}{{ url('login-local') }}"
+                                  name="direct_email" class="btn-direct btn-admin" value="{{ direct_admin.delivery_email }}" />
+                            </p>
                             {% endfor %}
                         {% else %}
                             <p>No administrators found in this realm</p>
@@ -45,8 +51,10 @@ page can be easily identified in it's respective JavaScript file -->
                         <h2>{{ _('Moderators') }}</h2>
                         {% if direct_moderators %}
                             {% for direct_moderator in direct_moderators %}
-                            <p><input type="submit" formaction="{{ direct_moderator.realm.uri }}{{ url('login-local') }}"
-                                      name="direct_email" class="btn-direct btn-admin" value="{{ direct_moderator.delivery_email }}" /></p>
+                            <p>
+                                <input type="submit" formaction="{{ direct_moderator.realm.uri }}{{ url('login-local') }}"
+                                  name="direct_email" class="btn-direct btn-admin" value="{{ direct_moderator.delivery_email }}" />
+                            </p>
                             {% endfor %}
                         {% else %}
                             <p>No moderators found in this realm</p>
@@ -54,8 +62,10 @@ page can be easily identified in it's respective JavaScript file -->
                         <h2>{{ _('Guest users') }}</h2>
                         {% if guest_users %}
                             {% for guest_user in guest_users %}
-                            <p><input type="submit" formaction="{{ guest_user.realm.uri }}{{ url('login-local') }}"
-                                      name="direct_email" class="btn-direct btn-admin" value="{{ guest_user.delivery_email }}" /></p>
+                            <p>
+                                <input type="submit" formaction="{{ guest_user.realm.uri }}{{ url('login-local') }}"
+                                  name="direct_email" class="btn-direct btn-admin" value="{{ guest_user.delivery_email }}" />
+                            </p>
                             {% endfor %}
                         {% else %}
                             <p>No guest users found in this realm</p>
@@ -66,8 +76,10 @@ page can be easily identified in it's respective JavaScript file -->
                         <h2>{{ _('Normal users') }}</h2>
                         {% if direct_users %}
                             {% for direct_user in direct_users %}
-                            <p><input type="submit" formaction="{{ direct_user.realm.uri }}{{ url('login-local') }}"
-                                      name="direct_email" class="btn-direct btn-admin" value="{{ direct_user.delivery_email }}" /></p>
+                            <p>
+                                <input type="submit" formaction="{{ direct_user.realm.uri }}{{ url('login-local') }}"
+                                  name="direct_email" class="btn-direct btn-admin" value="{{ direct_user.delivery_email }}" />
+                            </p>
                             {% endfor %}
                         {% else %}
                             <p>No normal users found in this realm</p>
diff --git a/templates/zerver/for-open-source.html b/templates/zerver/for-open-source.html
index 91c8f4a6f3..5432267684 100644
--- a/templates/zerver/for-open-source.html
+++ b/templates/zerver/for-open-source.html
@@ -86,8 +86,7 @@
                         </div>
                     </li>
                 </ul>
-                <blockquote class="twitter-tweet" data-cards="hidden"><p
-                lang="en" dir="ltr">We just moved the Lichess team (~100 persons) to <a href="https://twitter.com/zulip?ref_src=twsrc%5Etfw">@zulip</a>, and I&#39;m loving it. The topics in particular make it vastly superior to slack &amp; discord, when it comes to dealing with many conversations.<br />Zulip is also open-source! <a href="https://t.co/lxHjf3YPMe">https://t.co/lxHjf3YPMe</a></p>&mdash; Thibault D (@ornicar) <a href="https://twitter.com/ornicar/status/1412672302601457664?ref_src=twsrc%5Etfw">July 7, 2021</a></blockquote> <script async src="https://platform.twitter.com/widgets.js"></script>
+                <blockquote class="twitter-tweet" data-cards="hidden"><p lang="en" dir="ltr">We just moved the Lichess team (~100 persons) to <a href="https://twitter.com/zulip?ref_src=twsrc%5Etfw">@zulip</a>, and I&#39;m loving it. The topics in particular make it vastly superior to slack &amp; discord, when it comes to dealing with many conversations.<br />Zulip is also open-source! <a href="https://t.co/lxHjf3YPMe">https://t.co/lxHjf3YPMe</a></p>&mdash; Thibault D (@ornicar) <a href="https://twitter.com/ornicar/status/1412672302601457664?ref_src=twsrc%5Etfw">July 7, 2021</a></blockquote> <script async src="https://platform.twitter.com/widgets.js"></script>
             </div>
         </div>
         <div class="feature-half">
diff --git a/tools/lib/template_parser.py b/tools/lib/template_parser.py
index 5d368c471c..b433a99df8 100644
--- a/tools/lib/template_parser.py
+++ b/tools/lib/template_parser.py
@@ -250,6 +250,8 @@ def validate(
             {e}"""
         )
 
+    prevent_dangling_tags(fn, tokens)
+
     class State:
         def __init__(self, func: Callable[[Token], None]) -> None:
             self.depth = 0
@@ -365,6 +367,51 @@ def validate(
         raise TemplateParserException("Missing end tag")
 
 
+def prevent_dangling_tags(fn: str, tokens: List[Token]) -> None:
+    """
+    Prevent this kind of HTML:
+
+        <div attr attr
+          attr attr>Stuff</div>
+
+    We prefer:
+        <div attr attr
+          attr attr>
+            Stuff
+        </div>
+
+    We may eventually have the pretty_printer code do this
+    automatically, but there are some complications with
+    legacy code.
+    """
+    min_row: Optional[int] = None
+    for token in tokens:
+        if token.kind in ("handlebars_singleton_end", "html_singleton_end"):
+            continue
+
+        # We only apply this validation for a couple tag types, because
+        # our existing templates may have some funny edge cases.  We eventually
+        # want to be more aggressive here. We may need to be extra careful
+        # with tags like <pre> that have whitespace sensitivities.
+        if token.tag not in ("div", "button", "p"):
+            continue
+        if min_row and token.line < min_row:
+            raise TemplateParserException(
+                f"""
+
+            Please fix line {token.line} at {fn} (col {token.col})
+            by moving this tag so that it closes the block at the
+            same indentation level as its start tag:
+
+                {token.s}
+                """
+            )
+        else:
+            min_row = None
+        if token.line_span > 1:
+            min_row = token.line + token.line_span
+
+
 def is_django_block_tag(tag: str) -> bool:
     return tag in [
         "autoescape",