React-instantsearch: SSR : bug on highlighting

[vvo]

Do you want to request a feature or report a bug?

Bug

Bug: What is the current behavior?

The next.js example has a weird behavior on reloading, it shows co<em>ca</em> as text instead of the actual coca:

Bug: What is the expected behavior?

It should not show html tags as raw.

Bug: What browsers are impacted? Which versions?

all

What is the version you are using? Always use the latest one before opening a bug issue.

Latest

[mthuret]

The root cause of this bug came from the timestamp present in our highlighted tags.

A first request is made to recover the first results, they have a tag like: ais-higlight-timestamp1.

When the app is finally rendered with those injected results, then the highlightedTags changed and are now: ais-highlight-timestamp2.

I can see several ways to fix this:

1. Removing the timestamp.
2. Somehow pass the timestamp used for the first request for the first rendering.

[mthuret]

cc @vvo @marielaures @Haroenv

[Haroenv]

Yes, I think that it's reasonable to expect that people won't put __ais-highlight__ in their data more than __ais-highlight-123123__

in highlightTags.js I would suggest the following diff:

- const timestamp = Date.now().toString();

export default {
-  highlightPreTag: `<ais-highlight-${timestamp}>`,
-  highlightPostTag: `</ais-highlight-${timestamp}>`,
+  highlightPreTag: `__ais-highlight__`,
+  highlightPreTag: `__/ais-highlight__`,
};

cc @rayrutjes who discussed this yesterday in Vue InstantSearch, where the conclusion was that we'll probably have to drop XSS safety completely there, in React InstantSearch I think we can still keep this, but should focus on making arrays work too.

[vvo]

the timestamp is used to limit the attack surface: it's harder to inject a timestamp at exactly the right moment to be displayed on a victim running APP.

Server-side wide this does not make much sense because we could be in situations where the generated timestamp is exactly the same for all requests to the html content (example: a nodejs service sending html of react-instantsearch).

The part that I don't get is why when we reload the page, the loaded html renders as text tags (<em>test</em> instead of test), it should already be resolved and rendered to test since it's part of the rendering.

[rayrutjes]

the timestamp is used to limit the attack surface: it's harder to inject a timestamp at exactly the right moment to be displayed on a victim running APP.

Not sure this adds much given the string is escaped anyway.

I'm currently working on a POC where we would escape all highlighted values at data retrieval to measure the impact on perfs.

The current implementations we have are not optimal for Algolia friendly users, in a sense that all the values now contain that weird placeholder. What if someone just wants to use the _highlightResult..value to do some dirty advanced stuff?

I'm open for discussing this. I'd love to be able to finish the quick POC to help us move forward in the discussion.

There will always be pros and cons because of the nature of the feature :)

[mthuret]

@vvo The flow of resolution with SSR is this:

1. A query is performed with an external helper to retrieve the first results. Currently it's using the preTag/postTag by default => em. But it could use our custom module HighlightTags and have something like ais-highlight-timestamp1.

2. Those results are given as a prop of <InstantSearch/>. It's now that the app is rendered and the html generated and send back to the client. It has some results containing highlighted value with ais-highlight-timestamp1. So, because the app is using the <Highlight> widget, those value are parsed. <Highlight> is also importing the HighlightTags, but it's after, so now, the highlightingTags we are looking for are ais-highlight-timestamp2. The Highlight component found then no highlighted value and the <ais-highlight-timestamp1>value</ais-highlight-timestamp1> or <em>value</em> is seen as a regular value.

[vvo]

A query is performed with an external helper to retrieve the first results. Currently it's using the preTag/postTag by default => em. But it could use our custom module HighlightTags and have something like ais-highlight-timestamp1.

If that's the real issue (because then when rendered react instantsearch tries to replace ais-highlight-timestamp) then we should expose as properties inside the HighlightComponent the generated tokens and pass them to the helper when doing the request.

This should resolve the issue since when rendered it will do the good replacement and then the react-instantsearch rendered frontend will just use new ones (as it is today)

[mthuret]

Fix with 4.1.0.beta3