SONAR-21889 Fix SSF-539

(cherry picked from commit b5abb73908a7963c9055351fed7d39398b335450)
SonarSource · Apr 24, 2024 · 4a950bd · 4a950bd
1 parent 4af898f
commit 4a950bd
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 7 deletions.
diff --git a/...uth/src/main/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImpl.java b/...uth/src/main/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImpl.java
@@ -25,6 +25,7 @@
 import com.google.gson.GsonBuilder;
 import java.io.UnsupportedEncodingException;
 import java.lang.reflect.Type;
+import java.nio.file.Path;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
@@ -124,12 +125,17 @@ private static Optional<String> sanitizeRedirectUrl(@Nullable String url) {
       return empty();
     }
 
-    String sanitizedUrl = url.trim();
-    boolean isValidUrl = VALID_RETURN_TO.matcher(sanitizedUrl).matches();
+    String trimmedUrl = url.trim();
+    boolean isValidUrl = VALID_RETURN_TO.matcher(trimmedUrl).matches();
     if (!isValidUrl) {
       return empty();
     }
 
-    return Optional.of(sanitizedUrl);
+    Path sanitizedPath = escapePathTraversalChars(trimmedUrl);
+    return Optional.of(sanitizedPath.toString());
+  }
+
+  private static Path escapePathTraversalChars(String sanitizedUrl) {
+    return Path.of(sanitizedUrl).normalize();
   }
 }
diff --git a/...src/test/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImplTest.java b/...src/test/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImplTest.java
@@ -21,7 +21,9 @@
 
 import com.tngtech.java.junit.dataprovider.DataProvider;
 import com.tngtech.java.junit.dataprovider.DataProviderRunner;
+import com.tngtech.java.junit.dataprovider.UseDataProvider;
 import java.util.Optional;
+import javax.annotation.Nullable;
 import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -54,7 +56,7 @@ public void setUp() {
 
   @Test
   public void init_create_cookie() {
-    when(request.getParameter("return_to")).thenReturn("/settings");
+    when(request.getParameter("return_to")).thenReturn("/admin/settings");
 
     underTest.init(request, response);
 
@@ -114,7 +116,7 @@ public void get_return_to_parameter() {
 
   @Test
   public void get_return_to_is_empty_when_no_cookie() {
-    when(request.getCookies()).thenReturn(new Cookie[] {});
+    when(request.getCookies()).thenReturn(new Cookie[]{});
 
     Optional<String> redirection = underTest.getReturnTo(request);
 
@@ -123,7 +125,7 @@ public void get_return_to_is_empty_when_no_cookie() {
 
   @Test
   public void get_return_to_is_empty_when_no_value() {
-    when(request.getCookies()).thenReturn(new Cookie[] {new Cookie(AUTHENTICATION_COOKIE_NAME, "{}")});
+    when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(AUTHENTICATION_COOKIE_NAME, "{}")});
 
     Optional<String> redirection = underTest.getReturnTo(request);
 
@@ -132,7 +134,7 @@ public void get_return_to_is_empty_when_no_value() {
 
   @Test
   public void delete() {
-    when(request.getCookies()).thenReturn(new Cookie[] {new Cookie(AUTHENTICATION_COOKIE_NAME, "{\"return_to\":\"/settings\"}")});
+    when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(AUTHENTICATION_COOKIE_NAME, "{\"return_to\":\"/settings\"}")});
 
     underTest.delete(request, response);
 
@@ -143,4 +145,35 @@ public void delete() {
     assertThat(updatedCookie.getPath()).isEqualTo("/");
     assertThat(updatedCookie.getMaxAge()).isZero();
   }
+
+  @DataProvider
+  public static Object[][] payloadToSanitizeAndExpectedOutcome() {
+    return new Object[][]{
+      {generatePath("/admin/settings"), "/admin/settings"},
+      {generatePath("/admin/../../settings"), "/settings"},
+      {generatePath("/admin/../settings"), "/settings"},
+      {generatePath("/admin/settings/.."), "/admin"},
+      {generatePath("/admin/..%2fsettings/"), "/settings"},
+      {generatePath("/admin/%2e%2e%2fsettings/"), "/settings"},
+      {generatePath("../admin/settings"), null},
+    };
+  }
+
+  private static String generatePath(String returnTo) {
+    return "{\"return_to\":\"" + returnTo + "\"}";
+  }
+
+  @Test
+  @UseDataProvider("payloadToSanitizeAndExpectedOutcome")
+  public void getReturnTo_whenContainingPathTraversalCharacters_sanitizeThem(String payload, @Nullable String expectedSanitizedUrl) {
+    when(request.getCookies()).thenReturn(new Cookie[]{wrapCookie(AUTHENTICATION_COOKIE_NAME, payload)});
+
+    Optional<String> redirection = underTest.getReturnTo(request);
+
+    assertThat(redirection).isEqualTo(Optional.ofNullable(expectedSanitizedUrl));
+  }
+
+  private Cookie wrapCookie(String name, String value) {
+    return new javax.servlet.http.Cookie(name, value);
+  }
 }