From 73781c8a96e35062b2631263bd6afd37f7f7016b Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Thu, 7 Sep 2023 16:13:13 +0400 Subject: [PATCH] FIX: Do not consider code-blocks when parsing mentions (#23280) We have the max_mentions_per_chat_message site settings; when a user tries to mention more users than allowed, no one gets mentioned. Chat messages may contain code-blocks with strings that look like mentions: def foo @bar + @baz end The problem is that the parsing code considers these as real mentions and counts them when checking the limit. This commit fixes the problem. --- .../discourse/app/lib/mentions-parser.js | 36 ++++++ .../discourse/app/lib/plugin-api.js | 6 +- .../javascripts/discourse/app/lib/text.js | 10 +- .../discourse/tests/unit/lib/text-test.js | 40 ++++++- .../pretty-text/addon/pretty-text.js | 4 + .../discourse/models/chat-message.js | 57 +++++----- .../chat-composer-warnings-tracker.js | 86 +++++--------- .../javascripts/acceptance/mentions-test.js | 106 ++++++++++++++++++ 8 files changed, 260 insertions(+), 85 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/lib/mentions-parser.js create mode 100644 plugins/chat/test/javascripts/acceptance/mentions-test.js diff --git a/app/assets/javascripts/discourse/app/lib/mentions-parser.js b/app/assets/javascripts/discourse/app/lib/mentions-parser.js new file mode 100644 index 00000000000..757b7aad801 --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/mentions-parser.js @@ -0,0 +1,36 @@ +export class MentionsParser { + constructor(prettyText) { + this.prettyText = prettyText; + } + + parse(markdown) { + const tokens = this.prettyText.parse(markdown); + const mentions = this.#parse(tokens); + return [...new Set(mentions)]; + } + + #parse(tokens) { + const mentions = []; + let insideMention = false; + for (const token of tokens) { + if (token.children) { + this.#parse(token.children).forEach((mention) => + mentions.push(mention) + ); + } else { + if (token.type === "mention_open") { + insideMention = true; + } else if (insideMention && token.type === "text") { + mentions.push(this.#extractMention(token.content)); + insideMention = false; + } + } + } + + return mentions; + } + + #extractMention(mention) { + return mention.trim().substring(1); + } +} diff --git a/app/assets/javascripts/discourse/app/lib/plugin-api.js b/app/assets/javascripts/discourse/app/lib/plugin-api.js index 43e81431cf4..76e076fcc36 100644 --- a/app/assets/javascripts/discourse/app/lib/plugin-api.js +++ b/app/assets/javascripts/discourse/app/lib/plugin-api.js @@ -2126,7 +2126,7 @@ class PluginApi { * Support for setting a Sidebar panel. */ setSidebarPanel(name) { - this._lookupContainer("service:sidebar-state").setPanel(name); + this._lookupContainer("service:sidebar-state")?.setPanel(name); } /** @@ -2134,7 +2134,7 @@ class PluginApi { * Set combined sidebar section mode. In this mode, sections from all panels are displayed together. */ setCombinedSidebarMode() { - this._lookupContainer("service:sidebar-state").setCombinedMode(); + this._lookupContainer("service:sidebar-state")?.setCombinedMode(); } /** @@ -2158,7 +2158,7 @@ class PluginApi { * Hide sidebar switch panels buttons in separated mode. */ hideSidebarSwitchPanelButtons() { - this._lookupContainer("service:sidebar-state").hideSwitchPanelButtons(); + this._lookupContainer("service:sidebar-state")?.hideSwitchPanelButtons(); } /** diff --git a/app/assets/javascripts/discourse/app/lib/text.js b/app/assets/javascripts/discourse/app/lib/text.js index c8cfc3fc55e..2fb0e8aa28f 100644 --- a/app/assets/javascripts/discourse/app/lib/text.js +++ b/app/assets/javascripts/discourse/app/lib/text.js @@ -9,6 +9,7 @@ import { helperContext } from "discourse-common/lib/helpers"; import { htmlSafe } from "@ember/template"; import loadScript from "discourse/lib/load-script"; import { sanitize as textSanitize } from "pretty-text/sanitizer"; +import { MentionsParser } from "discourse/lib/mentions-parser"; function getOpts(opts) { let context = helperContext(); @@ -71,10 +72,17 @@ export function sanitizeAsync(text, options) { export function parseAsync(md, options = {}, env = {}) { return loadMarkdownIt().then(() => { - return createPrettyText(options).opts.engine.parse(md, env); + return createPrettyText(options).parse(md, env); }); } +export async function parseMentions(markdown, options) { + await loadMarkdownIt(); + const prettyText = createPrettyText(options); + const mentionsParser = new MentionsParser(prettyText); + return mentionsParser.parse(markdown); +} + function loadMarkdownIt() { return new Promise((resolve) => { let markdownItURL = Session.currentProp("markdownItURL"); diff --git a/app/assets/javascripts/discourse/tests/unit/lib/text-test.js b/app/assets/javascripts/discourse/tests/unit/lib/text-test.js index d4554d09ddf..d6cef6e3f5b 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/text-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/text-test.js @@ -1,5 +1,10 @@ import { module, test } from "qunit"; -import { cookAsync, excerpt, parseAsync } from "discourse/lib/text"; +import { + cookAsync, + excerpt, + parseAsync, + parseMentions, +} from "discourse/lib/text"; module("Unit | Utility | text", function () { test("parseAsync", async function (assert) { @@ -35,3 +40,36 @@ module("Unit | Utility | text", function () { ); }); }); + +module("Unit | Utility | text | parseMentions", function () { + test("parses mentions from markdown", async function (assert) { + const markdown = "Hey @user1, @user2, @group1, @group2, @here, @all"; + const mentions = await parseMentions(markdown); + assert.deepEqual(mentions, [ + "user1", + "user2", + "group1", + "group2", + "here", + "all", + ]); + }); + + test("ignores duplicated mentions", async function (assert) { + const markdown = "Hey @john, @john, @john, @john"; + const mentions = await parseMentions(markdown); + assert.deepEqual(mentions, ["john"]); + }); + + test("ignores mentions in codeblocks", async function (assert) { + const markdown = `Hey + \`\`\` + def foo + @bar = true + end + \`\`\` + `; + const mentions = await parseMentions(markdown); + assert.equal(mentions.length, 0); + }); +}); diff --git a/app/assets/javascripts/pretty-text/addon/pretty-text.js b/app/assets/javascripts/pretty-text/addon/pretty-text.js index f0e93ae1282..750e6ab4ef1 100644 --- a/app/assets/javascripts/pretty-text/addon/pretty-text.js +++ b/app/assets/javascripts/pretty-text/addon/pretty-text.js @@ -125,6 +125,10 @@ export default class { return result ? result : ""; } + parse(markdown, env = {}) { + return this.opts.engine.parse(markdown, env); + } + sanitize(html) { return this.opts.sanitizer(html).trim(); } diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-message.js b/plugins/chat/assets/javascripts/discourse/models/chat-message.js index 3decdc33508..5b02f0d08fa 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-message.js @@ -4,7 +4,7 @@ import { TrackedArray, TrackedObject } from "@ember-compat/tracked-built-ins"; import ChatMessageReaction from "discourse/plugins/chat/discourse/models/chat-message-reaction"; import Bookmark from "discourse/models/bookmark"; import I18n from "I18n"; -import { generateCookFunction } from "discourse/lib/text"; +import { generateCookFunction, parseMentions } from "discourse/lib/text"; import transformAutolinks from "discourse/plugins/chat/discourse/lib/transform-auto-links"; import { getOwner } from "discourse-common/lib/get-owner"; import discourseLater from "discourse-common/lib/later"; @@ -170,33 +170,11 @@ export default class ChatMessage { } async cook() { - const site = getOwner(this).lookup("service:site"); - if (this.isDestroyed || this.isDestroying) { return; } - - const markdownOptions = { - featuresOverride: - site.markdown_additional_options?.chat?.limited_pretty_text_features, - markdownItRules: - site.markdown_additional_options?.chat - ?.limited_pretty_text_markdown_rules, - hashtagTypesInPriorityOrder: - site.hashtag_configurations?.["chat-composer"], - hashtagIcons: site.hashtag_icons, - }; - - if (ChatMessage.cookFunction) { - this.cooked = ChatMessage.cookFunction(this.message); - } else { - const cookFunction = await generateCookFunction(markdownOptions); - ChatMessage.cookFunction = (raw) => { - return transformAutolinks(cookFunction(raw)); - }; - - this.cooked = ChatMessage.cookFunction(this.message); - } + await this.#ensureCookFunctionInitialized(); + this.cooked = ChatMessage.cookFunction(this.message); } get read() { @@ -263,6 +241,10 @@ export default class ChatMessage { this.version++; } + async parseMentions() { + return await parseMentions(this.message, this.#markdownOptions); + } + toJSONDraft() { if ( this.message?.length === 0 && @@ -361,6 +343,31 @@ export default class ChatMessage { } } + async #ensureCookFunctionInitialized() { + if (ChatMessage.cookFunction) { + return; + } + + const cookFunction = await generateCookFunction(this.#markdownOptions); + ChatMessage.cookFunction = (raw) => { + return transformAutolinks(cookFunction(raw)); + }; + } + + get #markdownOptions() { + const site = getOwner(this).lookup("service:site"); + return { + featuresOverride: + site.markdown_additional_options?.chat?.limited_pretty_text_features, + markdownItRules: + site.markdown_additional_options?.chat + ?.limited_pretty_text_markdown_rules, + hashtagTypesInPriorityOrder: + site.hashtag_configurations?.["chat-composer"], + hashtagIcons: site.hashtag_icons, + }; + } + #initChatMessageReactionModel(reactions = []) { return reactions.map((reaction) => ChatMessageReaction.create(reaction)); } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-composer-warnings-tracker.js b/plugins/chat/assets/javascripts/discourse/services/chat-composer-warnings-tracker.js index 43a8d4adaad..4c1f82eae69 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-composer-warnings-tracker.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-composer-warnings-tracker.js @@ -2,7 +2,6 @@ import Service, { inject as service } from "@ember/service"; import { ajax } from "discourse/lib/ajax"; import discourseDebounce from "discourse-common/lib/debounce"; import { bind } from "discourse-common/utils/decorators"; -import { mentionRegex } from "pretty-text/mentions"; import { cancel } from "@ember/runloop"; import { tracked } from "@glimmer/tracking"; @@ -34,11 +33,7 @@ export default class ChatComposerWarningsTracker extends Service { @bind reset() { - this.unreachableGroupMentions = []; - this.unreachableGroupMentions = []; - this.overMembersLimitGroupMentions = []; - this.tooManyMentions = false; - this.channelWideMentionDisallowed = false; + this.#resetMentionStats(); this.mentionsCount = 0; cancel(this.mentionsTimer); } @@ -63,61 +58,42 @@ export default class ChatComposerWarningsTracker extends Service { return; } - const mentions = this._extractMentions(currentMessage.message); - this.mentionsCount = mentions?.length; + currentMessage.parseMentions().then((mentions) => { + this.mentionsCount = mentions?.length; - if (this.mentionsCount > 0) { - this.tooManyMentions = - this.mentionsCount > this.siteSettings.max_mentions_per_chat_message; + if (this.mentionsCount > 0) { + this.tooManyMentions = + this.mentionsCount > this.siteSettings.max_mentions_per_chat_message; - if (!this.tooManyMentions) { - const newMentions = mentions.filter( - (mention) => !(mention in this._mentionWarningsSeen) - ); + if (!this.tooManyMentions) { + const newMentions = mentions.filter( + (mention) => !(mention in this._mentionWarningsSeen) + ); - this.channelWideMentionDisallowed = - !currentMessage.channel.allowChannelWideMentions && - (mentions.includes("here") || mentions.includes("all")); + this.channelWideMentionDisallowed = + !currentMessage.channel.allowChannelWideMentions && + (mentions.includes("here") || mentions.includes("all")); - if (newMentions?.length > 0) { - this._recordNewWarnings(newMentions, mentions); - } else { - this._rebuildWarnings(mentions); - } - } - } else { - this.tooManyMentions = false; - this.channelWideMentionDisallowed = false; - this.unreachableGroupMentions = []; - this.overMembersLimitGroupMentions = []; - } - } - - _extractMentions(message) { - const regex = mentionRegex(this.siteSettings.unicode_usernames); - const mentions = []; - let mentionsLeft = true; - - while (mentionsLeft) { - const matches = message.match(regex); - - if (matches) { - const mention = matches[1] || matches[2]; - mentions.push(mention); - message = message.replaceAll(`${mention}`, ""); - - if (mentions.length > this.siteSettings.max_mentions_per_chat_message) { - mentionsLeft = false; + if (newMentions?.length > 0) { + this.#recordNewWarnings(newMentions, mentions); + } else { + this.#rebuildWarnings(mentions); + } } } else { - mentionsLeft = false; + this.#resetMentionStats(); } - } - - return mentions; + }); } - _recordNewWarnings(newMentions, mentions) { + #resetMentionStats() { + this.tooManyMentions = false; + this.channelWideMentionDisallowed = false; + this.unreachableGroupMentions = []; + this.overMembersLimitGroupMentions = []; + } + + #recordNewWarnings(newMentions, mentions) { ajax("/chat/api/mentions/groups.json", { data: { mentions: newMentions }, }) @@ -135,12 +111,12 @@ export default class ChatComposerWarningsTracker extends Service { this._mentionWarningsSeen[warning] = MENTION_RESULT["invalid"]; }); - this._rebuildWarnings(mentions); + this.#rebuildWarnings(mentions); }) - .catch(this._rebuildWarnings(mentions)); + .catch(this.#rebuildWarnings(mentions)); } - _rebuildWarnings(mentions) { + #rebuildWarnings(mentions) { const newWarnings = mentions.reduce( (memo, mention) => { if ( diff --git a/plugins/chat/test/javascripts/acceptance/mentions-test.js b/plugins/chat/test/javascripts/acceptance/mentions-test.js new file mode 100644 index 00000000000..02a51030122 --- /dev/null +++ b/plugins/chat/test/javascripts/acceptance/mentions-test.js @@ -0,0 +1,106 @@ +import { acceptance } from "discourse/tests/helpers/qunit-helpers"; +import { test } from "qunit"; +import { fillIn, visit } from "@ember/test-helpers"; +import pretender, { response } from "discourse/tests/helpers/create-pretender"; + +acceptance("Chat | Mentions", function (needs) { + const channelId = 1; + const actingUser = { + id: 1, + username: "acting_user", + }; + const channel = { + id: channelId, + chatable_id: 1, + chatable_type: "Category", + meta: { message_bus_last_ids: {}, can_delete_self: true }, + current_user_membership: { following: true }, + allow_channel_wide_mentions: false, + chatable: { id: 1 }, + }; + + needs.settings({ chat_enabled: true }); + + needs.user({ + ...actingUser, + has_chat_enabled: true, + chat_channels: { + public_channels: [channel], + direct_message_channels: [], + meta: { message_bus_last_ids: {} }, + tracking: {}, + }, + }); + + needs.hooks.beforeEach(function () { + pretender.post(`/chat/drafts`, () => response({})); + pretender.get(`/chat/api/channels/${channelId}/messages`, () => + response({ + messages: [], + meta: { + can_load_more_future: false, + }, + }) + ); + pretender.get("/chat/api/mentions/groups.json", () => + response({ + unreachable: [], + over_members_limit: [], + invalid: ["and"], + }) + ); + }); + + test("shows warning when mention limit exceeded", async function (assert) { + this.siteSettings.max_mentions_per_chat_message = 2; + + await visit(`/chat/c/-/${channelId}`); + await fillIn(".chat-composer__input", `Hey @user1 @user2 @user3`); + + assert.dom(".chat-mention-warnings").exists(); + }); + + test("shows warning for @here mentions when channel-wide mentions are disabled", async function (assert) { + await visit(`/chat/c/-/${channelId}`); + await fillIn(".chat-composer__input", `Hey @here`); + + assert.dom(".chat-mention-warnings").exists(); + }); + + test("shows warning for @all mention when channel-wide mentions are disabled", async function (assert) { + await visit(`/chat/c/-/${channelId}`); + await fillIn(".chat-composer__input", `Hey @all`); + + assert.dom(".chat-mention-warnings").exists(); + }); + + test("ignores duplicates when counting mentions", async function (assert) { + this.siteSettings.max_mentions_per_chat_message = 2; + + await visit(`/chat/c/-/${channelId}`); + const mention = `@user1`; + await fillIn( + ".chat-composer__input", + `Hey ${mention} ${mention} ${mention}` + ); + + assert.dom(".chat-mention-warnings").doesNotExist(); + }); + + test("doesn't consider code-blocks when counting mentions", async function (assert) { + this.siteSettings.max_mentions_per_chat_message = 2; + + await visit(`/chat/c/-/${channelId}`); + // since @bar is inside a code-block it shouldn't be considered a mention + const message = `Hey @user1 @user2 + \`\`\` + def foo + @bar = true + end + \`\`\` + `; + await fillIn(".chat-composer__input", message); + + assert.dom(".chat-mention-warnings").doesNotExist(); + }); +});