address DOM text reinterpreted as HTML alert#38949
Conversation
|
/gemini review |
Summary of ChangesHello, 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 addresses security concerns related to how DOM elements are queried and how HTML content is rendered within tooltips. By enforcing stricter scoping for selectors and introducing a sanitization step for dynamic content, the changes reduce the risk of malicious script execution and improve the robustness of the UI components. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 the 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 counterproductive. 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. 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 updates Bootstrap JS files to use $(document).find(selector) instead of $(selector) and introduces HTML sanitization for tooltips using DOMPurify. The review feedback highlights two critical issues: first, calling $(document).find() with a falsy selector will throw a SyntaxError in jQuery, so guards must be added to return an empty jQuery collection instead; second, sanitizing tooltip content when html is false can cause regressions by stripping plain text characters, so sanitization should only be applied when html is true.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
|
|
||
| var $parent = $(selector) | ||
| var $parent = $(document).find(selector) |
There was a problem hiding this comment.
If selector is falsy (e.g., undefined or ""), calling $(document).find(selector) will throw a SyntaxError in jQuery (unlike $(selector) which safely returns an empty jQuery collection). To prevent runtime errors when data-target or href are missing or empty, we should guard the call and return an empty jQuery object if selector is falsy.
| var $parent = $(document).find(selector) | |
| var $parent = selector ? $(document).find(selector) : $() |
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | ||
| var $target = $(document).find(selector) |
There was a problem hiding this comment.
If selector is falsy, calling $(document).find(selector) will throw a SyntaxError. We should guard against falsy values and return an empty jQuery collection instead.
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | |
| var $target = $(document).find(selector) | |
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | |
| var $target = selector ? $(document).find(selector) : $() |
| || (href = $trigger.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | ||
|
|
||
| return $(target) | ||
| return $(document).find(target) |
There was a problem hiding this comment.
If target is falsy, calling $(document).find(target) will throw a SyntaxError. We should guard against falsy values and return an empty jQuery collection instead.
| return $(document).find(target) | |
| return target ? $(document).find(target) : $() |
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | ||
| var $target = $(document).find(selector) |
There was a problem hiding this comment.
If selector is falsy, calling $(document).find(selector) will throw a SyntaxError. We should guard against falsy values and return an empty jQuery collection instead.
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | |
| var $target = $(document).find(selector) | |
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | |
| var $target = selector ? $(document).find(selector) : $() |
| var title = this.getTitle() | ||
|
|
||
| $tip.find('.tooltip-inner')[this.options.html ? 'html' : 'text'](title) | ||
| $tip.find('.tooltip-inner')[this.options.html ? 'html' : 'text'](sanitizeHtml(title)) |
There was a problem hiding this comment.
Sanitizing the tooltip title when html is false can cause unexpected regressions. For example, plain text tooltips containing characters like < or > (or custom tag-like text) might have their content stripped or corrupted by DOMPurify. Since jQuery's .text() method already safely escapes HTML characters, we should only apply sanitizeHtml when this.options.html is true.
| $tip.find('.tooltip-inner')[this.options.html ? 'html' : 'text'](sanitizeHtml(title)) | |
| $tip.find('.tooltip-inner')[this.options.html ? 'html' : 'text'](this.options.html ? sanitizeHtml(title) : title) |
| } | ||
|
|
||
| var $parent = $(selector) | ||
| var $parent = $(document).find(selector) |
There was a problem hiding this comment.
If selector is falsy, calling $(document).find(selector) will throw a SyntaxError. We should guard against falsy values and return an empty jQuery collection instead.
| var $parent = $(document).find(selector) | |
| var $parent = selector ? $(document).find(selector) : $() |
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | ||
| var $target = $(document).find(selector) |
There was a problem hiding this comment.
If selector is falsy, calling $(document).find(selector) will throw a SyntaxError. We should guard against falsy values and return an empty jQuery collection instead.
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | |
| var $target = $(document).find(selector) | |
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | |
| var $target = selector ? $(document).find(selector) : $() |
| || (href = $trigger.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | ||
|
|
||
| return $(target) | ||
| return $(document).find(target) |
There was a problem hiding this comment.
If target is falsy, calling $(document).find(target) will throw a SyntaxError. We should guard against falsy values and return United jQuery collection instead.
| return $(document).find(target) | |
| return target ? $(document).find(target) : $() |
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | ||
| var $target = $(document).find(selector) |
There was a problem hiding this comment.
If selector is falsy, calling $(document).find(selector) will throw a SyntaxError. We should guard against falsy values and return an empty jQuery collection instead.
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | |
| var $target = $(document).find(selector) | |
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | |
| var $target = selector ? $(document).find(selector) : $() |
| var title = this.getTitle() | ||
|
|
||
| $tip.find('.tooltip-inner')[this.options.html ? 'html' : 'text'](title) | ||
| $tip.find('.tooltip-inner')[this.options.html ? 'html' : 'text'](sanitizeHtml(title)) |
There was a problem hiding this comment.
Sanitizing the tooltip title when html is false can cause unexpected regressions. For example, plain text tooltips containing characters like < or > (or custom tag-like text) might have their content stripped or corrupted by DOMPurify. Since jQuery's .text() method already safely escapes HTML characters, we should only apply sanitizeHtml when this.options.html is true.
| $tip.find('.tooltip-inner')[this.options.html ? 'html' : 'text'](sanitizeHtml(title)) | |
| $tip.find('.tooltip-inner')[this.options.html ? 'html' : 'text'](this.options.html ? sanitizeHtml(title) : title) |
There was a problem hiding this comment.
Code Review
This pull request updates several Bootstrap JavaScript components to query elements using $(document).find(selector) instead of direct jQuery selections, and introduces HTML sanitization for tooltips using DOMPurify. The review feedback correctly highlights that passing empty, null, or undefined selectors to $(document).find() will throw jQuery syntax errors, and recommends adding truthy guards to prevent runtime crashes. Additionally, the feedback suggests adding a type check in the sanitizeHtml function to ensure only string values are passed to DOMPurify.sanitize.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
|
|
||
| var $parent = $(selector) | ||
| var $parent = $(document).find(selector) |
There was a problem hiding this comment.
If selector is empty, undefined, or null, passing it directly to $(document).find() will throw a jQuery syntax error. To prevent runtime exceptions and preserve the original behavior where an empty selector safely returned an empty jQuery collection, we should add a guard to ensure selector is truthy before querying.
| var $parent = $(document).find(selector) | |
| var $parent = selector ? $(document).find(selector) : $() |
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | ||
| var $target = $(document).find(selector) |
There was a problem hiding this comment.
If selector is empty or undefined, calling $(document).find(selector) will throw a syntax error. Adding a guard to check if selector is truthy before querying ensures that the click handler does not crash when the target is missing.
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | |
| var $target = $(document).find(selector) | |
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | |
| var $target = selector ? $(document).find(selector) : $() |
| || (href = $trigger.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | ||
|
|
||
| return $(target) | ||
| return $(document).find(target) |
There was a problem hiding this comment.
If target is empty or undefined, $(document).find(target) will throw a syntax error. Guarding the query with a truthy check ensures robust behavior.
| return $(document).find(target) | |
| return target ? $(document).find(target) : $() |
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | ||
| var $target = $(document).find(selector) |
There was a problem hiding this comment.
If selector is empty or undefined, calling $(document).find(selector) will throw a syntax error. Adding a guard to check if selector is truthy before querying ensures that the click handler does not crash when the target is missing.
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | |
| var $target = $(document).find(selector) | |
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | |
| var $target = selector ? $(document).find(selector) : $() |
| function sanitizeHtml(string) { | ||
| if (typeof DOMPurify !== 'undefined') { | ||
| return DOMPurify.sanitize(string) | ||
| } | ||
| return string | ||
| } |
There was a problem hiding this comment.
If string is not of type 'string' (e.g., undefined, null, or an object), passing it directly to DOMPurify.sanitize might cause unexpected behavior, such as returning the string "undefined" or throwing a TypeError depending on the DOMPurify version. Adding a type check ensures we only sanitize actual string values.
| function sanitizeHtml(string) { | |
| if (typeof DOMPurify !== 'undefined') { | |
| return DOMPurify.sanitize(string) | |
| } | |
| return string | |
| } | |
| function sanitizeHtml(string) { | |
| if (typeof DOMPurify !== 'undefined' && typeof string === 'string') { | |
| return DOMPurify.sanitize(string) | |
| } | |
| return string | |
| } |
| } | ||
|
|
||
| var $parent = $(selector) | ||
| var $parent = $(document).find(selector) |
There was a problem hiding this comment.
If selector is empty, undefined, or null, passing it directly to $(document).find() will throw a jQuery syntax error. To prevent runtime exceptions and preserve the original behavior where an empty selector safely returned an empty jQuery collection, we should add a guard to ensure selector is truthy before querying.
| var $parent = $(document).find(selector) | |
| var $parent = selector ? $(document).find(selector) : $() |
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | ||
| var $target = $(document).find(selector) |
There was a problem hiding this comment.
If selector is empty or undefined, calling $(document).find(selector) will throw a syntax error. Adding a guard to check if selector is truthy before querying ensures that the click handler does not crash when the target is missing.
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | |
| var $target = $(document).find(selector) | |
| var selector = $this.attr('data-target') || (href = $this.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | |
| var $target = selector ? $(document).find(selector) : $() |
| || (href = $trigger.attr('href')) && href.replace(/.*(?=#[^\s]+$)/, '') // strip for ie7 | ||
|
|
||
| return $(target) | ||
| return $(document).find(target) |
There was a problem hiding this comment.
If target is empty or undefined, $(document).find(target) will throw a syntax error. Guarding the query with a truthy check ensures robust behavior.
| return $(document).find(target) | |
| return target ? $(document).find(target) : $() |
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | ||
| var $target = $(document).find(selector) |
There was a problem hiding this comment.
If selector is empty or undefined, calling $(document).find(selector) will throw a syntax error. Adding a guard to check if selector is truthy before querying ensures that the click handler does not crash when the target is missing.
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | |
| var $target = $(document).find(selector) | |
| var selector = $this.attr('data-target') || (href && href.replace(/.*(?=#[^\s]+$)/, '')) // strip for ie7 | |
| var $target = selector ? $(document).find(selector) : $() |
| function sanitizeHtml(string) { | ||
| if (typeof DOMPurify !== 'undefined') { | ||
| return DOMPurify.sanitize(string) | ||
| } | ||
| return string | ||
| } |
There was a problem hiding this comment.
If string is not of type 'string' (e.g., undefined, null, or an object), passing it directly to DOMPurify.sanitize might cause unexpected behavior, such as returning the string "undefined" or throwing a TypeError depending on the DOMPurify version. Adding a type check ensures we only sanitize actual string values.
| function sanitizeHtml(string) { | |
| if (typeof DOMPurify !== 'undefined') { | |
| return DOMPurify.sanitize(string) | |
| } | |
| return string | |
| } | |
| function sanitizeHtml(string) { | |
| if (typeof DOMPurify !== 'undefined' && typeof string === 'string') { | |
| return DOMPurify.sanitize(string) | |
| } | |
| return string | |
| } |
1 participant
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.