Fix data string escape leakage by moving error strings to the DOM (Fixes #12789)

2023-02-23 10:24:34 +00:00 · 2023-02-23 10:24:34 +00:00 · 02f3447b06
--- a/bedrock/newsletter/templates/newsletter/management.html
+++ b/bedrock/newsletter/templates/newsletter/management.html
@ -20,29 +20,6 @@

 {% block body_class %}newsletter-management{% endblock %}

-{% set recovery_href = 'href="' + url('newsletter.recovery') + '"'|escape %}
-
-{% block string_data %}
-  {#
-  Note the outer single quotes wrapping data-error-token-not-found, not
-  doubles - this is critical to avoiding some over-escaping of quotes
-  in `recovery_href` when we bleach during the ftl() call
-  See: https://github.com/mozilla/bedrock/issues/12789
-  #}
-
-  #}
-  {% if ftl_has_messages('newsletters-the-supplied-link-has-expired-v2') %}
-    data-error-token-not-found='{{ ftl("newsletters-the-supplied-link-has-expired-v2", recovery_href=recovery_href) }}'
-  {% else %}
-    data-error-token-not-found='{{ ftl("newsletters-the-supplied-link-has-expired") }}'
-  {% endif %}
-  data-error-invalid-email="{{ ftl('newsletters-this-is-not-a-valid-email') }}"
-  data-error-invalid-newsletter="{{ ftl('newsletters-is-not-a-valid-newsletter', newsletter='%newsletter%') }}"
-  data-error-select-country="{{ ftl('newsletters-please-select-country') }}"
-  data-error-select-lang="{{ ftl('newsletters-please-select-language') }}"
-  data-error-try-again-later="{{ ftl('newsletters-something-is-amiss-with') }}"
-{% endblock %}
-
 {% set maintenance_mode = switch('newsletter-maintenance-mode') %}

 {% block content %}
@ -168,6 +145,35 @@
  </div>
  {% endif %}
 </main>
+
+{# Error strings exist in the DOM instead of JSON as they can still be needed when fetching data fails! #}
+{% set recovery_href = 'href="' + url('newsletter.recovery') + '"' %}
+<div class="template-error-strings" hidden>
+  <ul>
+    <li class="error-token-not-found">
+      {% if ftl_has_messages('newsletters-the-supplied-link-has-expired-v2') %}
+        {{ ftl("newsletters-the-supplied-link-has-expired-v2", recovery_href=recovery_href) }}
+      {% else %}
+        {{ ftl("newsletters-the-supplied-link-has-expired") }}
+      {% endif %}
+    </li>
+    <li class="error-invalid-email">
+      {{ ftl('newsletters-this-is-not-a-valid-email') }}
+    </li>
+    <li class="error-invalid-newsletter">
+      {{ ftl('newsletters-is-not-a-valid-newsletter', newsletter='%newsletter%') }}
+    </li>
+    <li class="error-select-country">
+      {{ ftl('newsletters-please-select-country') }}
+    </li>
+    <li class="error-select-lang">
+      {{ ftl('newsletters-please-select-language') }}
+    </li>
+    <li class="error-try-again-later">
+      {{ ftl('newsletters-something-is-amiss-with') }}
+    </li>
+  </ul>
+</div>
 {% endblock %}

 {# Don't display the footer if there is a token present. bug 1247446 #}
--- a/media/js/newsletter/management.es6.js
+++ b/media/js/newsletter/management.es6.js
@ -479,8 +479,7 @@ const NewsletterManagementForm = {
        FormUtils.clearFormErrors(_form);

        errors.forEach((error) => {
-            const item = `<li>${error}</li>`;
-            list.insertAdjacentHTML('afterbegin', item);
+            list.insertAdjacentElement('afterbegin', error);
        });

        errorContainer.classList.remove('hidden');
@ -538,32 +537,40 @@ const NewsletterManagementForm = {
     * @returns {String}
     */
    handleFormError: (msg, newsletterId) => {
-        const strings = document.getElementById('strings');
+        const strings = document.querySelector('.template-error-strings');
        let error;

        switch (msg) {
            case FormUtils.errorList.NOT_FOUND:
-                error = strings.getAttribute('data-error-token-not-found');
+                error = strings.querySelector('.error-token-not-found');
                break;
            case FormUtils.errorList.EMAIL_INVALID_ERROR:
-                error = strings.getAttribute('data-error-invalid-email');
+                error = strings.querySelector('.error-invalid-email');
                break;
            case FormUtils.errorList.NEWSLETTER_ERROR:
-                error = strings.getAttribute('data-error-invalid-newsletter');
+                error = strings.querySelector('.error-invalid-newsletter');

                // replace '%newsletter%' placeholder with actual newsletter ID.
                if (typeof newsletterId === 'string') {
-                    error = error.replace('%newsletter%', newsletterId);
+                    const temp = error.textContent.replace(
+                        '%newsletter%',
+                        newsletterId
+                    );
+                    error.textContent = temp;
                }
                break;
            case FormUtils.errorList.COUNTRY_ERROR:
-                error = strings.getAttribute('data-error-select-country');
+                error = strings.querySelector('.error-select-country');
                break;
            case FormUtils.errorList.LANGUAGE_ERROR:
-                error = strings.getAttribute('data-error-select-lang');
+                error = strings.querySelector('.error-select-lang');
                break;
            default:
-                error = strings.getAttribute('data-error-try-again-later');
+                error = strings.querySelector('.error-try-again-later');
+        }
+
+        if (error) {
+            return error.cloneNode(true);
        }

        return error;
@ -797,7 +804,11 @@ const NewsletterManagementForm = {
                        NewsletterManagementForm.onDataError(e);
                    });
            })
-            .catch(NewsletterManagementForm.redirectToRecoveryPage);
+            .catch(() => {
+                if (!FormUtils.getUserToken()) {
+                    NewsletterManagementForm.redirectToRecoveryPage();
+                }
+            });
    }
 };

--- a/tests/unit/spec/newsletter/management.js
+++ b/tests/unit/spec/newsletter/management.js
@ -13,7 +13,6 @@ const TOKEN_MOCK = 'a1a2a3a4-abc1-12ab-a123-12345a12345b';
 describe('management.es6.js', function () {
    beforeEach(function () {
        const form = `<div id="newsletter-management-test-form">
-            <div id="strings" data-error-token-not-found="The supplied link has expired. You will receive a new one in the next newsletter." data-error-invalid-email="This is not a valid email address. Please check the spelling." data-error-invalid-newsletter="%newsletter% is not a valid newsletter" data-error-select-country="Please select a country or region" data-error-select-lang="Please select a language" data-error-try-again-later="Something is amiss with our system, sorry! Please try again later."></div>
            <header class="mzp-l-content mzp-t-content-lg">
                <h1>Manage your Email Preferences</h1>
                <div class="js-intro-msg">
@ -96,6 +95,28 @@ describe('management.es6.js', function () {
                    </div>
                </div>
            </form>
+            <div class="template-error-strings" hidden="">
+                <ul>
+                    <li class="error-token-not-found">
+                        The supplied link has expired. Please <a href="/en-US/newsletter/recovery/">request a new link here</a>.
+                    </li>
+                    <li class="error-invalid-email">
+                        This is not a valid email address. Please check the spelling.
+                    </li>
+                    <li class="error-invalid-newsletter">
+                        %newsletter% is not a valid newsletter
+                    </li>
+                    <li class="error-select-country">
+                        Please select a country or region
+                    </li>
+                    <li class="error-select-lang">
+                        Please select a language
+                    </li>
+                    <li class="error-try-again-later">
+                        Something is amiss with our system, sorry! Please try again later.
+                    </li>
+                </ul>
+            </div>
        </div>`;

        document.body.insertAdjacentHTML('beforeend', form);
@ -579,11 +600,16 @@ describe('management.es6.js', function () {
                spyOn(NewsletterManagementForm, 'getFormLang').and.returnValue(
                    undefined
                );
-                expect(NewsletterManagementForm.validateFields()).toEqual([
-                    'bargain-hunters-weekly is not a valid newsletter',
-                    'Please select a country or region',
+                const errors = NewsletterManagementForm.validateFields();
+                expect(errors[0].textContent.trim()).toEqual(
+                    'bargain-hunters-weekly is not a valid newsletter'
+                );
+                expect(errors[1].textContent.trim()).toEqual(
+                    'Please select a country or region'
+                );
+                expect(errors[2].textContent.trim()).toEqual(
                    'Please select a language'
-                ]);
+                );
            });
        });
    });
@ -873,11 +899,11 @@ describe('management.es6.js', function () {
            ).and.returnValue(window.Promise.resolve(stringData));

            return NewsletterManagementForm.init().then(() => {
-                expect(
-                    document.querySelector('.mzp-c-form-errors li:nth-child(1)')
-                        .innerText
-                ).toEqual(
-                    'The supplied link has expired. You will receive a new one in the next newsletter.'
+                const error = document
+                    .querySelector('.mzp-c-form-errors li:nth-child(1)')
+                    .innerHTML.trim();
+                expect(error).toEqual(
+                    'The supplied link has expired. Please <a href="/en-US/newsletter/recovery/">request a new link here</a>.'
                );
            });
        });
@ -899,10 +925,10 @@ describe('management.es6.js', function () {
            ).and.returnValue(window.Promise.resolve(stringData));

            return NewsletterManagementForm.init().then(() => {
-                expect(
-                    document.querySelector('.mzp-c-form-errors li:nth-child(1)')
-                        .innerText
-                ).toEqual(
+                const error = document
+                    .querySelector('.mzp-c-form-errors li:nth-child(1)')
+                    .innerHTML.trim();
+                expect(error).toEqual(
                    'Something is amiss with our system, sorry! Please try again later.'
                );
            });