TNL-3429: implement display_name_with_default for studio text sanitation - #11697
Conversation
|
Thanks for the pull request, @Kelketek! 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?number=11697&repo=edx%2Fedx-platform |
|
@smarnach The only required note I saw on the previous copy of this PR was to change a straggling template. Can you confirm, and that this PR addresses that note? |
|
@Kelketek I went through all the comments as well, and as far as I can tell this one was the only one still needing attention. You addressed the point mentioned in the second paragraph of that comment. I can't tell whether you followed the suggestion in the last paragraph. If you did, I think you can ping upstream again on TNL-3429 (and point them to the new PR). |
|
Thanks @Kelketek @smarnach. I've got the same open questions, so looking for confirmation. Also, I'm hoping to land the following PR tomorrow, which hits the changes in container.html, but uses the page directive to make the entire template safe by default: https://github.com/edx/edx-platform/pull/11638/files |
|
@robrap I did find a straggling issue or two, which I made commits for. But the items you mentioned seem to work correctly now after updating that template, and the extra issue I found with the xblock display name label in studio (which you can check by loading up a new HTML block and setting its display name to have extra characters) |
|
@Kelketek This is good to go. 👍 When you rebase/squash to resolve conflicts, you should just be able to accept all my changes for cms/templates/container.html. FYI: there are no |
e3a0208 to
4c110c3
Compare
4c110c3 to
f1c1c7c
Compare
|
@smarnach As this is a new PR, I will still need your explicit +1 on this. It has been rebased and should be ready for merge if tests pass and I have your OK. |
|
@Kelketek I still think this looks good, but was searching around to make sure we weren't missing anything. Can you confirm if you manually tested in a content library? I think this is ultimately again using |
|
@robrap Yes, I have checked. Just went to double-check. It does seem to work correctly. |
|
Sounds good. Feel free to merge with a second thumb. Thank you. |
|
👍 |
TNL-3429: implement display_name_with_default for studio text sanitation
4 participants
JIRA tickets: TNL-3429
Discussion:
This PR replaces https://github.com/edx/edx-platform/pull/11588. 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:
These two templates are changed to HTML-escape the display name fields:
Dependencies: None
Sandbox: LMS Studio
Testing instructions:
Note: in the screenshots, the fields of interest are highlighted in purple.
§: Intro to <HTML> & "JS"§§: Intro to <HTML> & "JS". Both names are rendered correctly on the course outline page.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