SONAR-10991 Don't display error responses containing HTML content

5 files changed, 68 insertions, 19 deletions

diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/bootstrap/ScannerWsClient.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/bootstrap/ScannerWsClient.java
index 638a099fdf2..9f12820a1fd 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/bootstrap/ScannerWsClient.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/bootstrap/ScannerWsClient.java
@@ -28,6 +28,8 @@ import com.google.gson.JsonObject;
 import com.google.gson.JsonParser;
 import java.util.ArrayList;
 import java.util.List;
+import javax.annotation.CheckForNull;
+import org.apache.commons.lang.StringUtils;
 import org.sonar.api.CoreProperties;
 import org.sonar.api.utils.MessageException;
 import org.sonar.api.utils.log.Logger;
@@ -45,7 +47,7 @@ import static java.net.HttpURLConnection.HTTP_FORBIDDEN;
 import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED;
 
 public class ScannerWsClient {
-
+  private static final int MAX_ERROR_MSG_LEN = 128;
   private static final Logger LOG = Loggers.get(ScannerWsClient.class);
 
   private final WsClient target;
@@ -59,13 +61,13 @@ public class ScannerWsClient {
   }
 
   /**
-   * If an exception is not thrown, the response needs to be closed by either calling close() directly, or closing the 
+   * If an exception is not thrown, the response needs to be closed by either calling close() directly, or closing the
    * body content's stream/reader.
-   * @throws IllegalStateException if the request could not be executed due to
-   *     a connectivity problem or timeout. Because networks can
-   *     fail during an exchange, it is possible that the remote server
-   *     accepted the request before the failure
-   * @throws HttpException if the response code is not in range [200..300)
+   *
+   * @throws IllegalStateException if the request could not be executed due to a connectivity problem or timeout. Because networks can
+   *                               fail during an exchange, it is possible that the remote server accepted the request before the failure
+   * @throws MessageException      if there was a problem with authentication or if a error message was parsed from the response.
+   * @throws HttpException         if the response code is not in range [200..300). Consider using {@link #createErrorMessage(HttpException)} to create more relevant messages for the users.
    */
   public WsResponse call(WsRequest request) {
     Preconditions.checkState(!globalMode.isMediumTest(), "No WS call should be made in medium test mode");
@@ -101,12 +103,35 @@ public class ScannerWsClient {
     }
     if (code == HTTP_FORBIDDEN || code == HTTP_BAD_REQUEST) {
       // SONAR-4397 Details are in response content
-      throw MessageException.of(tryParseAsJsonError(response.content()));
+      String jsonMsg = tryParseAsJsonError(response.content());
+      if (jsonMsg != null) {
+        throw MessageException.of(jsonMsg);
+      }
     }
+
+    // if failed, throws an HttpException
     response.failIfNotSuccessful();
   }
 
-  public static String tryParseAsJsonError(String responseContent) {
+  /**
+   * Tries to form a short and relevant error message from the exception, to be displayed in the console.
+   */
+  public static String createErrorMessage(HttpException exception) {
+    String json = tryParseAsJsonError(exception.content());
+    if (json != null) {
+      return json;
+    }
+
+    String msg = "HTTP code " + exception.code();
+    if (isHtml(exception.content())) {
+      return msg;
+    }
+
+    return msg + ": " + StringUtils.left(exception.content(), MAX_ERROR_MSG_LEN);
+  }
+
+  @CheckForNull
+  private static String tryParseAsJsonError(String responseContent) {
     try {
       JsonParser parser = new JsonParser();
       JsonObject obj = parser.parse(responseContent).getAsJsonObject();
@@ -117,7 +142,11 @@ public class ScannerWsClient {
       }
       return Joiner.on(", ").join(errorMessages);
     } catch (Exception e) {
-      return responseContent;
+      return null;
     }
   }
+
+  private static boolean isHtml(String responseContent) {
+    return StringUtils.stripToEmpty(responseContent).startsWith("<!DOCTYPE html>");
+  }
 }
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ReportPublisher.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ReportPublisher.java
index 11cd6a0f63d..59f6c3e1a6c 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ReportPublisher.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ReportPublisher.java
@@ -193,7 +193,7 @@ public class ReportPublisher implements Startable {
     try {
       response = wsClient.call(post).failIfNotSuccessful();
     } catch (HttpException e) {
-      throw MessageException.of(String.format("Failed to upload report - %d: %s", e.code(), ScannerWsClient.tryParseAsJsonError(e.content())));
+      throw MessageException.of(String.format("Failed to upload report - %s", ScannerWsClient.createErrorMessage(e)));
     }
 
     try (InputStream protobuf = response.contentStream()) {
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/DefaultQualityProfileLoader.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/DefaultQualityProfileLoader.java
index 8d68be56d4f..6e5a6f48d61 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/DefaultQualityProfileLoader.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/DefaultQualityProfileLoader.java
@@ -68,9 +68,9 @@ public class DefaultQualityProfileLoader implements QualityProfileLoader {
       return loadAndOverrideIfNeeded(profileName, url);
     } catch (HttpException e) {
       if (e.code() == 404) {
-        throw MessageException.of(errorMsg.get() + ": " + ScannerWsClient.tryParseAsJsonError(e.content()));
+        throw MessageException.of(errorMsg.get() + ": " + ScannerWsClient.createErrorMessage(e));
       }
-      throw new IllegalStateException(errorMsg.get(), e);
+      throw new IllegalStateException(errorMsg.get() + ": " + ScannerWsClient.createErrorMessage(e));
     } catch (MessageException e) {
       throw e;
     } catch (Exception e) {
diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/bootstrap/ScannerWsClientTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/bootstrap/ScannerWsClientTest.java
index b217be5c145..62d2a10b024 100644
--- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/bootstrap/ScannerWsClientTest.java
+++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/bootstrap/ScannerWsClientTest.java
@@ -21,6 +21,7 @@ package org.sonar.scanner.bootstrap;
 
 import java.util.Collections;
 import java.util.List;
+import org.apache.commons.lang.StringUtils;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -29,6 +30,7 @@ import org.sonar.api.utils.MessageException;
 import org.sonar.api.utils.log.LogTester;
 import org.sonar.api.utils.log.LoggerLevel;
 import org.sonarqube.ws.client.GetRequest;
+import org.sonarqube.ws.client.HttpException;
 import org.sonarqube.ws.client.MockWsResponse;
 import org.sonarqube.ws.client.WsClient;
 import org.sonarqube.ws.client.WsRequest;
@@ -49,7 +51,7 @@ public class ScannerWsClientTest {
   WsClient wsClient = mock(WsClient.class, Mockito.RETURNS_DEEP_STUBS);
 
   @Test
-  public void log_and_profile_request_if_debug_level() throws Exception {
+  public void log_and_profile_request_if_debug_level() {
     WsRequest request = newRequest();
     WsResponse response = newResponse().setRequestUrl("https://local/api/issues/search");
     when(wsClient.wsConnector().call(request)).thenReturn(response);
@@ -69,7 +71,25 @@ public class ScannerWsClientTest {
   }
 
   @Test
-  public void fail_if_requires_credentials() throws Exception {
+  public void create_error_msg_from_json() {
+    String content = "{\"errors\":[{\"msg\":\"missing scan permission\"}, {\"msg\":\"missing another permission\"}]}";
+    assertThat(ScannerWsClient.createErrorMessage(new HttpException("url", 400, content))).isEqualTo("missing scan permission, missing another permission");
+  }
+
+  @Test
+  public void create_error_msg_from_html() {
+    String content = "<!DOCTYPE html><html>something</html>";
+    assertThat(ScannerWsClient.createErrorMessage(new HttpException("url", 400, content))).isEqualTo("HTTP code 400");
+  }
+
+  @Test
+  public void create_error_msg_from_long_content() {
+    String content = StringUtils.repeat("mystring", 1000);
+    assertThat(ScannerWsClient.createErrorMessage(new HttpException("url", 400, content))).hasSize(15 + 128);
+  }
+
+  @Test
+  public void fail_if_requires_credentials() {
     expectedException.expect(MessageException.class);
     expectedException
       .expectMessage("Not authorized. Analyzing this project requires to be authenticated. Please provide the values of the properties sonar.login and sonar.password.");
@@ -82,7 +102,7 @@ public class ScannerWsClientTest {
   }
 
   @Test
-  public void fail_if_credentials_are_not_valid() throws Exception {
+  public void fail_if_credentials_are_not_valid() {
     expectedException.expect(MessageException.class);
     expectedException.expectMessage("Not authorized. Please check the properties sonar.login and sonar.password.");
 
@@ -94,7 +114,7 @@ public class ScannerWsClientTest {
   }
 
   @Test
-  public void fail_if_requires_permission() throws Exception {
+  public void fail_if_requires_permission() {
     expectedException.expect(MessageException.class);
     expectedException.expectMessage("missing scan permission, missing another permission");
 
@@ -108,7 +128,7 @@ public class ScannerWsClientTest {
   }
 
   @Test
-  public void fail_if_bad_request() throws Exception {
+  public void fail_if_bad_request() {
     expectedException.expect(MessageException.class);
     expectedException.expectMessage("Boo! bad request! bad!");
 
diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ReportPublisherTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ReportPublisherTest.java
index 43293b246e1..bcccb4a1dd7 100644
--- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ReportPublisherTest.java
+++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ReportPublisherTest.java
@@ -121,7 +121,7 @@ public class ReportPublisherTest {
     when(wsClient.call(any(WsRequest.class))).thenReturn(response);
 
     exception.expect(MessageException.class);
-    exception.expectMessage("Failed to upload report - 404: Organization with key 'MyOrg' does not exist");
+    exception.expectMessage("Failed to upload report - Organization with key 'MyOrg' does not exist");
     underTest.upload(temp.newFile());
   }