diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImpl.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImpl.java index 633591f10238..9cdaa9d466c8 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImpl.java +++ b/server/sonar-webserver-auth/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 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/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImplTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImplTest.java index 252b6fcdf5e7..aa499933316b 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2AuthenticationParametersImplTest.java +++ b/server/sonar-webserver-auth/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 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 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 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); + } }