Skip to content

Make TinyMCE more tolerant about HTML elements #3437

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
andy-armstrong merged 2 commits into from
Apr 23, 2014
Merged

Make TinyMCE more tolerant about HTML elements #3437

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
b5e9961
Make TinyMCE more tolerant about HTML elements
andy-armstrong Apr 22, 2014
4cfa280
Remove verify_html as it isn't supported in TinyMCE 4
andy-armstrong Apr 23, 2014
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
22 changes: 21 additions & 1 deletion cms/djangoapps/contentstore/features/html-editor.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,26 @@ Feature: CMS.HTML Editor
--></style>
"""

Scenario: TinyMCE and CodeMirror preserve span tags

@cahrens cahrens Apr 23, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Did you go through Jennifer's reports to see if additional test cases can be added? We can always do that after the release as well...

@andy-armstrong andy-armstrong Apr 23, 2014

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 skimmed through them yesterday and didn't see anything different to what we have been seeing (spans, links, script tags etc). I'll do a more thorough analysis this morning, and I'll add tests for anything else I find.

Given I have created a Blank HTML Page
When I edit the page
And type "<span>Test</span>" in the code editor and press OK
And I save the page
Then the page text contains:
"""
<span>Test</span>
"""

Scenario: TinyMCE and CodeMirror preserve math tags
Given I have created a Blank HTML Page
When I edit the page
And type "<math><msup><mi>x</mi><mn>2</mn></msup></math>" in the code editor and press OK
And I save the page
Then the page text contains:
"""
<math><msup><mi>x</mi><mn>2</mn></msup></math>
"""

Scenario: TinyMCE toolbar buttons are as expected
Given I have created a Blank HTML Page
When I edit the page
Expand All @@ -57,7 +77,7 @@ Feature: CMS.HTML Editor
When I edit the page
And type "<img src="/static/image.jpg">" in the code editor and press OK
Then the src link is rewritten to "c4x/MITx/999/asset/image.jpg"
And the code editor displays "<p><img src="/static/image.jpg" alt="" /></p>"
And the code editor displays "<p><img src="/static/image.jpg" /></p>"

@nasthagiri nasthagiri Apr 23, 2014

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.

Is it intended behavior for the code editor to add the "p" and "/p" tags around the img?

@cahrens cahrens Apr 23, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this is fine (and would be impossible to prevent).

@andy-armstrong andy-armstrong Apr 23, 2014

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.

This is just something that TinyMCE does to clean up the HTML, so there's nothing we can do about it (I tried all the options I could find). Unfortunately this is one of the complaints we have received, because users don't like their HTML getting extra tags. It is especially bad when it decides to turn trailing whitespace into

 

.


Scenario: Code format toolbar button wraps text with code tags
Given I have created a Blank HTML Page
Expand Down
7 changes: 7 additions & 0 deletions common/lib/xmodule/xmodule/js/src/html/edit.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,15 @@ class @HTMLEditingDescriptor
height: '400px',
menubar: false,
statusbar: false,

# Necessary to avoid stripping of style tags.
valid_children : "+body[style]",

@cahrens cahrens Apr 23, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this still necessary given the changes below?

@andy-armstrong andy-armstrong Apr 23, 2014

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 thought not, but when I removed it the style test failed. That is rather baffling, but I decided it safer to leave it.

I also tried putting "script" in here too in case that helped with script tags, but it didn't.


# Allow any elements to be used, e.g. link, script, math
valid_elements: "*[*]",

@cahrens cahrens Apr 23, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In the StackOverflow post you sent me, someone recommended adding a + to allow any children. Did you try that?

@andy-armstrong andy-armstrong Apr 23, 2014

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 did try it and it didn't seem to make a difference to any of my test cases. The TinyMCE documentation recommends *[*] so that's what I went with.

@nasthagiri nasthagiri Apr 23, 2014

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.

Can you convince me why I shouldn't be super-scared about these changes?

We are no longer validating HTML and we are now allowing Script tags. Does this not expose us to XSS cross-site security issues? For example, would a rogue course editor be able to insert code that takes your edX cookies and sends them to another server for spoofing attacks?

@nasthagiri nasthagiri Apr 23, 2014

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.

On the TinyMCE website, they recommend filtering HTML input with tools such as HTMLPurifier to prevent XSS vulnerabilities: http://www.tinymce.com/wiki.php/FAQ

Here's a forum where they point to their FAQ: http://www.tinymce.com/forum/viewtopic.php?id=20207

@cahrens cahrens Apr 23, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We are not exposing any vulnerabilities that did not exist before the TinyMCE upgrade. It doesn't make sense to limit just what users input thorough TinyMCE given that they can enter anything in other components, and import anything via XML.

@andy-armstrong andy-armstrong Apr 23, 2014

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.

@nasthagiri The problem here is that we have already set the expectation that course developers can put arbitrary stuff in their HTML. The reason for this bug fix is precisely that we are stripping out their script tags etc and they need them to be there. I think that rogue editors are much less likely than on a public website, but I don't see that there's much we can do on that front.

extended_valid_elements: "*[*]",
invalid_elements: "",

setup: @setupTinyMCE,
# Cannot get access to tinyMCE Editor instance (for focusing) until after it is rendered.
# The tinyMCE callback passes in the editor as a parameter.
Expand Down