SONAR-7484 WS restrict action key suffixes to protobuf, json and text

4 files changed, 65 insertions, 40 deletions

diff --git a/server/sonar-server/src/main/java/org/sonar/server/ws/ServletRequest.java b/server/sonar-server/src/main/java/org/sonar/server/ws/ServletRequest.java
index 73b705ebf84..17473174f89 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/ws/ServletRequest.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/ws/ServletRequest.java
@@ -31,15 +31,17 @@ import org.sonar.api.server.ws.internal.ValidatingRequest;
 import org.sonarqube.ws.MediaTypes;
 
 import static com.google.common.base.Objects.firstNonNull;
+import static org.apache.commons.lang.StringUtils.substringAfterLast;
 
 public class ServletRequest extends ValidatingRequest {
 
   private final HttpServletRequest source;
   private final Map<String, Object> params;
-  private static final Map<String, String> SUPPORTED_FORMATS = ImmutableMap.of(
-    "JSON", MediaTypes.JSON,
-    "PROTOBUF", MediaTypes.PROTOBUF,
-    "TEXT", MediaTypes.TXT);
+
+  static final Map<String, String> SUPPORTED_MEDIA_TYPES_BY_URL_SUFFIX = ImmutableMap.of(
+    "json", MediaTypes.JSON,
+    "protobuf", MediaTypes.PROTOBUF,
+    "text", MediaTypes.TXT);
 
   public ServletRequest(HttpServletRequest source, Map<String, Object> params) {
     this.source = source;
@@ -103,7 +105,7 @@ public class ServletRequest extends ValidatingRequest {
 
   @CheckForNull
   private static String mediaTypeFromUrl(String url) {
-    String mediaType = url.substring(url.lastIndexOf('.') + 1);
-    return SUPPORTED_FORMATS.get(mediaType.toUpperCase(Locale.ENGLISH));
+    String formatSuffix = substringAfterLast(url, ".");
+    return SUPPORTED_MEDIA_TYPES_BY_URL_SUFFIX.get(formatSuffix.toLowerCase(Locale.ENGLISH));
   }
 }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/ws/WebServiceEngine.java b/server/sonar-server/src/main/java/org/sonar/server/ws/WebServiceEngine.java
index 274955d3e60..fa1100c9c75 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/ws/WebServiceEngine.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/ws/WebServiceEngine.java
@@ -22,6 +22,8 @@ package org.sonar.server.ws;
 import java.io.OutputStreamWriter;
 import java.nio.charset.StandardCharsets;
 import java.util.List;
+import java.util.Locale;
+import javax.annotation.Nullable;
 import org.apache.commons.lang.StringUtils;
 import org.picocontainer.Startable;
 import org.sonar.api.i18n.I18n;
@@ -40,8 +42,12 @@ import org.sonar.server.exceptions.ServerException;
 import org.sonar.server.user.UserSession;
 import org.sonarqube.ws.MediaTypes;
 
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Strings.isNullOrEmpty;
 import static java.lang.String.format;
+import static org.apache.commons.lang.StringUtils.substringAfterLast;
 import static org.sonar.server.ws.RequestVerifier.verifyRequest;
+import static org.sonar.server.ws.ServletRequest.SUPPORTED_MEDIA_TYPES_BY_URL_SUFFIX;
 
 /**
  * @since 4.2
@@ -83,19 +89,20 @@ public class WebServiceEngine implements LocalConnector, Startable {
   @Override
   public LocalResponse call(LocalRequest request) {
     String controller = StringUtils.substringBeforeLast(request.getPath(), "/");
-    String action = StringUtils.substringAfterLast(request.getPath(), "/");
+    String action = substringAfterLast(request.getPath(), "/");
     DefaultLocalResponse localResponse = new DefaultLocalResponse();
-    execute(new LocalRequestAdapter(request), localResponse, controller, action);
+    execute(new LocalRequestAdapter(request), localResponse, controller, action, null);
     return localResponse;
   }
 
-  public void execute(Request request, Response response, String controllerPath, String actionKey) {
+  public void execute(Request request, Response response, String controllerPath, String actionKey, @Nullable String actionExtension) {
     try {
       WebService.Action action = getAction(controllerPath, actionKey);
       if (request instanceof ValidatingRequest) {
         ((ValidatingRequest) request).setAction(action);
         ((ValidatingRequest) request).setLocalConnector(this);
       }
+      checkActionExtension(actionExtension);
       verifyRequest(action, request);
       action.handler().handle(request, response);
     } catch (IllegalArgumentException e) {
@@ -111,16 +118,13 @@ public class WebServiceEngine implements LocalConnector, Startable {
   }
 
   private WebService.Action getAction(String controllerPath, String actionKey) {
-    String actionKeyWithoutFormatSuffix = actionKey.contains(".") ?
-      actionKey.substring(0, actionKey.lastIndexOf('.'))
-      : actionKey;
     WebService.Controller controller = context.controller(controllerPath);
     if (controller == null) {
       throw new BadRequestException(format("Unknown web service: %s", controllerPath));
     }
-    WebService.Action action = controller.action(actionKeyWithoutFormatSuffix);
+    WebService.Action action = controller.action(actionKey);
     if (action == null) {
-      throw new BadRequestException(format("Unknown action: %s/%s", controllerPath, actionKeyWithoutFormatSuffix));
+      throw new BadRequestException(format("Unknown action: %s/%s", controllerPath, actionKey));
     }
     return action;
   }
@@ -144,4 +148,12 @@ public class WebServiceEngine implements LocalConnector, Startable {
       json.close();
     }
   }
+
+  private static void checkActionExtension(@Nullable String actionExtension) {
+    if (isNullOrEmpty(actionExtension)) {
+      return;
+    }
+    checkArgument(SUPPORTED_MEDIA_TYPES_BY_URL_SUFFIX.get(actionExtension.toLowerCase(Locale.ENGLISH)) != null, "Unknown action extension: %s", actionExtension);
+  }
+
 }
diff --git a/server/sonar-server/src/test/java/org/sonar/server/ws/WebServiceEngineTest.java b/server/sonar-server/src/test/java/org/sonar/server/ws/WebServiceEngineTest.java
index a78cf7ce4f4..b586eb8c961 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/ws/WebServiceEngineTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/ws/WebServiceEngineTest.java
@@ -51,47 +51,59 @@ public class WebServiceEngineTest {
   @Rule
   public UserSessionRule userSessionRule = UserSessionRule.standalone();
   I18n i18n = mock(I18n.class);
-  WebServiceEngine engine = new WebServiceEngine(new WebService[] {new SystemWs()}, i18n, userSessionRule);
+
+  WebServiceEngine underTest = new WebServiceEngine(new WebService[] {new SystemWs()}, i18n, userSessionRule);
 
   @Before
   public void start() {
-    engine.start();
+    underTest.start();
   }
 
   @After
   public void stop() {
-    engine.stop();
+    underTest.stop();
   }
 
   @Test
   public void load_ws_definitions_at_startup() {
-    assertThat(engine.controllers()).hasSize(1);
-    assertThat(engine.controllers().get(0).path()).isEqualTo("api/system");
+    assertThat(underTest.controllers()).hasSize(1);
+    assertThat(underTest.controllers().get(0).path()).isEqualTo("api/system");
   }
 
   @Test
   public void execute_request() {
     ValidatingRequest request = new SimpleRequest("GET");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "health");
+    underTest.execute(request, response, "api/system", "health", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("good");
   }
 
   @Test
-  public void execute_request_with_format_type() {
+  public void execute_request_with_action_suffix() {
     ValidatingRequest request = new SimpleRequest("GET");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "health.protobuf");
+    underTest.execute(request, response, "api/system", "health", "PROTOBUF");
 
     assertThat(response.stream().outputAsString()).isEqualTo("good");
   }
 
   @Test
+  public void bad_request_if_action_suffix_is_not_supported() {
+    ValidatingRequest request = new SimpleRequest("GET");
+    ServletResponse response = new ServletResponse();
+    underTest.execute(request, response, "api/system", "health", "bat");
+
+    assertThat(response.stream().httpStatus()).isEqualTo(400);
+    assertThat(response.stream().mediaType()).isEqualTo(MediaTypes.JSON);
+    assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[{\"msg\":\"Unknown action extension: bat\"}]}");
+  }
+
+  @Test
   public void no_content() {
     ValidatingRequest request = new SimpleRequest("GET");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "alive");
+    underTest.execute(request, response, "api/system", "alive", null);
 
     assertThat(response.stream().outputAsString()).isEmpty();
   }
@@ -100,7 +112,7 @@ public class WebServiceEngineTest {
   public void bad_controller() {
     ValidatingRequest request = new SimpleRequest("GET");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/xxx", "health");
+    underTest.execute(request, response, "api/xxx", "health", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[{\"msg\":\"Unknown web service: api/xxx\"}]}");
   }
@@ -109,7 +121,7 @@ public class WebServiceEngineTest {
   public void bad_action() {
     ValidatingRequest request = new SimpleRequest("GET");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "xxx");
+    underTest.execute(request, response, "api/system", "xxx", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[{\"msg\":\"Unknown action: api/system/xxx\"}]}");
   }
@@ -118,7 +130,7 @@ public class WebServiceEngineTest {
   public void method_get_not_allowed() {
     ValidatingRequest request = new SimpleRequest("GET");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "ping");
+    underTest.execute(request, response, "api/system", "ping", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[{\"msg\":\"HTTP method POST is required\"}]}");
   }
@@ -127,7 +139,7 @@ public class WebServiceEngineTest {
   public void method_post_required() {
     ValidatingRequest request = new SimpleRequest("POST");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "ping");
+    underTest.execute(request, response, "api/system", "ping", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("pong");
   }
@@ -136,7 +148,7 @@ public class WebServiceEngineTest {
   public void unknown_parameter_is_set() {
     ValidatingRequest request = new SimpleRequest("GET").setParam("unknown", "Unknown");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "fail_with_undeclared_parameter");
+    underTest.execute(request, response, "api/system", "fail_with_undeclared_parameter", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[{\"msg\":\"BUG - parameter 'unknown' is undefined for action 'fail_with_undeclared_parameter'\"}]}");
   }
@@ -145,7 +157,7 @@ public class WebServiceEngineTest {
   public void required_parameter_is_not_set() {
     ValidatingRequest request = new SimpleRequest("GET");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "print");
+    underTest.execute(request, response, "api/system", "print", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[{\"msg\":\"The 'message' parameter is missing\"}]}");
   }
@@ -154,7 +166,7 @@ public class WebServiceEngineTest {
   public void optional_parameter_is_not_set() {
     ValidatingRequest request = new SimpleRequest("GET").setParam("message", "Hello World");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "print");
+    underTest.execute(request, response, "api/system", "print", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("Hello World by -");
   }
@@ -165,7 +177,7 @@ public class WebServiceEngineTest {
       .setParam("message", "Hello World")
       .setParam("author", "Marcel");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "print");
+    underTest.execute(request, response, "api/system", "print", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("Hello World by Marcel");
   }
@@ -176,7 +188,7 @@ public class WebServiceEngineTest {
       .setParam("message", "Hello World")
       .setParam("format", "json");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "print");
+    underTest.execute(request, response, "api/system", "print", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("Hello World by -");
   }
@@ -187,7 +199,7 @@ public class WebServiceEngineTest {
       .setParam("message", "Hello World")
       .setParam("format", "html");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "print");
+    underTest.execute(request, response, "api/system", "print", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[{\"msg\":\"Value of parameter 'format' (html) must be one of: [json, xml]\"}]}");
   }
@@ -196,7 +208,7 @@ public class WebServiceEngineTest {
   public void internal_error() {
     ValidatingRequest request = new SimpleRequest("GET");
     ServletResponse response = new ServletResponse();
-    engine.execute(request, response, "api/system", "fail");
+    underTest.execute(request, response, "api/system", "fail", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[{\"msg\":\"Unexpected\"}]}");
     assertThat(response.stream().httpStatus()).isEqualTo(500);
@@ -210,11 +222,10 @@ public class WebServiceEngineTest {
     ServletResponse response = new ServletResponse();
     when(i18n.message(Locale.ENGLISH, "bad.request.reason", "bad.request.reason", 0)).thenReturn("reason #0");
 
-    engine.execute(request, response, "api/system", "fail_with_i18n_message");
+    underTest.execute(request, response, "api/system", "fail_with_i18n_message", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo(
-      "{\"errors\":[{\"msg\":\"reason #0\"}]}"
-      );
+      "{\"errors\":[{\"msg\":\"reason #0\"}]}");
     assertThat(response.stream().httpStatus()).isEqualTo(400);
     assertThat(response.stream().mediaType()).isEqualTo(MediaTypes.JSON);
   }
@@ -224,7 +235,7 @@ public class WebServiceEngineTest {
     ValidatingRequest request = new SimpleRequest("GET").setParam("count", "3");
     ServletResponse response = new ServletResponse();
 
-    engine.execute(request, response, "api/system", "fail_with_multiple_messages");
+    underTest.execute(request, response, "api/system", "fail_with_multiple_messages", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":["
       + "{\"msg\":\"Bad request reason #0\"},"
@@ -245,7 +256,7 @@ public class WebServiceEngineTest {
     when(i18n.message(Locale.ENGLISH, "bad.request.reason", "bad.request.reason", 1)).thenReturn("reason #1");
     when(i18n.message(Locale.ENGLISH, "bad.request.reason", "bad.request.reason", 2)).thenReturn("reason #2");
 
-    engine.execute(request, response, "api/system", "fail_with_multiple_i18n_messages");
+    underTest.execute(request, response, "api/system", "fail_with_multiple_i18n_messages", null);
 
     assertThat(response.stream().outputAsString()).isEqualTo("{\"errors\":[" +
       "{\"msg\":\"reason #0\"}," +
diff --git a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/java_ws_controller.rb b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/java_ws_controller.rb
index 088c13c959d..d9a159be64b 100644
--- a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/java_ws_controller.rb
+++ b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/java_ws_controller.rb
@@ -31,7 +31,7 @@ class Api::JavaWsController < Api::ApiController
     ws_request = Java::OrgSonarServerWs::ServletRequest.new(servlet_request, params.to_java)
     ws_response = Java::OrgSonarServerWs::ServletResponse.new()
     engine = Java::OrgSonarServerPlatform::Platform.component(Java::OrgSonarServerWs::WebServiceEngine.java_class)
-    engine.execute(ws_request, ws_response, params[:wspath], params[:wsaction])
+    engine.execute(ws_request, ws_response, params[:wspath], params[:wsaction], params[:responseFormat])
 
     ws_response.getHeaderNames().to_a.each do |name|
       response.header[name] = ws_response.getHeader(name)