diff --git a/app/assets/javascripts/discourse/app/lib/user-search.js b/app/assets/javascripts/discourse/app/lib/user-search.js index 26405ba547d0d..0f88f6018e652 100644 --- a/app/assets/javascripts/discourse/app/lib/user-search.js +++ b/app/assets/javascripts/discourse/app/lib/user-search.js @@ -5,6 +5,7 @@ import { camelCaseToSnakeCase } from "discourse/lib/case-converter"; import discourseDebounce from "discourse/lib/debounce"; import { isTesting } from "discourse/lib/environment"; import discourseLater from "discourse/lib/later"; +import { cloneJSON } from "discourse/lib/object"; import { userPath } from "discourse/lib/url"; import { emailValid } from "discourse/lib/utilities"; import { CANCELLED_STATUS } from "discourse/modifiers/d-autocomplete"; @@ -41,7 +42,7 @@ function performSearch( ) { let cached = cache[term]; if (cached) { - resultsFn(cached); + resultsFn(cloneJSON(cached)); return; } @@ -105,7 +106,7 @@ function performSearch( }) .finally(function () { oldSearch = null; - resultsFn(returnVal); + resultsFn(cloneJSON(returnVal)); }); } diff --git a/app/assets/javascripts/discourse/tests/unit/lib/user-search-test.js b/app/assets/javascripts/discourse/tests/unit/lib/user-search-test.js index 309c6e31340f2..367f883594f22 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/user-search-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/user-search-test.js @@ -262,4 +262,100 @@ module("Unit | Utility | user-search", function (hooks) { ); }); }); + + test("it protects cached results from mutation by consumers", async function (assert) { + pretender.get("/u/search/users", (request) => { + if (request.url.includes("term=test_cache")) { + return response({ + users: [ + { + id: 123, + username: "test_cache_user", + name: "Test Cache User", + avatar_template: + "https://avatars.discourse.org/v3/letter/t/41988e/{size}.png", + }, + ], + groups: [], + }); + } + return response({ users: [], groups: [] }); + }); + + // First userSearch call - this will populate the cache + let firstResults = await userSearch({ term: "test_cache" }); + assert.strictEqual( + firstResults.length, + 1, + "First call should return 1 result" + ); + + const userFromFirstCall = firstResults[0]; + assert.strictEqual( + userFromFirstCall.username, + "test_cache_user", + "First call should have username" + ); + assert.strictEqual( + userFromFirstCall.name, + "Test Cache User", + "First call should have name" + ); + assert.present( + userFromFirstCall.avatar_template, + "First call should have avatar_template" + ); + + const originalKeys = Object.keys(userFromFirstCall); + // Simulate what can happen in consumers of the cache result that mutate that object + Object.keys(userFromFirstCall).forEach((key) => { + if (key === "id") { + return; + } + delete userFromFirstCall[key]; + }); + + // Verify the mutation happened + assert.present(userFromFirstCall.id, "User object should still have id"); + assert.strictEqual( + Object.keys(userFromFirstCall).length, + 1, + "User object should be mutated to only have id" + ); + ["username", "name", "avatar_template"].forEach((key) => { + assert.blank( + userFromFirstCall[key], + `User object should not have ${key} after mutation` + ); + }); + + // Second userSearch call - this should return fresh objects from cache that are not mutated + let secondResults = await userSearch({ term: "test_cache" }); + assert.strictEqual( + secondResults.length, + 1, + "Second call should return 1 result" + ); + + const userFromSecondCall = secondResults[0]; + assert.strictEqual( + userFromSecondCall.username, + "test_cache_user", + "Second call should have username (cache should be protected from mutation)" + ); + assert.strictEqual( + userFromSecondCall.name, + "Test Cache User", + "Second call should have name (cache should be protected from mutation)" + ); + assert.present( + userFromSecondCall.avatar_template, + "Second call should have avatar_template (cache should be protected from mutation)" + ); + assert.strictEqual( + Object.keys(userFromSecondCall).length, + originalKeys.length, + "Second call should have all original properties" + ); + }); }); diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-composer.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-composer.gjs index 8c51c58dbe477..389c2af42ef73 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-composer.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-composer.gjs @@ -482,7 +482,7 @@ export default class ChatComposer extends Component { autoSelectFirstSuggestion: true, transformComplete: (obj) => { if (obj.isUser) { - this.#addMentionedUser(obj); + this.#addMentionedUser(cloneJSON(obj)); } return obj.username || obj.name; diff --git a/plugins/chat/spec/system/chat_composer_spec.rb b/plugins/chat/spec/system/chat_composer_spec.rb index ba48820638ba7..fa733e943f294 100644 --- a/plugins/chat/spec/system/chat_composer_spec.rb +++ b/plugins/chat/spec/system/chat_composer_spec.rb @@ -321,6 +321,31 @@ end end + context "when using user autocomplete" do + fab!(:autocomplete_user) do + Fabricate(:user, username: "ac_test_user", name: "Autocomplete Test User") + end + + before do + SiteSetting.enable_mentions = true + channel_1.add(autocomplete_user) + end + + it "fails on subsequent autocomplete selection due to cache pollution" do + chat_page.visit_channel(channel_1) + + find(".chat-composer__input").send_keys("@#{autocomplete_user.username}") + find(".autocomplete.ac-user", text: "ac_test_user").click + expect(channel_page.composer).to have_value("@#{autocomplete_user.username} ") + find(".chat-composer__input").set("") + + # 2nd autocomplete attempt to ensure user object is not mutated by existing store record behaviour + find(".chat-composer__input").send_keys("@#{autocomplete_user.username}") + find(".autocomplete.ac-user", text: "ac_test_user").click + expect(channel_page.composer).to have_value("@#{autocomplete_user.username} ") + end + end + context "with floatkit autocomplete disabled" do before { SiteSetting.floatkit_autocomplete_composer = false }