Skip to content

Fix some mako-missing-default xsslint issues in test files. #16709

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
jzoldak merged 2 commits into from
Nov 30, 2017
Merged

Fix some mako-missing-default xsslint issues in test files. #16709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a36130
Fix some mako-missing-default xsslint issues in test files.
Nov 29, 2017
1ab92ee
Fix an introduced xsslint issue.
Nov 29, 2017
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
1 change: 1 addition & 0 deletions common/test/templates/theme-footer.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
<%page expression_filter="h"/>
# intentionally left blank
1 change: 1 addition & 0 deletions common/test/templates/theme-google-analytics.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
<%page expression_filter="h"/>
# intentionally left blank
1 change: 1 addition & 0 deletions common/test/templates/theme-head-extra.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
<%page expression_filter="h"/>
# intentionally left blank
1 change: 1 addition & 0 deletions common/test/templates/theme-header.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
<%page expression_filter="h"/>
# intentionally left blank
1 change: 1 addition & 0 deletions common/test/test-theme/lms/templates/dashboard.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<%page expression_filter="h"/>
<%inherit file="dashboard.html" />
<%block name="pagetitle">Overridden Title!</%block>
${parent.body()}

@robrap robrap Nov 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jzoldak: I'd be surprised if HTML-escaping wouldn't need to be turned off for ${parent.body()}. I didn't review all these files individually, but just a note that some care needs to go into adding default HTML-escaping, except of course on empty templates like some of these.

Additionally, we should really bring down the threshold on this rule once the dust settles, or people may introduce new templates without this page expression, which is what we want to protect against (i.e. new issues).

@UmanShahzad UmanShahzad Nov 30, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understand. In this case this is just a test file, so it should be OK, but we should definitely be following best practices moving forward given this situation we ran into.

@robrap robrap Nov 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @UmanShahzad. A couple of additional details:

  1. Although it is a test file, you never know who is going to copy and paste code from where, so it would be good if the file was accurate.
  2. When we introduce HTML-escaping by default, it affects the whole template and can cause new linter errors. In this case, it did, as @efischer19 points out in this comment:
    https://github.com/edx/edx-platform/pull/16723#issuecomment-348235004

He is upping the thresholds in the following PR, but it would have been nice to not have added new issues:
https://github.com/edx/edx-platform/pull/16723

@robrap robrap Nov 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI: You can see the violations for all files you touched here:
https://build.testeng.edx.org/job/edx-platform-quality-pr/43752/artifact/edx-platform/reports/xsscommitlint/xsscommitlint.report/*view*/

Some examples are:

Linting common/test/test_sites/test_site/templates/courseware/tabs.html:
common/test/test_sites/test_site/templates/courseware/tabs.html: 21:12: mako-unwanted-html-filter:   <a href="${tab.link_func(course, reverse) | h}" class="${tab_class}">
common/test/test_sites/test_site/templates/courseware/tabs.html: 22:18: mako-unwanted-html-filter:   Test Site Tab: ${_(tab.name) | h}

As a separate discussion, we could debate whether mako-unwanted-html-filter should break the build or not since strictly speaking it isn't a problem. The problem is simply that if it isn't cleaned up by removing the | h, it makes it difficult to know what files are safe by default and which aren't, and which expressions are safe and which aren't.
@efischer19: Maybe you can just fix these on your PR, rather than upping the threshold?

@robrap robrap Nov 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just in case this isn't fixed as part of @efischer19's PR, the other failure that could be cleaned up is:

Linting common/test/test_sites/test_site/templates/static_templates/contact.html:
common/test/test_sites/test_site/templates/static_templates/contact.html: 25:14: mako-html-entities:       <p>${_("We are always seeking feedback to improve our courses. If you are an enrolled student and have any questions, feedback, suggestions, or any other issues specific to a particular class, please post on the discussion forums of that&nbsp;class.")}</p>

We could then adjust the thresholds back down.

@UmanShahzad UmanShahzad Nov 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the problem is I had no clue I was introducing these violations with this PR. You can tell at a glance for things like pylint or pep8, but it doesn't seem obvious at all (to me and maybe others) with xsslint reporting atm. Reporting should be improved to show the specific errors a particular PR introduces and nothing more, to keep it simple and prevent something like this.

@robrap robrap Nov 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@UmanShahzad: Agreed. It used to tell you which thresholds were breaking, but now that is hard to see. See this comment and following comments for a discussion of this:
https://github.com/edx/edx-platform/pull/16719#issuecomment-348209133

FYI: @jzoldak

@efischer19 efischer19 Nov 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree. It seems like @UmanShahzad got "hit" by this because he touched a file to improve it, then the tooling started reporting all the previously un-reported errors that existed in that file. It feels like we're punishing good intentions to some degree.

@robrap robrap Nov 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@efischer19: It's a little different from the way you describe it. It's not that you get hit by tooling for touching a file. It is that when you add HTML-escaping by default to the top of a file it affects all expressions in that template, and sometimes new problems appear. So, some care is needed when adding HTML-escaping by default. You can't just drop it in to any template that doesn't have it without this care, or we would have done that to get rid of these violations.

@efischer19 efischer19 Nov 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, okay - that does make more sense. Thanks!

Expand Down
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/courseware/syllabus.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
## mako
<%page expression_filter="h"/>
<%namespace name='static' file='/static_content.html'/>
<%include file="${static.get_template_path('courseware/test_relative_path.html')}" />
<%include file="${static.get_template_path('/courseware/test_absolute_path.html')}" />
Expand Down
2 changes: 1 addition & 1 deletion common/test/test_sites/test_site/templates/courseware/tabs.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from django.utils.translation import ugettext as _
from django.core.urlresolvers import reverse
%>
<%page args="tab_list, active_page, default_tab, tab_image" />
<%page args="tab_list, active_page, default_tab, tab_image" expression_filter="h" />

<%
def url_class(is_active):
Expand Down
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/courseware/test_absolute_path.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
## mako
<%page expression_filter="h"/>
<%namespace name='static' file='/static_content.html'/>
<div>Microsite absolute path template contents</div>
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/courseware/test_relative_path.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
## mako
<%page expression_filter="h"/>
<%namespace name='static' file='/static_content.html'/>
<div>Microsite relative path template contents</div>
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/footer.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
## mako
<%page expression_filter="h"/>
<%namespace name='static' file='static_content.html'/>
<%!
from django.core.urlresolvers import reverse
Expand Down
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/head-extra.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<%page expression_filter="h"/>
<%namespace name='static' file='../../static_content.html'/>
<% style_overrides_file = static.get_value('css_overrides_file') %>

Expand Down
3 changes: 2 additions & 1 deletion common/test/test_sites/test_site/templates/login-sidebar.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<%!
<%page expression_filter="h"/>
<%!
from django.utils.translation import ugettext as _
from django.core.urlresolvers import reverse
%>
Expand Down
3 changes: 2 additions & 1 deletion common/test/test_sites/test_site/templates/register-sidebar.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<%!
<%page expression_filter="h"/>
<%!
from django.utils.translation import ugettext as _
from django.core.urlresolvers import reverse
%>
Expand Down
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/static_templates/about.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<%page expression_filter="h"/>
<%! from django.core.urlresolvers import reverse %>
<%namespace name='static' file='../../../static_content.html'/>

Expand Down
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/static_templates/contact.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<%page expression_filter="h"/>
<%inherit file="../main.html" />
<%namespace name='static' file='../static_content.html'/>
<%!
Expand Down
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/static_templates/copyright.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
<%page expression_filter="h"/>
This is a copyright page for an Open edX site.
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/static_templates/faq.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<%page expression_filter="h"/>
<%inherit file="../main.html" />
<%namespace name='static' file='../static_content.html'/>
<%!
Expand Down
1 change: 1 addition & 0 deletions common/test/test_sites/test_site/templates/static_templates/tos.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<%page expression_filter="h"/>
<%inherit file="../main.html" />
<%namespace name='static' file='../static_content.html'/>
<%!
Expand Down
1 change: 1 addition & 0 deletions lms/templates/shoppingcart/test/fake_payment_error.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<%page expression_filter="h"/>
<html>
<head>
<title>Payment Error</title>
Expand Down
1 change: 1 addition & 0 deletions lms/templates/shoppingcart/test/fake_payment_page.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<%page expression_filter="h"/>
<html>
<head><title>Payment Form</title>
</head>
Expand Down