From edf34ada6380c83be48d52fedc5ad96f06272e6f Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Tue, 23 Jul 2024 12:07:39 -0700 Subject: [PATCH] util: Rename clean_user_content_links to postprocess_content. Signed-off-by: Anders Kaseorg --- tools/linter_lib/custom_check.py | 2 +- web/src/compose.js | 3 +- web/src/info_overlay.ts | 4 +- web/src/postprocess_content.ts | 88 +++++++++++++++++++++++++++ web/src/stream_edit.js | 4 +- web/src/stream_settings_ui.js | 4 +- web/src/templates.js | 4 +- web/src/util.ts | 88 --------------------------- web/tests/postprocess_content.test.js | 33 ++++++++++ web/tests/util.test.js | 25 -------- 10 files changed, 132 insertions(+), 123 deletions(-) create mode 100644 web/src/postprocess_content.ts create mode 100644 web/tests/postprocess_content.test.js diff --git a/tools/linter_lib/custom_check.py b/tools/linter_lib/custom_check.py index a183d8d9ed..832b45795b 100644 --- a/tools/linter_lib/custom_check.py +++ b/tools/linter_lib/custom_check.py @@ -139,7 +139,7 @@ js_rules = RuleList( {"pattern": r"\+.*\$t\(.+\)", "description": "Do not concatenate i18n strings"}, { "pattern": "[.]html[(]", - "exclude_pattern": r"""\.html\(("|'|render_|\w+_html|html|message\.content|util\.clean_user_content_links|rendered_|$|\)|error_html|widget_elem|\$error|\$\("

"\))""", + "exclude_pattern": r"""\.html\(("|'|render_|\w+_html|html|message\.content|postprocess_content|rendered_|$|\)|error_html|widget_elem|\$error|\$\("

"\))""", "exclude": { "web/src/portico", "web/src/lightbox.ts", diff --git a/web/src/compose.js b/web/src/compose.js index 7cc3d0bb78..6b5b8d2fab 100644 --- a/web/src/compose.js +++ b/web/src/compose.js @@ -21,6 +21,7 @@ import * as markdown from "./markdown"; import * as message_events from "./message_events"; import * as onboarding_steps from "./onboarding_steps"; import * as people from "./people"; +import {postprocess_content} from "./postprocess_content"; import * as rendered_markdown from "./rendered_markdown"; import * as scheduled_messages from "./scheduled_messages"; import * as sent_messages from "./sent_messages"; @@ -350,7 +351,7 @@ export function render_and_show_preview($preview_spinner, $preview_content_box, rendered_preview_html = rendered_content; } - $preview_content_box.html(util.clean_user_content_links(rendered_preview_html)); + $preview_content_box.html(postprocess_content(rendered_preview_html)); rendered_markdown.update_elements($preview_content_box); } diff --git a/web/src/info_overlay.ts b/web/src/info_overlay.ts index 163b70b028..69be3cc39b 100644 --- a/web/src/info_overlay.ts +++ b/web/src/info_overlay.ts @@ -13,11 +13,11 @@ import * as keydown_util from "./keydown_util"; import * as markdown from "./markdown"; import * as overlays from "./overlays"; import {page_params} from "./page_params"; +import {postprocess_content} from "./postprocess_content"; import * as rendered_markdown from "./rendered_markdown"; import * as scroll_util from "./scroll_util"; import {current_user} from "./state_data"; import {user_settings} from "./user_settings"; -import * as util from "./util"; // Make it explicit that our toggler is undefined until // set_up_toggler is called. @@ -269,7 +269,7 @@ export function set_up_toggler(): void { raw_content: row.markdown, ...markdown.render(row.markdown), }; - row.output_html = util.clean_user_content_links(message.content); + row.output_html = postprocess_content(message.content); } } diff --git a/web/src/postprocess_content.ts b/web/src/postprocess_content.ts new file mode 100644 index 0000000000..77ab47e162 --- /dev/null +++ b/web/src/postprocess_content.ts @@ -0,0 +1,88 @@ +import {$t} from "./i18n"; + +let inertDocument: Document | undefined; + +export function postprocess_content(html: string): string { + if (inertDocument === undefined) { + inertDocument = new DOMParser().parseFromString("", "text/html"); + } + const template = inertDocument.createElement("template"); + template.innerHTML = html; + + for (const elt of template.content.querySelectorAll("a")) { + // Ensure that all external links have target="_blank" + // rel="opener noreferrer". This ensures that external links + // never replace the Zulip web app 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 web app using our hashchange system, do not require + // these attributes. + const href = elt.getAttribute("href"); + if (href === null) { + continue; + } + let url; + try { + url = new URL(href, window.location.href); + } catch { + elt.removeAttribute("href"); + elt.removeAttribute("title"); + continue; + } + + // eslint-disable-next-line no-script-url + if (["data:", "javascript:", "vbscript:"].includes(url.protocol)) { + // Remove unsafe links completely. + elt.removeAttribute("href"); + elt.removeAttribute("title"); + 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"); + } + + if (elt.parentElement?.classList.contains("message_inline_image")) { + // For inline images we want to handle the tooltips explicitly, and disable + // the browser's built in handling of the title attribute. + const title = elt.getAttribute("title"); + if (title !== null) { + elt.setAttribute("aria-label", title); + elt.removeAttribute("title"); + } + } else { + // For non-image user uploads, the following block ensures that the title + // attribute always displays the filename as a security measure. + let title: string; + let legacy_title: string; + if ( + url.origin === window.location.origin && + url.pathname.startsWith("/user_uploads/") + ) { + // We add the word "download" to make clear what will + // happen when clicking the file. This is particularly + // important in the desktop app, where hovering a URL does + // not display the URL like it does in the web app. + title = legacy_title = $t( + {defaultMessage: "Download {filename}"}, + {filename: url.pathname.slice(url.pathname.lastIndexOf("/") + 1)}, + ); + } else { + title = url.toString(); + legacy_title = href; + } + elt.setAttribute( + "title", + ["", legacy_title].includes(elt.title) ? title : `${title}\n${elt.title}`, + ); + } + } + return template.innerHTML; +} diff --git a/web/src/stream_edit.js b/web/src/stream_edit.js index 149229860f..b2d2e5fc23 100644 --- a/web/src/stream_edit.js +++ b/web/src/stream_edit.js @@ -20,6 +20,7 @@ import {$t, $t_html} from "./i18n"; import * as keydown_util from "./keydown_util"; import * as narrow_state from "./narrow_state"; import * as popovers from "./popovers"; +import {postprocess_content} from "./postprocess_content"; import * as scroll_util from "./scroll_util"; import * as settings_components from "./settings_components"; import * as settings_config from "./settings_config"; @@ -38,7 +39,6 @@ import * as sub_store from "./sub_store"; import * as ui_report from "./ui_report"; import * as user_groups from "./user_groups"; import {user_settings} from "./user_settings"; -import * as util from "./util"; export function setup_subscriptions_tab_hash(tab_key_value) { if ($("#subscription_overlay .right").hasClass("show")) { @@ -132,7 +132,7 @@ export function update_stream_description(sub) { const $edit_container = stream_settings_containers.get_edit_container(sub); $edit_container.find("input.description").val(sub.description); const html = render_stream_description({ - rendered_description: util.clean_user_content_links(sub.rendered_description), + rendered_description: postprocess_content(sub.rendered_description), }); $edit_container.find(".stream-description").html(html); } diff --git a/web/src/stream_settings_ui.js b/web/src/stream_settings_ui.js index a461546fea..acd21244a0 100644 --- a/web/src/stream_settings_ui.js +++ b/web/src/stream_settings_ui.js @@ -22,6 +22,7 @@ import * as message_live_update from "./message_live_update"; import * as message_view_header from "./message_view_header"; import * as narrow_state from "./narrow_state"; import * as overlays from "./overlays"; +import {postprocess_content} from "./postprocess_content"; import * as resize from "./resize"; import * as scroll_util from "./scroll_util"; import * as search_util from "./search_util"; @@ -39,7 +40,6 @@ import * as stream_settings_components from "./stream_settings_components"; import * as stream_settings_data from "./stream_settings_data"; import * as stream_ui_updates from "./stream_ui_updates"; import * as sub_store from "./sub_store"; -import * as util from "./util"; export function is_sub_already_present(sub) { return stream_ui_updates.row_for_stream_id(sub.stream_id).length > 0; @@ -133,7 +133,7 @@ export function update_stream_description(sub, description, rendered_description // Update stream row const $sub_row = stream_ui_updates.row_for_stream_id(sub.stream_id); - $sub_row.find(".description").html(util.clean_user_content_links(sub.rendered_description)); + $sub_row.find(".description").html(postprocess_content(sub.rendered_description)); // Update stream settings stream_edit.update_stream_description(sub); diff --git a/web/src/templates.js b/web/src/templates.js index b672ab7bb1..2dbcb70d6e 100644 --- a/web/src/templates.js +++ b/web/src/templates.js @@ -2,7 +2,7 @@ import Handlebars from "handlebars/runtime"; import * as common from "./common"; import {default_html_elements, intl} from "./i18n"; -import * as util from "./util"; +import {postprocess_content} from "./postprocess_content"; // Below, we register Zulip-specific extensions to the Handlebars API. // @@ -112,7 +112,7 @@ Handlebars.registerHelper("tr", function (options) { Handlebars.registerHelper( "rendered_markdown", - (content) => new Handlebars.SafeString(util.clean_user_content_links(content)), + (content) => new Handlebars.SafeString(postprocess_content(content)), ); Handlebars.registerHelper("numberFormat", (number) => number.toLocaleString()); diff --git a/web/src/util.ts b/web/src/util.ts index 88e2b3068b..8619889f5b 100644 --- a/web/src/util.ts +++ b/web/src/util.ts @@ -1,7 +1,6 @@ import _ from "lodash"; import * as blueslip from "./blueslip"; -import {$t} from "./i18n"; import type {MatchedMessage, Message, RawMessage} from "./message_store"; import type {UpdateMessageEvent} from "./types"; import {user_settings} from "./user_settings"; @@ -292,93 +291,6 @@ export function canonicalize_stream_synonyms(text: string): string { return text; } -let inertDocument: Document | undefined; - -export function clean_user_content_links(html: string): string { - if (inertDocument === undefined) { - inertDocument = new DOMParser().parseFromString("", "text/html"); - } - const template = inertDocument.createElement("template"); - template.innerHTML = html; - - for (const elt of template.content.querySelectorAll("a")) { - // Ensure that all external links have target="_blank" - // rel="opener noreferrer". This ensures that external links - // never replace the Zulip web app 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 web app using our hashchange system, do not require - // these attributes. - const href = elt.getAttribute("href"); - if (href === null) { - continue; - } - let url; - try { - url = new URL(href, window.location.href); - } catch { - elt.removeAttribute("href"); - elt.removeAttribute("title"); - continue; - } - - // eslint-disable-next-line no-script-url - if (["data:", "javascript:", "vbscript:"].includes(url.protocol)) { - // Remove unsafe links completely. - elt.removeAttribute("href"); - elt.removeAttribute("title"); - 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"); - } - - if (elt.parentElement?.classList.contains("message_inline_image")) { - // For inline images we want to handle the tooltips explicitly, and disable - // the browser's built in handling of the title attribute. - const title = elt.getAttribute("title"); - if (title !== null) { - elt.setAttribute("aria-label", title); - elt.removeAttribute("title"); - } - } else { - // For non-image user uploads, the following block ensures that the title - // attribute always displays the filename as a security measure. - let title: string; - let legacy_title: string; - if ( - url.origin === window.location.origin && - url.pathname.startsWith("/user_uploads/") - ) { - // We add the word "download" to make clear what will - // happen when clicking the file. This is particularly - // important in the desktop app, where hovering a URL does - // not display the URL like it does in the web app. - title = legacy_title = $t( - {defaultMessage: "Download {filename}"}, - {filename: url.pathname.slice(url.pathname.lastIndexOf("/") + 1)}, - ); - } else { - title = url.toString(); - legacy_title = href; - } - elt.setAttribute( - "title", - ["", legacy_title].includes(elt.title) ? title : `${title}\n${elt.title}`, - ); - } - } - return template.innerHTML; -} - export function filter_by_word_prefix_match( items: T[], search_term: string, diff --git a/web/tests/postprocess_content.test.js b/web/tests/postprocess_content.test.js new file mode 100644 index 0000000000..644fc33b59 --- /dev/null +++ b/web/tests/postprocess_content.test.js @@ -0,0 +1,33 @@ +"use strict"; + +const {strict: assert} = require("assert"); + +const {zrequire} = require("./lib/namespace"); +const {run_test} = require("./lib/test"); + +const {postprocess_content} = zrequire("postprocess_content"); + +run_test("postprocess_content", () => { + assert.equal( + postprocess_content( + 'good ' + + 'upload ' + + 'invalid ' + + 'unsafe ' + + 'fragment' + + '

' + + 'upload ' + + 'button ' + + "
", + ), + 'good ' + + 'upload ' + + "invalid " + + "unsafe " + + 'fragment' + + '
' + + 'upload ' + + 'button ' + + "
", + ); +}); diff --git a/web/tests/util.test.js b/web/tests/util.test.js index 58e4d44031..00f82b8539 100644 --- a/web/tests/util.test.js +++ b/web/tests/util.test.js @@ -302,31 +302,6 @@ run_test("move_array_elements_to_front", () => { assert.deepEqual(emails_actual, emails_expected); }); -run_test("clean_user_content_links", () => { - assert.equal( - util.clean_user_content_links( - 'good ' + - 'upload ' + - 'invalid ' + - 'unsafe ' + - 'fragment' + - '
' + - 'upload ' + - 'button ' + - "
", - ), - 'good ' + - 'upload ' + - "invalid " + - "unsafe " + - 'fragment' + - '
' + - 'upload ' + - 'button ' + - "
", - ); -}); - run_test("filter_by_word_prefix_match", () => { const strings = ["stream-hyphen_underscore/slash", "three word stream"]; const values = [0, 1];