[PPP-5353]-XSS Findings For Pentaho-platform-plugin-reporting #864
Conversation
This comment has been minimized.
This comment has been minimized.
| @@ -15,26 +15,26 @@ | |||
| * | |||
| */ | |||
| define(["dojo/_base/declare", "dijit/_WidgetBase", "dijit/_Templated", "dojo/on", "dojo/query", | |||
| "pentaho/common/button", "pentaho/common/Dialog", "dojo/text!pentaho/reportviewer/GlassPane.html"], | |||
| function(declare, _WidgetBase, _Templated, on, query, button, Dialog, templateStr){ | |||
| "pentaho/common/button", "pentaho/common/Dialog", "dojo/text!pentaho/reportviewer/GlassPane.html","common-ui/util/xss"], | |||
| @@ -72,7 +73,7 @@ define(function() { | |||
| if(!enabled) { return null; } | |||
|
|
|||
| // May be null in case popups blocked | |||
| var logWin = window.open('', options.winname || 'report_viewer_log'); | |||
| var logWin = xssUtil.setHtml(window.open('', options.winname || 'report_viewer_log')); | |||
There was a problem hiding this comment.
Why is setHtml used in this case?
| @@ -822,7 +822,7 @@ define([ 'common-ui/util/util', 'common-ui/util/timeutil', 'common-ui/util/forma | |||
| if(isRunningIFrameInSameOrigin) { | |||
| if (!top.mantle_initialized) { | |||
| this._topMantleOpenTabRegistration = top.mantle_openTab = function(name, title, url) { | |||
| window.open(url, '_blank'); | |||
| window.open(xssUtil.sanitizeHtml(url), '_blank'); | |||
There was a problem hiding this comment.
Why is setHtml used in this case? Not sure it applies for an empty URL. Please coordinate with @mmujahiduddin regarding sanitizeUrl: pentaho/pentaho-platform#5758 (comment).
| */ | ||
|
|
||
| define([ 'common-ui/util/util', 'common-ui/util/timeutil', 'common-ui/util/formatting', 'pentaho/common/Messages', | ||
| "dojo/dom", "dojo/on", "dojo/_base/lang", "dijit/registry", "dojo/has", "dojo/sniff", "dojo/dom-class", | ||
| 'pentaho/reportviewer/ReportDialog', "dojo/dom-style", | ||
| "dojo/query", "common-ui/util/_a11y", "dojo/dom-geometry", "dojo/parser", "dojo/window", "dojo/_base/window", | ||
| 'cdf/lib/jquery', 'amd!cdf/lib/jquery.ui', "common-repo/pentaho-ajax", "dijit/ProgressBar", "common-data/xhr"], | ||
| 'cdf/lib/jquery', 'amd!cdf/lib/jquery.ui', "common-repo/pentaho-ajax", "dijit/ProgressBar", "common-data/xhr","common-ui/util/xss"], |
| @@ -1193,7 +1193,7 @@ define([ 'common-ui/util/util', 'common-ui/util/timeutil', 'common-ui/util/forma | |||
| hideDlgAndPane(registry.byId('feedbackScreen')); | |||
|
|
|||
| //Show loading screen | |||
| $('#notification-message').html(_Messages.getString('LoadingPage') + " " + mainJobStatus.page + " " + _Messages.getString('Of') + " " + mainJobStatus.totalPages); | |||
| xssUtil.setHtml($('notification-message'), _Messages.getString('LoadingPage') + " " + mainJobStatus.page + " " + _Messages.getString('Of') + " " + mainJobStatus.totalPages); | |||
There was a problem hiding this comment.
I can't find the comment from another PR review where I wrote this, so I'm repeating here.
Note that the query selector passed to jQuery should remain as #notification-message.
The XSS Utility setHtml(elem, html) method does not support jQuery objects in the first argument, only actual HTMLElement.
On the other hand, if we keep using the pattern $('#notification-message').html(xssUtil.sanitizeHtml(html)) with jQuery instances, likely, the corresponding AppScan findings will not be automatically resolved.
As such, we should consider adding support for jQuery objects to the existing setHtml(..) method, and then use it with these jQuery cases.
| @@ -1236,7 +1236,7 @@ define([ 'common-ui/util/util', 'common-ui/util/timeutil', 'common-ui/util/forma | |||
| isPageCountUpdated = true; | |||
| } | |||
|
|
|||
| $('#notification-message').html(_Messages.getString('LoadingPage') + " " + mainJobStatus.page + " " + _Messages.getString('Of') + " " + mainJobStatus.totalPages); | |||
| xssUtil.setHtml($('#notification-message'), _Messages.getString('LoadingPage') + " " + mainJobStatus.page + " " + _Messages.getString('Of') + " " + mainJobStatus.totalPages); | |||
There was a problem hiding this comment.
Idem, regarding adding jQuery support to setHtml(..).
| @@ -1246,13 +1246,13 @@ define([ 'common-ui/util/util', 'common-ui/util/timeutil', 'common-ui/util/forma | |||
| case "QUEUED": | |||
| case "WORKING": | |||
| // Although we are hiding the screen latter, update the label anyway; | |||
| $('#notification-message').html(_Messages.getString('LoadingPage') + " " + mainJobStatus.page + " " + _Messages.getString('Of') + " " + mainJobStatus.totalPages); | |||
| xssUtil.setHtml($('#notification-message'), _Messages.getString('LoadingPage') + " " + mainJobStatus.page + " " + _Messages.getString('Of') + " " + mainJobStatus.totalPages); | |||
There was a problem hiding this comment.
Idem, regarding adding jQuery support to setHtml(..).
| me._hideAsyncScreens(); | ||
| me._keepPolling(mainJobStatus.uuid, url, mainReportGeneration); | ||
| break; | ||
| case "FINISHED": | ||
| // Although we are hiding the screen latter, update the label anyway; | ||
| $('#notification-message').html(_Messages.getString('LoadingPage') + " " + mainJobStatus.page + " " + _Messages.getString('Of') + " " + mainJobStatus.totalPages); | ||
| xssUtil.setHtml($('#notification-message'), _Messages.getString('LoadingPage') + " " + mainJobStatus.page + " " + _Messages.getString('Of') + " " + mainJobStatus.totalPages); |
There was a problem hiding this comment.
Idem, regarding adding jQuery support to setHtml(..).
Rangashivani
force-pushed
the
BACKLOG-41215
branch
from
02d1bf2 to
bb71b27
Compare
|
Note:Frogbot also supports Contextual Analysis, Secret Detection, IaC and SAST Vulnerabilities Scanning. This features are included as part of the JFrog Advanced Security package, which isn't enabled on your system. |
✅ Build finished in 5m 58sBuild command: mvn clean verify -B -e -Daudit -Djs.no.sandbox -pl core👌 All tests passed! Tests run: 540, Failures: 0, Skipped: 3 Test Results ℹ️ This is an automatic message |
PPP-5353-XSS Findings For Pentaho-platform-plugin-reporting