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/09 10:14:43 UTC
[GitHub] [echarts] susiwen8 opened a new pull request #14434: Fix(xss): tooltip has xss problem
susiwen8 opened a new pull request #14434:
URL: https://github.com/apache/echarts/pull/14434
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
Tooltip formatter provides params which might come from user input. We should escape it.
### Fixed issues
Close #14429
## Details
### Before: What was the problem?
### After: How is it fixed in this PR?
<!-- THE RESULT AFTER FIXING AND A SIMPLE EXPLANATION ABOUT HOW IT IS FIXED. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
## Usage
### Are there any API changes?
- [ ] The API has been changed.
<!-- LIST THE API CHANGES HERE -->
### Related test cases or examples to use the new APIs
NA.
## Others
### Merging options
- [ ] Please squash the commits into a single one when merge.
### Other information
----------------------------------------------------------------
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
[GitHub] [echarts] pissang commented on a change in pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#discussion_r590967184
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -509,9 +509,9 @@ class TooltipView extends ComponentView {
cbParams.axisIndex = axisItem.axisIndex;
cbParams.axisType = axisItem.axisType;
cbParams.axisId = axisItem.axisId;
- cbParams.axisValue = axisHelper.getAxisRawValue(
+ cbParams.axisValue = formatUtil.encodeHTML(axisHelper.getAxisRawValue(
Review comment:
Yes
----------------------------------------------------------------
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
[GitHub] [echarts] pissang commented on a change in pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#discussion_r590967606
##########
File path: src/model/mixin/dataFormat.ts
##########
@@ -80,7 +80,7 @@ export class DataFormatMixin {
seriesIndex: (this as any).seriesIndex,
seriesId: isSeries ? this.id : null,
seriesName: isSeries ? this.name : null,
- name: name,
Review comment:
This is still not fixed.
----------------------------------------------------------------
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
[GitHub] [echarts] pissang commented on a change in pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#discussion_r590994893
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -509,9 +509,9 @@ class TooltipView extends ComponentView {
cbParams.axisIndex = axisItem.axisIndex;
cbParams.axisType = axisItem.axisType;
cbParams.axisId = axisItem.axisId;
- cbParams.axisValue = axisHelper.getAxisRawValue(
+ cbParams.axisValue = formatUtil.encodeHTML(axisHelper.getAxisRawValue(
Review comment:
I found I thought it wrong. It won't be an issue if we escaped '&', '"' in the strings. It will be displayed normally in HTML. In this case, I think an extra option is not necessary.
@100pah Any thoughts?
----------------------------------------------------------------
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
[GitHub] [echarts] pissang commented on a change in pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#discussion_r590194079
##########
File path: src/model/mixin/dataFormat.ts
##########
@@ -80,7 +80,7 @@ export class DataFormatMixin {
seriesIndex: (this as any).seriesIndex,
seriesId: isSeries ? this.id : null,
seriesName: isSeries ? this.name : null,
- name: name,
Review comment:
We should not encode the params, it is not only used in tooltip.
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -509,9 +509,9 @@ class TooltipView extends ComponentView {
cbParams.axisIndex = axisItem.axisIndex;
cbParams.axisType = axisItem.axisType;
cbParams.axisId = axisItem.axisId;
- cbParams.axisValue = axisHelper.getAxisRawValue(
+ cbParams.axisValue = formatUtil.encodeHTML(axisHelper.getAxisRawValue(
Review comment:
I think it's very common that chars like '&', '"' exist in the strings. We should not do such a general encoding. At least we should use an option to enable it optionally.
----------------------------------------------------------------
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
[GitHub] [echarts] susiwen8 commented on pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#issuecomment-795202334
I made a test for `xss` in `pie3.html`, and there were no problem.
Could we just encode return value form `formatter` in `html` mode? @pissang @100pah
----------------------------------------------------------------
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
[GitHub] [echarts] susiwen8 edited a comment on pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
susiwen8 edited a comment on pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#issuecomment-795211372
I made a test for `xss` in `pie3.html`, and there were no problem.
～～Could we just encode return value from `formatter` in `html` mode? @pissang @100pah ～～Bad idea, since we support html element now
----------------------------------------------------------------
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
[GitHub] [echarts] pissang commented on a change in pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#discussion_r590994893
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -509,9 +509,9 @@ class TooltipView extends ComponentView {
cbParams.axisIndex = axisItem.axisIndex;
cbParams.axisType = axisItem.axisType;
cbParams.axisId = axisItem.axisId;
- cbParams.axisValue = axisHelper.getAxisRawValue(
+ cbParams.axisValue = formatUtil.encodeHTML(axisHelper.getAxisRawValue(
Review comment:
I found I thought it wrong. It won't be an issue if we escaped '&', '"' in the strings. It will be displayed normally in HTML. In this case, I think an extra option is not necessary.
----------------------------------------------------------------
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
[GitHub] [echarts] susiwen8 commented on a change in pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#discussion_r590221712
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -509,9 +509,9 @@ class TooltipView extends ComponentView {
cbParams.axisIndex = axisItem.axisIndex;
cbParams.axisType = axisItem.axisType;
cbParams.axisId = axisItem.axisId;
- cbParams.axisValue = axisHelper.getAxisRawValue(
+ cbParams.axisValue = formatUtil.encodeHTML(axisHelper.getAxisRawValue(
Review comment:
> we should use an option to enable it optionally.
You mean add an new option for user?
##########
File path: src/component/tooltip/TooltipView.ts
##########
@@ -509,9 +509,9 @@ class TooltipView extends ComponentView {
cbParams.axisIndex = axisItem.axisIndex;
cbParams.axisType = axisItem.axisType;
cbParams.axisId = axisItem.axisId;
- cbParams.axisValue = axisHelper.getAxisRawValue(
+ cbParams.axisValue = formatUtil.encodeHTML(axisHelper.getAxisRawValue(
Review comment:
> we should use an option to enable it optionally.
You mean add an new option for user?
----------------------------------------------------------------
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
[GitHub] [echarts] echarts-bot[bot] commented on pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#issuecomment-793666179
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/echarts/wiki/How-to-make-a-pull-request).
The pull request is marked to be `PR: author is committer` because you are a committer of this project.
----------------------------------------------------------------
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
[GitHub] [echarts] susiwen8 closed pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
susiwen8 closed pull request #14434:
URL: https://github.com/apache/echarts/pull/14434
----------------------------------------------------------------
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
[GitHub] [echarts] 100pah commented on a change in pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
100pah commented on a change in pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#discussion_r591027667
##########
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.
I think, the **principle** can be:
If the html is assembled by echarts (e.g., when echarts give a default tooltip html formatter), echarts takes charge of the `encodeHTML`.
If the html is assembled by users (e.g., when users set `tooltip.formatter` as a function and use `renderMode: 'html'`), users take charge of `encodeHTML`.
----------------------------------------------------------------
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
[GitHub] [echarts] susiwen8 edited a comment on pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
susiwen8 edited a comment on pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#issuecomment-795211372
I made a test for `xss` in `pie3.html`, and there were no problem.
~~Could we just encode return value from `formatter` in `html` mode? @pissang @100pah ~~Bad idea, since we support html element now
----------------------------------------------------------------
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
[GitHub] [echarts] 100pah commented on a change in pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [echarts] susiwen8 commented on pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#issuecomment-795211372
I made a test for `xss` in `pie3.html`, and there were no problem.
Could we just encode return value from `formatter` in `html` mode? @pissang @100pah
----------------------------------------------------------------
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
[GitHub] [echarts] susiwen8 edited a comment on pull request #14434: Fix(xss): tooltip has xss problem
Posted by GitBox <gi...@apache.org>.
susiwen8 edited a comment on pull request #14434:
URL: https://github.com/apache/echarts/pull/14434#issuecomment-795211372
I made a test for `xss` in `pie3.html`, and there were no problem.
~~Could we just encode return value from `formatter` in `html` mode? @pissang @100pah~~ Bad idea, since we support html element now
----------------------------------------------------------------
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