From fe776f125d1f8215f1a8992e18269595eaafa7f1 Mon Sep 17 00:00:00 2001 From: kameshsr Date: Thu, 2 Jan 2025 16:52:33 +0530 Subject: [PATCH 1/2] MOSIP-38604 Fixed security bug in sonar Signed-off-by: kameshsr --- .../resident/constant/ResidentConstants.java | 2 + .../controller/OrderCardController.java | 48 ++++++++++++++++++- .../controller/OrderCardControllerTest.java | 16 ++++++- 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/resident/resident-service/src/main/java/io/mosip/resident/constant/ResidentConstants.java b/resident/resident-service/src/main/java/io/mosip/resident/constant/ResidentConstants.java index ceda77e8ff2..4fcf2541eb6 100644 --- a/resident/resident-service/src/main/java/io/mosip/resident/constant/ResidentConstants.java +++ b/resident/resident-service/src/main/java/io/mosip/resident/constant/ResidentConstants.java @@ -327,4 +327,6 @@ private ResidentConstants() { public static final String DISCARD_DRAFT_ID = "mosip.resident.discard.pending.drafts"; public static final String DISCARD_DRAFT_VERSION = "mosip.resident.discard.pending.drafts.version"; public static final String REG_PROC_CREDENTIAL_PARTNER_POLICY_URL = "mosip.resident.reg-processer-credential-partner-policy-url"; + + public static final String ALLOWED_URL = "auth.allowed.urls"; } diff --git a/resident/resident-service/src/main/java/io/mosip/resident/controller/OrderCardController.java b/resident/resident-service/src/main/java/io/mosip/resident/controller/OrderCardController.java index c746db2bb72..80e14a813cc 100644 --- a/resident/resident-service/src/main/java/io/mosip/resident/controller/OrderCardController.java +++ b/resident/resident-service/src/main/java/io/mosip/resident/controller/OrderCardController.java @@ -4,7 +4,10 @@ import static io.mosip.resident.constant.ResidentConstants.API_RESPONSE_TIME_ID; import java.net.URI; +import java.net.URISyntaxException; +import java.util.List; import java.util.Map; +import java.util.Objects; import io.mosip.resident.util.AvailableClaimUtility; import org.springframework.beans.factory.annotation.Autowired; @@ -142,7 +145,50 @@ public ResponseEntity physicalCardOrderRedirect(@RequestParam(name = "re String response = orderCardService.physicalCardOrder(redirectUrl, paymentTransactionId, eventId, residentFullAddress,individualId,errorCode,errorMessage); logger.debug("OrderCardController::physicalCardOrderRedirect()::exit"); - return ResponseEntity.status(HttpStatus.FOUND).location(URI.create(response)).build(); + String safeResponseUrl = validateAndSanitizeUrl(response); + + if (safeResponseUrl == null) { + logger.warn("OrderCardController::physicalCardOrderRedirect()::Invalid redirect URL: {}", response); + return ResponseEntity.status(HttpStatus.BAD_REQUEST).body("Invalid redirect URL"); + } + + return ResponseEntity.status(HttpStatus.FOUND).location(URI.create(safeResponseUrl)).build(); + } + private String validateAndSanitizeUrl(String url) { + if (url == null || url.isEmpty()) { + logger.warn("URL is null or empty."); + return null; + } + + try { + URI uri = new URI(url); + String host = uri.getHost(); + String scheme = uri.getScheme(); // Extract protocol (http/https) + String path = uri.getPath(); + + // Ensure path is normalized with a trailing slash + String normalizedPath = (path == null || path.isEmpty()) ? "/" : path; + + // Reconstruct base URL with scheme, host, and normalized path + String baseUrl = scheme + "://" + host + normalizedPath; + + // Example: Whitelisted domains with protocol and trailing slash + List allowedUrls = List.of(Objects.requireNonNull(env.getProperty(ResidentConstants.ALLOWED_URL))); + + logger.debug("Validating URL. Constructed Base URL: {}, Allowed URLs: {}", baseUrl, allowedUrls); + + if (host != null && scheme != null && allowedUrls.contains(baseUrl)) { + return uri.toString(); + } + } catch (URISyntaxException e) { + logger.error("Invalid URL syntax: {}", url, e); + } + + logger.warn("URL validation failed for: {}", url); + return null; // URL is invalid or not allowed + } + + } \ No newline at end of file diff --git a/resident/resident-service/src/test/java/io/mosip/resident/controller/OrderCardControllerTest.java b/resident/resident-service/src/test/java/io/mosip/resident/controller/OrderCardControllerTest.java index 865b78dfe22..e5f70a1ba3f 100644 --- a/resident/resident-service/src/test/java/io/mosip/resident/controller/OrderCardControllerTest.java +++ b/resident/resident-service/src/test/java/io/mosip/resident/controller/OrderCardControllerTest.java @@ -29,6 +29,7 @@ import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.context.annotation.Import; +import org.springframework.core.env.Environment; import org.springframework.http.MediaType; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.ContextConfiguration; @@ -116,6 +117,9 @@ public class OrderCardControllerTest { @Mock private AvailableClaimUtility availableClaimUtility; + @Mock + private Environment environment; + @Before public void setUp() throws Exception { responseWrapper = new ResponseWrapper<>(); @@ -149,10 +153,20 @@ public void testPhysicalCardOrder() throws Exception { @Test public void testPhysicalCardOrderRedirect() throws Exception { + Mockito.when(environment.getProperty(Mockito.anyString())).thenReturn("https://resident.dev1.mosip.net/"); Mockito.when(orderCardService.physicalCardOrder(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), - Mockito.any(), Mockito.any(), Mockito.any())).thenReturn("URL"); + Mockito.any(), Mockito.any(), Mockito.any())).thenReturn("https://resident.dev1.mosip.net/"); mockMvc.perform(MockMvcRequestBuilders.get( "/physical-card/order-redirect?redirectUrl=aHR0cHM6Ly93d3cubWFkZWludGV4dC5jb20v&paymentTransactionId=12345dsvdvds&eventId=123456&residentFullAddress=fgfhfghgf")).andExpect(status().isFound()); } + @Test + public void testPhysicalCardOrderRedirectFailure() throws Exception { + Mockito.when(environment.getProperty(Mockito.anyString())).thenReturn("https://resident.dev2.mosip.net/"); + Mockito.when(orderCardService.physicalCardOrder(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.any(), Mockito.any())).thenReturn("https://resident.dev1.mosip.net/"); + mockMvc.perform(MockMvcRequestBuilders.get( + "/physical-card/order-redirect?redirectUrl=aHR0cHM6Ly93d3cubWFkZWludGV4dC5jb20v&paymentTransactionId=12345dsvdvds&eventId=123456&residentFullAddress=fgfhfghgf")).andExpect(status().isBadRequest()); + } + } \ No newline at end of file From 8e98a16d3dfafb60b8c8aa5817c6ee45c306b941 Mon Sep 17 00:00:00 2001 From: kameshsr Date: Tue, 7 Jan 2025 11:01:25 +0530 Subject: [PATCH 2/2] MOSIP-38604 Added unit test cases Signed-off-by: kameshsr --- .../controller/OrderCardControllerTest.java | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/resident/resident-service/src/test/java/io/mosip/resident/controller/OrderCardControllerTest.java b/resident/resident-service/src/test/java/io/mosip/resident/controller/OrderCardControllerTest.java index e5f70a1ba3f..9d340e62d12 100644 --- a/resident/resident-service/src/test/java/io/mosip/resident/controller/OrderCardControllerTest.java +++ b/resident/resident-service/src/test/java/io/mosip/resident/controller/OrderCardControllerTest.java @@ -169,4 +169,31 @@ public void testPhysicalCardOrderRedirectFailure() throws Exception { "/physical-card/order-redirect?redirectUrl=aHR0cHM6Ly93d3cubWFkZWludGV4dC5jb20v&paymentTransactionId=12345dsvdvds&eventId=123456&residentFullAddress=fgfhfghgf")).andExpect(status().isBadRequest()); } + @Test + public void testPhysicalCardOrderRedirectFailureUrlEmpty() throws Exception { + Mockito.when(environment.getProperty(Mockito.anyString())).thenReturn("https://resident.dev2.mosip.net/"); + Mockito.when(orderCardService.physicalCardOrder(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(""); + mockMvc.perform(MockMvcRequestBuilders.get( + "/physical-card/order-redirect?redirectUrl=aHR0cHM6Ly93d3cubWFkZWludGV4dC5jb20v&paymentTransactionId=12345dsvdvds&eventId=123456&residentFullAddress=fgfhfghgf")).andExpect(status().isBadRequest()); + } + + @Test + public void testPhysicalCardOrderRedirectFailureUrlNull() throws Exception { + Mockito.when(environment.getProperty(Mockito.anyString())).thenReturn("https://resident.dev2.mosip.net/"); + Mockito.when(orderCardService.physicalCardOrder(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(null); + mockMvc.perform(MockMvcRequestBuilders.get( + "/physical-card/order-redirect?redirectUrl=aHR0cHM6Ly93d3cubWFkZWludGV4dC5jb20v&paymentTransactionId=12345dsvdvds&eventId=123456&residentFullAddress=fgfhfghgf")).andExpect(status().isBadRequest()); + } + + @Test + public void testPhysicalCardOrderRedirectFailureUrlInvalidUrl() throws Exception { + Mockito.when(environment.getProperty(Mockito.anyString())).thenReturn("https://resident.dev2.mosip.net/"); + Mockito.when(orderCardService.physicalCardOrder(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.any(), Mockito.any())).thenReturn("https://resident.dev1.mosip.net /"); + mockMvc.perform(MockMvcRequestBuilders.get( + "/physical-card/order-redirect?redirectUrl=aHR0cHM6Ly93d3cubWFkZWludGV4dC5jb20v&paymentTransactionId=12345dsvdvds&eventId=123456&residentFullAddress=fgfhfghgf")).andExpect(status().isBadRequest()); + } + } \ No newline at end of file