From 1172095459cf80678e3532c7b7862aeaced58785 Mon Sep 17 00:00:00 2001 From: Teryk Bellahsene Date: Wed, 18 May 2016 16:48:44 +0200 Subject: [PATCH] SONAR-7484 WS restrict action key suffixes to protobuf, json and text --- .../org/sonar/server/ws/ServletRequest.java | 14 +++-- .../org/sonar/server/ws/WebServiceEngine.java | 28 ++++++--- .../sonar/server/ws/WebServiceEngineTest.java | 61 +++++++++++-------- .../app/controllers/api/java_ws_controller.rb | 2 +- 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 params; - private static final Map SUPPORTED_FORMATS = ImmutableMap.of( - "JSON", MediaTypes.JSON, - "PROTOBUF", MediaTypes.PROTOBUF, - "TEXT", MediaTypes.TXT); + + static final Map SUPPORTED_MEDIA_TYPES_BY_URL_SUFFIX = ImmutableMap.of( + "json", MediaTypes.JSON, + "protobuf", MediaTypes.PROTOBUF, + "text", MediaTypes.TXT); public ServletRequest(HttpServletRequest source, Map 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) -- 2.39.5