fix(escape): make sure that __escaped does not get removed (#3830)

* fix(escape): make sure that __escaped does not get removed

When mapping over an array, extra non-enumerable properties don't get copied into the new array. This means that the `__escaped` gets removed due to transformItems, or addAbsolutePosition.

We fix this by setting that mark back just before calling the render function. We want transformItems to happen after escaping, so it's no option to put the mark before.

Note also that if we would put the escaping before, we would still need to capture the marker, since it will be removed by the earlier transformations.

The new tests I added fail without my modification.

See also algolia/vue-instantsearch#639 which highlighted 😅 this issue.

* Apply suggestions from code review

Co-Authored-By: François Chalifour <francoischalifour@users.noreply.github.com>

* chore: naming

* test: remove unneeded mock

* test: add comment

src/connectors/hits/__tests__/connectHits-test.js

@@ -3,7 +3,10 @@ import { TAG_PLACEHOLDER } from '../../../lib/escape-highlight';
 import connectHits from '../connectHits';
 
 jest.mock('../../../lib/utils/hits-absolute-position', () => ({
-  addAbsolutePosition: hits => hits,
+  // The real implementation creates a new array instance, which can cause bugs,
+  // especially with the __escaped mark, we thus make sure the mock also has the
+  // same behavior regarding the array.
+  addAbsolutePosition: hits => hits.map(x => x),
 }));
 
 describe('connectHits', () => {
@@ -50,7 +53,7 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/hits/js/#co
     );
 
     widget.render({
-      results: new SearchResults(helper.state, [{}]),
+      results: new SearchResults(helper.state, [{ hits: [] }]),
       state: helper.state,
       helper,
       createURL: () => '#',
@@ -217,10 +220,13 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/hits/js/#co
       createURL: () => '#',
     });
 
+    const expectedHits = [{ name: 'transformed' }, { name: 'transformed' }];
+    expectedHits.__escaped = true;
+
     expect(rendering).toHaveBeenNthCalledWith(
       2,
       expect.objectContaining({
-        hits: [{ name: 'transformed' }, { name: 'transformed' }],
+        hits: expectedHits,
         results,
       }),
       expect.anything()
@@ -254,13 +260,16 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/hits/js/#co
       createURL: () => '#',
     });
 
+    const expectedHits = [
+      { name: 'name 1', __queryID: 'theQueryID' },
+      { name: 'name 2', __queryID: 'theQueryID' },
+    ];
+    expectedHits.__escaped = true;
+
     expect(rendering).toHaveBeenNthCalledWith(
       2,
       expect.objectContaining({
-        hits: [
-          { name: 'name 1', __queryID: 'theQueryID' },
-          { name: 'name 2', __queryID: 'theQueryID' },
-        ],
+        hits: expectedHits,
       }),
       expect.anything()
     );
@@ -322,30 +331,61 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/hits/js/#co
       createURL: () => '#',
     });
 
+    const expectedHits = [
+      {
+        name: 'hello',
+        _highlightResult: {
+          name: {
+            value: 'HE<MARK>LLO</MARK>',
+          },
+        },
+      },
+      {
+        name: 'halloween',
+        _highlightResult: {
+          name: {
+            value: 'HA<MARK>LLO</MARK>WEEN',
+          },
+        },
+      },
+    ];
+    expectedHits.__escaped = true;
+
     expect(rendering).toHaveBeenNthCalledWith(
       2,
       expect.objectContaining({
-        hits: [
-          {
-            name: 'hello',
-            _highlightResult: {
-              name: {
-                value: 'HE<MARK>LLO</MARK>',
-              },
-            },
-          },
-          {
-            name: 'halloween',
-            _highlightResult: {
-              name: {
-                value: 'HA<MARK>LLO</MARK>WEEN',
-              },
-            },
-          },
-        ],
+        hits: expectedHits,
         results,
       }),
       expect.anything()
     );
   });
+
+  it('keeps the __escaped mark', () => {
+    const rendering = jest.fn();
+    const makeWidget = connectHits(rendering);
+    const widget = makeWidget({});
+
+    const helper = jsHelper({}, '', {});
+    helper.search = jest.fn();
+
+    widget.init({
+      helper,
+      state: helper.state,
+      createURL: () => '#',
+      onHistoryChange: () => {},
+    });
+
+    const results = new SearchResults(helper.state, [
+      { hits: [{ whatever: 'i like kittens' }] },
+    ]);
+    widget.render({
+      results,
+      state: helper.state,
+      helper,
+      createURL: () => '#',
+    });
+
+    expect(results.hits.__escaped).toBe(true);
+  });
 });

src/connectors/hits/connectHits.js

@@ -74,10 +74,12 @@ export default function connectHits(renderFn, unmountFn) {
       },
 
       render({ results, instantSearchInstance }) {
-        if (escapeHTML && results.hits && results.hits.length > 0) {
+        if (escapeHTML && results.hits.length > 0) {
           results.hits = escapeHits(results.hits);
         }
 
+        const initialEscaped = results.hits.__escaped;
+
         results.hits = addAbsolutePosition(
           results.hits,
           results.page,
@@ -88,6 +90,11 @@ export default function connectHits(renderFn, unmountFn) {
 
         results.hits = transformItems(results.hits);
 
+        // Make sure the escaped tag stays, even after mapping over the hits.
+        // This prevents the hits from being double-escaped if there are multiple
+        // hits widgets mounted on the page.
+        results.hits.__escaped = initialEscaped;
+
         renderFn(
           {
             hits: results.hits,

src/connectors/infinite-hits/__tests__/connectInfiniteHits-test.ts

@@ -9,7 +9,10 @@ import connectInfiniteHits from '../connectInfiniteHits';
 import { Client } from '../../../types';
 
 jest.mock('../../../lib/utils/hits-absolute-position', () => ({
-  addAbsolutePosition: hits => hits,
+  // The real implementation creates a new array instance, which can cause bugs,
+  // especially with the __escaped mark, we thus make sure the mock also has the
+  // same behavior regarding the array.
+  addAbsolutePosition: hits => hits.map(x => x),
 }));
 
 describe('connectInfiniteHits', () => {
@@ -605,6 +608,33 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/infinite-hi
     );
   });
 
+  it('keeps the __escaped mark', () => {
+    const rendering = jest.fn();
+    const makeWidget = connectInfiniteHits(rendering);
+    const widget = makeWidget({});
+
+    const helper = jsHelper({} as Client, '', {});
+    helper.search = jest.fn();
+
+    widget.init!({
+      ...defaultInitOptions,
+      helper,
+      state: helper.state,
+    });
+
+    const results = new SearchResults(helper.state, [
+      { hits: [{ whatever: 'i like kittens' }] },
+    ]);
+    widget.render!({
+      ...defaultRenderOptions,
+      results,
+      state: helper.state,
+      helper,
+    });
+
+    expect((results.hits as any).__escaped).toBe(true);
+  });
+
   describe('routing', () => {
     describe('getWidgetState', () => {
       it('should give back the object unmodified if the default value is selected', () => {

src/connectors/infinite-hits/connectInfiniteHits.ts

@@ -179,9 +179,10 @@ const connectInfiniteHits: InfiniteHitsConnector = (
           prevState = currentState;
         }
 
-        if (escapeHTML && results.hits && results.hits.length > 0) {
+        if (escapeHTML && results.hits.length > 0) {
           results.hits = escapeHits(results.hits);
         }
+        const initialEscaped = (results.hits as any).__escaped;
 
         results.hits = addAbsolutePosition(
           results.hits,
@@ -193,6 +194,11 @@ const connectInfiniteHits: InfiniteHitsConnector = (
 
         results.hits = transformItems(results.hits);
 
+        // Make sure the escaped tag stays after mapping over the hits.
+        // This prevents the hits from being double-escaped if there are multiple
+        // hits widgets mounted on the page.
+        (results.hits as any).__escaped = initialEscaped;
+
         if (lastReceivedPage < page! || !hitsCache.length) {
           hitsCache = [...hitsCache, ...results.hits];
           lastReceivedPage = page!;