[Table Visualization][Bug] Fix Url content display in table

[ananzh]

Description

Currently, the new table can not format Url. If we set to use URL format in index pattern field, table will display it as string.

In this PR, we switch the format from string to html. To make html understandable by react as a DOM element, we use dangerouslySetInnerHTML to transform it. For the security, since the content is not from random input but fetched from stored data, we should be safe as long as data is not messed.

To provide more security protection, we also add dompurify package to sanitize the html content.

Issue Resolved:

#2905

More discussions:

#2905 (comment)

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
  • yarn test:ftr
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[ashwin-pc]

Is this the only way to do this? afaik the goal here was just to make links possible for table cell elements. Cant we do that in a simpler way without allowing full blown HTML in a table cell?

[joshuarrrr]

I share Ashwin's discomfort with this approach in general, so I dug a bit deeper in the code base to try to figure out why things are set up this way. And my takeaway is that this fix is OK as is (and matches similar implementations in other visualizations like metric and markdown, and discover), but that the entire concept of a field formatter that outputs raw HTML strings is not desirable or necessary long-term.

1. It seems the primary purpose of the field formatters (provided by the data plugin) is plain text formatting, which seems both sensible and necessary. I see no need to get rid of these or fundamentally change them.
2. Only 3 of the converters also have a htmlConvert method: color, source, and url. These return string-ified HTML from templates, so using them in React is awkward and requires dangerouslySetInnerHTML. Given the project's evolution from Angular to React, we'd be better off with formatters that just provide components rather than HTML strings, but that's out of scope for this PR
3. Whenever we use dangerouslySetInnerHTML (which should decrease over time), we should follow the convention of providing an explanatory comment. For example, see

   OpenSearch-Dashboards/src/plugins/vis_type_metric/public/components/metric_vis_value.tsx

   Lines 86 to 93 in 808129b

   	/*
   	* Justification for dangerouslySetInnerHTML:
   	* This is one of the visualizations which makes use of the HTML field formatters.
   	* Since these formatters produce raw HTML, this visualization needs to be able to render them as-is, relying
   	* on the field formatter to only produce safe HTML.
   	* `metric.value` is set by the MetricVisComponent, so this component must make sure this value never contains
   	* any unsafe HTML (e.g. by bypassing the field formatter).
   	*/

[ananzh]

Hi @ashwin-pc, there are 3 types for url format that could be set into indexPattern field. There are link, img and audio.
There are two converter in OSD, one is html and one is text. For these 3 types, here is a compare of text converter vs html converter

• img

"/plugins/indexPatternManagement/assets/icons/177.120.218.48.png"
vs
"<span ng-non-bindable><img src="/plugins/indexPatternManagement/assets/icons/177.120.218.48.png" alt="A dynamically-specified image located at /plugins/indexPatternManagement/assets/icons/177.120.218.48.png" style="width:auto; height:auto; max-width:none; max-height:none;"></span>"

• audio

"177.120.218.48"
vs
"<span ng-non-bindable><audio controls preload="none" src="177.120.218.48"></span>"

• link

"177.120.218.48"
vs
"<span ng-non-bindable><a href="http://localhost:5603/jxv/app/177.120.218.48" target="_blank" rel="noopener noreferrer">177.120.218.48</a></span>"

@AMoo-Miki suggested using EUILink, but it only works form link. It doesn't work for img and audio. To make it format correctly, we have to use html converter then we need to translate these html contents.

[ananzh]

I opened an issue here

[ashwin-pc]

Thanks for the detailed context @joshuarrrr @ananzh. Yeah I can get behind that solution.

[joshuarrrr]

nit - this list is not exhaustive; it can actually format lots of other data types too

[ananzh]

will modify the comment msg

[joshuarrrr]

Instead of completely disabling eslint for the line, we should use the specific rule and add an explanatory comment (see comment below).

<div dangerouslySetInnerHTML={{ __html: dompurify.sanitize(htmlContent) }} /> // eslint-disable-line react/no-danger

[ananzh]

will add this. thx

[ananzh]

@ashwin-pc @joshuarrrr, just see that in metrics vis, we also use this dangerouslySetInnerHTML and doesn't include a dompurify package to sanitize the content.

      <div
         className="mtrVis__value"
         style={metricValueStyle}
          /*
           * Justification for dangerouslySetInnerHTML:
           * This is one of the visualizations which makes use of the HTML field formatters.
           * Since these formatters produce raw HTML, this visualization needs to be able to render them as-is, relying
           * on the field formatter to only produce safe HTML.
           * `metric.value` is set by the MetricVisComponent, so this component must make sure this value never contains
           * any unsafe HTML (e.g. by bypassing the field formatter).
           */
          dangerouslySetInnerHTML={{ __html: metric.value }} // eslint-disable-line react/no-danger
        />

As commented by @kavilla here, seems we should also sanitize other vis contents. But I think it should be a separate issue or raise another PR, since this is a bug fix, right?

[codecov-commenter]

Codecov Report

Merging #2918 (de01754) into main (a26fb43) will decrease coverage by 0.04%.
The diff coverage is 69.11%.

@@            Coverage Diff             @@
##             main    #2918      +/-   ##
==========================================
- Coverage   66.70%   66.65%   -0.05%     
==========================================
  Files        3219     3219              
  Lines       61477    61531      +54     
  Branches     9418     9431      +13     
==========================================
+ Hits        41010    41016       +6     
- Misses      18227    18277      +50     
+ Partials     2240     2238       -2     

Flag	Coverage Δ
Linux	66.60% <69.11%> (-0.05%)	⬇️
Windows	66.60% <69.11%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...c/plugins/data_source/server/client/client_pool.ts	58.82% <ø> (ø)
...gins/data_source/server/client/configure_client.ts	63.63% <31.81%> (-23.55%)	⬇️
.../plugins/data_source/server/data_source_service.ts	71.42% <66.66%> (-3.58%)	⬇️
...ents/edit_data_source/components/header/header.tsx	93.75% <75.00%> (-6.25%)	⬇️
.../data_source_management/public/components/utils.ts	95.83% <75.00%> (-4.17%)	⬇️
...components/create_form/create_data_source_form.tsx	96.96% <83.33%> (-1.40%)	⬇️
...rce/components/edit_form/edit_data_source_form.tsx	90.76% <86.66%> (-0.69%)	⬇️
...-eslint-plugin-eslint/rules/no_restricted_paths.js	96.96% <100.00%> (ø)
...e_data_source_wizard/create_data_source_wizard.tsx	97.50% <100.00%> (+0.72%)	⬆️
...c/components/edit_data_source/edit_data_source.tsx	88.23% <100.00%> (+0.48%)	⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[joshuarrrr]

@ananzh Do you mind also opening an issue for

As commented by @kavilla #2905 (comment), seems we should also sanitize other vis contents. But I think it should be a separate issue or raise another PR, since this is a bug fix, right?

Even if we complete #2932, we may still have other instances of html that needs sanitation. And it's a parallel fix someone may decide to pickup before we can complete 2932.

[ananzh]

#2932

Hey @joshuarrrr, thanks for the comment. I will create an issue to track all the other instances of html that needs sanitation. Will raise a PR for that.

[joshuarrrr]

@ananzh Should this also be backported to 2.x (and not to 1.x or 1.3)?

[ananzh]

@joshuarrrr yes, since it is a change of table in 2.4, I added backport to 2.x

[ananzh]

Issue for all other html content using dangerouslySetInnerHTML
#3082

[kavilla]

@ananzh looks like this conflicts with the reporting plugin https://github.com/opensearch-project/reporting/blob/main/dashboards-reports/package.json#L22. Should we create a PR into their repo?