You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2021/03/10 04:39:09 UTC
[GitHub] [echarts] 100pah commented on a change in pull request #14434: Fix(xss): tooltip has xss problem
100pah commented on a change in pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#discussion_r591016806
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -505,18 +512,20 @@ class TooltipView extends ComponentView {
const series = ecModel.getSeriesByIndex(idxItem.seriesIndex);
const dataIndex = idxItem.dataIndexInside;
const cbParams = series.getDataParams(dataIndex) as TooltipCallbackDataParams;
+ cbParams.name = encode ? encodeHTML(cbParams.name) : cbParams.name;
cbParams.axisDim = axisItem.axisDim;
cbParams.axisIndex = axisItem.axisIndex;
cbParams.axisType = axisItem.axisType;
cbParams.axisId = axisItem.axisId;
- cbParams.axisValue = axisHelper.getAxisRawValue(
+ const cbAxisValue = axisHelper.getAxisRawValue(
axisModel.axis, { value: axisValue as number }
- );
+ ) as string;
+ cbParams.axisValue = encode ? encodeHTML(cbAxisValue) : cbAxisValue;
Review comment:
I think we should not `encodeHTML` here.
`encodeHTML` only make sense in HTML rendering.
The encoded string should not be exposed to users in callback.
That is, when we provide the "non-encoded" string to users,
users can encode it themselves if needed.
But if we provide an "encoded" string, user can not do anything others to them.
Especially when the `renderMode` is `richText` rather than `html`,
there should be no encode.
I think #14429 is probably not an issue.
See the [comment](https://github.com/apache/echarts/issues/14429#issuecomment-794847914) I've added just now.
##########
File path: src/component/tooltip/TooltipModel.ts
##########
@@ -46,6 +46,10 @@ export interface TooltipOption extends CommonTooltipOption<TopLevelFormatterPara
* If show popup content
*/
showContent?: boolean
+ /**
+ * escape html, prevent xss
+ */
+ escapeContent?: boolean
Review comment:
I think we should neither add this option, nor `encodeHTML` internally.
The reason is:
+ See the [comment](https://github.com/apache/echarts/issues/14429#issuecomment-794847914), users should do the encode themselves if the assemble html snippet in `tooltip.formatter`.
+ If echarts do the "encodeHTML" internally by this option `escapeContent`, echarts should not only do `encodeHTML` to part of the params but left other params not encoded. Otherwise users will be confused.
But for some complex data structure like `params.data`, it's not an easy work to encode it.
it's not easy to do it completely.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org