You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "amaannawab923 (via GitHub)" <gi...@apache.org> on 2025/06/17 15:35:12 UTC
[PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
amaannawab923 opened a new pull request, #33791:
URL: https://github.com/apache/superset/pull/33791
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Fixes: https://github.com/apache/superset/issues/33771
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2275567135
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
+ if (typeof value === 'string' && isProbablyHTML(value)) {
+ return removeHTMLTags(value);
+ }
Review Comment:
### Insufficient HTML Sanitization <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sanitization of HTML values relies on 'isProbablyHTML' for detection and 'removeHTMLTags' for sanitization, which may be insufficient for preventing XSS attacks
###### Why this matters
Without proper HTML sanitization, malicious users could potentially inject harmful scripts that execute in users' browsers when viewing table data
###### Suggested change ∙ *Feature Preview*
Replace with a proven HTML sanitization library like DOMPurify:
```typescript
import DOMPurify from 'dompurify';
const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
if (typeof value === 'string') {
return DOMPurify.sanitize(value);
}
return value;
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7ea64bb-a981-4e63-b97f-41a684775f05/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7ea64bb-a981-4e63-b97f-41a684775f05?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7ea64bb-a981-4e63-b97f-41a684775f05?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7ea64bb-a981-4e63-b97f-41a684775f05?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a7ea64bb-a981-4e63-b97f-41a684775f05)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fbd9b9d8-8d56-4859-8662-5f2f9dec2e1d -->
[](fbd9b9d8-8d56-4859-8662-5f2f9dec2e1d)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2276145208
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
Review Comment:
Yes this is different , its about extracting value from Html Tag ->
Input :
< /a href="https://www.google.com/search?q=United Kingdom">United Kingdom<\/a>"
Expected Output:
United Kingdom
Sanitise Html function would just sanitise that html to remove malicious code
For now , i guess this is component specific , because it will only come to use here in Table
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #33791:
URL: https://github.com/apache/superset/pull/33791#issuecomment-3189388130
Add simple covering unit tests and I think this is mergeable!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 closed pull request #33791: fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content
URL: https://github.com/apache/superset/pull/33791
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 closed pull request #33791: fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content
URL: https://github.com/apache/superset/pull/33791
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33791:
URL: https://github.com/apache/superset/pull/33791#issuecomment-2981199293
@sadpandajoe Ephemeral environment spinning up at http://35.89.173.179:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #33791:
URL: https://github.com/apache/superset/pull/33791#issuecomment-3180966281
Converting to draft until adjusted / ready for another round of review
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on PR #33791:
URL: https://github.com/apache/superset/pull/33791#issuecomment-3187064533
> Converting to draft until adjusted / ready for another round of review
Did the changes as per recommendation @mistercrunch
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2276145208
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
Review Comment:
Yes this is different , its about extracting value from Html Tag ->
Input :
<a href="https://www.google.com/search?q=United Kingdom">United Kingdom</a>
Expected Output:
United Kingdom
Sanitise Html function would just sanitise that html to remove malicious code
For now , i guess this is component specific , because it will only come to use here in Table
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #33791:
URL: https://github.com/apache/superset/pull/33791#issuecomment-2981181889
@sadpandajoe Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/15713882455). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 closed pull request #33791: fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content
URL: https://github.com/apache/superset/pull/33791
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2276145208
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
Review Comment:
Yes this is different , its about extracting value from Html Tag ->
Input :
"<a href="https://www.google.com/search?q=United Kingdom">United Kingdom</a>"
Expected Output:
United Kingdom
Sanitise Html function would just sanitise that html to remove malicious code
For now , i guess this is component specific , because it will only come to use here in Table
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
Review Comment:
Yes this is different , its about extracting value from Html Tag ->
Input :
< /a href="https://www.google.com/search?q=United Kingdom">United Kingdom</a>"
Expected Output:
United Kingdom
Sanitise Html function would just sanitise that html to remove malicious code
For now , i guess this is component specific , because it will only come to use here in Table
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2277078531
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
+ if (typeof value === 'string' && isProbablyHTML(value)) {
+ return removeHTMLTags(value);
+ }
Review Comment:
Agreed - moving sanitizeHtmlValue to the HTML utils module would improve reusability. That's a better place for HTML sanitization utilities.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2271161993
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -243,6 +243,24 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const isHtml = (value: any): boolean => {
Review Comment:
comment crafted with Claude ->
---
Hi @amaannawab923, thanks for working on this fix!
For handling HTML content in table cells, we already have comprehensive HTML utilities available in the codebase that should be used:
📍 HTML Utilities Location: @superset-ui/core/src/utils/html
Key functions available:
- removeHTMLTags(str) - Strips all HTML tags from a string (useful for getting plain text for context menu operations)
- sanitizeHtml(htmlString) - Safely sanitizes HTML content using XSS filtering
- isProbablyHTML(text) - Detects if a string contains valid HTML
- safeHtmlSpan(possiblyHtmlString) - Returns a safe React component for rendering HTML
The table chart already uses these utilities in superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts:54 for sanitizing HTML content when allowRenderHtml is enabled.
For the context menu issue with HTML <a> tags, you might want to use removeHTMLTags() to extract the plain text value for drill-down operations. This is already being done in the drill detail menu items (see
DrillDetailMenuItems.tsx:93).
Example usage:
import { removeHTMLTags } from '@superset-ui/core';
// When handling context menu for HTML cells
const plainTextValue = removeHTMLTags(htmlCellValue);
This approach ensures consistency with how HTML is handled throughout the codebase and maintains proper security sanitization.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2276152061
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
Review Comment:
I think the function name is causing confusion with html sanitisation , should probably rename it to , ExtractTextFromHtml
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #33791:
URL: https://github.com/apache/superset/pull/33791#issuecomment-3192875172
@msyavuz why the hold?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz merged PR #33791:
URL: https://github.com/apache/superset/pull/33791
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #33791:
URL: https://github.com/apache/superset/pull/33791#issuecomment-3192993692
The issue has a bounty assigned. We need to remove that before this get's merged.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2277077691
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
+ if (typeof value === 'string' && isProbablyHTML(value)) {
+ return removeHTMLTags(value);
+ }
Review Comment:
assuming it's generic enough and can be reused elsewhere, let's move the function definition to the html utils module
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on PR #33791:
URL: https://github.com/apache/superset/pull/33791#issuecomment-3190797854
> Add simple covering unit tests and I think this is mergeable!
Done ! Added Unit tests & Thanks for the review
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2152589116
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -243,6 +243,24 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const isHtml = (value: any): boolean => {
+ if (typeof value !== 'string') {
+ return false;
+ }
+ return value.includes('<') && value.includes('>');
+};
+
+const stripHtmlAndGetValue = (value: any): string | number | boolean => {
+ // Create a temporary DOM element to parse the HTML
+ const tempDiv = document.createElement('div');
+ tempDiv.innerHTML = value;
+
+ // Get the text content (this removes all HTML tags)
+ const textContent = tempDiv.textContent || tempDiv.innerText || '';
+
+ return textContent.trim();
+};
Review Comment:
### Missing security and environment documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The stripHtmlAndGetValue function's security implications and browser dependency are not documented.
###### Why this matters
The function uses innerHTML which has security implications, and it requires a browser environment. Without documentation, developers may not be aware of these important constraints.
###### Suggested change ∙ *Feature Preview*
/**
* Strips HTML tags from a string and returns the text content.
* @warning Only use with trusted HTML content as it uses innerHTML
* @warning Requires browser environment (document object)
*/
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e46509b-a402-41ec-83d0-25ac21962751/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e46509b-a402-41ec-83d0-25ac21962751?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e46509b-a402-41ec-83d0-25ac21962751?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e46509b-a402-41ec-83d0-25ac21962751?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8e46509b-a402-41ec-83d0-25ac21962751)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c7991bc1-0311-46bc-b6a9-959598361703 -->
[](c7991bc1-0311-46bc-b6a9-959598361703)
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -243,6 +243,24 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const isHtml = (value: any): boolean => {
+ if (typeof value !== 'string') {
+ return false;
+ }
+ return value.includes('<') && value.includes('>');
+};
Review Comment:
### Unreliable HTML Detection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The HTML detection logic is too simplistic and could lead to false positives or negatives in identifying HTML content.
###### Why this matters
The current implementation might incorrectly identify strings containing '<' and '>' as HTML even when they're not, or miss malformed HTML content.
###### Suggested change ∙ *Feature Preview*
Use a more robust HTML detection method:
```typescript
const isHtml = (value: any): boolean => {
if (typeof value !== 'string') {
return false;
}
const doc = new DOMParser().parseFromString(value, 'text/html');
return Array.from(doc.body.childNodes).some(node => node.nodeType === 1);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c51800b-8f76-4fd3-bdad-07e908d1a37c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c51800b-8f76-4fd3-bdad-07e908d1a37c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c51800b-8f76-4fd3-bdad-07e908d1a37c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c51800b-8f76-4fd3-bdad-07e908d1a37c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c51800b-8f76-4fd3-bdad-07e908d1a37c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:782aa2a6-7469-4e57-a9af-68edbae7dec9 -->
[](782aa2a6-7469-4e57-a9af-68edbae7dec9)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2271161993
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -243,6 +243,24 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const isHtml = (value: any): boolean => {
Review Comment:
comment crafted with Claude ->
---
Hi @amaannawab923, thanks for working on this fix!
For handling HTML content in table cells, we already have comprehensive HTML utilities available in the codebase that should be used:
📍 HTML Utilities Location: @superset-ui/core/src/utils/html
Key functions available:
- removeHTMLTags(str) - Strips all HTML tags from a string (useful for getting plain text for context menu operations)
- sanitizeHtml(htmlString) - Safely sanitizes HTML content using XSS filtering
- isProbablyHTML(text) - Detects if a string contains valid HTML
- safeHtmlSpan(possiblyHtmlString) - Returns a safe React component for rendering HTML
The table chart already uses these utilities in superset-frontend/plugins/plugin-chart-table/src/utils/formatValue.ts:54 for sanitizing HTML content when allowRenderHtml is enabled.
For the context menu issue with HTML <a> tags, you might want to use removeHTMLTags() to extract the plain text value for drill-down operations. This is already being done in the drill detail menu items (see
DrillDetailMenuItems.tsx:93).
Example usage:
import { removeHTMLTags } from '@superset-ui/core';
// When handling context menu for HTML cells
const plainTextValue = removeHTMLTags(htmlCellValue);
This approach ensures consistency with how HTML is handled throughout the codebase and maintains proper security sanitization.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2275788956
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
Review Comment:
can't use the sanitizeHtml from the utils? Guessing this is different? does this function belongs in the utils or is it component-specific?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Context-Menu): Fixing Context Menu for Table Chart with Html Content [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #33791:
URL: https://github.com/apache/superset/pull/33791#discussion_r2277109387
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -251,6 +253,13 @@ function SelectPageSize({
const getNoResultsMessage = (filter: string) =>
filter ? t('No matching records found') : t('No records found');
+const sanitizeHtmlValue = (value: DataRecordValue): DataRecordValue => {
+ if (typeof value === 'string' && isProbablyHTML(value)) {
+ return removeHTMLTags(value);
+ }
Review Comment:
Done
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org