From b69c6d62bfe8d9f341744645daa64dfc82df0e15 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 23 Dec 2015 18:48:30 -0500 Subject: [PATCH] Make base.html Mako template safe by default Make base.html Mako template safe by default by: 1. Add page-level default of html escaping 2. Fix escaping of all variables in base.html 3. Fix escaping of all dependent underscore templates Also includes additional best practices for certificates and textbooks JavaScript/Underscore in order to complete that work. TNL-3425 --- .../views/tests/test_certificates.py | 2 +- .../certificates/views/certificate_editor.js | 10 ++-- cms/static/js/views/edit_chapter.js | 10 ++-- cms/static/js/views/edit_textbook.js | 2 +- cms/static/js/views/show_textbook.js | 6 ++- cms/templates/base.html | 43 +++++++++-------- .../js/certificate-details.underscore | 34 ++++++------- .../js/certificate-editor.underscore | 40 ++++++++-------- cms/templates/js/edit-chapter.underscore | 20 ++++---- cms/templates/js/edit-textbook.underscore | 18 +++---- cms/templates/js/publish-xblock.underscore | 48 +++++++++---------- cms/templates/js/show-textbook.underscore | 18 +++---- cms/templates/js/signatory-details.underscore | 18 +++---- cms/templates/js/signatory-editor.underscore | 40 ++++++++-------- 14 files changed, 157 insertions(+), 152 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_certificates.py b/cms/djangoapps/contentstore/views/tests/test_certificates.py index 8f72f7ba1d..d6d52cbba1 100644 --- a/cms/djangoapps/contentstore/views/tests/test_certificates.py +++ b/cms/djangoapps/contentstore/views/tests/test_certificates.py @@ -1,7 +1,7 @@ #-*- coding: utf-8 -*- """ -Group Configuration Tests. +Certificates Tests. """ import json import mock diff --git a/cms/static/js/certificates/views/certificate_editor.js b/cms/static/js/certificates/views/certificate_editor.js index 7e205c6d19..11cc32a99d 100644 --- a/cms/static/js/certificates/views/certificate_editor.js +++ b/cms/static/js/certificates/views/certificate_editor.js @@ -97,11 +97,11 @@ function($, _, Backbone, gettext, return { id: this.model.get('id'), uniqueId: _.uniqueId(), - name: this.model.escape('name'), - description: this.model.escape('description'), - course_title: this.model.escape('course_title'), - org_logo_path: this.model.escape('org_logo_path'), - is_active: this.model.escape('is_active'), + name: this.model.get('name'), + description: this.model.get('description'), + course_title: this.model.get('course_title'), + org_logo_path: this.model.get('org_logo_path'), + is_active: this.model.get('is_active'), isNew: this.model.isNew() }; }, diff --git a/cms/static/js/views/edit_chapter.js b/cms/static/js/views/edit_chapter.js index c15eb1b19f..f70839c2d4 100644 --- a/cms/static/js/views/edit_chapter.js +++ b/cms/static/js/views/edit_chapter.js @@ -12,8 +12,8 @@ define(["js/views/baseview", "underscore", "underscore.string", "jquery", "gette }, render: function() { this.$el.html(this.template({ - name: this.model.escape('name'), - asset_path: this.model.escape('asset_path'), + name: this.model.get('name'), + asset_path: this.model.get('asset_path'), order: this.model.get('order'), error: this.model.validationError })); @@ -52,8 +52,10 @@ define(["js/views/baseview", "underscore", "underscore.string", "jquery", "gette asset_path: this.$("input.chapter-asset-path").val() }); var msg = new FileUploadModel({ - title: _.template(gettext("Upload a new PDF to “<%= name %>”"), - {name: course.escape('name')}), + title: _.template( + gettext("Upload a new PDF to “<%= name %>”"), + {name: window.course.escape('name')} + ), message: gettext("Please select a PDF file to upload."), mimeTypes: ['application/pdf'] }); diff --git a/cms/static/js/views/edit_textbook.js b/cms/static/js/views/edit_textbook.js index fcd4503b83..3616a5e994 100644 --- a/cms/static/js/views/edit_textbook.js +++ b/cms/static/js/views/edit_textbook.js @@ -13,7 +13,7 @@ define(["js/views/baseview", "underscore", "jquery", "js/views/edit_chapter", "c className: "textbook", render: function() { this.$el.html(this.template({ - name: this.model.escape('name'), + name: this.model.get('name'), error: this.model.validationError })); this.addAll(); diff --git a/cms/static/js/views/show_textbook.js b/cms/static/js/views/show_textbook.js index c7caa4b4f9..2dcc470154 100644 --- a/cms/static/js/views/show_textbook.js +++ b/cms/static/js/views/show_textbook.js @@ -29,8 +29,10 @@ define(["js/views/baseview", "underscore", "gettext", "common/js/components/view if(e && e.preventDefault) { e.preventDefault(); } var textbook = this.model, collection = this.model.collection; var msg = new PromptView.Warning({ - title: _.template(gettext("Delete “<%= name %>”?"), - {name: textbook.escape('name')}), + title: _.template( + gettext("Delete “<%= name %>”?"), + {name: textbook.get('name')} + ), message: gettext("Deleting a textbook cannot be undone and once deleted any reference to it in your courseware's navigation will also be removed."), actions: { primary: { diff --git a/cms/templates/base.html b/cms/templates/base.html index 51db3f154c..1603c9f209 100644 --- a/cms/templates/base.html +++ b/cms/templates/base.html @@ -1,11 +1,12 @@ ## coding=utf-8 <%namespace name='static' file='static_content.html'/> <%! -from django.utils.translation import ugettext as _ +from openedx.core.djangolib.markup import ugettext as _ from openedx.core.djangolib.js_utils import ( dump_js_escaped_json, js_escaped_string ) %> +<%page expression_filter="h"/> @@ -16,9 +17,9 @@ from openedx.core.djangolib.js_utils import ( <%block name="title"> | % if context_course: <% ctx_loc = context_course.location %> - ${context_course.display_name_with_default_escaped | h} | + ${context_course.display_name_with_default} | % elif context_library: - ${context_library.display_name_with_default_escaped | h} | + ${context_library.display_name_with_default} | % endif ${settings.STUDIO_NAME} @@ -82,24 +83,24 @@ from openedx.core.djangolib.js_utils import ( diff --git a/cms/templates/js/certificate-details.underscore b/cms/templates/js/certificate-details.underscore index 7bed814728..d78f27ea05 100644 --- a/cms/templates/js/certificate-details.underscore +++ b/cms/templates/js/certificate-details.underscore @@ -1,46 +1,46 @@

- <%= name %> + <%- name %>

    <% if (!_.isUndefined(id)) { %>
  1. - <%= gettext('ID') %>: - <%= id %> + <%- gettext('ID') %>: + <%- id %>
    2. <% } %> <% if (showDetails) { %>
    -

    <%= gettext("Certificate Details") %>

    +

    <%- gettext("Certificate Details") %>

    - <%= gettext('Course Title') %>: - <%= course.get('name') %> + <%- gettext('Course Title') %>: + <%- course.get('name') %>

    <% if (course_title) { %>

    - <%= gettext('Course Title Override') %>: - <%= course_title %> + <%- gettext('Course Title Override') %>: + <%- course_title %>

    <% } %>

    - <%= gettext('Course Number') %>: - <%= course.get('num') %> + <%- gettext('Course Number') %>: + <%- course.get('num') %>

    <% if (course.get('display_course_number')) { %>

    - <%= gettext('Course Number Override') %>: - <%= course.get('display_course_number') %> + <%- gettext('Course Number Override') %>: + <%- course.get('display_course_number') %>

    <% } %>
    @@ -50,9 +50,9 @@
    -

    <%= gettext("Certificate Signatories") %>

    +

    <%- gettext("Certificate Signatories") %>

    -

    <%= gettext("It is strongly recommended that you include four or fewer signatories. If you include additional signatories, preview the certificate in Print View to ensure the certificate will print correctly on one page.") %>

    +

    <%- gettext("It is strongly recommended that you include four or fewer signatories. If you include additional signatories, preview the certificate in Print View to ensure the certificate will print correctly on one page.") %>

    <% } %> @@ -61,10 +61,10 @@
      <% if (CMS.User.isGlobalStaff || !is_active) { %>
    • - +
      • -
    • - +
    • +
      • <% } %>
    diff --git a/cms/templates/js/certificate-editor.underscore b/cms/templates/js/certificate-editor.underscore index 003951d91f..513113b805 100644 --- a/cms/templates/js/certificate-editor.underscore +++ b/cms/templates/js/certificate-editor.underscore @@ -2,52 +2,52 @@
    <% if (error && error.message) { %>
    - <%= gettext(error.message) %> + <%- gettext(error.message) %>
    <% } %>
    - <%= gettext("Certificate Information") %> + <%- gettext("Certificate Information") %>
    - - " value="<%= name %>" aria-describedby="certificate-name-<%=uniqueId %>-tip" /> - <%= gettext("Name of the certificate") %> + + " value="<%- name %>" aria-describedby="certificate-name-<%-uniqueId %>-tip" /> + <%- gettext("Name of the certificate") %>
    - - - <%= gettext("Description of the certificate") %> + + + <%- gettext("Description of the certificate") %>
    -

    <%= gettext("Certificate Details") %>

    +

    <%- gettext("Certificate Details") %>

    - <%= gettext("Course Title") %>: - <%= course.get('name') %> + <%- gettext("Course Title") %>: + <%- course.get('name') %>
    - - " value="<%= course_title %>" aria-describedby="certificate-course-title-<%=uniqueId %>-tip" /> - <%= gettext("Specify an alternative to the official course title to display on certificates. Leave blank to use the official course title.") %> + + " value="<%- course_title %>" aria-describedby="certificate-course-title-<%-uniqueId %>-tip" /> + <%- gettext("Specify an alternative to the official course title to display on certificates. Leave blank to use the official course title.") %>
    -

    <%= gettext("Certificate Signatories") %>

    +

    <%- gettext("Certificate Signatories") %>

    -

    <%= gettext("It is strongly recommended that you include four or fewer signatories. If you include additional signatories, preview the certificate in Print View to ensure the certificate will print correctly on one page.") %>

    +

    <%- gettext("It is strongly recommended that you include four or fewer signatories. If you include additional signatories, preview the certificate in Print View to ensure the certificate will print correctly on one page.") %>

    - - <%= gettext("(Add signatories for a certificate)") %> + + <%- gettext("(Add signatories for a certificate)") %>
    - + <% if (!isNew && (CMS.User.isGlobalStaff || !is_active)) { %> - <%= gettext("Delete") %> + <%- gettext("Delete") %> <% } %>
    diff --git a/cms/templates/js/edit-chapter.underscore b/cms/templates/js/edit-chapter.underscore index 7671b62eed..66851249bb 100644 --- a/cms/templates/js/edit-chapter.underscore +++ b/cms/templates/js/edit-chapter.underscore @@ -1,14 +1,14 @@ -
    -name <% if (error && error.attributes && error.attributes.name) { print('error'); } %>"> - - " value="<%= name %>" type="text"> - <%= gettext("provide the title/name of the chapter that will be used in navigating") %> + + " value="<%- name %>" type="text"> + <%- gettext("provide the title/name of the chapter that will be used in navigating") %>
    -
    -asset <% if (error && error.attributes && error.attributes.asset_path) { print('error'); } %>"> - - " value="<%= asset_path %>" type="text" dir="ltr"> - <%= gettext("upload a PDF file or provide the path to a Studio asset file") %> - + + " value="<%- asset_path %>" type="text" dir="ltr"> + <%- gettext("upload a PDF file or provide the path to a Studio asset file") %> +
    - <%= gettext("delete chapter") %> + <%- gettext("delete chapter") %> diff --git a/cms/templates/js/edit-textbook.underscore b/cms/templates/js/edit-textbook.underscore index d7ce6256d0..fa42651d7d 100644 --- a/cms/templates/js/edit-textbook.underscore +++ b/cms/templates/js/edit-textbook.underscore @@ -2,27 +2,27 @@
    <% if (error && error.message) { %>
    - <%= gettext(error.message) %> + <%- gettext(error.message) %>
    <% } %>
    - <%= gettext("Textbook information") %> + <%- gettext("Textbook information") %>
    - - " value="<%= name %>"> - <%= gettext("provide the title/name of the text book as you would like your students to see it") %> + + " value="<%- name %>"> + <%- gettext("provide the title/name of the text book as you would like your students to see it") %>
    - <%= gettext("Chapter information") %> + <%- gettext("Chapter information") %>
      - +
      - - + +
      diff --git a/cms/templates/js/publish-xblock.underscore b/cms/templates/js/publish-xblock.underscore index 8afcafa364..766cec8aa5 100644 --- a/cms/templates/js/publish-xblock.underscore +++ b/cms/templates/js/publish-xblock.underscore @@ -19,43 +19,43 @@ if (visibilityState === 'live') { var visibleToStaffOnly = visibilityState === 'staff_only'; %> -
      -

      <%= gettext("Publishing Status") %> - <%= title %> +
      +

      <%- gettext("Publishing Status") %> + <%- title %>

      <% if (hasChanges && editedOn && editedBy) { var message = gettext("Draft saved on %(last_saved_date)s by %(edit_username)s") %> - <%= interpolate(message, { - last_saved_date: '' + editedOn + '', - edit_username: '' + editedBy + '' }, true) %> + <%= interpolate(_.escape(message), { + last_saved_date: '' + _.escape(editedOn) + '', + edit_username: '' + _.escape(editedBy) + '' }, true) %> <% } else if (publishedOn && publishedBy) { var message = gettext("Last published %(last_published_date)s by %(publish_username)s"); %> - <%= interpolate(message, { - last_published_date: '' + publishedOn + '', - publish_username: '' + publishedBy + '' }, true) %> + <%= interpolate(_.escape(message), { + last_published_date: '' + _.escape(publishedOn) + '', + publish_username: '' + _.escape(publishedBy) + '' }, true) %> <% } else { %> - <%= gettext("Previously published") %> + <%- gettext("Previously published") %> <% } %>

      <% if (!course.get('self_paced')) { %>
      -
      <%= releaseLabel %>
      +
      <%- releaseLabel %>

      <% if (releaseDate) { %> - <%= releaseDate %> + <%- releaseDate %> - <%= interpolate( + <%- interpolate( gettext('with %(release_date_from)s'), { release_date_from: releaseDateFrom }, true ) %> <% } else { %> - <%= gettext("Unscheduled") %> + <%- gettext("Unscheduled") %> <% } %>

      @@ -64,40 +64,40 @@ var visibleToStaffOnly = visibilityState === 'staff_only';
      <% if (released && published && !hasChanges) { %> - <%= gettext("Is Visible To:") %> + <%- gettext("Is Visible To:") %> <% } else { %> - <%= gettext("Will Be Visible To:") %> + <%- gettext("Will Be Visible To:") %> <% } %>
      <% if (visibleToStaffOnly) { %>

      - <%= gettext("Staff Only") %> + <%- gettext("Staff Only") %> <% if (!hasExplicitStaffLock) { %> - <%= interpolate( + <%- interpolate( gettext("with %(section_or_subsection)s"),{ section_or_subsection: staffLockFrom }, true ) %> <% } %>

      <% } else { %> -

      <%= gettext("Staff and Students") %>

      +

      <%- gettext("Staff and Students") %>

      <% } %> <% if (hasContentGroupComponents) { %>

      - <%= gettext("Some content in this unit is visible only to particular content groups") %> + <%- gettext("Some content in this unit is visible only to particular content groups") %>

      <% } %> @@ -107,12 +107,12 @@ var visibleToStaffOnly = visibilityState === 'staff_only'; diff --git a/cms/templates/js/show-textbook.underscore b/cms/templates/js/show-textbook.underscore index b758b40a51..ac303ba3a2 100644 --- a/cms/templates/js/show-textbook.underscore +++ b/cms/templates/js/show-textbook.underscore @@ -1,19 +1,19 @@ -
      +
      -

      <%= name %>

      +

      <%- name %>

      <% if(chapters.length > 1) {%>

      - <%= chapters.length %> PDF Chapters + <%- chapters.length %> PDF Chapters

      <% } else if(chapters.length === 1) { %>

      - <%= chapters.at(0).get("asset_path") %> + <%- chapters.at(0).get("asset_path") %>

      <% } %> @@ -22,8 +22,8 @@
        <% chapters.each(function(chapter) { %>
      1. - <%= chapter.get('name') %> - <%= chapter.get('asset_path') %> + <%- chapter.get('name') %> + <%- chapter.get('asset_path') %>
        2. <% }) %>
      @@ -34,13 +34,13 @@ diff --git a/cms/templates/js/signatory-details.underscore b/cms/templates/js/signatory-details.underscore index d2eca1b178..8a0006f5c0 100644 --- a/cms/templates/js/signatory-details.underscore +++ b/cms/templates/js/signatory-details.underscore @@ -2,30 +2,30 @@ <% if (CMS.User.isGlobalStaff || !certificate.get('is_active')) { %>
      - <%= gettext("Edit") %> + <%- gettext("Edit") %>
      <% } %> -
      <%= gettext("Signatory") %> <%= signatory_number %> 
      +
      <%- gettext("Signatory") %> <%- signatory_number %> 
      - <%= gettext("Name") %>:  - <%= name %> + <%- gettext("Name") %>:  + <%- name %>
      - <%= gettext("Title") %>:  - <%= title.replace(new RegExp('\r?\n','g'), '
      ') %>       + <%- gettext("Title") %>:  + <%= _.escape(title).replace(new RegExp('\r?\n','g'), '
      ') %>
      - <%= gettext("Organization") %>:  - <%= organization %> + <%- gettext("Organization") %>:  + <%- organization %>
      <% if (signature_image_path != "") { %>
      - <%= gettext('Signature Image') %> + <%- gettext('Signature Image') %>
      <% } %>
      diff --git a/cms/templates/js/signatory-editor.underscore b/cms/templates/js/signatory-editor.underscore index 0a2f3a4523..01a53188d9 100644 --- a/cms/templates/js/signatory-editor.underscore +++ b/cms/templates/js/signatory-editor.underscore @@ -2,48 +2,48 @@ <% if (is_editing_all_collections && signatories_count > 1 && (total_saved_signatories > 1 || isNew) ) { %> - <%= gettext("Delete") %> + <%- gettext("Delete") %> <% } %> -
      <%= gettext("Signatory") %> <%= signatory_number %>
      +
      <%- gettext("Signatory") %> <%- signatory_number %>
      - <%= gettext("Certificate Signatory Configuration") %> + <%- gettext("Certificate Signatory Configuration") %>
      - - " value="<%= name %>" aria-describedby="signatory-name-<%= signatory_number %>-tip" /> - <%= gettext("The name of this signatory as it should appear on certificates.") %> + + " value="<%- name %>" aria-describedby="signatory-name-<%- signatory_number %>-tip" /> + <%- gettext("The name of this signatory as it should appear on certificates.") %> <% if(error && error.name) { %> - <%= error.name %> + <%- error.name %> <% } %>
      - - - <%= gettext("Titles more than 100 characters may prevent students from printing their certificate on a single page.") %> + + + <%- gettext("Titles more than 100 characters may prevent students from printing their certificate on a single page.") %> <% if(error && error.title) { %> - <%= error.title %> + <%- error.title %> <% } %>
      - - " value="<%= organization %>" aria-describedby="signatory-organization-<%= signatory_number %>-tip" /> - <%= gettext("The organization that this signatory belongs to, as it should appear on certificates.") %> + + " value="<%- organization %>" aria-describedby="signatory-organization-<%- signatory_number %>-tip" /> + <%- gettext("The organization that this signatory belongs to, as it should appear on certificates.") %> <% if(error && error.organization) { %> - <%= error.organization %> + <%- error.organization %> <% } %>
      - + <% if (signature_image_path != "") { %> -
      Signature Image
      +
      Signature Image
      <% } %>
      - " value="<%= signature_image_path %>" aria-describedby="signatory-signature-<%= signatory_number %>-tip" readonly /> - <%= gettext("Image must be in PNG format") %> + " value="<%- signature_image_path %>" aria-describedby="signatory-signature-<%- signatory_number %>-tip" readonly /> + <%- gettext("Image must be in PNG format") %>
      - +