SONAR-11475 Sanitize URL when doing redirection during OAuth authentication

diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2Redirection.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2Redirection.java
index 18619fecda3aa6753f9e05817d397cfd7be85e64..ffb0c82000f488e56dcc0e08801d24ba6b5084d3 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2Redirection.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/OAuth2Redirection.java
@@ -23,8 +23,9 @@ import java.util.Optional;
 import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import org.elasticsearch.common.Nullable;
 
-import static org.apache.commons.lang.StringUtils.isBlank;
+import static org.elasticsearch.common.Strings.isNullOrEmpty;
 import static org.sonar.server.authentication.Cookies.findCookie;
 import static org.sonar.server.authentication.Cookies.newCookieBuilder;
 
@@ -39,13 +40,13 @@ public class OAuth2Redirection {
   private static final String RETURN_TO_PARAMETER = "return_to";
 
   public void create(HttpServletRequest request, HttpServletResponse response) {
-    String redirectTo = request.getParameter(RETURN_TO_PARAMETER);
-    if (isBlank(redirectTo)) {
+    Optional<String> redirectTo = sanitizeRedirectUrl(request.getParameter(RETURN_TO_PARAMETER));
+    if (!redirectTo.isPresent()) {
       return;
     }
     response.addCookie(newCookieBuilder(request)
       .setName(REDIRECT_TO_COOKIE)
-      .setValue(redirectTo)
+      .setValue(redirectTo.get())
       .setHttpOnly(true)
       .setExpiry(-1)
       .build());
@@ -60,7 +61,7 @@ public class OAuth2Redirection {
     delete(request, response);
 
     String redirectTo = cookie.get().getValue();
-    if (isBlank(redirectTo)) {
+    if (isNullOrEmpty(redirectTo)) {
       return Optional.empty();
     }
     return Optional.of(redirectTo);
@@ -75,4 +76,17 @@ public class OAuth2Redirection {
       .build());
   }
 
+  private static Optional<String> sanitizeRedirectUrl(@Nullable String url){
+    if (isNullOrEmpty(url)){
+      return Optional.empty();
+    }
+    if (url.startsWith("//") || url.startsWith("/\\")){
+      return Optional.empty();
+    }
+    if (!url.startsWith("/")){
+      return Optional.empty();
+    }
+    return Optional.of(url);
+  }
+
 }

diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/OAuth2RedirectionTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/OAuth2RedirectionTest.java
index 09d13bd0c94e3cacd18faebebf07974af646a4f2..6425d86cfd6f34eaf67176996a8431b19de8dc81 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/authentication/OAuth2RedirectionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/OAuth2RedirectionTest.java
@@ -84,6 +84,25 @@ public class OAuth2RedirectionTest {
     verify(response, never()).addCookie(any());
   }
 
+  @Test
+  public void does_not_create_cookie_when_return_to_is_not_local_url() {
+    when(request.getParameter("return_to")).thenReturn("http://external_url");
+    underTest.create(request, response);
+    verify(response, never()).addCookie(any());
+
+    when(request.getParameter("return_to")).thenReturn("//local_file");
+    underTest.create(request, response);
+    verify(response, never()).addCookie(any());
+
+    when(request.getParameter("return_to")).thenReturn("/\\local_file");
+    underTest.create(request, response);
+    verify(response, never()).addCookie(any());
+
+    when(request.getParameter("return_to")).thenReturn("something_else");
+    underTest.create(request, response);
+    verify(response, never()).addCookie(any());
+  }
+
   @Test
   public void get_and_delete() throws Exception {
     when(request.getCookies()).thenReturn(new Cookie[]{new Cookie("REDIRECT_TO", "/settings")});

diff --git a/tests/src/test/java/org/sonarqube/tests/user/OAuth2IdentityProviderTest.java b/tests/src/test/java/org/sonarqube/tests/user/OAuth2IdentityProviderTest.java
index 65312825703defa12dec68500954953a881e0b77..668c8b76750514ea57c84f6321062e7530019fe3 100644 (file)
--- a/tests/src/test/java/org/sonarqube/tests/user/OAuth2IdentityProviderTest.java
+++ b/tests/src/test/java/org/sonarqube/tests/user/OAuth2IdentityProviderTest.java
@@ -32,6 +32,7 @@ import org.junit.Before;
 import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
+import org.sonarqube.pageobjects.LoginPage;
 import org.sonarqube.pageobjects.Navigation;
 import org.sonarqube.tests.Category4Suite;
 import org.sonarqube.tests.Tester;
@@ -43,6 +44,7 @@ import org.sonarqube.ws.client.user.CreateRequest;
 
 import static com.codeborne.selenide.Condition.visible;
 import static com.codeborne.selenide.Selenide.$;
+import static com.codeborne.selenide.WebDriverRunner.url;
 import static org.assertj.core.api.Assertions.assertThat;
 
 /**
@@ -216,10 +218,43 @@ public class OAuth2IdentityProviderTest {
     assertThat(user.getExternalProvider()).isEqualTo(FAKE_PROVIDER_KEY);
   }
 
+  @Test
+  public void redirect_user() {
+    simulateRedirectionToCallback();
+    enablePlugin();
+    // Provision the user and grand him admin access
+    tester.users().service().create(CreateRequest.builder().setLogin(USER_LOGIN).setName(USER_NAME).setLocal(false).build());
+    tester.wsClient().permissions().addUser(new AddUserWsRequest().setLogin(USER_LOGIN).setPermission("admin"));
+    Navigation navigation = tester.openBrowser();
+
+    // Try to access a page requiring admin permission
+    LoginPage login = navigation.open("/settings", LoginPage.class);
+    navigation.shouldBeRedirectedToLogin();
+    login.useOAuth2().shouldBeLoggedIn();
+
+    // The user has well been redirected to the requested page
+    $("#settings-page").shouldBe(visible);
+  }
+
+  @Test
+  public void do_not_redirect_to_forged_urls_after_login() {
+    enablePlugin();
+    Navigation navigation = tester.openBrowser();
+
+    simulateRedirectionToCallback();
+    navigation.open("/sessions/init/" + FAKE_PROVIDER_KEY + "?return_to=https%3A%2F%2Fsonarsource.com");
+    assertThat(url()).doesNotContain("https://www.sonarsource.com/");
+    navigation.shouldBeLoggedIn();
+
+    simulateRedirectionToCallback();
+    navigation.logOut().open("/sessions/init/" + FAKE_PROVIDER_KEY + "?return_to=javascript%3Awindow.location.href%3D%27https%3A%2F%2Fwww.sonarsource.com%2F");
+    assertThat(url()).doesNotContain("https://www.sonarsource.com/");
+    navigation.shouldBeLoggedIn();
+  }
 
   private void authenticateWithFakeAuthProvider() {
     WsResponse response = tester.wsClient().wsConnector().call(
-      new GetRequest(("/sessions/init/" + FAKE_PROVIDER_KEY)));
+      new GetRequest("/sessions/init/" + FAKE_PROVIDER_KEY));
     assertThat(response.code()).isEqualTo(200);
   }