Skip to content

(WIP) Fix TNL-3429. #11588

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

Closed
agaylard wants to merge 5 commits into from
Closed

(WIP) Fix TNL-3429. #11588

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f135d63
Fix TNL-3429.
Feb 18, 2016
7c81088
Fix pylint wrinkles.
Feb 19, 2016
a0c3bb5
Add missing |h filter.
Feb 19, 2016
70ff67c
Remove selective escape_html logic.
Feb 19, 2016
5f39190
Remove empty line.
Feb 19, 2016
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
2 changes: 1 addition & 1 deletion cms/djangoapps/contentstore/views/component.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def container_handler(request, usage_key_string):

# Fetch the XBlock info for use by the container page. Note that it includes information
# about the block's ancestors and siblings for use by the Unit Outline.
xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page)
xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page, escape_html=False)

@robrap robrap Feb 18, 2016

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.

@agaylard Can you explain why you think this parameter is needed? I would assume that this name is never escaped until it is used in a template, at which point it would be escaped as needed using the proper filters based on context.

@agaylard agaylard Feb 19, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right -- it isn't needed. I've removed it in the commit titled 'Remove selective escape_html logic.'


if is_unit_page:
add_container_page_publishing_info(xblock, xblock_info)
Expand Down
7 changes: 4 additions & 3 deletions cms/djangoapps/contentstore/views/course.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ def course_handler(request, course_key_string=None):
course_key = CourseKey.from_string(course_key_string)
with modulestore().bulk_operations(course_key):
course_module = get_course_and_check_access(course_key, request.user, depth=None)
return JsonResponse(_course_outline_json(request, course_module))
return JsonResponse(_course_outline_json(request, course_module, escape_html=False))
elif request.method == 'POST': # not sure if this is only post. If one will have ids, it goes after access
return _create_or_rerun_course(request)
elif not has_studio_write_access(request.user, CourseKey.from_string(course_key_string)):
Expand Down Expand Up @@ -333,14 +333,15 @@ def course_search_index_handler(request, course_key_string):
}), content_type=content_type, status=200)


def _course_outline_json(request, course_module):
def _course_outline_json(request, course_module, escape_html=True):
"""
Returns a JSON representation of the course module and recursively all of its children.
"""
return create_xblock_info(
course_module,
include_child_info=True,
course_outline=True,
escape_html=escape_html,
include_children_predicate=lambda xblock: not xblock.category == 'vertical',
user=request.user
)
Expand Down Expand Up @@ -590,7 +591,7 @@ def course_index(request, course_key):
if settings.FEATURES.get('ENABLE_COURSEWARE_INDEX', False):
reindex_link = "/course/{course_id}/search_reindex".format(course_id=unicode(course_key))
sections = course_module.get_children()
course_structure = _course_outline_json(request, course_module)
course_structure = _course_outline_json(request, course_module, escape_html=False)
locator_to_show = request.REQUEST.get('show', None)
course_release_date = get_default_time_display(course_module.start) if course_module.start != DEFAULT_START_DATE else _("Unscheduled")
settings_url = reverse_course_url('settings_handler', course_key)
Expand Down
33 changes: 19 additions & 14 deletions cms/djangoapps/contentstore/views/item.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ def xblock_outline_handler(request, usage_key_string):
root_xblock,
include_child_info=True,
course_outline=True,
escape_html=False,
include_children_predicate=lambda xblock: not xblock.category == 'vertical'
))
else:
Expand All @@ -358,7 +359,7 @@ def xblock_container_handler(request, usage_key_string):
if response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'):
with modulestore().bulk_operations(usage_key.course_key):
response = _get_module_info(
_get_xblock(usage_key, request.user), include_ancestor_info=True, include_publishing_info=True
_get_xblock(usage_key, request.user), include_ancestor_info=True, include_publishing_info=True, escape_html=False
)
return JsonResponse(response)
else:
Expand Down Expand Up @@ -740,7 +741,7 @@ def _get_xblock(usage_key, user):
return JsonResponse({"error": "Can't find item by location: " + unicode(usage_key)}, 404)


def _get_module_info(xblock, rewrite_static_links=True, include_ancestor_info=False, include_publishing_info=False):
def _get_module_info(xblock, rewrite_static_links=True, include_ancestor_info=False, include_publishing_info=False, escape_html=True):
"""
metadata, data, id representation of a leaf module fetcher.
:param usage_key: A UsageKey
Expand All @@ -761,7 +762,7 @@ def _get_module_info(xblock, rewrite_static_links=True, include_ancestor_info=Fa

# Note that children aren't being returned until we have a use case.
xblock_info = create_xblock_info(
xblock, data=data, metadata=own_metadata(xblock), include_ancestor_info=include_ancestor_info
xblock, data=data, metadata=own_metadata(xblock), include_ancestor_info=include_ancestor_info, escape_html=escape_html
)
if include_publishing_info:
add_container_page_publishing_info(xblock, xblock_info)
Expand Down Expand Up @@ -803,7 +804,8 @@ def _get_gating_info(course, xblock):

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):
user=None, course=None, escape_html=True):

@robrap robrap Feb 19, 2016

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.

Minor: don't need the new beeline here.

"""
Creates the information needed for client-side XBlockInfo.

Expand Down Expand Up @@ -849,7 +851,8 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
graders,
include_children_predicate=include_children_predicate,
user=user,
course=course
course=course,
escape_html=escape_html
)
else:
child_info = None
Expand Down Expand Up @@ -886,7 +889,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F

xblock_info = {
"id": unicode(xblock.location),
"display_name": xblock.display_name_with_default_escaped,
"display_name": xblock.display_name_with_default_escaped if escape_html else xblock.display_name_with_default,

@robrap robrap Feb 18, 2016

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.

Why wouldn't this always be unescaped (i.e. xblock.display_name_with_default) and allow the template to escape how and if it needs to?

@agaylard agaylard Feb 19, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Correct; also fixed in 'Remove selective escape_html logic.'

"category": xblock.category,
"edited_on": get_default_time_display(xblock.subtree_edited_on) if xblock.subtree_edited_on else None,
"published": published,
Expand Down Expand Up @@ -939,7 +942,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
if metadata is not None:
xblock_info["metadata"] = metadata
if include_ancestor_info:
xblock_info['ancestor_info'] = _create_xblock_ancestor_info(xblock, course_outline)
xblock_info['ancestor_info'] = _create_xblock_ancestor_info(xblock, course_outline, escape_html)
if child_info:
xblock_info['child_info'] = child_info
if visibility_state == VisibilityState.staff_only:
Expand Down Expand Up @@ -1064,14 +1067,14 @@ def _compute_visibility_state(xblock, child_info, is_unit_with_changes, is_cours
return VisibilityState.ready


def _create_xblock_ancestor_info(xblock, course_outline):
def _create_xblock_ancestor_info(xblock, course_outline, escape_html=True):
"""
Returns information about the ancestors of an xblock. Note that the direct parent will also return
information about all of its children.
"""
ancestors = []

def collect_ancestor_info(ancestor, include_child_info=False):
def collect_ancestor_info(ancestor, include_child_info=False, escape_html=True):
"""
Collect xblock info regarding the specified xblock and its ancestors.
"""
Expand All @@ -1081,16 +1084,17 @@ def collect_ancestor_info(ancestor, include_child_info=False):
ancestor,
include_child_info=include_child_info,
course_outline=course_outline,
include_children_predicate=direct_children_only
include_children_predicate=direct_children_only,
escape_html=escape_html
))
collect_ancestor_info(get_parent_xblock(ancestor))
collect_ancestor_info(get_parent_xblock(xblock), include_child_info=True)
collect_ancestor_info(get_parent_xblock(ancestor), escape_html=escape_html)
collect_ancestor_info(get_parent_xblock(xblock), include_child_info=True, escape_html=escape_html)
return {
'ancestors': ancestors
}


def _create_xblock_child_info(xblock, course_outline, graders, include_children_predicate=NEVER, user=None, course=None): # pylint: disable=line-too-long
def _create_xblock_child_info(xblock, course_outline, graders, include_children_predicate=NEVER, user=None, course=None, escape_html=True): # pylint: disable=line-too-long
"""
Returns information about the children of an xblock, as well as about the primary category
of xblock expected as children.
Expand All @@ -1111,6 +1115,7 @@ def _create_xblock_child_info(xblock, course_outline, graders, include_children_
graders=graders,
user=user,
course=course,
escape_html=escape_html
) for child in xblock.get_children()
]
return child_info
Expand Down Expand Up @@ -1158,4 +1163,4 @@ def _xblock_type_and_display_name(xblock):
"""
return _('{section_or_subsection} "{display_name}"').format(
section_or_subsection=xblock_type_display_name(xblock),
display_name=xblock.display_name_with_default_escaped)
display_name=xblock.display_name_with_default)
8 changes: 4 additions & 4 deletions cms/templates/container.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
)
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>

@robrap robrap Feb 18, 2016

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.

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

@agaylard agaylard Feb 19, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oops, yes. Fixed.

<%block name="bodyclass">is-signedin course container view-container</%block>

<%namespace name='static' file='static_content.html'/>
Expand Down Expand Up @@ -57,15 +57,15 @@
ancestor_url = xblock_studio_url(ancestor)
%>
% if ancestor_url:
<a href="${ancestor_url | h}" class="navigation-item navigation-link navigation-parent">${ancestor.display_name_with_default_escaped | h}</a>
<a href="${ancestor_url | h}" class="navigation-item navigation-link navigation-parent">${ancestor.display_name_with_default | h}</a>
% else:
<span class="navigation-item navigation-parent">${ancestor.display_name_with_default_escaped | h}</span>
<span class="navigation-item navigation-parent">${ancestor.display_name_with_default | h}</span>
% endif
% endfor
</small>
<div class="wrapper-xblock-field incontext-editor is-editable"
data-field="display_name" data-field-display-name="${_("Display Name")}">
<h1 class="page-header-title xblock-field-value incontext-editor-value"><span class="title-value">${xblock.display_name_with_default_escaped | h}</span></h1>
<h1 class="page-header-title xblock-field-value incontext-editor-value"><span class="title-value">${xblock.display_name_with_default | h}</span></h1>
</div>
</div>

Expand Down
4 changes: 2 additions & 2 deletions cms/templates/js/course-outline.underscore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@ if (is_proctored_exam) {
<% } %>
<% if (xblockInfo.isVertical()) { %>
<span class="unit-title item-title">
<a href="<%= xblockInfo.get('studio_url') %>"><%= xblockInfo.get('display_name') %></a>
<a href="<%= xblockInfo.get('studio_url') %>"><%- xblockInfo.get('display_name') %></a>

@robrap robrap Feb 19, 2016

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.

@agaylard For Underscore templates, there is no equivalent syntax for making the page safe by default. Instead, it should be understood that using <%- should be the rule, and using <%= should be the rare exception, so we'd be switching almost all of these, rather than a few particular ones.

Again, this isn't a requirement for this PR, but it certainly would be helpful if you want to wrap up that work. Of course, as you know, the Mako and underscore templates are related so if you escape here you need to make sure the data is not escaped when sent from Mako to JavaScript.

</span>
<% } else { %>
<span class="wrapper-<%= xblockType %>-title wrapper-xblock-field incontext-editor is-editable" data-field="display_name" data-field-display-name="<%= gettext("Display Name") %>">
<span class="<%= xblockType %>-title item-title xblock-field-value incontext-editor-value"><%= xblockInfo.get('display_name') %></span>
<span class="<%= xblockType %>-title item-title xblock-field-value incontext-editor-value"><%- xblockInfo.get('display_name') %></span>
</span>
<% } %>
</h3>
Expand Down
2 changes: 1 addition & 1 deletion cms/templates/js/unit-outline.underscore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<div class="<%= xblockType %>-header">
<h3 class="<%= xblockType %>-header-details">
<span class="<%= xblockType %>-title item-title">
<a href="<%= xblockInfo.get('studio_url') %>"><%= xblockInfo.get('display_name') %></a>
<a href="<%= xblockInfo.get('studio_url') %>"><%- xblockInfo.get('display_name') %></a>
</span>
</h3>
</div>
Expand Down