launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

196	+ success: function (diff_text) {
197	+ container.set('innerHTML', diff_text);
198	+ (new rc.NumberToggle()).render();
199	+ namespace.setup_inline_comments(previewdiff_id);
200	+ },
201	+ failure: function(ignore, response, args) {
202	+ container.set('innerHTML', response.statusText);
203	+ Y.lp.anim.red_flash({node: diff_node}).run();
204	+ },

Anywhere that sets innerHTML needs to be ultra-careful, as it's a really easy way to inadvertently introduce a security vulnerability.

For the success case, I'd name the variable diff_html rather than diff_text. For the failure case, the message should just be text so don't use innerHTML.
-- 
https://code.launchpad.net/~cprov/launchpad/diff-navigator/+merge/208186
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.