feat: add plugin tooltip option prefixCls#7512
Conversation
Summary of ChangesHello @XiaoMing-xmg666, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the tooltip plugin by introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a prefixCls option to the tooltip plugin, allowing for customization of the CSS class prefix. The changes span the plugin's options, constructor, and style applications, and are supported by new tests and updated documentation. My main feedback is on the plugin's constructor, where the current option-merging logic could unintentionally override crucial default styles. I've suggested a more robust merging strategy to prevent this potential issue.
| const combineOptions = Object.assign( | ||
| { | ||
| style: { | ||
| [`.${options.prefixCls || ''}tooltip`]: { | ||
| visibility: 'hidden', | ||
| }, | ||
| }, | ||
| }, | ||
| Tooltip.defaultOptions, | ||
| options, | ||
| ); | ||
| super(context, combineOptions); |
There was a problem hiding this comment.
The current implementation for combining options might lead to issues. Object.assign performs a shallow merge. If a user provides a style object in the options, it will completely overwrite the default style that sets visibility: 'hidden'. This could cause the tooltip to be visible by default, which is likely not the intended behavior and a regression for users customizing styles.
To fix this, I recommend using a deep merge for the options, especially for the style property. This ensures that user-provided styles are merged with the default styles, preserving the essential visibility: 'hidden' rule. You'll need to import deepMix from @antv/util at the top of the file.
| const combineOptions = Object.assign( | |
| { | |
| style: { | |
| [`.${options.prefixCls || ''}tooltip`]: { | |
| visibility: 'hidden', | |
| }, | |
| }, | |
| }, | |
| Tooltip.defaultOptions, | |
| options, | |
| ); | |
| super(context, combineOptions); | |
| const defaultStyledOptions = { | |
| style: { | |
| [`.${options.prefixCls || ''}tooltip`]: { | |
| visibility: 'hidden', | |
| }, | |
| }, | |
| }; | |
| const finalOptions = deepMix({}, defaultStyledOptions, Tooltip.defaultOptions, options); | |
| super(context, finalOptions); |
There was a problem hiding this comment.
@XiaoMing-xmg666 这里得改下 Object.assign 可能会意外覆盖 style
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v5 #7512 +/- ##
=======================================
Coverage 95.59% 95.59%
=======================================
Files 188 188
Lines 9888 9892 +4
Branches 2134 2137 +3
=======================================
+ Hits 9452 9456 +4
+ Misses 436 404 -32
- Partials 0 32 +32
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| template: { | ||
| prefixCls: this.options.prefixCls || '', | ||
| }, |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a prefixCls option to the tooltip plugin for custom CSS class prefixes. However, a significant security vulnerability exists: the prefixCls string is not sanitized, which could lead to CSS Injection and Cross-Site Scripting (XSS) if exposed to untrusted input. Furthermore, the current option merging in the constructor uses a shallow merge, potentially causing user-provided styles to incorrectly overwrite default styles.
| const combineOptions = Object.assign( | ||
| { | ||
| style: { | ||
| [`.${options.prefixCls || ''}tooltip`]: { |
There was a problem hiding this comment.
The prefixCls option is used to construct a CSS selector key without any sanitization. This creates a potential CSS Injection and Cross-Site Scripting (XSS) vulnerability if an attacker can control this option, as they could inject malicious CSS rules. It is recommended to sanitize the input to allow only safe characters like alphanumeric characters, hyphens, and underscores.
| [`.${options.prefixCls || ''}tooltip`]: { | |
| [`.${(options.prefixCls || '').replace(/[^a-zA-Z0-9_-]/g, '')}tooltip`]: { |
| y, | ||
| style: { | ||
| '.tooltip': { | ||
| [`.${this.options.prefixCls || ''}tooltip`]: { |
There was a problem hiding this comment.
Similar to the constructor, the show method uses the prefixCls option to construct a CSS selector without sanitization, which is vulnerable to CSS injection.
| [`.${this.options.prefixCls || ''}tooltip`]: { | |
| [`.${(this.options.prefixCls || '').replace(/[^a-zA-Z0-9_-]/g, '')}tooltip`]: { |
| offset, | ||
| style, | ||
| template: { | ||
| prefixCls: this.options.prefixCls || '', |
There was a problem hiding this comment.
The prefixCls is passed to the component's template without sanitization. If the underlying TooltipComponent renders this value directly into the DOM (e.g., as a class name in an unescaped template), it could lead to Cross-Site Scripting (XSS). Sanitizing the prefix ensures it remains a safe string for DOM attributes.
| prefixCls: this.options.prefixCls || '', | |
| prefixCls: (this.options.prefixCls || '').replace(/[^a-zA-Z0-9_-]/g, ''), |
3 participants
fixes: #7153
