#77 Fix code scanning alert - Server-side request forgery (#95)

conorheffron · Sep 26, 2024 · 5bca6b1 · 5bca6b1
1 parent fc66466
commit 5bca6b1
Show file tree

Hide file tree

Showing 11 changed files with 33 additions and 59 deletions.
diff --git a/pom.xml b/pom.xml
@@ -5,7 +5,7 @@
 
 	<groupId>conorheffron</groupId>
 	<artifactId>ironoc</artifactId>
-	<version>4.4.2</version>
+	<version>4.4.3</version>
 	<packaging>war</packaging>
 
 	<distributionManagement>

diff --git a/src/main/java/com/ironoc/portfolio/client/GitClient.java b/src/main/java/com/ironoc/portfolio/client/GitClient.java
@@ -7,6 +7,7 @@
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.http.HttpMethod;
 import org.springframework.stereotype.Component;
+import org.springframework.web.util.UriComponentsBuilder;
 
 import javax.net.ssl.HttpsURLConnection;
 import java.io.IOException;
@@ -37,7 +38,7 @@ public HttpsURLConnection createConn(String url) throws IOException {
             log.error("The url is not valid for GIT client connection, url={}", url);
             return null;
         }
-        URL apiUrlEndpoint = new URL(url);
+        URL apiUrlEndpoint = new URL(UriComponentsBuilder.fromHttpUrl(url).toUriString());
         HttpsURLConnection conn = (HttpsURLConnection) apiUrlEndpoint.openConnection();
         String token = secretManager.getGitSecret();
         if (StringUtils.isBlank(token)) {

diff --git a/src/main/java/com/ironoc/portfolio/config/Properties.java b/src/main/java/com/ironoc/portfolio/config/Properties.java
@@ -6,7 +6,6 @@
 public enum Properties {
 
     GIT_API_ENDPOINT("com.ironoc.portfolio.github.api.endpoint"),
-    GIT_REPOS_URI("com.ironoc.portfolio.github.uri.repos"),
     GIT_TIMEOUT_CONNECT ("com.ironoc.portfolio.github.timeout.connect"),
     GIT_TIMEOUT_READ("com.ironoc.portfolio.github.timeout.read"),
     GIT_INSTANCE_FOLLOW_REDIRECTS("com.ironoc.portfolio.github.instance-follow-redirects"),

diff --git a/src/main/java/com/ironoc/portfolio/config/PropertyConfig.java b/src/main/java/com/ironoc/portfolio/config/PropertyConfig.java
@@ -20,11 +20,6 @@ public String getGitApiEndpoint() {
         return environment.getRequiredProperty(propertyKey.getGitApiEndpoint());
     }
 
-    @Override
-    public String getGitReposUri() {
-        return environment.getRequiredProperty(propertyKey.getGitReposUri());
-    }
-
     @Override
     public Integer getGitTimeoutConnect() {
         return Integer.valueOf(environment.getRequiredProperty(propertyKey.getGitTimeoutConnect()));

diff --git a/src/main/java/com/ironoc/portfolio/config/PropertyConfigI.java b/src/main/java/com/ironoc/portfolio/config/PropertyConfigI.java
@@ -4,8 +4,6 @@ public interface PropertyConfigI {
 
     String getGitApiEndpoint();
 
-    String getGitReposUri();
-
     Integer getGitTimeoutConnect();
 
     Integer getGitTimeoutRead();

diff --git a/src/main/java/com/ironoc/portfolio/config/PropertyKey.java b/src/main/java/com/ironoc/portfolio/config/PropertyKey.java
@@ -10,11 +10,6 @@ public String getGitApiEndpoint() {
         return Properties.GIT_API_ENDPOINT.getKey();
     }
 
-    @Override
-    public String getGitReposUri() {
-        return Properties.GIT_REPOS_URI.getKey();
-    }
-
     @Override
     public String getGitTimeoutConnect() {
         return Properties.GIT_TIMEOUT_CONNECT.getKey();

diff --git a/src/main/java/com/ironoc/portfolio/config/PropertyKeyI.java b/src/main/java/com/ironoc/portfolio/config/PropertyKeyI.java
@@ -4,8 +4,6 @@ public interface PropertyKeyI {
 
     String getGitApiEndpoint();
 
-    String getGitReposUri();
-
     String getGitTimeoutConnect();
 
     String getGitTimeoutRead();

diff --git a/src/main/java/com/ironoc/portfolio/service/GitDetailsService.java b/src/main/java/com/ironoc/portfolio/service/GitDetailsService.java
@@ -12,6 +12,7 @@
 import org.apache.commons.lang3.StringUtils;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Service;
+import org.springframework.web.util.UriComponentsBuilder;
 
 import javax.net.ssl.HttpsURLConnection;
 import java.io.IOException;
@@ -58,19 +59,20 @@ public List<RepositoryDetailDto> getRepoDetails(String username) {
             }
         }
         // further end-point validation (contains User ID)
-        String apiEndpoint = propertyConfig.getGitApiEndpoint();
-        String apiUri = propertyConfig.getGitReposUri();
-        String url = apiEndpoint + username + apiUri;
+        String uri = propertyConfig.getGitApiEndpoint();
+        String apiUri = UriComponentsBuilder.fromHttpUrl(uri)
+                .buildAndExpand(username)
+                .toUriString();
         if (StringUtils.isBlank(apiUri) | StringUtils.isBlank(apiUri)
-                | !urlUtils.isValidURL(url)) {
-            warn("URL is not valid: url={}", url);
+                | !urlUtils.isValidURL(apiUri)) {
+            warn("URL is not valid: url={}", apiUri);
             return Collections.emptyList();
         }
-        info("Triggering repositories GET request: url={}", url);
+        info("Triggering repositories GET request: url={}", apiUri);
         List<RepositoryDetailDto> repositoryDetailDtos = new ArrayList<>();
         InputStream inputStream = null;
         try {
-            HttpsURLConnection conn = gitClient.createConn(url);
+            HttpsURLConnection conn = gitClient.createConn(apiUri);
             inputStream = gitClient.readInputStream(conn);
             repositoryDetailDtos = Arrays.asList(objectMapper.readValue(inputStream,
                     RepositoryDetailDto[].class));

diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml
@@ -7,10 +7,8 @@ com:
           read: 100000
         instance-follow-redirects: true
         follow-redirects: true
-        uri:
-          repos: /repos
         api:
-          endpoint: https://api.github.com/users/
+          endpoint: https://api.github.com/users/{username}/repos
 
 spring:
   mvc:

diff --git a/src/test/java/com/ironoc/portfolio/config/PropertyConfigTest.java b/src/test/java/com/ironoc/portfolio/config/PropertyConfigTest.java
@@ -42,22 +42,6 @@ public void test_getGitApiEndpoint_success() {
         assertThat(result, is(TEST_PROP_VAL));
     }
 
-    @Test
-    public void test_getGitReposUri_success() {
-        // given
-        when(propertyKeyMock.getGitReposUri()).thenReturn(Properties.GIT_REPOS_URI.getKey());
-        when(environmentMock.getRequiredProperty(Properties.GIT_REPOS_URI.getKey())).thenReturn(TEST_PROP_VAL);
-
-        // when
-        String result = propertyConfig.getGitReposUri();
-
-        // then
-        verify(propertyKeyMock).getGitReposUri();
-        verify(environmentMock).getRequiredProperty(Properties.GIT_REPOS_URI.getKey());
-
-        assertThat(result, is(TEST_PROP_VAL));
-    }
-
     @Test
     public void test_getGitFollowRedirects_success() {
         // given

diff --git a/src/test/java/com/ironoc/portfolio/service/GitDetailsServiceTest.java b/src/test/java/com/ironoc/portfolio/service/GitDetailsServiceTest.java
@@ -5,6 +5,7 @@
 import com.ironoc.portfolio.config.PropertyConfigI;
 import com.ironoc.portfolio.domain.RepositoryDetailDomain;
 import com.ironoc.portfolio.dto.RepositoryDetailDto;
+import com.ironoc.portfolio.job.GitDetailsJob;
 import com.ironoc.portfolio.utils.UrlUtils;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -52,15 +53,14 @@ public class GitDetailsServiceTest {
     private HttpsURLConnection httpsURLConnectionMock;
 
     @Mock
-    private InputStream inputStreamMock;
+    private UrlUtils urlUtilsMock;
 
     @Mock
-    private UrlUtils urlUtilsMock;
+    private GitRepoCache gitRepoCacheMock;
 
     private ObjectMapper objectMapper = new ObjectMapper();
 
-    @Mock
-    private GitRepoCache gitRepoCacheMock;
+    private static final String TEST_URI = "https://unittest.github.com/users/{username}/repos";
 
     @Test
     public void test_get_repos_success() throws IOException {
@@ -85,8 +85,8 @@ public void test_get_repos_success() throws IOException {
                 .isPrivate(false)
                 .build();
 
-        when(propertyConfigMock.getGitApiEndpoint()).thenReturn("https://unittest.github.com/users/");
-        when(propertyConfigMock.getGitReposUri()).thenReturn("/repos");
+        when(propertyConfigMock.getGitApiEndpoint())
+                .thenReturn(TEST_URI);
         when(urlUtilsMock.isValidURL(anyString())).thenReturn(true);
         when(gitClient.createConn(anyString())).thenReturn(httpsURLConnectionMock);
         when(gitClient.readInputStream(httpsURLConnectionMock)).thenReturn(jsonInputStream);
@@ -98,10 +98,8 @@ public void test_get_repos_success() throws IOException {
 
         // then
         verify(propertyConfigMock).getGitApiEndpoint();
-        verify(propertyConfigMock).getGitReposUri();
         verify(urlUtilsMock).isValidURL(anyString());
         verify(propertyConfigMock).getGitApiEndpoint();
-        verify(propertyConfigMock).getGitReposUri();
         verify(gitClient).createConn(anyString());
         verify(gitClient).readInputStream(any(HttpsURLConnection.class));
         verify(objectMapperMock).readValue(any(InputStream.class), any(Class.class));
@@ -141,8 +139,7 @@ public void test_get_repos_parseNull_values_success() throws IOException {
                 .isPrivate(false)
                 .build();
 
-        when(propertyConfigMock.getGitApiEndpoint()).thenReturn("https://unittest.github.com/users/");
-        when(propertyConfigMock.getGitReposUri()).thenReturn("/repos");
+        when(propertyConfigMock.getGitApiEndpoint()).thenReturn(TEST_URI);
         when(urlUtilsMock.isValidURL(anyString())).thenReturn(true);
         when(gitClient.createConn(anyString())).thenReturn(httpsURLConnectionMock);
         when(gitClient.readInputStream(httpsURLConnectionMock)).thenReturn(jsonInputStream);
@@ -154,10 +151,8 @@ public void test_get_repos_parseNull_values_success() throws IOException {
 
         // then
         verify(propertyConfigMock).getGitApiEndpoint();
-        verify(propertyConfigMock).getGitReposUri();
         verify(urlUtilsMock).isValidURL(anyString());
         verify(propertyConfigMock).getGitApiEndpoint();
-        verify(propertyConfigMock).getGitReposUri();
         verify(gitClient).createConn(anyString());
         verify(gitClient).readInputStream(any(HttpsURLConnection.class));
         verify(objectMapperMock).readValue(any(InputStream.class), any(Class.class));
@@ -183,6 +178,8 @@ public void test_getRepoDetails_result_cached_success() {
         List<RepositoryDetailDto> results = gitDetailsService.getRepoDetails(testUserId);
 
         // then
+        verify(gitRepoCacheMock).get(GitDetailsJob.USERNAME_HOME_PAGE);
+
         assertThat(results, is(notNullValue()));
         assertThat(results, is(hasSize(0)));
     }
@@ -191,11 +188,14 @@ public void test_getRepoDetails_result_cached_success() {
     public void test_getRepoDetails_url_invalid_fail() {
         // given
         String testUserId = "conorheffron-test-id";
+        when(propertyConfigMock.getGitApiEndpoint()).thenReturn(TEST_URI);
 
         // when
         List<RepositoryDetailDto> results = gitDetailsService.getRepoDetails(testUserId);
 
         // then
+        verify(propertyConfigMock).getGitApiEndpoint();
+
         assertThat(results, is(notNullValue()));
         assertThat(results, is(hasSize(0)));
     }
@@ -214,10 +214,14 @@ public void test_mapRepositoriesToResponse_success() throws IOException {
         Optional<RepositoryDetailDomain> result = results.stream().findFirst();
         assertThat(result.get().getName(), is("bio-cell-red-edge"));
         assertThat(result.get().getFullName(), is("conorheffron/bio-cell-red-edge"));
-        assertThat(result.get().getDescription(), is("Edge Detection of Biological Cell (Image Processing Script)"));
-        assertThat(result.get().getTopics(), is("[biology, computer-vision, image-processing, scikitlearn-machine-learning]"));
-        assertThat(result.get().getAppHome(), is("https://conorheffron.github.io/bio-cell-red-edge/"));
-        assertThat(result.get().getRepoUrl(), is("https://github.com/conorheffron/bio-cell-red-edge"));
+        assertThat(result.get().getDescription(),
+                is("Edge Detection of Biological Cell (Image Processing Script)"));
+        assertThat(result.get().getTopics(),
+                is("[biology, computer-vision, image-processing, scikitlearn-machine-learning]"));
+        assertThat(result.get().getAppHome(),
+                is("https://conorheffron.github.io/bio-cell-red-edge/"));
+        assertThat(result.get().getRepoUrl(),
+                is("https://github.com/conorheffron/bio-cell-red-edge"));
         RepositoryDetailDomain result2 = results.get(1);
         assertThat(result2.getName(), is("booking-sys"));
         assertThat(result2.getFullName(), is("conorheffron/booking-sys"));