Audit .html() calls take #2 #5175
Conversation
| @@ -447,6 +446,8 @@ var IPython = (function (IPython) { | |||
| var pre = this.element.find('div.'+subclass).last().find('pre'); | |||
| var html = utils.fixCarriageReturn( | |||
| pre.html() + utils.fixConsole(text)); | |||
| // The only user content injected with with this HTML call is | |||
|
@minrk I moved the todos into the |
| @@ -245,6 +245,8 @@ var IPython = (function (IPython) { | |||
| * @method set_rendered | |||
| */ | |||
| TextCell.prototype.set_rendered = function(text) { | |||
| // TODO: This HTML needs to be treated as potentially dangerous | |||
| // user input. | |||
There was a problem hiding this comment.
This suggests that this method should be made safe in a future fix, which I do not think is true. set_rendered ought to be treated as an unsafe method, and any user html should be cleaned before calling set_rendered.
|
@minrk the last commit treats |
| rendered.append( | ||
| $("<div/>") | ||
| .append($("<div/>").text('Error rendering Markdown!').addClass("js-error")) | ||
| .append($("<div/>").text(e.toString()).addClass("js-error")) |
There was a problem hiding this comment.
this block should go back to calling set_rendered
There was a problem hiding this comment.
Is there a possibility that e.toString() can have use injected html? Some sort of manipulation of the input such that it throws an error with user content (should I add a TODO here?)
There was a problem hiding this comment.
Don't revert the whole change, you can still use .text here, just use set_rendered instead of rendered.append (since rendered is undefined now).
|
I'm going to give this a test locally to make sure things still work. |
|
This works locally |
|
I think this looks good. Have you gone through all the |
Yes, there are quite a few |
| element | ||
| .append($('<div/>').text(msg).addClass('js-error')) | ||
| .append($('<div/>').text(err.toString()).addClass('js-error')) | ||
| .append($('<div/>').text('See your browser Javascript console for more details.').addClass('js-error')); |
There was a problem hiding this comment.
Is this missing the leading <br/> tag from above?
There was a problem hiding this comment.
don't need br because there are multiple divs, where there was one div with explicit br before.
|
A few comments, but probably ready to go. |
Audit .html() calls take ipython#2
sequel to #5041
closes #5034