From: Dominik Mizyn Date: Mon, 15 Jul 2019 14:03:16 +0000 (+0200) Subject: XSS Vulnerability fix in DashboardController X-Git-Tag: 3.2.0~237^2 X-Git-Url: https://gerrit.onap.org/r/gitweb?p=portal.git;a=commitdiff_plain;h=fbbbba21e22b18803dda7b9af1aee00b8aa08696 XSS Vulnerability fix in DashboardController Custom data validator used to fix this issue. Issue-ID: OJSI-15 Change-Id: I84bfb81e5d87f80211d46d1141cbf8e4075660fe Signed-off-by: Dominik Mizyn --- diff --git a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/controller/DashboardController.java b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/controller/DashboardController.java index 727d190d..6137aec9 100644 --- a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/controller/DashboardController.java +++ b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/controller/DashboardController.java @@ -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 getWidgetData(HttpServletRequest request, @RequestParam String resourceType) { - if (!isValidResourceType(resourceType)) - return new PortalRestResponse(PortalRestStatusEnum.ERROR, - "Unexpected resource type " + resourceType, null); - return new PortalRestResponse(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 saveWidgetDataBulk(@RequestBody CommonWidgetMeta commonWidgetMeta) { logger.debug(EELFLoggerDelegate.debugLogger, "saveWidgetDataBulk: argument is {}", commonWidgetMeta); - if (commonWidgetMeta.getCategory() == null || commonWidgetMeta.getCategory().trim().equals("")) - return new PortalRestResponse(PortalRestStatusEnum.ERROR, "ERROR", - "Category cannot be null or empty"); - if (!isValidResourceType(commonWidgetMeta.getCategory())) - return new PortalRestResponse(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(PortalRestStatusEnum.ERROR, err, null); + return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, err, null); } - return new PortalRestResponse(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(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(PortalRestStatusEnum.ERROR, err, null); - return new PortalRestResponse(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(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 deleteWidgetData(@RequestBody CommonWidget commonWidget) { logger.debug(EELFLoggerDelegate.debugLogger, "deleteWidgetData: argument is {}", commonWidget); - return new PortalRestResponse(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>> 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>()); + new HashMap<>()); } else if (searchString == null || searchString.length() == 0) { return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, "searchPortal: String string is null", - new HashMap>()); + 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>()); + new HashMap<>()); } } @@ -308,7 +332,7 @@ public class DashboardController extends EPRestrictedBaseController { */ @RequestMapping(value = "/activeUsers", method = RequestMethod.GET, produces = "application/json") public List getActiveUsers(HttpServletRequest request) { - List activeUsers = null; + List activeUsers; List 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 results = new HashMap(); + Map 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 results = new HashMap(); + Map 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 results = new HashMap(); + Map results = new HashMap<>(); results.put("windowWidth", String.valueOf(windowWidth)); return new PortalRestResponse<>(PortalRestStatusEnum.OK, "success", results); } catch (Exception e) { diff --git a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidget.java b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidget.java index 90277877..e9d720e3 100644 --- a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidget.java +++ b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidget.java @@ -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; diff --git a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidgetMeta.java b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidgetMeta.java index 51a02652..0a999495 100644 --- a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidgetMeta.java +++ b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/transport/CommonWidgetMeta.java @@ -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 items; - - public CommonWidgetMeta(){ - - } - - public CommonWidgetMeta(String category, List items){ - this.category = category; - this.items = items; - } - - public String getCategory() { - return category; - } - public void setCategory(String category) { - this.category = category; - } - public List getItems() { - return items; - } - public void setItems(List items) { - this.items = items; - } } diff --git a/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/controller/DashboardControllerTest.java b/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/controller/DashboardControllerTest.java index 417568da..cd130e9f 100644 --- a/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/controller/DashboardControllerTest.java +++ b/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/controller/DashboardControllerTest.java @@ -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 actualResponse = dashboardController.getWidgetData(mockedRequest, resourceType); assertEquals(expectedData,actualResponse); - } - + } + + @Test + public void getWidgetDataTestXSS() { + + String resourceType = "“>"; + PortalRestResponse expectedData = new PortalRestResponse<>(); + expectedData.setStatus(PortalRestStatusEnum.ERROR); + expectedData.setMessage("Unexpected resource type “>"); + expectedData.setResponse(null); + + PortalRestResponse 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 actualResponse = dashboardController.saveWidgetDataBulk(commonWidgetMeta); assertEquals(expectedData,actualResponse); } + + @Test + public void saveWidgetDataBulkXSSTest() { + CommonWidgetMeta commonWidgetMeta= mockCommonWidgetMeta(); + commonWidgetMeta.setCategory(""); + + PortalRestResponse expectedData = new PortalRestResponse<>(); + expectedData.setStatus(PortalRestStatusEnum.ERROR); + expectedData.setResponse("ERROR"); + expectedData.setMessage("Unsafe resource type " + commonWidgetMeta.toString()); + + PortalRestResponse 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("
X"); + PortalRestResponse expectedData = new PortalRestResponse(); + expectedData.setStatus(PortalRestStatusEnum.ERROR); + expectedData.setResponse("ERROR"); + expectedData.setMessage("Unsafe resource type " + commonWidget.toString()); + + Mockito.when(adminRolesService.isSuperAdmin(Matchers.anyObject())).thenReturn(true); + PortalRestResponse 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 expectedData = new PortalRestResponse(); 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 expectedData = new PortalRestResponse(); 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 expectedData = new PortalRestResponse(); 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("