FIX: user autocomplete search cache pollution (#34208)

tyb-talks · web-flow · commit a71eb4108a7e · 2025-08-11T13:14:25.000+08:00
The current chat autocomplete pipeline can mutate the user object selected for autocomplete here: https://github.com/discourse/discourse/blob/071e82140cf375e2875b87d1664ae596c19320dd/plugins/chat/assets/javascripts/discourse/components/chat-composer.gjs#L485 Specifically this occurs when we attempt to create a record in the store service where it already exists, and the logic for fetching the right record there deletes some properties from the input object: https://github.com/discourse/discourse/blob/216df43502e25fcb9a228bfc09f90548712d7f49/app/assets/javascripts/discourse/app/services/store.js#L426-L435 This mutates the object that is eventually used to replace the search term (and therefore causes an error as it is now missing all its properties except for `id`). This also pollutes the upstream cache in the userSearch JS module. This PR fixes both issues by cloning the user objects before they get passed into the processing functions.
diff --git 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
@@ -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
@@ -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
@@ -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 }
 

Original file line number	Diff line number	Diff line change
`@@ -482,7 +482,7 @@ export default class ChatComposer extends Component {`
`482`	`482`	`autoSelectFirstSuggestion: true,`
`483`	`483`	`transformComplete: (obj) => {`
`484`	`484`	`if (obj.isUser) {`
`485`		`- this.#addMentionedUser(obj);`
	`485`	`+ this.#addMentionedUser(cloneJSON(obj));`
`486`	`486`	`}`
`487`	`487`
`488`	`488`	`return obj.username \|\| obj.name;`