(WIP) Fix TNL-3429. by agaylard · Pull Request #11588 · openedx/openedx-platform

agaylard · 2016-02-18T21:18:01Z

JIRA tickets: TNL-3429

Discussion:
This PR replaces https://github.com/edx/edx-platform/pull/10756. It builds on work done by @robrap in https://github.com/edx/edx-platform/pull/10962, for TNL-3425.

These changes aim to follow https://openedx.atlassian.net/wiki/display/TNL/XSS+Protection .

The change to cms/templates/container.html replaces calls to display_name_with_default_escaped, which is deprecated and does not properly perform HTML-escaping, with display_name_with_default, which makes use of the |h filter for escaping.

The changes to these three files selectively apply escaping, using either the deprecated display_name_with_default_escaped or the preferred display_name_with_default, depending on the context, so that only the course outline and unit pages get the non-escaped data:

cms/djangoapps/contentstore/views/component.py
cms/djangoapps/contentstore/views/course.py
cms/djangoapps/contentstore/views/item.py

These two templates are changed to HTML-escape the display name fields:

cms/templates/js/course-outline.underscore
cms/templates/js/unit-outline.underscore

Dependencies: None

Sandbox: LMS Studio

Testing instructions:
Note: in the screenshots, the fields of interest are highlighted in purple.

Start up Studio as the edxapp user: "paver devstack studio".
Open a browser to Studio: http://127.0.0.1:8001. The home page loads.
Click the "Sign in" button. The sign in page loads.
Sign in as as the "staff@example.com" user.
Click on "edX Demonstration Course". This opens the course outline page.
Scroll to the bottom of the page.
Click "New Section" and create a section named §: Intro to <HTML> & "JS"
Click "New Subsection" and create a subsection named §§: Intro to <HTML> & "JS". Both names are rendered correctly on the course outline page.
If necessary, click on the triangle alongside the subsection name to expand the field.
Click "New Unit". The unit page loads.
Without the fix, this reproduces bug TNL-3429:
With the fix, the text is rendered correctly:
Return to the course outline page. Click to edit the section name. Without the fix, the text is rendered in the text-field incorrectly (the < and > characters are HTML-escaped, but not the & and " characters, which appear as expected; the partial escaping is obvious with this particular test-data):

With these changes, the text is rendered as correctly:

This editing bug occurs on the unit page too, and is also fixed by this change. I am not aware of any JIRA ticket tracking this issue.

Author notes and concerns:
This PR changes the way that display_name text is sent via JSON. Previously, the < and > characters would be escaped to < and >. No other HTML-sensitive characters were handled. Now, these characters are sent as-is. For the course outline page and the unit page, the escaping to HTML is performed on the browser by the underscore templates (properly, not just for angle-brackets).

Reviewers

@smarnach, TBD

openedx-webhooks · 2016-02-18T21:18:06Z

Thanks for the pull request, @agaylard! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=11588

robrap · 2016-02-18T21:50:03Z

 from openedx.core.djangolib.markup import HTML, ugettext as _
 %>
-<%block name="title">${xblock.display_name_with_default_escaped} ${xblock_type_display_name(xblock) | h}</%block>
+<%block name="title">${xblock.display_name_with_default} ${xblock_type_display_name(xblock) | h}</%block>


This is missing the | h that would do the html escaping.

Oops, yes. Fixed.

robrap · 2016-02-18T21:54:14Z

Thanks again @agaylard. Let me know your thoughts.

mushtaqak · 2016-02-19T08:05:05Z

Would it be more better if we make cms/templates/container.html safe by default using <%page expression_filter="h"/>

robrap · 2016-02-19T12:38:02Z

@mushtaqak it would certainly be better, and will soon be a requirement, but that hasn't been communicated yet because I'm still working on the docs. If you want to help communicate about the details, it would be great to have help on this front. Thank you.

robrap · 2016-02-19T14:34:54Z

 def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=False, include_child_info=False,
                       course_outline=False, include_children_predicate=NEVER, parent_xblock=None, graders=None,
                       user=None, course=None):
+


Minor: don't need the new beeline here.

robrap · 2016-02-19T15:05:17Z

 from openedx.core.djangolib.markup import HTML, ugettext as _
 %>
-<%block name="title">${xblock.display_name_with_default_escaped} ${xblock_type_display_name(xblock) | h}</%block>
+<%block name="title">${xblock.display_name_with_default | h} ${xblock_type_display_name(xblock) | h}</%block>


@agaylard - As @mushtaqak pointed out, we are moving toward making all templates "safe by default" by adding a page directive like you can see in index.html in this PR. Here's a comment about some things to look for if you make these changes.

As I noted earlier, more instructions will be coming and this will be a requirement in the future, but it isn't now. It's up to you if you want us to help chip away at this. However, it is important for you (and everyone) to understand that there is nothing special about this particular field, and we will want all fields to be escaped properly.

Thanks.

@robrap Yeah sure. Whatever you need me to for this, I will gladly help out :)

Thanks @mushtaqak. I actually took care of this in a different PR.

FYI: the instructions for this will soon be available in Read the Docs, and they will have changed slightly for internationalization with _() and HTML().

robrap · 2016-02-19T15:13:59Z

@agaylard Thanks you. This all looks good as is, assuming you searched and found all usages.

As noted above, you are welcome to take one fixing one, some, or all of the templates above as described. Either way, I wanted you to be informed. Just let me know your intention and I'll give you a thumbs up now, or I can help get the additions reviewed for you.

Also, once this is merged, it would be nice to send an FYI to the community about the change for outside XBlocks. Thanks again.

agaylard · 2016-02-19T15:44:02Z

@robrap , thanks for the feedback. I'm keen to get this PR merged and close TNL-3429, so I'd like to call a halt. For now, anyway.

agaylard · 2016-02-19T15:53:49Z

jenkins run js

robrap · 2016-02-19T16:01:33Z

@agaylard Understood. Please take 5-10 minutes to review the comments and helper pages related to this topic so you are informed for moving forward.

Also, I decided to do a quick search for xblockInfo.get('display_name') and found a match in xblock-outline.underscore that looks like it needs to be fixed.

Additionally, it is used in several .js files (e.g. edit_xblock.js, container.js, edit_tabs.js, and library.js). Please check how and where each of these are using the display name to make sure that it doesn't need escaping in other places, but possibly under a different name. Thank you.

agaylard · 2016-02-19T16:21:22Z

@robrap : got it, will do.

smarnach · 2016-02-19T22:57:32Z

@agaylard Code changes look good. I'm waiting for the sandbox provisioning to finish before testing manually.

smarnach · 2016-02-21T12:37:13Z

@agaylard I've added links to the sandbox to the PR description and tested the changes manually. All working fine, 👍.

Kelketek · 2016-03-01T20:14:43Z

@robrap @agaylard has left our team, sorry to say. As this used his Repo, I've opened a new copy of this PR with a slight fix here: https://github.com/edx/edx-platform/pull/11697 This PR will supersede that one, and I am closing this one now.

Fix TNL-3429.

f135d63

robrap reviewed Feb 18, 2016
View reviewed changes

Andrew Gaylard added 2 commits February 19, 2016 09:05

Fix pylint wrinkles.

7c81088

Add missing |h filter.

a0c3bb5

Remove selective escape_html logic.

70ff67c

robrap reviewed Feb 19, 2016
View reviewed changes

Remove empty line.

5f39190

robrap reviewed Feb 19, 2016
View reviewed changes

Kelketek mentioned this pull request Feb 29, 2016

TNL-3429: implement display_name_with_default for studio text sanitation #11697

Merged

1 task

Kelketek closed this Mar 1, 2016

Uh oh!

Conversation

agaylard commented Feb 18, 2016

Uh oh!

openedx-webhooks commented Feb 18, 2016

Uh oh!

robrap Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

agaylard Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

robrap commented Feb 18, 2016

Uh oh!

mushtaqak commented Feb 19, 2016

Uh oh!

robrap commented Feb 19, 2016

Uh oh!

robrap Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

robrap Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

mushtaqak Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

robrap Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

robrap commented Feb 19, 2016

Uh oh!

agaylard commented Feb 19, 2016

Uh oh!

agaylard commented Feb 19, 2016

Uh oh!

robrap commented Feb 19, 2016

Uh oh!

agaylard commented Feb 19, 2016

Uh oh!

smarnach commented Feb 19, 2016

Uh oh!

smarnach commented Feb 21, 2016

Uh oh!

Kelketek commented Mar 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants