Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

SSR : bug on highlighting #168

Closed
vvo opened this issue Jul 17, 2017 · 8 comments
Closed

SSR : bug on highlighting #168

vvo opened this issue Jul 17, 2017 · 8 comments

Comments

@vvo
Copy link
Contributor

vvo commented Jul 17, 2017

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:

2017-07-17 15 54 09

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 mthuret changed the title Next.js example bug on highlighting SSR : bug on highlighting Jul 17, 2017
@mthuret
Copy link
Contributor

mthuret commented Jul 17, 2017

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
Copy link
Contributor

mthuret commented Jul 18, 2017

cc @vvo @marielaures @Haroenv

@Haroenv
Copy link
Contributor

Haroenv commented Jul 18, 2017

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
Copy link
Contributor Author

vvo commented Jul 20, 2017

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
Copy link
Member

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
Copy link
Contributor

mthuret commented Jul 20, 2017

@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
Copy link
Contributor Author

vvo commented Jul 20, 2017

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
Copy link
Contributor

mthuret commented Jul 25, 2017

Fix with 4.1.0.beta3

@mthuret mthuret closed this as completed Jul 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants