From 37f9e0c51405b634fea0d9fadafdb7d55190233d Mon Sep 17 00:00:00 2001 From: Dominik Mizyn Date: Fri, 31 May 2019 15:23:46 +0200 Subject: [PATCH] XSS Vulnerability fix in RoleManageController @SafeHtml and SecureString used to secure this class Issue-ID: OJSI-208 Change-Id: Ie01799933add3419cacf0fc716ce2da6da0a2853 Signed-off-by: Dominik Mizyn --- .../portal/controller/RoleManageController.java | 44 +++++++++++- .../portal/domain/CentralV2RoleFunction.java | 5 ++ .../controller/RoleManageControllerTest.java | 79 ++++++++++++++++++++++ 3 files changed, 127 insertions(+), 1 deletion(-) diff --git a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/controller/RoleManageController.java b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/controller/RoleManageController.java index c8e22d39..3fda5392 100644 --- a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/controller/RoleManageController.java +++ b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/controller/RoleManageController.java @@ -50,6 +50,11 @@ import java.util.TreeSet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import javax.validation.ConstraintViolation; +import javax.validation.Valid; +import javax.validation.Validation; +import javax.validation.Validator; +import javax.validation.ValidatorFactory; import org.apache.commons.lang.StringUtils; import org.json.JSONObject; import org.onap.portalapp.controller.EPRestrictedBaseController; @@ -79,6 +84,7 @@ 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.SecureString; import org.onap.portalsdk.core.domain.AuditLog; import org.onap.portalsdk.core.domain.Role; import org.onap.portalsdk.core.logging.logic.EELFLoggerDelegate; @@ -111,6 +117,8 @@ import com.fasterxml.jackson.databind.type.TypeFactory; @EnableAspectJAutoProxy @EPAuditLog public class RoleManageController extends EPRestrictedBaseController { + private static final ValidatorFactory VALIDATOR_FACTORY = Validation.buildDefaultValidatorFactory(); + private static final String PIPE = "|"; private static final String ROLE_INVALID_CHARS = "%=():,\"\""; @@ -497,8 +505,17 @@ public class RoleManageController extends EPRestrictedBaseController { } @RequestMapping(value = { "/portalApi/role_function_list/saveRoleFunction/{appId}" }, method = RequestMethod.POST) - public PortalRestResponse saveRoleFunction(HttpServletRequest request, HttpServletResponse response, @RequestBody CentralV2RoleFunction roleFunc, + public PortalRestResponse saveRoleFunction(HttpServletRequest request, HttpServletResponse response, @Valid @RequestBody CentralV2RoleFunction roleFunc, @PathVariable("appId") Long appId) throws Exception { + if (roleFunc!=null) { + Validator validator = VALIDATOR_FACTORY.getValidator(); + Set> constraintViolations = validator.validate(roleFunc); + + if(!constraintViolations.isEmpty()){ + logger.error(EELFLoggerDelegate.errorLogger, "saveRoleFunction: Failed"); + return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, "Data is not valid", "ERROR"); + } + } EPUser user = EPUserUtils.getUserSession(request); boolean saveOrUpdateResponse = false; try { @@ -594,6 +611,19 @@ public class RoleManageController extends EPRestrictedBaseController { public PortalRestResponse removeRoleFunction(HttpServletRequest request, HttpServletResponse response, @RequestBody String roleFunc, @PathVariable("appId") Long appId) throws Exception { EPUser user = EPUserUtils.getUserSession(request); + + if (roleFunc!=null) { + SecureString secureString = new SecureString(roleFunc); + + Validator validator = VALIDATOR_FACTORY.getValidator(); + Set> constraintViolations = validator.validate(secureString); + + if(!constraintViolations.isEmpty()){ + logger.error(EELFLoggerDelegate.errorLogger, "removeRoleFunction: Failed"); + return new PortalRestResponse<>(PortalRestStatusEnum.ERROR, "Data is not valid", "ERROR"); + } + } + try { EPApp requestedApp = appService.getApp(appId); if (isAuthorizedUser(user, requestedApp)) { @@ -656,6 +686,18 @@ public class RoleManageController extends EPRestrictedBaseController { @RequestMapping(value = { "/portalApi/centralizedApps" }, method = RequestMethod.GET) public List getCentralizedAppRoles(HttpServletRequest request, HttpServletResponse response, String userId) throws IOException { + if(userId!=null) { + SecureString secureString = new SecureString(userId); + + Validator validator = VALIDATOR_FACTORY.getValidator(); + Set> constraintViolations = validator.validate(secureString); + + if(!constraintViolations.isEmpty()){ + logger.error(EELFLoggerDelegate.errorLogger, "removeRoleFunction: Failed"); + return null; + } + } + EPUser user = EPUserUtils.getUserSession(request); List applicationsList = null; if (adminRolesService.isAccountAdmin(user) || adminRolesService.isSuperAdmin(user) || adminRolesService.isRoleAdmin(user)) { diff --git a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/domain/CentralV2RoleFunction.java b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/domain/CentralV2RoleFunction.java index d2ded5ad..a761103f 100644 --- a/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/domain/CentralV2RoleFunction.java +++ b/ecomp-portal-BE-common/src/main/java/org/onap/portalapp/portal/domain/CentralV2RoleFunction.java @@ -39,6 +39,7 @@ package org.onap.portalapp.portal.domain; import java.io.Serializable; +import org.hibernate.validator.constraints.SafeHtml; import org.onap.portalsdk.core.domain.support.DomainVo; import com.fasterxml.jackson.annotation.JsonIgnore; @@ -50,14 +51,18 @@ public class CentralV2RoleFunction extends DomainVo implements Serializable, Com * */ private static final long serialVersionUID = -4018975640065252688L; + @SafeHtml private String code; + @SafeHtml private String name; @JsonIgnore private Long appId; @JsonIgnore private Long roleId; private String type; + @SafeHtml private String action; + @SafeHtml private String editUrl; diff --git a/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/controller/RoleManageControllerTest.java b/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/controller/RoleManageControllerTest.java index 8bfa39c3..9673cb2c 100644 --- a/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/controller/RoleManageControllerTest.java +++ b/ecomp-portal-BE-common/src/test/java/org/onap/portalapp/portal/controller/RoleManageControllerTest.java @@ -370,6 +370,48 @@ public class RoleManageControllerTest { assertEquals(expected, actual); } + @Test + public void saveRoleFunctionXSSTest() throws Exception { + PowerMockito.mockStatic(EPUserUtils.class); + PowerMockito.mockStatic(EcompPortalUtils.class); + EPUser user = mockUser.mockEPUser(); + Mockito.when(EPUserUtils.getUserSession(mockedRequest)).thenReturn(user); + Mockito.when(EcompPortalUtils.checkIfRemoteCentralAccessAllowed()).thenReturn(true); + Mockito.when(adminRolesService.isAccountAdminOfApplication(user, CentralApp())).thenReturn(true); + Mockito.when(appService.getApp((long) 1)).thenReturn(CentralApp()); + Mockito.doNothing().when(roleFunctionListController).saveRoleFunction(mockedRequest, mockedResponse, "test"); + CentralV2RoleFunction addNewFunc = new CentralV2RoleFunction(); + addNewFunc.setCode("“>"); + addNewFunc.setType("Test"); + addNewFunc.setAction("Test"); + addNewFunc.setName("Test"); + CentralV2RoleFunction roleFunction = mockCentralRoleFunction(); + roleFunction.setCode("Test|Test|Test"); + Mockito.when(externalAccessRolesService.getRoleFunction("Test|Test|Test", "test")).thenReturn(roleFunction); + Mockito.when(externalAccessRolesService.saveCentralRoleFunction(Matchers.anyObject(), Matchers.anyObject())) + .thenReturn(true); + Mockito.when(EcompPortalUtils.getFunctionCode(roleFunction.getCode())).thenReturn("Test"); + Mockito.when(EcompPortalUtils.getFunctionType(roleFunction.getCode())).thenReturn("Test"); + Mockito.when(EcompPortalUtils.getFunctionAction(roleFunction.getCode())).thenReturn("Test"); + Mockito.when(EPUserUtils.getUserSession(mockedRequest)).thenReturn(user); + List userList = new ArrayList<>(); + userList.add(user); + List appList = new ArrayList<>(); + appList.add(CentralApp()); + Mockito.when(externalAccessRolesService.getUser("guestT")).thenReturn(userList); + StringWriter sw = new StringWriter(); + PrintWriter writer = new PrintWriter(sw); + Mockito.when(mockedResponse.getWriter()).thenReturn(writer); + ResponseEntity response = new ResponseEntity<>(HttpStatus.OK); + Mockito.when(externalAccessRolesService.getNameSpaceIfExists(Matchers.anyObject())).thenReturn(response); + Mockito.when(externalAccessRolesService.getApp(Matchers.anyString())).thenReturn(appList); + PortalRestResponse actual = roleManageController.saveRoleFunction(mockedRequest, mockedResponse, + addNewFunc, (long) 1); + PortalRestResponse expected = new PortalRestResponse(PortalRestStatusEnum.ERROR, + "Data is not valid", "ERROR"); + assertEquals(expected, actual); + } + @Test public void saveRoleFunctionExceptionTest() throws Exception { Mockito.when(appService.getApp((long) 1)).thenReturn(CentralApp()); @@ -420,6 +462,36 @@ public class RoleManageControllerTest { assertEquals(expected, actual); } + @Test + public void removeRoleFunctionXSSTest() throws Exception { + PowerMockito.mockStatic(EPUserUtils.class); + PowerMockito.mockStatic(EcompPortalUtils.class); + EPUser user = mockUser.mockEPUser(); + Mockito.when(EPUserUtils.getUserSession(mockedRequest)).thenReturn(user); + Mockito.when(EcompPortalUtils.checkIfRemoteCentralAccessAllowed()).thenReturn(true); + Mockito.when(adminRolesService.isAccountAdminOfApplication(user, CentralApp())).thenReturn(true); + Mockito.when(EPUserUtils.getUserSession(mockedRequest)).thenReturn(user); + Mockito.when(appService.getApp((long) 1)).thenReturn(CentralApp()); + String roleFun = ""; + CentralV2RoleFunction roleFunction = mockCentralRoleFunction(); + Mockito.when(externalAccessRolesService.getRoleFunction("Test|Test|Test", "test")).thenReturn(roleFunction); + StringWriter sw = new StringWriter(); + PrintWriter writer = new PrintWriter(sw); + Mockito.when(mockedResponse.getWriter()).thenReturn(writer); + Mockito.when(externalAccessRolesService.deleteCentralRoleFunction(Matchers.anyString(), Matchers.anyObject())) + .thenReturn(true); + List appList = new ArrayList<>(); + appList.add(CentralApp()); + ResponseEntity response = new ResponseEntity<>(HttpStatus.OK); + Mockito.when(externalAccessRolesService.getNameSpaceIfExists(Matchers.anyObject())).thenReturn(response); + Mockito.when(externalAccessRolesService.getApp(Matchers.anyString())).thenReturn(appList); + PortalRestResponse actual = roleManageController.removeRoleFunction(mockedRequest, mockedResponse, + roleFun, (long) 1); + PortalRestResponse expected = new PortalRestResponse(PortalRestStatusEnum.ERROR, + "Data is not valid", "ERROR"); + assertEquals(expected, actual); + } + @Test public void removeRoleFunctionExceptionTest() throws Exception { EPUser user = mockUser.mockEPUser(); @@ -908,6 +980,13 @@ public class RoleManageControllerTest { List actual = roleManageController.getCentralizedAppRoles(mockedRequest, mockedResponse, user.getOrgUserId()); assertEquals(cenApps.size(), actual.size()); } + + @Test + public void getCentralizedAppRolesXSSTest() throws IOException { + String id = (""); + List actual = roleManageController.getCentralizedAppRoles(mockedRequest, mockedResponse, id); + assertNull(actual); + } @Test public void getCentralizedAppRolesExceptionTest() throws IOException { -- 2.16.6