diff --git a/frontend_tests/casper_lib/common.js b/frontend_tests/casper_lib/common.js index 0024d4efdd..b6d41e1058 100644 --- a/frontend_tests/casper_lib/common.js +++ b/frontend_tests/casper_lib/common.js @@ -2,6 +2,8 @@ var util = require("util"); var test_credentials = require('../../var/casper/test_credentials.js').test_credentials; +casper.options.clientScripts.push("frontend_tests/casper_lib/polyfill.js"); + function timestamp() { return new Date().getTime(); } diff --git a/frontend_tests/casper_lib/polyfill.js b/frontend_tests/casper_lib/polyfill.js new file mode 100644 index 0000000000..62cd4a5047 --- /dev/null +++ b/frontend_tests/casper_lib/polyfill.js @@ -0,0 +1,12 @@ +/* eslint-env browser */ + +// PhantomJS doesn’t support new DOMParser().parseFromString(…, "text/html"). +var real_parseFromString = DOMParser.prototype.parseFromString; +DOMParser.prototype.parseFromString = function (string, type) { + if (type === "text/html") { + var doc = document.implementation.createHTMLDocument(""); + doc.documentElement.innerHTML = string; + return doc; + } + return real_parseFromString.apply(this, arguments); +}; diff --git a/frontend_tests/node_tests/compose.js b/frontend_tests/node_tests/compose.js index 6a3413524d..25961c090f 100644 --- a/frontend_tests/node_tests/compose.js +++ b/frontend_tests/node_tests/compose.js @@ -1,3 +1,5 @@ +const { JSDOM } = require("jsdom"); + set_global('bridge', false); set_global('blueslip', global.make_zblueslip({ @@ -7,6 +9,7 @@ set_global('blueslip', global.make_zblueslip({ const noop = function () {}; set_global('$', global.make_zjquery()); +set_global('DOMParser', new JSDOM().window.DOMParser); set_global('i18n', global.stub_i18n); const _navigator = { diff --git a/frontend_tests/node_tests/copy_and_paste.js b/frontend_tests/node_tests/copy_and_paste.js index 44d146afa5..4420fd1421 100644 --- a/frontend_tests/node_tests/copy_and_paste.js +++ b/frontend_tests/node_tests/copy_and_paste.js @@ -7,8 +7,9 @@ set_global('compose_ui', {}); const { JSDOM } = require("jsdom"); const { window } = new JSDOM('

Hello world

'); -const { document } = window; +const { DOMParser, document } = window; set_global('$', require('jquery')(window)); +set_global('DOMParser', DOMParser); set_global('document', document); const copy_and_paste = zrequire('copy_and_paste'); diff --git a/frontend_tests/node_tests/templates.js b/frontend_tests/node_tests/templates.js index 21fd7457f9..0742dea8b4 100644 --- a/frontend_tests/node_tests/templates.js +++ b/frontend_tests/node_tests/templates.js @@ -9,6 +9,7 @@ zrequire('stream_edit'); const { JSDOM } = require("jsdom"); const { window } = new JSDOM(); global.$ = require('jquery')(window); +set_global('DOMParser', window.DOMParser); // When writing these tests, the following command might be helpful: // ./tools/get-handlebar-vars static/templates/*.hbs diff --git a/frontend_tests/node_tests/util.js b/frontend_tests/node_tests/util.js index 44802f1b52..67af1ed781 100644 --- a/frontend_tests/node_tests/util.js +++ b/frontend_tests/node_tests/util.js @@ -1,4 +1,7 @@ +const { JSDOM } = require("jsdom"); + set_global('$', global.make_zjquery()); +set_global('DOMParser', new JSDOM().window.DOMParser); set_global('blueslip', global.make_zblueslip({})); set_global('document', {}); @@ -301,3 +304,17 @@ run_test('move_array_elements_to_front', () => { assert(emails_actual[i] === emails_expected[i]); } }); + +run_test("clean_user_content_links", () => { + window.location.href = "http://zulip.zulipdev.com/"; + assert.equal( + util.clean_user_content_links( + 'good ' + + 'invalid ' + + 'fragment' + ), + 'good ' + + 'invalid ' + + 'fragment' + ); +}); diff --git a/static/js/compose.js b/static/js/compose.js index f40c33e05f..598adfb34c 100644 --- a/static/js/compose.js +++ b/static/js/compose.js @@ -736,7 +736,7 @@ exports.render_and_show_preview = function (preview_spinner, preview_content_box rendered_preview_html = rendered_content; } - preview_content_box.html(rendered_preview_html); + preview_content_box.html(util.clean_user_content_links(rendered_preview_html)); if (page_params.emojiset === "text") { preview_content_box.find(".emoji").replaceWith(function () { const text = $(this).attr("title"); diff --git a/static/js/stream_edit.js b/static/js/stream_edit.js index ee86057338..af50cb26ed 100644 --- a/static/js/stream_edit.js +++ b/static/js/stream_edit.js @@ -1,3 +1,4 @@ +const util = require("./util"); const render_settings_deactivation_stream_modal = require("../templates/settings/deactivation_stream_modal.hbs"); const render_stream_member_list_entry = require('../templates/stream_member_list_entry.hbs'); const render_subscription_settings = require('../templates/subscription_settings.hbs'); @@ -111,7 +112,9 @@ exports.update_stream_name = function (sub, new_name) { exports.update_stream_description = function (sub) { const stream_settings = exports.settings_for_sub(sub); stream_settings.find('input.description').val(sub.description); - stream_settings.find('.stream-description-editable').html(sub.rendered_description); + stream_settings.find('.stream-description-editable').html( + util.clean_user_content_links(sub.rendered_description) + ); }; exports.invite_user_to_stream = function (user_email, sub, success, failure) { @@ -458,7 +461,9 @@ exports.change_stream_description = function (e) { $(".stream_change_property_info")); }, error: function (xhr) { - sub_settings.find('.stream-description-editable').html(sub.rendered_description); + sub_settings.find('.stream-description-editable').html( + util.clean_user_content_links(sub.rendered_description) + ); ui_report.error(i18n.t("Error"), xhr, $(".stream_change_property_info")); }, }); diff --git a/static/js/subs.js b/static/js/subs.js index 12e5ba0288..2e002637dd 100644 --- a/static/js/subs.js +++ b/static/js/subs.js @@ -1,3 +1,4 @@ +const util = require('./util'); const render_subscription = require('../templates/subscription.hbs'); const render_subscription_settings = require('../templates/subscription_settings.hbs'); const render_subscription_table_body = require('../templates/subscription_table_body.hbs'); @@ -162,7 +163,7 @@ exports.update_stream_description = function (sub, description, rendered_descrip // Update stream row const sub_row = exports.row_for_stream_id(sub.stream_id); - sub_row.find(".description").html(sub.rendered_description); + sub_row.find(".description").html(util.clean_user_content_links(sub.rendered_description)); // Update stream settings stream_edit.update_stream_description(sub); diff --git a/static/js/templates.js b/static/js/templates.js index b0bf200ed0..a4dd99061d 100644 --- a/static/js/templates.js +++ b/static/js/templates.js @@ -1,3 +1,5 @@ +const util = require("./util"); + // Below, we register Zulip-specific extensions to the handlebars API. // // IMPORTANT: When adding a new handlebars helper, update the @@ -73,4 +75,9 @@ Handlebars.registerHelper('tr', function (context, options) { return new Handlebars.SafeString(result); }); +Handlebars.registerHelper( + "rendered_markdown", + content => new Handlebars.SafeString(util.clean_user_content_links(content)) +); + window.templates = exports; diff --git a/static/js/util.js b/static/js/util.js index adb5439104..961d00e14d 100644 --- a/static/js/util.js +++ b/static/js/util.js @@ -347,4 +347,36 @@ exports.convert_message_topic = function (message) { } }; +exports.clean_user_content_links = function (html) { + const content = new DOMParser().parseFromString(html, "text/html").body; + for (const elt of content.getElementsByTagName("a")) { + // Ensure that all external links have target="_blank" + // rel="opener noreferrer". This ensures that external links + // never replace the Zulip webapp while also protecting + // against reverse tabnapping attacks, without relying on the + // correctness of how Zulip's markdown processor generates links. + // + // Fragment links, which we intend to only open within the + // Zulip webapp using our hashchange system, do not require + // these attributes. + let url; + try { + url = new URL(elt.getAttribute("href"), window.location.href); + } catch { + elt.removeAttribute("href"); + continue; + } + + // We detect URLs that are just fragments by comparing the URL + // against a new URL generated using only the hash. + if (url.hash === "" || url.href !== new URL(url.hash, window.location.href).href) { + elt.setAttribute("target", "_blank"); + elt.setAttribute("rel", "noopener noreferrer"); + } else { + elt.removeAttribute("target"); + } + } + return content.innerHTML; +}; + window.util = exports; diff --git a/static/templates/draft.hbs b/static/templates/draft.hbs index d4f97c795e..8271d72b55 100644 --- a/static/templates/draft.hbs +++ b/static/templates/draft.hbs @@ -36,7 +36,7 @@ -
{{{content}}}
+
{{rendered_markdown content}}
diff --git a/static/templates/me_message.hbs b/static/templates/me_message.hbs index fb4fbd5110..1ff0d1b168 100644 --- a/static/templates/me_message.hbs +++ b/static/templates/me_message.hbs @@ -9,9 +9,7 @@ {{/if}} - - {{{ status_message }}} - + {{rendered_markdown status_message}} {{#if edited_status_msg}} {{> edited_notice}} diff --git a/static/templates/message_body.hbs b/static/templates/message_body.hbs index c79f2a746a..d170205c94 100644 --- a/static/templates/message_body.hbs +++ b/static/templates/message_body.hbs @@ -36,7 +36,7 @@ {{#unless status_message}} -
{{#if use_match_properties}}{{{msg/match_content}}}{{else}}{{{msg/content}}}{{/if}}
+
{{#if use_match_properties}}{{rendered_markdown msg/match_content}}{{else}}{{rendered_markdown msg/content}}{{/if}}
{{/unless}} {{#if edited_in_left_col}} diff --git a/static/templates/message_edit_form.hbs b/static/templates/message_edit_form.hbs index d5aab0574d..e8dd4251ae 100644 --- a/static/templates/message_edit_form.hbs +++ b/static/templates/message_edit_form.hbs @@ -24,7 +24,7 @@ - + diff --git a/static/templates/subscription.hbs b/static/templates/subscription.hbs index 1c68826fa9..bcbd6fa639 100644 --- a/static/templates/subscription.hbs +++ b/static/templates/subscription.hbs @@ -16,7 +16,7 @@
-
{{{rendered_description}}}
+
{{rendered_markdown rendered_description}}
{{#if is_old_stream}}
diff --git a/static/templates/subscription_settings.hbs b/static/templates/subscription_settings.hbs index 55984a3c42..02f08031de 100644 --- a/static/templates/subscription_settings.hbs +++ b/static/templates/subscription_settings.hbs @@ -33,7 +33,7 @@
- {{{rendered_description}}} + {{rendered_markdown rendered_description}} {{#if can_change_name_description}} diff --git a/static/templates/user_profile_modal.hbs b/static/templates/user_profile_modal.hbs index 84b4db1135..a72038b6bb 100644 --- a/static/templates/user_profile_modal.hbs +++ b/static/templates/user_profile_modal.hbs @@ -55,7 +55,7 @@ {{this.value}} {{else}} {{#if this.rendered_value}} -
{{{this.rendered_value}}}
+
{{rendered_markdown this.rendered_value}}
{{else}}
{{this.value}}
{{/if}} diff --git a/tools/linter_lib/custom_check.py b/tools/linter_lib/custom_check.py index 13c9c95175..4039cfa352 100644 --- a/tools/linter_lib/custom_check.py +++ b/tools/linter_lib/custom_check.py @@ -136,7 +136,7 @@ js_rules = RuleList( 'exclude': {'frontend_tests/'}, 'description': '.includes() is incompatible with Internet Explorer. Use .indexOf() !== -1 instead.'}, {'pattern': '[.]html[(]', - 'exclude_pattern': r'''[.]html[(]("|'|render_|html|message.content|sub.rendered_description|i18n.t|rendered_|$|[)]|error_text|widget_elem|[$]error|[$][(]"

"[)])''', + 'exclude_pattern': r'''\.html\(("|'|render_|html|message\.content|util\.clean_user_content_links|i18n\.t|rendered_|$|\)|error_text|widget_elem|\$error|\$\("

"\))''', 'exclude': {'static/js/portico', 'static/js/lightbox.js', 'static/js/ui_report.js', 'static/js/confirm_dialog.js', 'frontend_tests/'}, diff --git a/tools/webpack.config.ts b/tools/webpack.config.ts index 0a673e269c..285274cab6 100644 --- a/tools/webpack.config.ts +++ b/tools/webpack.config.ts @@ -110,7 +110,7 @@ export default (env?: string): webpack.Configuration[] => { knownHelpers: ['if', 'unless', 'each', 'with', // The ones below are defined in static/js/templates.js 'plural', 'eq', 'and', 'or', 'not', - 't', 'tr'], + 't', 'tr', 'rendered_markdown'], preventIndent: true, }, },