From 1e6f850be8a0884585ce7456531330464e94493a Mon Sep 17 00:00:00 2001 From: Joris Melchior Date: Fri, 26 Aug 2022 13:26:22 -0400 Subject: [PATCH] GEODE-10411: fix XSS vulnerability in pulse (#7836) * GEODE-10411: fix XSS vulnerability in pulse - html encode data coming from Geode queries - add cookie parameters to increase browsing security * Fix spotless check errors --- .../pulse/tests/DataBrowserResultLoader.java | 74 +++++----- .../tools/pulse/tests/PulseTestData.java | 5 +- .../testQueryResultClusterSmallJSInject.txt | 23 +++ .../src/main/webapp/META-INF/context.xml | 26 ++++ geode-pulse/src/main/webapp/WEB-INF/web.xml | 8 + .../pulsescript/pages/DataBrowserQuery.js | 139 +++++++++--------- .../pages/DataBrowserQueryHistory.js | 26 ++-- .../pulse/tests/ui/PulseAutomatedTest.java | 65 ++++++-- 8 files changed, 234 insertions(+), 132 deletions(-) create mode 100644 geode-pulse/geode-pulse-test/src/main/resources/testQueryResultClusterSmallJSInject.txt create mode 100644 geode-pulse/src/main/webapp/META-INF/context.xml diff --git a/geode-pulse/geode-pulse-test/src/main/java/org/apache/geode/tools/pulse/tests/DataBrowserResultLoader.java b/geode-pulse/geode-pulse-test/src/main/java/org/apache/geode/tools/pulse/tests/DataBrowserResultLoader.java index f2278004c3fd..392f0c9c4880 100644 --- a/geode-pulse/geode-pulse-test/src/main/java/org/apache/geode/tools/pulse/tests/DataBrowserResultLoader.java +++ b/geode-pulse/geode-pulse-test/src/main/java/org/apache/geode/tools/pulse/tests/DataBrowserResultLoader.java @@ -17,13 +17,11 @@ package org.apache.geode.tools.pulse.tests; import java.io.BufferedReader; -import java.io.File; -import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; -import java.net.URL; import java.nio.charset.StandardCharsets; +import java.util.stream.Collectors; public class DataBrowserResultLoader { /* Constants for executing Data Browser queries */ @@ -33,7 +31,8 @@ public class DataBrowserResultLoader { public static final String QUERY_TYPE_FOUR = "query4"; public static final String QUERY_TYPE_FIVE = "query5"; public static final String QUERY_TYPE_SIX = "query6"; - public static final String QUERY_TYPE_SEVENE = "query7"; + public static final String QUERY_TYPE_SEVEN = "query7"; + public static final String QUERY_TYPE_EIGHT = "query8"; private static final DataBrowserResultLoader dbResultLoader = new DataBrowserResultLoader(); @@ -43,41 +42,46 @@ public static DataBrowserResultLoader getInstance() { public String load(String queryString) throws IOException { - URL url = null; - InputStream inputStream = null; - BufferedReader streamReader = null; - String inputStr = null; - StringBuilder sampleQueryResultResponseStrBuilder = null; + String fileName; + String fileContent = ""; try { - ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); - if (queryString.equals(QUERY_TYPE_ONE)) { - url = classLoader.getResource("testQueryResultClusterSmall.txt"); - } else if (queryString.equals(QUERY_TYPE_TWO)) { - url = classLoader.getResource("testQueryResultSmall.txt"); - } else if (queryString.equals(QUERY_TYPE_THREE)) { - url = classLoader.getResource("testQueryResult.txt"); - } else if (queryString.equals(QUERY_TYPE_FOUR)) { - url = classLoader.getResource("testQueryResultWithStructSmall.txt"); - } else if (queryString.equals(QUERY_TYPE_FIVE)) { - url = classLoader.getResource("testQueryResultClusterWithStruct.txt"); - } else if (queryString.equals(QUERY_TYPE_SIX)) { - url = classLoader.getResource("testQueryResultHashMapSmall.txt"); - } else if (queryString.equals(QUERY_TYPE_SEVENE)) { - url = classLoader.getResource("testQueryResult1000.txt"); - } else { - url = classLoader.getResource("testQueryResult.txt"); + switch (queryString) { + case QUERY_TYPE_ONE: + fileName = "testQueryResultClusterSmall.txt"; + break; + case QUERY_TYPE_TWO: + fileName = "testQueryResultSmall.txt"; + break; + case QUERY_TYPE_THREE: + fileName = "testQueryResult.txt"; + break; + case QUERY_TYPE_FOUR: + fileName = "testQueryResultWithStructSmall.txt"; + break; + case QUERY_TYPE_FIVE: + fileName = "testQueryResultClusterWithStruct.txt"; + break; + case QUERY_TYPE_SIX: + fileName = "testQueryResultHashMapSmall.txt"; + break; + case QUERY_TYPE_SEVEN: + fileName = "testQueryResult1000.txt"; + break; + case QUERY_TYPE_EIGHT: + fileName = "testQueryResultClusterSmallJSInject.txt"; + break; + default: + fileName = "testQueryResult.txt"; + break; } - File sampleQueryResultFile = new File(url.getPath()); - inputStream = new FileInputStream(sampleQueryResultFile); - streamReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); - sampleQueryResultResponseStrBuilder = new StringBuilder(); - - while ((inputStr = streamReader.readLine()) != null) { - sampleQueryResultResponseStrBuilder.append(inputStr); - } + InputStream inputStream = getClass().getResourceAsStream("/" + fileName); + assert inputStream != null; + BufferedReader streamReader = + new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); + fileContent = streamReader.lines().collect(Collectors.joining(System.lineSeparator())); // close stream reader streamReader.close(); @@ -86,6 +90,6 @@ public String load(String queryString) throws IOException { ex.printStackTrace(); } - return sampleQueryResultResponseStrBuilder.toString(); + return fileContent; } } diff --git a/geode-pulse/geode-pulse-test/src/main/java/org/apache/geode/tools/pulse/tests/PulseTestData.java b/geode-pulse/geode-pulse-test/src/main/java/org/apache/geode/tools/pulse/tests/PulseTestData.java index 219c63f0b43d..edbc0e1b5eb1 100644 --- a/geode-pulse/geode-pulse-test/src/main/java/org/apache/geode/tools/pulse/tests/PulseTestData.java +++ b/geode-pulse/geode-pulse-test/src/main/java/org/apache/geode/tools/pulse/tests/PulseTestData.java @@ -94,6 +94,9 @@ public static class DataBrowser { public static final String partialRgnName = "R"; public static final String chkRgnClassName = "bttn chk checkbox_true_full"; public static final String notChkRgnClassName = "bttn chk checkbox_false_full"; + public static final String resultClusterHeadingsXPath = "//div[@id='clusterDetails']/div/div"; + public static final String resultClusterCellXPath = + "//tr/td[contains(@title, '"],"ID":["int",5],"active":["boolean",false],"pk":["java.lang.String","5"],"collectionHolderMap":["java.util.HashMap",{"3":["org.apache.geode.cache.query.data.CollectionHolder",{"arr":["java.lang.String[]",["0","1","2","3","4","SUN","IBM","YHOO","GOOG","MSFT"]]}],"2":["org.apache.geode.cache.query.data.CollectionHolder",{"arr":["java.lang.String[]",["0","1","2","3","4","SUN","IBM","YHOO","GOOG","MSFT"]]}],"1":["org.apache.geode.cache.query.data.CollectionHolder",{"arr":["java.lang.String[]",["0","1","2","3","4","SUN","IBM","YHOO","GOOG","MSFT"]]}],"0":["org.apache.geode.cache.query.data.CollectionHolder",{"arr":["java.lang.String[]",["0","1","2","3","4","SUN","IBM","YHOO","GOOG","MSFT"]]}]}],"createTime":["long",0],"positions":["",{"APPL":["org.apache.geode.cache.query.data.Position",{"id":["int",19],"secId":["java.lang.String",""],"mktValue":["double",20],"sharesOutstanding":["double",19000],"col":["java.util.HashSet",[["java.lang.String","1"],["java.lang.String","0"]]]}],"ORCL":["org.apache.geode.cache.query.data.Position",{"id":["int",20],"secId":["java.lang.String","ORCL"],"mktValue":["double",21],"sharesOutstanding":["double",20000],"col":["java.util.HashSet",[["java.lang.String","1"],["java.lang.String","0"]]]}]}],"p1":["org.apache.geode.cache.query.data.Position",{"id":["int",17],"secId":["java.lang.String","MSFT"],"mktValue":["double",18],"sharesOutstanding":["double",17000],"col":["java.util.HashSet",[["java.lang.String","1"],["java.lang.String","0"]]]}],"p2":["org.apache.geode.cache.query.data.Position",{"id":["int",18],"secId":["java.lang.String","AOL"],"mktValue":["double",19],"sharesOutstanding":["double",18000],"col":["java.util.HashSet",[["java.lang.String","1"],["java.lang.String","0"]]]}],"floatMinValue":["float",1.4E-45],"longMinValue":["float",-9.223372E18],"doubleMinValue":["double",4.9E-324]}] + ] +} \ No newline at end of file diff --git a/geode-pulse/src/main/webapp/META-INF/context.xml b/geode-pulse/src/main/webapp/META-INF/context.xml new file mode 100644 index 000000000000..e70eb6cecaa2 --- /dev/null +++ b/geode-pulse/src/main/webapp/META-INF/context.xml @@ -0,0 +1,26 @@ + + + + + + + + + + \ No newline at end of file diff --git a/geode-pulse/src/main/webapp/WEB-INF/web.xml b/geode-pulse/src/main/webapp/WEB-INF/web.xml index 5da192888e93..e16a8dd3892c 100644 --- a/geode-pulse/src/main/webapp/WEB-INF/web.xml +++ b/geode-pulse/src/main/webapp/WEB-INF/web.xml @@ -43,6 +43,14 @@ spring.profiles.default pulse.authentication.default + + + + true + __SAME_SITE_STRICT__ + + + springSecurityFilterChain org.springframework.web.filter.DelegatingFilterProxy diff --git a/geode-pulse/src/main/webapp/scripts/pulsescript/pages/DataBrowserQuery.js b/geode-pulse/src/main/webapp/scripts/pulsescript/pages/DataBrowserQuery.js index ce37dfb53185..e5056bacb88a 100644 --- a/geode-pulse/src/main/webapp/scripts/pulsescript/pages/DataBrowserQuery.js +++ b/geode-pulse/src/main/webapp/scripts/pulsescript/pages/DataBrowserQuery.js @@ -68,10 +68,10 @@ function executeDBQuery(){ } // Determine selected members query to be execute on - if($("#membersList").html() != ""){ + if($("#membersList").html() !== ""){ var selectedMembers = $( "input[type=checkbox][name=Member]:checked" ); for(var i=0; i< selectedMembers.length; i++){ - if(selectedMemberNames == ""){ + if(selectedMemberNames === ""){ selectedMemberNames = selectedMembers[i].value; }else{ selectedMemberNames += ","+selectedMembers[i].value; @@ -803,14 +803,14 @@ function formDataForPopUpGrid(data){ //Function for converting raw response into expected format function convertRawResponseToExpectedFormat(rawResponeData){ - if(rawResponeData == null || rawResponeData == undefined){ + if(rawResponeData === null || rawResponeData === undefined){ return; } var finalResponseData = {}; var finalResponseResults = []; - if(rawResponeData.result != null || rawResponeData.result != undefined){ + if(rawResponeData.result != null || rawResponeData.result !== undefined){ var rawResponeDataResult = rawResponeData.result; for(var i=0; i 0){ // search expected object type in expResponseResult - var flagObjectFound = false; - for(var j=0 ; j < expResponseResult.length ; j++){ - if(expResponseResult[j].objectType == rawResponseResult[i][0]){ + let flagObjectFound = false; + for(let j=0 ; j < expResponseResult.length ; j++){ + if(expResponseResult[j].objectType === rawResponseResult[i][0]){ // required object found flagObjectFound = true; - var objectResults = expResponseResult[j].objectResults; - var type = rawResponseResult[i][0]; - var entry = rawResponseResult[i][1]; - - // if entry is not object then convert it into object - if(typeof(entry) != "object" ){ - var entryObj = {}; - entryObj[type] = rawResponseResult[i][1]; - entry = entryObj; - } + objectResults = expResponseResult[j].objectResults; + entry = htmlEncodeEntry(rawResponseResult[i]); // add unique id for new entry entry.uid = generateEntryUID(prefixForId, expResponseResult[j].objectType, objectResults.length); @@ -875,57 +869,12 @@ function convertToExpectedObjectsFormat(rawResponseResult, prefixForId){ break; } } - - if(!flagObjectFound){ // required object not found in expResponseResult - - var objectResults = []; - var type = rawResponseResult[i][0]; - var entry = rawResponseResult[i][1]; - - // if entry is not object then convert it into object - if(typeof(entry) != "object" ){ - var entryObj = {}; - entryObj[type] = rawResponseResult[i][1]; - entry = entryObj; - } - - // add unique id for new entry - entry.uid = generateEntryUID(prefixForId, type, objectResults.length); - - objectResults.push(entry); - - var newResultObject = {}; - newResultObject.objectType = type; - newResultObject.objectResults = objectResults; - - expResponseResult.push(newResultObject); + if(!flagObjectFound){ // required object not found in expResponseResult + expResponseResult.push(addToExpResponseResult(rawResponseResult[i], prefixForId)); } - }else{ // expResponseResult is empty - - var objectResults = []; - var type = rawResponseResult[i][0]; - var entry = rawResponseResult[i][1]; - - // if entry is not object then convert it into object - if(typeof(entry) != "object" ){ - var entryObj = {}; - entryObj[type] = rawResponseResult[i][1]; - entry = entryObj; - } - - // add unique id for new entry - entry.uid = generateEntryUID(prefixForId, type, objectResults.length); - - objectResults.push(entry); - - var newResultObject = {}; - newResultObject.objectType = type; - newResultObject.objectResults = objectResults; - - expResponseResult.push(newResultObject); + expResponseResult.push(addToExpResponseResult(rawResponseResult[i], prefixForId)); } - } } } @@ -933,6 +882,54 @@ function convertToExpectedObjectsFormat(rawResponseResult, prefixForId){ return expResponseResult; } +// Add results to the expected responseResults +function addToExpResponseResult(rawResponseResultEntry, prefixForId) { + let objectResults = []; + let type = rawResponseResultEntry[0]; + let entry = htmlEncodeEntry(rawResponseResultEntry, prefixForId); + + // add unique id for new entry + entry.uid = generateEntryUID(prefixForId, type, objectResults.length); + + objectResults.push(entry); + + let newResultObject = {}; + newResultObject.objectType = type; + newResultObject.objectResults = objectResults; + + return newResultObject; +} + +// Ensure that strings are HTML encoded to reduce likelihood of XSS attacks +function htmlEncodeEntry(rawResponseResultEntry, prefixForId) { + let type = htmlEncodeStringsAndObjects(rawResponseResultEntry[0]); + let entry = rawResponseResultEntry[1]; + + let entryObj = {}; + + // if entry is not object then convert it into object + if(typeof(entry) == "object" ){ + entryObj = htmlEncodeStringsAndObjects(entry); + } else { + entryObj[type] = htmlEncodeStringsAndObjects(entry) + } + + return entryObj; +} + +function htmlEncodeStringsAndObjects(raw) { + switch(typeof(raw)) { + case "string": + return $('
').text(raw).html();
+    case "object":
+      let objectAsString = JSON.stringify(raw);
+      objectAsString = $('
').text(objectAsString).html();
+      return JSON.parse(objectAsString);
+    default:
+      return raw
+  }
+}
+
 // Function to generate unique idetifier for entry
 function generateEntryUID(prefixForId, type, len) {
 
diff --git a/geode-pulse/src/main/webapp/scripts/pulsescript/pages/DataBrowserQueryHistory.js b/geode-pulse/src/main/webapp/scripts/pulsescript/pages/DataBrowserQueryHistory.js
index ef4b9e4a3aea..c41381644e64 100644
--- a/geode-pulse/src/main/webapp/scripts/pulsescript/pages/DataBrowserQueryHistory.js
+++ b/geode-pulse/src/main/webapp/scripts/pulsescript/pages/DataBrowserQueryHistory.js
@@ -21,25 +21,25 @@
 // updateQueryHistory()
 function updateQueryHistory(action,queryId) {
   
-  requestData = {
-    action:action,
-    queryId:queryId
+  let requestData = {
+    action: action,
+    queryId: queryId
   };
 
   $.getJSON("dataBrowserQueryHistory", requestData, function(data) {
-    
-    var queries = [];
-    if(data.queryHistory != undefined && data.queryHistory != null){
+
+    let queries = [];
+    if(data.queryHistory !== undefined && data.queryHistory != null){
       queries = data.queryHistory;
     }
-    var refHistoryConatiner = $("#detailsHistoryList");
-    var queryListHTML = "";
-    if(queries.length == 0){
+    const refHistoryContainer = $("#detailsHistoryList");
+    let queryListHTML = "";
+    if(queries.length === 0){
       // no queries found
       queryListHTML = "No Query Found";
     }else{
       queries.sort(dynamicSort("queryId", "desc"));
-      for(var i=0; i" +
@@ -50,7 +50,7 @@ function updateQueryHistory(action,queryId) {
               "
" +
                 " " +
               "
" +
-              "
" + queries[i].queryText +
+              "
" + queries[i].queryText.replaceAll("\"", "") +
               "
" +
               "
" + queries[i].queryDateTime +
               "
" +
@@ -59,7 +59,7 @@ function updateQueryHistory(action,queryId) {
       }
     }
     
-    refHistoryConatiner.html(queryListHTML);
+    refHistoryContainer.html(queryListHTML);
     //$('.queryHistoryScroll-pane').jScrollPane();/*Custome scroll*/    
 
     // Set eventsAdded = false as list is refreshed and slide events 
@@ -73,7 +73,7 @@ function updateQueryHistory(action,queryId) {
 // This function displays error if occurred 
 function resErrHandler(data){
   // Check for unauthorized access
-  if (data.status == 401) {
+  if (data.status === 401) {
     // redirect user on Login Page
     window.location.href = "login.html?error=UNAUTH_ACCESS";
   }else{
diff --git a/geode-pulse/src/uiTest/java/org/apache/geode/tools/pulse/tests/ui/PulseAutomatedTest.java b/geode-pulse/src/uiTest/java/org/apache/geode/tools/pulse/tests/ui/PulseAutomatedTest.java
index 5af912cfb058..3fc3f25f8f83 100644
--- a/geode-pulse/src/uiTest/java/org/apache/geode/tools/pulse/tests/ui/PulseAutomatedTest.java
+++ b/geode-pulse/src/uiTest/java/org/apache/geode/tools/pulse/tests/ui/PulseAutomatedTest.java
@@ -13,12 +13,6 @@
  * the License.
  *
  */
-/**
- * This test class contains automated tests for Pulse application related to 1. Different grid data
- * validations for example - Topology, Server Group, Redundancy Zone 2. Data Browser 3.
- *
- * @since GemFire 2014-04-02
- */
 package org.apache.geode.tools.pulse.tests.ui;
 
 import static org.apache.geode.tools.pulse.tests.ui.PulseTestUtils.assertMemberSortingByCpuUsage;
@@ -59,13 +53,16 @@
 import static org.apache.geode.tools.pulse.tests.ui.PulseTestUtils.verifyTextPresrntById;
 import static org.apache.geode.tools.pulse.tests.ui.PulseTestUtils.verifyTextPresrntByXpath;
 import static org.apache.geode.tools.pulse.tests.ui.PulseTestUtils.waitForElementWithId;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 
 import java.text.DateFormat;
 import java.text.SimpleDateFormat;
+import java.util.Collection;
 import java.util.List;
+import java.util.stream.Collectors;
 
 import org.junit.Assert;
 import org.junit.Before;
@@ -84,6 +81,12 @@
 import org.apache.geode.tools.pulse.tests.rules.ServerRule;
 import org.apache.geode.tools.pulse.tests.rules.WebDriverRule;
 
+/**
+ * This test class contains automated tests for Pulse application related to 1. Different grid data
+ * validations for example - Topology, Server Group, Redundancy Zone 2. Data Browser 3.
+ *
+ * @since GemFire 2014-04-02
+ */
 public class PulseAutomatedTest extends PulseBase {
 
   @ClassRule
@@ -851,7 +854,7 @@ public void testDataBrowserClearButton() {
     clickElementUsingXpath(PulseTestLocators.DataBrowser.btnClearXpath);
     String editorTextAfterClear = getTextUsingId(PulseTestLocators.DataBrowser.queryEditorTxtBoxId);
 
-    assertFalse(PulseTestData.DataBrowser.query1Text.equals(editorTextAfterClear));
+    assertNotEquals(PulseTestData.DataBrowser.query1Text, editorTextAfterClear);
   }
 
   @Ignore("WIP") // Data Browser's Query History not showing any data on button click, therefore
@@ -892,10 +895,50 @@ public void testDataBrowserHistoryQueue() {
     System.out.println("Query Text from History Table: " + queryText);
     System.out.println("Query Time from History Table: " + historyDateTime);
     // verify the query text, query datetime in history panel
-    assertTrue(DataBrowserResultLoader.QUERY_TYPE_ONE.equals(queryText));
-    assertTrue(historyDateTime.contains(queryTime[0]));
-
+    assertThat(queryText).isEqualTo(DataBrowserResultLoader.QUERY_TYPE_ONE);
+    assertThat(historyDateTime).contains(queryTime[0]);
   }
 
+  @Test
+  public void testDataBrowserHTMLEncode() {
+    // navigate to Data browser page
+    loadDataBrowserpage();
+
+    WebDriver driver = webDriverRule.getDriver();
+    List numOfReg = driver
+        .findElements(By.xpath(PulseTestLocators.DataBrowser.divDataRegions));
 
+    for (int i = 1; i <= numOfReg.size(); i++) {
+      if (getTextUsingId("treeDemo_" + i + "_span").equals(PulseTestData.DataBrowser.regName)) {
+        searchByIdAndClick("treeDemo_" + i + "_check"); // driver.findElement(By.id("treeDemo_" + i
+        // + "_check")).click();
+      }
+    }
+
+    sendKeysUsingId(PulseTestLocators.DataBrowser.queryEditorTxtBoxId,
+        DataBrowserResultLoader.QUERY_TYPE_EIGHT);
+    clickElementUsingId(PulseTestLocators.DataBrowser.btnExecuteQueryId);
+
+    clickElementUsingId(PulseTestLocators.DataBrowser.historyIcon);
+    String queryText = findElementByXpath(PulseTestLocators.DataBrowser.historyLst)
+        .findElement(By.cssSelector(PulseTestLocators.DataBrowser.queryText)).getText();
+
+    assertThat(queryText).isEqualTo(DataBrowserResultLoader.QUERY_TYPE_EIGHT);
+
+    List elements =
+        driver.findElements(By.xpath(PulseTestData.DataBrowser.resultClusterHeadingsXPath));
+    List filteredElements = elements.stream().filter(webElement -> webElement.getText()
+        .equals("org.apache.geode.cache.query.data.Portfolio")).collect(
+            Collectors.toList());
+    List finalElements = filteredElements.stream().map(webElement -> {
+      webElement.click();
+      return webElement.findElements(By.xpath(PulseTestData.DataBrowser.resultClusterCellXPath));
+    }).flatMap(Collection::stream).collect(Collectors.toList());
+
+    // confirm script text is displayed
+    assertThat(finalElements).hasSize(2);
+    finalElements.forEach(webElement -> {
+      assertThat(webElement.getAttribute("title")).isEqualTo("");
+    });
+  }
 }