XSS Vulnerability fix in DashboardController 53/91453/1
authorDominik Mizyn <d.mizyn@samsung.com>
Mon, 15 Jul 2019 14:03:16 +0000 (16:03 +0200)
committerDominik Mizyn <d.mizyn@samsung.com>
Mon, 15 Jul 2019 14:04:45 +0000 (16:04 +0200)
Custom data validator used to fix this issue.

Issue-ID: OJSI-15
Change-Id: I84bfb81e5d87f80211d46d1141cbf8e4075660fe
Signed-off-by: Dominik Mizyn <d.mizyn@samsung.com>
ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/controller/DashboardController.java
ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidget.java
ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidgetMeta.java
ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/controller/DashboardControllerTest.java

index 727d190..6137aec 100644 (file)
@@ -66,6 +66,8 @@ import org.onap.portalapp.portal.utils.EPCommonSystemProperties;
 import org.onap.portalapp.portal.utils.EcompPortalUtils;
 import org.onap.portalapp.portal.utils.PortalConstants;
 import org.onap.portalapp.util.EPUserUtils;
+import org.onap.portalapp.validation.DataValidator;
+import org.onap.portalapp.validation.SecureString;
 import org.onap.portalsdk.core.domain.AuditLog;
 import org.onap.portalsdk.core.domain.support.CollaborateList;
 import org.onap.portalsdk.core.logging.logic.EELFLoggerDelegate;
@@ -87,19 +89,23 @@ import org.springframework.web.bind.annotation.RestController;
 @RestController
 @RequestMapping("/portalApi/dashboard")
 public class DashboardController extends EPRestrictedBaseController {
+       private static final DataValidator DATA_VALIDATOR = new DataValidator();
+       private static final EELFLoggerDelegate logger = EELFLoggerDelegate.getLogger(DashboardController.class);
 
-       private static EELFLoggerDelegate logger = EELFLoggerDelegate.getLogger(DashboardController.class);
-
-       @Autowired
        private DashboardSearchService searchService;
-       @Autowired
        private AuditService auditService;
-       
-       @Autowired
        private AdminRolesService adminRolesService;
-       
+
+       @Autowired
+       public DashboardController(DashboardSearchService searchService,
+               AuditService auditService, AdminRolesService adminRolesService) {
+               this.searchService = searchService;
+               this.auditService = auditService;
+               this.adminRolesService = adminRolesService;
+       }
+
        public enum WidgetCategory {
-               EVENTS, NEWS, IMPORTANTRESOURCES;
+               EVENTS, NEWS, IMPORTANTRESOURCES
        }
 
        /**
@@ -129,11 +135,15 @@ public class DashboardController extends EPRestrictedBaseController {
        @RequestMapping(value = "/widgetData", method = RequestMethod.GET, produces = "application/json")
        public PortalRestResponse<CommonWidgetMeta> getWidgetData(HttpServletRequest request,
                        @RequestParam String resourceType) {
-               if (!isValidResourceType(resourceType))
-                       return new PortalRestResponse<CommonWidgetMeta>(PortalRestStatusEnum.ERROR,
-                                       "Unexpected resource type " + resourceType, null);
-               return new PortalRestResponse<CommonWidgetMeta>(PortalRestStatusEnum.OK, "success",
-                               searchService.getWidgetData(resourceType));
+               if (!isValidResourceType(resourceType)) {
+                       return new PortalRestResponse<>(PortalRestStatusEnum.ERROR,
+                               "Unexpected resource type " + resourceType, null);
+               }else if (!DATA_VALIDATOR.isValid(new SecureString(resourceType))){
+                       return new PortalRestResponse<>(PortalRestStatusEnum.ERROR,
+                               "Unsafe resource type " + resourceType, null);
+               }
+               return new PortalRestResponse<>(PortalRestStatusEnum.OK, "success",
+                       searchService.getWidgetData(resourceType));
        }
        
        
@@ -147,20 +157,23 @@ public class DashboardController extends EPRestrictedBaseController {
        @RequestMapping(value = "/widgetDataBulk", method = RequestMethod.POST, produces = "application/json")
        public PortalRestResponse<String> saveWidgetDataBulk(@RequestBody CommonWidgetMeta commonWidgetMeta) {
                logger.debug(EELFLoggerDelegate.debugLogger, "saveWidgetDataBulk: argument is {}", commonWidgetMeta);
-               if (commonWidgetMeta.getCategory() == null || commonWidgetMeta.getCategory().trim().equals(""))
-                       return new PortalRestResponse<String>(PortalRestStatusEnum.ERROR, "ERROR",
-                                       "Category cannot be null or empty");
-               if (!isValidResourceType(commonWidgetMeta.getCategory()))
-                       return new PortalRestResponse<String>(PortalRestStatusEnum.ERROR,
-                                       "Unexpected resource type " + commonWidgetMeta.getCategory(), null);
-               // validate dates
+               if (!DATA_VALIDATOR.isValid(commonWidgetMeta)){
+                       return new PortalRestResponse<>(PortalRestStatusEnum.ERROR,
+                               "Unsafe resource type " + commonWidgetMeta, "ERROR");
+               }else if (commonWidgetMeta.getCategory() == null || commonWidgetMeta.getCategory().trim().equals("")) {
+                       return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, "ERROR",
+                               "Category cannot be null or empty");
+               }else if (!isValidResourceType(commonWidgetMeta.getCategory())) {
+                       return new PortalRestResponse<>(PortalRestStatusEnum.ERROR,
+                               "Unexpected resource type " + commonWidgetMeta.getCategory(), null);
+               }
                for (CommonWidget cw : commonWidgetMeta.getItems()) {
                        String err = validateCommonWidget(cw);
                        if (err != null)
-                               return new PortalRestResponse<String>(PortalRestStatusEnum.ERROR, err, null);
+                               return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, err, null);
                }
-               return new PortalRestResponse<String>(PortalRestStatusEnum.OK, "success",
-                               searchService.saveWidgetDataBulk(commonWidgetMeta));
+               return new PortalRestResponse<>(PortalRestStatusEnum.OK, "success",
+                       searchService.saveWidgetDataBulk(commonWidgetMeta));
        }
 
        /**
@@ -175,17 +188,21 @@ public class DashboardController extends EPRestrictedBaseController {
                logger.debug(EELFLoggerDelegate.debugLogger, "saveWidgetData: argument is {}", commonWidget);
                EPUser user = EPUserUtils.getUserSession(request);
                if (adminRolesService.isSuperAdmin(user)) {
-                       if (commonWidget.getCategory() == null || commonWidget.getCategory().trim().isEmpty())
-                               return new PortalRestResponse<String>(PortalRestStatusEnum.ERROR, "ERROR",
-                                               "Category cannot be null or empty");
+                       if (commonWidget.getCategory() == null || commonWidget.getCategory().trim().isEmpty()) {
+                               return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, "ERROR",
+                                       "Category cannot be null or empty");
+                       }else if (!DATA_VALIDATOR.isValid(commonWidget)){
+                               return new PortalRestResponse<>(PortalRestStatusEnum.ERROR,
+                                       "Unsafe resource type " + commonWidget, "ERROR");
+                       }
                        String err = validateCommonWidget(commonWidget);
                        if (err != null)
-                               return new PortalRestResponse<String>(PortalRestStatusEnum.ERROR, err, null);
-                       return new PortalRestResponse<String>(PortalRestStatusEnum.OK, "success",
-                                       searchService.saveWidgetData(commonWidget));
+                               return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, err, null);
+                       return new PortalRestResponse<>(PortalRestStatusEnum.OK, "success",
+                               searchService.saveWidgetData(commonWidget));
                } else {
                        EcompPortalUtils.setBadPermissions(user, response, "saveWidgetData");
-                       return new PortalRestResponse<String>(PortalRestStatusEnum.ERROR, "Failed", null);
+                       return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, "Failed", null);
                }
        }
 
@@ -235,8 +252,12 @@ public class DashboardController extends EPRestrictedBaseController {
        @RequestMapping(value = "/deleteData", method = RequestMethod.POST, produces = "application/json")
        public PortalRestResponse<String> deleteWidgetData(@RequestBody CommonWidget commonWidget) {
                logger.debug(EELFLoggerDelegate.debugLogger, "deleteWidgetData: argument is {}", commonWidget);
-               return new PortalRestResponse<String>(PortalRestStatusEnum.OK, "success",
-                               searchService.deleteWidgetData(commonWidget));
+               if (!DATA_VALIDATOR.isValid(commonWidget)){
+                       return new PortalRestResponse<>(PortalRestStatusEnum.ERROR,
+                               "Unsafe resource type " + commonWidget, "ERROR");
+               }
+               return new PortalRestResponse<>(PortalRestStatusEnum.OK, "success",
+                       searchService.deleteWidgetData(commonWidget));
        }
 
        /**
@@ -251,7 +272,10 @@ public class DashboardController extends EPRestrictedBaseController {
        @RequestMapping(value = "/search", method = RequestMethod.GET, produces = "application/json")
        public PortalRestResponse<Map<String, List<SearchResultItem>>> searchPortal(HttpServletRequest request,
                        @RequestParam String searchString) {
-
+               if (!DATA_VALIDATOR.isValid(new SecureString(searchString))){
+                       return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, "searchPortal: String string is not safe",
+                               new HashMap<>());
+               }
                if (searchString != null)
                        searchString = searchString.trim();
                EPUser user = EPUserUtils.getUserSession(request);
@@ -259,10 +283,10 @@ public class DashboardController extends EPRestrictedBaseController {
                        if (user == null) {
                                return new PortalRestResponse<>(PortalRestStatusEnum.ERROR,
                                                "searchPortal: User object is null? - check logs",
-                                               new HashMap<String, List<SearchResultItem>>());
+                                       new HashMap<>());
                        } else if (searchString == null || searchString.length() == 0) {
                                return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, "searchPortal: String string is null",
-                                               new HashMap<String, List<SearchResultItem>>());
+                                       new HashMap<>());
                        } else {
                                logger.debug(EELFLoggerDelegate.debugLogger, "searchPortal: user {}, search string '{}'",
                                                user.getLoginId(), searchString);
@@ -294,7 +318,7 @@ public class DashboardController extends EPRestrictedBaseController {
                        MDC.put(EPCommonSystemProperties.STATUS_CODE, "ERROR");
                        MDC.remove(EPCommonSystemProperties.STATUS_CODE);
                        return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, e.getMessage() + " - check logs.",
-                                       new HashMap<String, List<SearchResultItem>>());
+                               new HashMap<>());
                }
        }
 
@@ -308,7 +332,7 @@ public class DashboardController extends EPRestrictedBaseController {
         */
        @RequestMapping(value = "/activeUsers", method = RequestMethod.GET, produces = "application/json")
        public List<String> getActiveUsers(HttpServletRequest request) {
-               List<String> activeUsers = null;
+               List<String> activeUsers;
                List<String> onlineUsers = new ArrayList<>();
                try {
                        EPUser user = EPUserUtils.getUserSession(request);
@@ -341,7 +365,7 @@ public class DashboardController extends EPRestrictedBaseController {
                        String updateDuration = SystemProperties.getProperty(EPCommonSystemProperties.ONLINE_USER_UPDATE_DURATION);                             
                        Integer rateInMiliSec = Integer.valueOf(updateRate)*1000;
                        Integer durationInMiliSec = Integer.valueOf(updateDuration)*1000;
-                       Map<String, String> results = new HashMap<String,String>();
+                       Map<String, String> results = new HashMap<>();
                        results.put("onlineUserUpdateRate", String.valueOf(rateInMiliSec));
                        results.put("onlineUserUpdateDuration", String.valueOf(durationInMiliSec));                     
                        return new PortalRestResponse<>(PortalRestStatusEnum.OK, "success", results);
@@ -362,7 +386,7 @@ public class DashboardController extends EPRestrictedBaseController {
                try {
                        String windowWidthString = SystemProperties.getProperty(EPCommonSystemProperties.WINDOW_WIDTH_THRESHOLD_RIGHT_MENU);    
                        Integer windowWidth = Integer.valueOf(windowWidthString);
-                       Map<String, String> results = new HashMap<String,String>();
+                       Map<String, String> results = new HashMap<>();
                        results.put("windowWidth", String.valueOf(windowWidth));
                        return new PortalRestResponse<>(PortalRestStatusEnum.OK, "success", results);
                } catch (Exception e) {
@@ -383,7 +407,7 @@ public class DashboardController extends EPRestrictedBaseController {
                try {
                        String windowWidthString = SystemProperties.getProperty(EPCommonSystemProperties.WINDOW_WIDTH_THRESHOLD_LEFT_MENU);     
                        Integer windowWidth = Integer.valueOf(windowWidthString);
-                       Map<String, String> results = new HashMap<String,String>();
+                       Map<String, String> results = new HashMap<>();
                        results.put("windowWidth", String.valueOf(windowWidth));
                        return new PortalRestResponse<>(PortalRestStatusEnum.OK, "success", results);
                } catch (Exception e) {
index 9027787..e9d720e 100644 (file)
@@ -49,6 +49,7 @@ import javax.validation.constraints.Size;
 import lombok.Getter;
 import lombok.NoArgsConstructor;
 import lombok.Setter;
+import lombok.ToString;
 import org.hibernate.validator.constraints.SafeHtml;
 import org.onap.portalsdk.core.domain.support.DomainVo;
 import com.fasterxml.jackson.annotation.JsonInclude;
@@ -62,6 +63,7 @@ import com.fasterxml.jackson.annotation.JsonInclude;
 @NoArgsConstructor
 @Getter
 @Setter
+@ToString
 public class CommonWidget extends DomainVo{
 
        private static final long serialVersionUID = 7897021982887364557L;
index 51a0265..0a99949 100644 (file)
@@ -39,33 +39,21 @@ package org.onap.portalapp.portal.transport;
 
 import java.util.List;
 import javax.validation.Valid;
+import lombok.AllArgsConstructor;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+import lombok.Setter;
+import lombok.ToString;
 import org.hibernate.validator.constraints.SafeHtml;
 
+@NoArgsConstructor
+@AllArgsConstructor
+@Getter
+@Setter
+@ToString
 public class CommonWidgetMeta {
        @SafeHtml
        private String category;
        @Valid
        private List<CommonWidget> items;
-
-       public CommonWidgetMeta(){
-
-       }
-
-       public CommonWidgetMeta(String category, List<CommonWidget> items){
-               this.category = category;
-               this.items = items;
-       }
-       
-       public String getCategory() {
-               return category;
-       }
-       public void setCategory(String category) {
-               this.category = category;
-       }
-       public List<CommonWidget> getItems() {
-               return items;
-       }
-       public void setItems(List<CommonWidget> items) {
-               this.items = items;
-       }
 }
index 417568d..cd130e9 100644 (file)
@@ -57,10 +57,8 @@ import org.mockito.Matchers;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
-import org.onap.portalapp.portal.controller.DashboardController;
 import org.onap.portalapp.portal.core.MockEPUser;
 import org.onap.portalapp.portal.domain.EPUser;
-import org.onap.portalapp.portal.domain.EcompAuditLog;
 import org.onap.portalapp.portal.ecomp.model.PortalRestResponse;
 import org.onap.portalapp.portal.ecomp.model.PortalRestStatusEnum;
 import org.onap.portalapp.portal.ecomp.model.SearchResultItem;
@@ -72,13 +70,10 @@ import org.onap.portalapp.portal.service.DashboardSearchServiceImpl;
 import org.onap.portalapp.portal.transport.CommonWidget;
 import org.onap.portalapp.portal.transport.CommonWidgetMeta;
 import org.onap.portalapp.portal.utils.EPCommonSystemProperties;
-import org.onap.portalapp.portal.utils.EcompPortalUtils;
-import org.onap.portalapp.portal.utils.PortalConstants;
 import org.onap.portalapp.util.EPUserUtils;
 import org.onap.portalsdk.core.domain.AuditLog;
 import org.onap.portalsdk.core.domain.support.CollaborateList;
 import org.onap.portalsdk.core.service.AuditService;
-import org.onap.portalsdk.core.service.AuditServiceImpl;
 import org.onap.portalsdk.core.util.SystemProperties;
 import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
@@ -92,12 +87,9 @@ public class DashboardControllerTest {
        
        @Mock
        DashboardSearchService searchService = new DashboardSearchServiceImpl();
-       
-       /*@Mock
-       AuditService auditService = new AuditServiceImpl();*/
-       
+
        @InjectMocks
-       DashboardController dashboardController = new DashboardController();
+       DashboardController dashboardController;
 
        @Mock
        AdminRolesService adminRolesService = new AdminRolesServiceImpl();
@@ -129,7 +121,7 @@ public class DashboardControllerTest {
                commonWidget.setHref("testhref");
                commonWidget.setTitle("testTitle");
            commonWidget.setContent("testcontent");
-           commonWidget.setEventDate("testDate");
+           commonWidget.setEventDate("2017-03-24");
            commonWidget.setSortOrder(1);                   
                widgetList.add(commonWidget);           
                commonWidgetMeta.setItems(widgetList);
@@ -163,8 +155,21 @@ public class DashboardControllerTest {
                
                PortalRestResponse<CommonWidgetMeta> actualResponse =   dashboardController.getWidgetData(mockedRequest, resourceType);
                assertEquals(expectedData,actualResponse);              
-       }       
-       
+       }
+
+       @Test
+       public void getWidgetDataTestXSS() {
+
+               String resourceType = "“><script>alert(“XSS”)</script>";
+               PortalRestResponse<CommonWidgetMeta> expectedData = new PortalRestResponse<>();
+               expectedData.setStatus(PortalRestStatusEnum.ERROR);
+               expectedData.setMessage("Unexpected resource type “><script>alert(“XSS”)</script>");
+               expectedData.setResponse(null);
+
+               PortalRestResponse<CommonWidgetMeta> actualResponse = dashboardController.getWidgetData(mockedRequest, resourceType);
+               assertEquals(expectedData, actualResponse);
+       }
+
        @Test
        public void getWidgetDataWithValidResourceTest() throws IOException {
                String resourceType = "EVENTS";
@@ -194,6 +199,20 @@ public class DashboardControllerTest {
                PortalRestResponse<String> actualResponse = dashboardController.saveWidgetDataBulk(commonWidgetMeta);
                assertEquals(expectedData,actualResponse);              
        }
+
+       @Test
+       public void saveWidgetDataBulkXSSTest() {
+               CommonWidgetMeta commonWidgetMeta= mockCommonWidgetMeta();
+               commonWidgetMeta.setCategory("<script>alert(‘XSS’)</script>");
+
+               PortalRestResponse<String> expectedData = new PortalRestResponse<>();
+               expectedData.setStatus(PortalRestStatusEnum.ERROR);
+               expectedData.setResponse("ERROR");
+               expectedData.setMessage("Unsafe resource type " + commonWidgetMeta.toString());
+
+               PortalRestResponse<String> actualResponse = dashboardController.saveWidgetDataBulk(commonWidgetMeta);
+               assertEquals(expectedData,actualResponse);
+       }
        
        @Test
        public void saveWidgetUnexpectedDataBulkTest() throws IOException {
@@ -261,6 +280,24 @@ public class DashboardControllerTest {
                assertEquals(expectedData,actualResponse);
                
        }
+
+       @Test
+       public void saveWidgetDataXSSTest() {
+
+               CommonWidget commonWidget = mockCommonWidget();
+               commonWidget.setId((long)1);
+               commonWidget.setContent("test");
+               commonWidget.setCategory("<form><a href=\"javascript:\\u0061lert&#x28;1&#x29;\">X");
+               PortalRestResponse<String> expectedData = new PortalRestResponse<String>();
+               expectedData.setStatus(PortalRestStatusEnum.ERROR);
+               expectedData.setResponse("ERROR");
+               expectedData.setMessage("Unsafe resource type " + commonWidget.toString());
+
+               Mockito.when(adminRolesService.isSuperAdmin(Matchers.anyObject())).thenReturn(true);
+               PortalRestResponse<String> actualResponse = dashboardController.saveWidgetData(commonWidget, mockedRequest, mockedResponse);
+               assertEquals(expectedData,actualResponse);
+
+       }
        
        @Test
        public void saveWidgetDataTitleTest() throws IOException {                              
@@ -268,6 +305,7 @@ public class DashboardControllerTest {
                commonWidget.setId((long)1);
                commonWidget.setContent("test");
                commonWidget.setTitle("test");
+               commonWidget.setEventDate("2017-05-06");
                PortalRestResponse<String> expectedData = new PortalRestResponse<String>();
                expectedData.setStatus(PortalRestStatusEnum.ERROR);
                expectedData.setMessage("Invalid category: test");
@@ -280,7 +318,8 @@ public class DashboardControllerTest {
        @Test
        public void saveWidgetDataErrorTest() throws IOException {
                                
-               CommonWidget commonWidget = mockCommonWidget();         
+               CommonWidget commonWidget = mockCommonWidget();
+               commonWidget.setEventDate("2017-03-05");
                PortalRestResponse<String> expectedData = new PortalRestResponse<String>();
                expectedData.setStatus(PortalRestStatusEnum.ERROR);
                expectedData.setMessage("Invalid category: test");
@@ -323,7 +362,7 @@ public class DashboardControllerTest {
        public void deleteWidgetDataTest() throws IOException {
                                
                CommonWidget commonWidget = mockCommonWidget();
-               
+               commonWidget.setEventDate("2017-03-25");
                PortalRestResponse<String> expectedData = new PortalRestResponse<String>();
                expectedData.setStatus(PortalRestStatusEnum.OK);
                expectedData.setMessage("success");
@@ -335,6 +374,20 @@ public class DashboardControllerTest {
                assertEquals(expectedData,actualResponse);
                
        }
+
+       @Test
+       public void deleteWidgetDataXSSTest() {
+
+               CommonWidget commonWidget = mockCommonWidget();
+               commonWidget.setCategory("<svg><script x:href='https://dl.dropbox.com/u/13018058/js.js' {Opera}");
+               PortalRestResponse<String> expectedData = new PortalRestResponse<>();
+               expectedData.setStatus(PortalRestStatusEnum.ERROR);
+               expectedData.setMessage("Unsafe resource type " + commonWidget.toString());
+               expectedData.setResponse("ERROR");
+               PortalRestResponse<String> actualResponse = dashboardController.deleteWidgetData(commonWidget);
+               assertEquals(expectedData,actualResponse);
+
+       }
                
        @Test
        public void getActiveUsersTest(){
@@ -541,6 +594,23 @@ public class DashboardControllerTest {
                PortalRestResponse<Map<String, List<SearchResultItem>>> actualResponse = dashboardController.searchPortal(mockedRequest, null);
                assertTrue(actualResponse.getStatus().compareTo(PortalRestStatusEnum.ERROR) == 0);
        }
+
+       @Test
+       public void searchPortalXSSTest(){
+               EPUser user = null;
+               String searchString = "\n"
+                       + "<form><textarea &#13; onkeyup='\\u0061\\u006C\\u0065\\u0072\\u0074&#x28;1&#x29;'>";
+               PowerMockito.mockStatic(EPUserUtils.class);
+               Mockito.when(EPUserUtils.getUserSession(mockedRequest)).thenReturn(user);
+               PortalRestResponse<Map<String, List<SearchResultItem>>> expectedResult = new PortalRestResponse<>();
+               expectedResult.setMessage("searchPortal: String string is not safe");
+               expectedResult.setResponse(new HashMap<>());
+               expectedResult.setStatus(PortalRestStatusEnum.ERROR);
+
+               PortalRestResponse<Map<String, List<SearchResultItem>>> actualResponse = dashboardController.searchPortal(mockedRequest, searchString);
+               assertEquals(expectedResult, actualResponse);
+       }
+
        @Test
        public void searchPortalTestWithException(){
                EPUser user = mockUser.mockEPUser();