This is an automated email from the ASF dual-hosted git repository. mmiller pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/master by this push: new 61242a7 ACCUMULO-4677 Sanitize PathParam values in Monitor (#289) 61242a7 is described below commit 61242a780a7512339616fa165e5d7a61564479bf Author: Kyle <gli...@users.noreply.github.com> AuthorDate: Mon Dec 4 14:58:44 2017 -0500 ACCUMULO-4677 Sanitize PathParam values in Monitor (#289) --- assemble/pom.xml | 28 ++++ assemble/src/main/assemblies/component.xml | 7 + pom.xml | 49 +++++++ server/monitor/pom.xml | 4 + .../java/org/apache/accumulo/monitor/Monitor.java | 6 +- .../monitor/rest/problems/ProblemsResource.java | 15 +- .../monitor/rest/tables/TablesResource.java | 30 +++- .../monitor/rest/trace/TracesResource.java | 26 ++-- .../rest/tservers/TabletServerResource.java | 25 ++-- .../accumulo/monitor/util/ParameterValidator.java | 34 +++++ .../org/apache/accumulo/monitor/view/WebViews.java | 57 +++++--- .../monitor/util/ParameterValidatorTest.java | 68 +++++++++ test/pom.xml | 66 +++++++++ .../apache/accumulo/test/monitor/WebViewsIT.java | 157 +++++++++++++++++++++ 14 files changed, 514 insertions(+), 58 deletions(-) diff --git a/assemble/pom.xml b/assemble/pom.xml index 2cf4d7a..4167de3 100644 --- a/assemble/pom.xml +++ b/assemble/pom.xml @@ -33,6 +33,10 @@ <optional>true</optional> </dependency> <dependency> + <groupId>com.fasterxml</groupId> + <artifactId>classmate</artifactId> + </dependency> + <dependency> <groupId>com.fasterxml.jackson.core</groupId> <artifactId>jackson-annotations</artifactId> </dependency> @@ -80,6 +84,10 @@ <artifactId>javax.annotation-api</artifactId> </dependency> <dependency> + <groupId>javax.el</groupId> + <artifactId>javax.el-api</artifactId> + </dependency> + <dependency> <groupId>javax.servlet</groupId> <artifactId>javax.servlet-api</artifactId> <optional>true</optional> @@ -285,6 +293,10 @@ </dependency> <dependency> <groupId>org.glassfish.jersey.ext</groupId> + <artifactId>jersey-bean-validation</artifactId> + </dependency> + <dependency> + <groupId>org.glassfish.jersey.ext</groupId> <artifactId>jersey-entity-filtering</artifactId> </dependency> <dependency> @@ -304,10 +316,26 @@ <artifactId>jersey-media-json-jackson</artifactId> </dependency> <dependency> + <groupId>org.glassfish.web</groupId> + <artifactId>el-impl</artifactId> + </dependency> + <dependency> + <groupId>org.glassfish.web</groupId> + <artifactId>javax.el</artifactId> + </dependency> + <dependency> + <groupId>org.hibernate</groupId> + <artifactId>hibernate-validator</artifactId> + </dependency> + <dependency> <groupId>org.javassist</groupId> <artifactId>javassist</artifactId> </dependency> <dependency> + <groupId>org.jboss.logging</groupId> + <artifactId>jboss-logging</artifactId> + </dependency> + <dependency> <groupId>org.slf4j</groupId> <artifactId>slf4j-api</artifactId> <optional>true</optional> diff --git a/assemble/src/main/assemblies/component.xml b/assemble/src/main/assemblies/component.xml index 470b5f7..405b7b3 100644 --- a/assemble/src/main/assemblies/component.xml +++ b/assemble/src/main/assemblies/component.xml @@ -52,6 +52,7 @@ <include>org.slf4j:slf4j-api</include> <include>org.slf4j:slf4j-log4j12</include> <!-- Jersey/Jackson-based webservice --> + <include>com.fasterxml:classmate</include> <include>com.fasterxml.jackson.core:jackson-annotations</include> <include>com.fasterxml.jackson.core:jackson-core</include> <include>com.fasterxml.jackson.core:jackson-databind</include> @@ -59,6 +60,7 @@ <include>com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider</include> <include>com.fasterxml.jackson.module:jackson-module-jaxb-annotations</include> <include>javax.annotation:javax.annotation-api</include> + <include>javax.el:javax.el-api</include> <include>javax.validation:validation-api</include> <include>javax.ws.rs:javax.ws.rs-api</include> <include>org.freemarker:freemarker</include> @@ -75,12 +77,17 @@ <include>org.glassfish.jersey.core:jersey-client</include> <include>org.glassfish.jersey.core:jersey-common</include> <include>org.glassfish.jersey.core:jersey-server</include> + <include>org.glassfish.jersey.ext:jersey-bean-validation</include> <include>org.glassfish.jersey.ext:jersey-entity-filtering</include> <include>org.glassfish.jersey.ext:jersey-mvc-freemarker</include> <include>org.glassfish.jersey.ext:jersey-mvc</include> <include>org.glassfish.jersey.media:jersey-media-jaxb</include> <include>org.glassfish.jersey.media:jersey-media-json-jackson</include> + <include>org.glassfish.web:javax.el</include> + <include>org.glassfish.web:el-impl</include> + <include>org.hibernate:hibernate-validator</include> <include>org.javassist:javassist</include> + <include>org.jboss.logging:jboss-logging</include> <include>log4j:log4j</include> </includes> </dependencySet> diff --git a/pom.xml b/pom.xml index 49ce9d8..4cce012 100644 --- a/pom.xml +++ b/pom.xml @@ -133,6 +133,7 @@ <httpclient.version>4.3.1</httpclient.version> <it.failIfNoSpecifiedTests>false</it.failIfNoSpecifiedTests> <jackson.version>2.9.0</jackson.version> + <javax.el.version>2.2.4</javax.el.version> <jersey.version>2.25.1</jersey.version> <jetty.version>9.3.21.v20170918</jetty.version> <maven.compiler.source>1.8</maven.compiler.source> @@ -163,6 +164,11 @@ <version>1.72</version> </dependency> <dependency> + <groupId>com.fasterxml</groupId> + <artifactId>classmate</artifactId> + <version>1.0.0</version> + </dependency> + <dependency> <groupId>com.fasterxml.jackson.core</groupId> <artifactId>jackson-annotations</artifactId> <version>${jackson.version}</version> @@ -260,6 +266,11 @@ <version>1.2</version> </dependency> <dependency> + <groupId>javax.el</groupId> + <artifactId>javax.el-api</artifactId> + <version>${javax.el.version}</version> + </dependency> + <dependency> <groupId>javax.servlet</groupId> <artifactId>javax.servlet-api</artifactId> <version>${servlet.api.version}</version> @@ -635,6 +646,11 @@ </dependency> <dependency> <groupId>org.glassfish.jersey.ext</groupId> + <artifactId>jersey-bean-validation</artifactId> + <version>${jersey.version}</version> + </dependency> + <dependency> + <groupId>org.glassfish.jersey.ext</groupId> <artifactId>jersey-entity-filtering</artifactId> <version>${jersey.version}</version> </dependency> @@ -659,16 +675,46 @@ <version>${jersey.version}</version> </dependency> <dependency> + <groupId>org.glassfish.jersey.test-framework</groupId> + <artifactId>jersey-test-framework-core</artifactId> + <version>${jersey.version}</version> + </dependency> + <dependency> + <groupId>org.glassfish.jersey.test-framework.providers</groupId> + <artifactId>jersey-test-framework-provider-grizzly2</artifactId> + <version>${jersey.version}</version> + </dependency> + <dependency> + <groupId>org.glassfish.web</groupId> + <artifactId>el-impl</artifactId> + <version>2.2</version> + </dependency> + <dependency> + <groupId>org.glassfish.web</groupId> + <artifactId>javax.el</artifactId> + <version>2.2.4</version> + </dependency> + <dependency> <groupId>org.hamcrest</groupId> <artifactId>hamcrest-core</artifactId> <version>1.3</version> </dependency> <dependency> + <groupId>org.hibernate</groupId> + <artifactId>hibernate-validator</artifactId> + <version>5.1.3.Final</version> + </dependency> + <dependency> <groupId>org.javassist</groupId> <artifactId>javassist</artifactId> <version>3.18.1-GA</version> </dependency> <dependency> + <groupId>org.jboss.logging</groupId> + <artifactId>jboss-logging</artifactId> + <version>3.1.3.GA</version> + </dependency> + <dependency> <groupId>org.powermock</groupId> <artifactId>powermock-api-easymock</artifactId> <version>${powermock.version}</version> @@ -1040,11 +1086,14 @@ <unusedDeclaredDependency>org.apache.hadoop:hadoop-minicluster:jar:${hadoop.version}</unusedDeclaredDependency> <unusedDeclaredDependency>org.apache.httpcomponents:httpclient:jar:${httpclient.version}</unusedDeclaredDependency> <unusedDeclaredDependency>org.glassfish.jersey.containers:jersey-container-jetty-http:jar:${jersey.version}</unusedDeclaredDependency> + <unusedDeclaredDependency>org.glassfish.jersey.ext:jersey-bean-validation:jar:${jersey.version}</unusedDeclaredDependency> + <unusedDeclaredDependency>org.glassfish.jersey.test-framework.providers:jersey-test-framework-provider-grizzly2:jar:${jersey.version}</unusedDeclaredDependency> <unusedDeclaredDependency>org.powermock:powermock-api-easymock:jar:${powermock.version}</unusedDeclaredDependency> <unusedDeclaredDependency>org.slf4j:slf4j-log4j12:jar:${slf4j.version}</unusedDeclaredDependency> <unusedDeclaredDependency>org.apache.httpcomponents:httpclient:jar:${httpclient.version}</unusedDeclaredDependency> <unusedDeclaredDependency>junit:junit:jar:4.12</unusedDeclaredDependency> <unusedDeclaredDependency>javax.servlet:javax.servlet-api:jar:${servlet.api.version}</unusedDeclaredDependency> + <unusedDeclaredDependency>javax.el:javax.el-api:jar:${javax.el.version}</unusedDeclaredDependency> </ignoredUnusedDeclaredDependencies> </configuration> </execution> diff --git a/server/monitor/pom.xml b/server/monitor/pom.xml index 8229d57..f22a48b 100644 --- a/server/monitor/pom.xml +++ b/server/monitor/pom.xml @@ -45,6 +45,10 @@ <artifactId>javax.servlet-api</artifactId> </dependency> <dependency> + <groupId>javax.validation</groupId> + <artifactId>validation-api</artifactId> + </dependency> + <dependency> <groupId>javax.ws.rs</groupId> <artifactId>javax.ws.rs-api</artifactId> </dependency> diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java index 68317b3..868d328 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java @@ -90,6 +90,7 @@ import org.eclipse.jetty.util.resource.Resource; import org.glassfish.jersey.jackson.JacksonFeature; import org.glassfish.jersey.logging.LoggingFeature; import org.glassfish.jersey.server.ResourceConfig; +import org.glassfish.jersey.server.ServerProperties; import org.glassfish.jersey.server.mvc.MvcFeature; import org.glassfish.jersey.server.mvc.freemarker.FreemarkerMvcFeature; import org.glassfish.jersey.servlet.ServletContainer; @@ -547,13 +548,14 @@ public class Monitor implements HighlyAvailableService { private ServletHolder getViewServlet() { final ResourceConfig rc = new ResourceConfig().packages("org.apache.accumulo.monitor.view") .register(new LoggingFeature(java.util.logging.Logger.getLogger(this.getClass().getSimpleName()))).register(FreemarkerMvcFeature.class) - .property(MvcFeature.TEMPLATE_BASE_PATH, "/templates"); + .property(MvcFeature.TEMPLATE_BASE_PATH, "/templates").property(ServerProperties.BV_SEND_ERROR_IN_RESPONSE, true); return new ServletHolder(new ServletContainer(rc)); } private ServletHolder getRestServlet() { final ResourceConfig rc = new ResourceConfig().packages("org.apache.accumulo.monitor.rest") - .register(new LoggingFeature(java.util.logging.Logger.getLogger(this.getClass().getSimpleName()))).register(JacksonFeature.class); + .register(new LoggingFeature(java.util.logging.Logger.getLogger(this.getClass().getSimpleName()))).register(JacksonFeature.class) + .property(ServerProperties.BV_SEND_ERROR_IN_RESPONSE, true); return new ServletHolder(new ServletContainer(rc)); } diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/problems/ProblemsResource.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/problems/ProblemsResource.java index 8c7fed7..f85f036 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/problems/ProblemsResource.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/problems/ProblemsResource.java @@ -16,11 +16,16 @@ */ package org.apache.accumulo.monitor.rest.problems; +import static org.apache.accumulo.monitor.util.ParameterValidator.ALPHA_NUM_REGEX; +import static org.apache.accumulo.monitor.util.ParameterValidator.RESOURCE_REGEX; + import java.util.ArrayList; import java.util.Iterator; import java.util.Map; import java.util.Map.Entry; +import javax.validation.constraints.NotNull; +import javax.validation.constraints.Pattern; import javax.ws.rs.Consumes; import javax.ws.rs.GET; import javax.ws.rs.POST; @@ -36,6 +41,7 @@ import org.apache.accumulo.server.client.HdfsZooInstance; import org.apache.accumulo.server.problems.ProblemReport; import org.apache.accumulo.server.problems.ProblemReports; import org.apache.accumulo.server.problems.ProblemType; +import org.apache.commons.lang.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -93,12 +99,12 @@ public class ProblemsResource { @POST @Consumes(MediaType.TEXT_PLAIN) @Path("summary") - public void clearTableProblems(@QueryParam("s") String tableID) { + public void clearTableProblems(@QueryParam("s") @NotNull @Pattern(regexp = ALPHA_NUM_REGEX) String tableID) { Logger log = LoggerFactory.getLogger(Monitor.class); try { ProblemReports.getInstance(Monitor.getContext()).deleteProblemReports(Table.ID.of(tableID)); } catch (Exception e) { - log.error("Failed to delete problem reports for table " + tableID, e); + log.error("Failed to delete problem reports for table " + (StringUtils.isEmpty(tableID) ? StringUtils.EMPTY : tableID), e); } } @@ -144,12 +150,13 @@ public class ProblemsResource { @POST @Consumes(MediaType.TEXT_PLAIN) @Path("details") - public void clearDetailsProblems(@QueryParam("table") String tableID, @QueryParam("resource") String resource, @QueryParam("ptype") String ptype) { + public void clearDetailsProblems(@QueryParam("table") @NotNull @Pattern(regexp = ALPHA_NUM_REGEX) String tableID, @QueryParam("resource") @NotNull @Pattern( + regexp = RESOURCE_REGEX) String resource, @QueryParam("ptype") @NotNull @Pattern(regexp = ALPHA_NUM_REGEX) String ptype) { Logger log = LoggerFactory.getLogger(Monitor.class); try { ProblemReports.getInstance(Monitor.getContext()).deleteProblemReport(Table.ID.of(tableID), ProblemType.valueOf(ptype), resource); } catch (Exception e) { - log.error("Failed to delete problem reports for table " + tableID, e); + log.error("Failed to delete problem reports for table " + (StringUtils.isBlank(tableID) ? "" : tableID), e); } } diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tables/TablesResource.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tables/TablesResource.java index 8031931..41d3825 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tables/TablesResource.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tables/TablesResource.java @@ -16,6 +16,11 @@ */ package org.apache.accumulo.monitor.rest.tables; +import static org.apache.accumulo.monitor.util.ParameterValidator.ALPHA_NUM_REGEX; +import static org.apache.accumulo.monitor.util.ParameterValidator.NAMESPACE_LIST_REGEX; +import static org.apache.accumulo.monitor.util.ParameterValidator.NAMESPACE_REGEX; + +import java.io.UnsupportedEncodingException; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -26,6 +31,8 @@ import java.util.TreeMap; import java.util.TreeSet; import java.util.stream.Collectors; +import javax.validation.constraints.NotNull; +import javax.validation.constraints.Pattern; import javax.ws.rs.GET; import javax.ws.rs.Path; import javax.ws.rs.PathParam; @@ -51,6 +58,7 @@ import org.apache.accumulo.server.master.state.MetaDataTableScanner; import org.apache.accumulo.server.master.state.TabletLocationState; import org.apache.accumulo.server.tables.TableManager; import org.apache.accumulo.server.util.TableInfoUtil; +import org.apache.commons.lang.StringUtils; import org.apache.hadoop.io.Text; /** @@ -65,6 +73,7 @@ import org.apache.hadoop.io.Text; public class TablesResource { private static final TabletServerStatus NO_STATUS = new TabletServerStatus(); + private static final java.util.regex.Pattern COMMA = java.util.regex.Pattern.compile(","); /** * Generates a table list based on the namespace @@ -81,9 +90,11 @@ public class TablesResource { /* * Add the tables that have the selected namespace Asterisk = All namespaces Hyphen = Default namespace */ - for (String key : namespaces.keySet()) { - if (namespace.equals("*") || namespace.equals(key) || (key.isEmpty() && namespace.equals("-"))) { - tableNamespace.addTable(new TableNamespace(key)); + if (null != namespace) { + for (String key : namespaces.keySet()) { + if (namespace.equals("*") || namespace.equals(key) || (key.isEmpty() && namespace.equals("-"))) { + tableNamespace.addTable(new TableNamespace(key)); + } } } @@ -161,7 +172,7 @@ public class TablesResource { */ @GET @Path("namespace/{namespace}") - public TablesList getTable(@PathParam("namespace") String namespace) { + public TablesList getTable(@PathParam("namespace") @NotNull @Pattern(regexp = NAMESPACE_REGEX) String namespace) throws UnsupportedEncodingException { return generateTables(namespace); } @@ -174,14 +185,15 @@ public class TablesResource { */ @GET @Path("namespaces/{namespaces}") - public TablesList getTableWithNamespace(@PathParam("namespaces") String namespaceList) { + public TablesList getTableWithNamespace(@PathParam("namespaces") @NotNull @Pattern(regexp = NAMESPACE_LIST_REGEX) String namespaceList) + throws UnsupportedEncodingException { SortedMap<String,Namespace.ID> namespaces = Namespaces.getNameToIdMap(Monitor.getContext().getInstance()); TablesList tableNamespace = new TablesList(); /* * Add the tables that have the selected namespace Asterisk = All namespaces Hyphen = Default namespace */ - for (String namespace : namespaceList.split(",")) { + for (String namespace : COMMA.split(namespaceList)) { for (String key : namespaces.keySet()) { if (namespace.equals("*") || namespace.equals(key) || (key.isEmpty() && namespace.equals("-"))) { tableNamespace.addTable(new TableNamespace(key)); @@ -201,12 +213,16 @@ public class TablesResource { */ @Path("{tableId}") @GET - public TabletServers getParticipatingTabletServers(@PathParam("tableId") String tableIdStr) throws Exception { + public TabletServers getParticipatingTabletServers(@PathParam("tableId") @NotNull @Pattern(regexp = ALPHA_NUM_REGEX) String tableIdStr) throws Exception { Instance instance = Monitor.getContext().getInstance(); Table.ID tableId = Table.ID.of(tableIdStr); TabletServers tabletServers = new TabletServers(Monitor.getMmi().tServerInfo.size()); + if (StringUtils.isBlank(tableIdStr)) { + return tabletServers; + } + TreeSet<String> locs = new TreeSet<>(); if (RootTable.ID.equals(tableId)) { locs.add(instance.getRootTabletLocation()); diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java index 0084427..a73623d 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/TracesResource.java @@ -18,6 +18,7 @@ package org.apache.accumulo.monitor.rest.trace; import static java.lang.Math.min; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.accumulo.monitor.util.ParameterValidator.ALPHA_NUM_REGEX; import java.io.IOException; import java.security.PrivilegedAction; @@ -28,6 +29,10 @@ import java.util.Map.Entry; import java.util.Set; import java.util.TreeMap; +import javax.validation.constraints.Max; +import javax.validation.constraints.Min; +import javax.validation.constraints.NotNull; +import javax.validation.constraints.Pattern; import javax.ws.rs.DefaultValue; import javax.ws.rs.GET; import javax.ws.rs.Path; @@ -59,6 +64,7 @@ import org.apache.accumulo.tracer.TraceDump; import org.apache.accumulo.tracer.TraceFormatter; import org.apache.accumulo.tracer.thrift.Annotation; import org.apache.accumulo.tracer.thrift.RemoteSpan; +import org.apache.commons.lang.StringUtils; import org.apache.hadoop.io.Text; import org.apache.hadoop.security.UserGroupInformation; @@ -77,12 +83,12 @@ public class TracesResource { * Generates a trace summary * * @param minutes - * Range of minutes to filter traces + * Range of minutes to filter traces Min of 0 minutes, Max of 30 days * @return Trace summary in specified range */ @Path("summary/{minutes}") @GET - public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") int minutes) throws Exception { + public RecentTracesList getTraces(@DefaultValue("10") @PathParam("minutes") @NotNull @Min(0) @Max(2592000) int minutes) throws Exception { RecentTracesList recentTraces = new RecentTracesList(); @@ -122,12 +128,13 @@ public class TracesResource { * @param type * Type of the trace * @param minutes - * Range of minutes + * Range of minutes, Min of 0 and Max 0f 30 days * @return List of traces filtered by type and range */ @Path("listType/{type}/{minutes}") @GET - public TraceType getTracesType(@PathParam("type") String type, @PathParam("minutes") int minutes) throws Exception { + public TraceType getTracesType(@PathParam("type") @NotNull @Pattern(regexp = ALPHA_NUM_REGEX) final String type, + @PathParam("minutes") @Min(0) @Max(2592000) int minutes) throws Exception { TraceType typeTraces = new TraceType(type); @@ -175,12 +182,9 @@ public class TracesResource { */ @Path("show/{id}") @GET - public TraceList getTracesType(@PathParam("id") String id) throws Exception { - TraceList traces = new TraceList(id); + public TraceList getTracesType(@PathParam("id") @NotNull @Pattern(regexp = ALPHA_NUM_REGEX) String id) throws Exception { - if (id == null) { - return null; - } + TraceList traces = new TraceList(id); Pair<Scanner,UserGroupInformation> entry = getScanner(); final Scanner scanner = entry.getFirst(); @@ -258,8 +262,8 @@ public class TracesResource { long startTime = endTime - millisSince; String startHexTime = Long.toHexString(startTime), endHexTime = Long.toHexString(endTime); - while (startHexTime.length() < endHexTime.length()) { - startHexTime = "0" + startHexTime; + if (startHexTime.length() < endHexTime.length()) { + StringUtils.leftPad(startHexTime, endHexTime.length(), '0'); } return new Range(new Text("start:" + startHexTime), new Text("start:" + endHexTime)); diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tservers/TabletServerResource.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tservers/TabletServerResource.java index d96e40d..d966cff 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tservers/TabletServerResource.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/rest/tservers/TabletServerResource.java @@ -16,12 +16,16 @@ */ package org.apache.accumulo.monitor.rest.tservers; +import static org.apache.accumulo.monitor.util.ParameterValidator.SERVER_REGEX; + import java.lang.management.ManagementFactory; import java.security.MessageDigest; import java.util.ArrayList; import java.util.Base64; import java.util.List; +import javax.validation.constraints.NotNull; +import javax.validation.constraints.Pattern; import javax.ws.rs.Consumes; import javax.ws.rs.GET; import javax.ws.rs.POST; @@ -101,7 +105,7 @@ public class TabletServerResource { */ @POST @Consumes(MediaType.TEXT_PLAIN) - public void clearDeadServer(@QueryParam("server") String server) throws Exception { + public void clearDeadServer(@QueryParam("server") @NotNull @Pattern(regexp = SERVER_REGEX) String server) throws Exception { DeadServerList obit = new DeadServerList(ZooUtil.getRoot(Monitor.getContext().getInstance()) + Constants.ZDEADTSERVERS); obit.delete(server); } @@ -140,28 +144,23 @@ public class TabletServerResource { /** * Generates details for the selected tserver * - * @param tserverAddr + * @param tserverAddress * TServer name * @return TServer details */ @Path("{address}") @GET - public TabletServerSummary getTserverDetails(@PathParam("address") String tserverAddr) throws Exception { - - String tserverAddress = tserverAddr; + public TabletServerSummary getTserverDetails(@PathParam("address") @NotNull @Pattern(regexp = SERVER_REGEX) String tserverAddress) throws Exception { boolean tserverExists = false; - if (tserverAddress != null && tserverAddress.isEmpty() == false) { - for (TabletServerStatus ts : Monitor.getMmi().getTServerInfo()) { - if (tserverAddress.equals(ts.getName())) { - tserverExists = true; - break; - } + for (TabletServerStatus ts : Monitor.getMmi().getTServerInfo()) { + if (tserverAddress.equals(ts.getName())) { + tserverExists = true; + break; } } - if (tserverAddress == null || tserverAddress.isEmpty() || tserverExists == false) { - + if (!tserverExists) { return null; } diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/util/ParameterValidator.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/util/ParameterValidator.java new file mode 100644 index 0000000..a736a29 --- /dev/null +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/util/ParameterValidator.java @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.accumulo.monitor.util; + +/** + * Simple utility class to validate Accumulo Monitor Query and Path parameters + */ +public interface ParameterValidator { + + String ALPHA_NUM_REGEX = "\\w+"; + String ALPHA_NUM_REGEX_BLANK_OK = "\\w*"; + + String RESOURCE_REGEX = "(\\w|:)+"; + + String NAMESPACE_REGEX = "[*-]?|(\\w)+"; + String NAMESPACE_LIST_REGEX = "[*-]?|(\\w+,?\\w*)+"; + + String SERVER_REGEX = "(\\w+([.-])*\\w*)+(:[0-9]+)*"; + String SERVER_REGEX_BLANK_OK = "((\\w+([.-])*\\w*)+(:[0-9]+)*)*"; +} diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/view/WebViews.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/view/WebViews.java index 750ea8c..9e26a48 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/view/WebViews.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/view/WebViews.java @@ -16,14 +16,24 @@ */ package org.apache.accumulo.monitor.view; +import static org.apache.accumulo.monitor.util.ParameterValidator.ALPHA_NUM_REGEX; +import static org.apache.accumulo.monitor.util.ParameterValidator.ALPHA_NUM_REGEX_BLANK_OK; +import static org.apache.accumulo.monitor.util.ParameterValidator.SERVER_REGEX_BLANK_OK; +import static org.apache.commons.lang.StringUtils.isBlank; import static org.apache.commons.lang.StringUtils.isEmpty; +import static org.apache.commons.lang.StringUtils.isNotBlank; import java.io.IOException; +import java.io.UnsupportedEncodingException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import javax.validation.constraints.Max; +import javax.validation.constraints.Min; +import javax.validation.constraints.NotNull; +import javax.validation.constraints.Pattern; import javax.ws.rs.DefaultValue; import javax.ws.rs.GET; import javax.ws.rs.Path; @@ -145,11 +155,11 @@ public class WebViews { @GET @Path("tservers") @Template(name = "/default.ftl") - public Map<String,Object> getTabletServers(@QueryParam("s") String server) { + public Map<String,Object> getTabletServers(@QueryParam("s") @Pattern(regexp = SERVER_REGEX_BLANK_OK) String server) { Map<String,Object> model = getModel(); model.put("title", "Tablet Server Status"); - if (server != null) { + if (isNotBlank(server)) { model.put("template", "server.ftl"); model.put("js", "server.js"); model.put("server", server); @@ -230,17 +240,22 @@ public class WebViews { @GET @Path("vis") @Template(name = "/default.ftl") - public Map<String,Object> getServerActivity(@QueryParam("shape") @DefaultValue("circles") String shape, @QueryParam("size") @DefaultValue("40") String size, - @QueryParam("motion") @DefaultValue("") String motion, @QueryParam("color") @DefaultValue("allavg") String color) { + public Map<String,Object> getServerActivity(@QueryParam("shape") @DefaultValue("circles") @Pattern(regexp = ALPHA_NUM_REGEX_BLANK_OK) String shape, + @QueryParam("size") @DefaultValue("40") @Min(1) @Max(100) int size, + @QueryParam("motion") @DefaultValue("") @Pattern(regexp = ALPHA_NUM_REGEX_BLANK_OK) String motion, @QueryParam("color") @DefaultValue("allavg") @Pattern( + regexp = ALPHA_NUM_REGEX_BLANK_OK) String color) { + + shape = isNotBlank(shape) ? shape : "circles"; + color = isNotBlank(color) ? color : "allavg"; Map<String,Object> model = getModel(); model.put("title", "Server Activity"); model.put("template", "vis.ftl"); model.put("shape", shape); - model.put("size", size); - model.put("motion", motion); - model.put("color", color); + model.put("size", String.valueOf(size)); + model.put("motion", isBlank(motion) ? "" : motion.trim()); + model.put("color", isBlank(color) ? "allavg" : color); // Are there a set of acceptable values? return model; } @@ -274,7 +289,8 @@ public class WebViews { @GET @Path("tables/{tableID}") @Template(name = "/default.ftl") - public Map<String,Object> getTables(@PathParam("tableID") String tableID) throws TableNotFoundException { + public Map<String,Object> getTables(@PathParam("tableID") @NotNull @Pattern(regexp = ALPHA_NUM_REGEX) String tableID) throws TableNotFoundException, + UnsupportedEncodingException { String tableName = Tables.getTableName(Monitor.getContext().getInstance(), Table.ID.of(tableID)); @@ -293,20 +309,19 @@ public class WebViews { * Returns trace summary template * * @param minutes - * Range of minutes + * Range of minutes, default 10 minutes Min of 0 Max of 30 days in minutes * @return Trace summary model */ @GET @Path("trace/summary") @Template(name = "/default.ftl") - public Map<String,Object> getTracesSummary(@QueryParam("minutes") @DefaultValue("10") String minutes) { - + public Map<String,Object> getTracesSummary(@QueryParam("minutes") @DefaultValue("10") @Min(0) @Max(2592000) int minutes) { Map<String,Object> model = getModel(); - model.put("title", "Traces for the last " + minutes + " minute(s)"); + model.put("title", "Traces for the last " + String.valueOf(minutes) + " minute(s)"); model.put("template", "summary.ftl"); model.put("js", "summary.js"); - model.put("minutes", minutes); + model.put("minutes", String.valueOf(minutes)); return model; } @@ -317,21 +332,21 @@ public class WebViews { * @param type * Type of trace * @param minutes - * Range of minutes + * Range of minutes, default 10 minutes Min of 0 Max of 30 days in minutes * @return Traces by type model */ @GET @Path("trace/listType") @Template(name = "/default.ftl") - public Map<String,Object> getTracesForType(@QueryParam("type") String type, @QueryParam("minutes") @DefaultValue("10") String minutes) { - + public Map<String,Object> getTracesForType(@QueryParam("type") @NotNull @Pattern(regexp = ALPHA_NUM_REGEX) String type, + @QueryParam("minutes") @DefaultValue("10") @Min(0) @Max(2592000) int minutes) { Map<String,Object> model = getModel(); - model.put("title", "Traces for " + type + " for the last " + minutes + " minute(s)"); + model.put("title", "Traces for " + type + " for the last " + String.valueOf(minutes) + " minute(s)"); model.put("template", "listType.ftl"); model.put("js", "listType.js"); model.put("type", type); - model.put("minutes", minutes); + model.put("minutes", String.valueOf(minutes)); return model; } @@ -346,7 +361,7 @@ public class WebViews { @GET @Path("trace/show") @Template(name = "/default.ftl") - public Map<String,Object> getTraceShow(@QueryParam("id") String id) { + public Map<String,Object> getTraceShow(@QueryParam("id") @NotNull @Pattern(regexp = ALPHA_NUM_REGEX) String id) throws Exception { Map<String,Object> model = getModel(); model.put("title", "Trace ID " + id); @@ -387,7 +402,7 @@ public class WebViews { @GET @Path("problems") @Template(name = "/default.ftl") - public Map<String,Object> getProblems(@QueryParam("table") String table) { + public Map<String,Object> getProblems(@QueryParam("table") @Pattern(regexp = ALPHA_NUM_REGEX_BLANK_OK) String table) { Map<String,Object> model = getModel(); model.put("title", "Per-Table Problem Report"); @@ -395,7 +410,7 @@ public class WebViews { model.put("template", "problems.ftl"); model.put("js", "problems.js"); - if (table != null) { + if (isNotBlank(table)) { model.put("table", table); } diff --git a/server/monitor/src/test/java/org/apache/accumulo/monitor/util/ParameterValidatorTest.java b/server/monitor/src/test/java/org/apache/accumulo/monitor/util/ParameterValidatorTest.java new file mode 100644 index 0000000..d698029 --- /dev/null +++ b/server/monitor/src/test/java/org/apache/accumulo/monitor/util/ParameterValidatorTest.java @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.accumulo.monitor.util; + +import java.util.regex.Pattern; + +import org.junit.Assert; +import org.junit.Test; + +/** + * Basic tests for ParameterValidator + */ +public class ParameterValidatorTest { + + @Test + public void testAlphaNumRegex() { + Pattern p = Pattern.compile(ParameterValidator.ALPHA_NUM_REGEX); + Assert.assertTrue(p.matcher("asdlkfj234kj324").matches()); + Assert.assertFalse(p.matcher("234-324").matches()); + Assert.assertFalse(p.matcher("").matches()); + + p = Pattern.compile(ParameterValidator.ALPHA_NUM_REGEX_BLANK_OK); + Assert.assertTrue(p.matcher("asdlkfj234kj324").matches()); + Assert.assertTrue(p.matcher("").matches()); + Assert.assertFalse(p.matcher("234-324").matches()); + } + + @Test + public void testServerRegex() throws Exception { + Pattern p = Pattern.compile(ParameterValidator.SERVER_REGEX); + Assert.assertTrue("Did not match hostname with dots", p.matcher("ab3cd.12d34.3xyz.net").matches()); + Assert.assertTrue("Did not match hostname with dash", p.matcher("abcd.123.server-foo.com").matches()); + Assert.assertTrue("Did not match hostname and port", p.matcher("abcd.123.server-foo.com:1234").matches()); + Assert.assertTrue("Did not match all numeric", p.matcher("127.0.0.1").matches()); + Assert.assertTrue("Did not match all numeric and port", p.matcher("127.0.0.1:9999").matches()); + Assert.assertTrue("Did not match all numeric and port", p.matcher("ServerName:9999").matches()); + + Assert.assertFalse(p.matcher("abcd.1234.*.xyz").matches()); + Assert.assertFalse(p.matcher("abcd.1234.;xyz").matches()); + Assert.assertFalse(p.matcher("abcd.12{3}4.xyz").matches()); + Assert.assertFalse(p.matcher("abcd.12[3]4.xyz").matches()); + Assert.assertFalse(p.matcher("abcd=4.xyz").matches()); + Assert.assertFalse(p.matcher("abcd=\"4.xyz\"").matches()); + Assert.assertFalse(p.matcher("abcd\"4.xyz\"").matches()); + + Pattern q = Pattern.compile(ParameterValidator.SERVER_REGEX_BLANK_OK); + Assert.assertTrue(q.matcher("abcd:9997").matches()); + Assert.assertTrue(q.matcher("abcd.123:9997").matches()); + Assert.assertTrue(q.matcher("abcd.123-xyz:9997").matches()); + Assert.assertTrue(q.matcher("abcd.123-xyz").matches()); + Assert.assertTrue(q.matcher("").matches()); + } + +} diff --git a/test/pom.xml b/test/pom.xml index 81133b7..00ff2c7 100644 --- a/test/pom.xml +++ b/test/pom.xml @@ -67,6 +67,14 @@ <artifactId>commons-lang</artifactId> </dependency> <dependency> + <groupId>javax.el</groupId> + <artifactId>javax.el-api</artifactId> + </dependency> + <dependency> + <groupId>javax.ws.rs</groupId> + <artifactId>javax.ws.rs-api</artifactId> + </dependency> + <dependency> <groupId>jline</groupId> <artifactId>jline</artifactId> </dependency> @@ -133,6 +141,17 @@ <dependency> <groupId>org.apache.hadoop</groupId> <artifactId>hadoop-client</artifactId> + <exclusions> + <!-- Pulls in an older jersey version --> + <exclusion> + <groupId>com.sun.jersey</groupId> + <artifactId>jersey-core</artifactId> + </exclusion> + <exclusion> + <groupId>com.sun.jersey</groupId> + <artifactId>jersey-client</artifactId> + </exclusion> + </exclusions> </dependency> <dependency> <groupId>org.apache.hadoop</groupId> @@ -141,6 +160,29 @@ <dependency> <groupId>org.apache.hadoop</groupId> <artifactId>hadoop-minicluster</artifactId> + <exclusions> + <!-- Pulls in an older jersey version --> + <exclusion> + <groupId>com.sun.jersey</groupId> + <artifactId>jersey-client</artifactId> + </exclusion> + <exclusion> + <groupId>com.sun.jersey</groupId> + <artifactId>jersey-core</artifactId> + </exclusion> + <exclusion> + <groupId>com.sun.jersey</groupId> + <artifactId>jersey-json</artifactId> + </exclusion> + <exclusion> + <groupId>com.sun.jersey</groupId> + <artifactId>jersey-server</artifactId> + </exclusion> + <exclusion> + <groupId>com.sun.jersey.contribs</groupId> + <artifactId>jersey-guice</artifactId> + </exclusion> + </exclusions> </dependency> <dependency> <groupId>org.apache.hadoop</groupId> @@ -174,10 +216,34 @@ <artifactId>easymock</artifactId> </dependency> <dependency> + <groupId>org.glassfish.jersey.core</groupId> + <artifactId>jersey-client</artifactId> + </dependency> + <dependency> + <groupId>org.glassfish.jersey.ext</groupId> + <artifactId>jersey-bean-validation</artifactId> + </dependency> + <dependency> + <groupId>org.glassfish.jersey.test-framework</groupId> + <artifactId>jersey-test-framework-core</artifactId> + </dependency> + <dependency> + <groupId>org.glassfish.jersey.test-framework.providers</groupId> + <artifactId>jersey-test-framework-provider-grizzly2</artifactId> + </dependency> + <dependency> <groupId>org.hamcrest</groupId> <artifactId>hamcrest-core</artifactId> </dependency> <dependency> + <groupId>org.powermock</groupId> + <artifactId>powermock-api-easymock</artifactId> + </dependency> + <dependency> + <groupId>org.powermock</groupId> + <artifactId>powermock-module-junit4</artifactId> + </dependency> + <dependency> <groupId>org.slf4j</groupId> <artifactId>slf4j-api</artifactId> </dependency> diff --git a/test/src/main/java/org/apache/accumulo/test/monitor/WebViewsIT.java b/test/src/main/java/org/apache/accumulo/test/monitor/WebViewsIT.java new file mode 100644 index 0000000..8e9169d --- /dev/null +++ b/test/src/main/java/org/apache/accumulo/test/monitor/WebViewsIT.java @@ -0,0 +1,157 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.accumulo.test.monitor; + +import static org.easymock.EasyMock.expect; + +import java.io.IOException; +import java.io.OutputStream; +import java.lang.annotation.Annotation; +import java.lang.reflect.Type; +import java.util.HashMap; + +import javax.ws.rs.WebApplicationException; +import javax.ws.rs.core.Application; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.MultivaluedMap; +import javax.ws.rs.core.Response; +import javax.ws.rs.ext.MessageBodyWriter; + +import org.apache.accumulo.core.client.Instance; +import org.apache.accumulo.core.client.impl.Table; +import org.apache.accumulo.core.client.impl.Tables; +import org.apache.accumulo.core.conf.DefaultConfiguration; +import org.apache.accumulo.monitor.Monitor; +import org.apache.accumulo.monitor.view.WebViews; +import org.apache.accumulo.server.AccumuloServerContext; +import org.apache.accumulo.test.categories.SunnyDayTests; +import org.easymock.EasyMock; +import org.glassfish.jersey.client.ClientConfig; +import org.glassfish.jersey.server.ResourceConfig; +import org.glassfish.jersey.test.JerseyTest; +import org.glassfish.jersey.test.TestProperties; +import org.junit.Assert; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; +import org.powermock.api.easymock.PowerMock; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +/** + * Basic tests for parameter validation constraints + */ +@Category(SunnyDayTests.class) +@RunWith(PowerMockRunner.class) +@PrepareForTest({Monitor.class, Tables.class}) +public class WebViewsIT extends JerseyTest { + + @Override + public Application configure() { + enable(TestProperties.LOG_TRAFFIC); + enable(TestProperties.DUMP_ENTITY); + ResourceConfig config = new ResourceConfig(WebViews.class); + config.register(WebViewsIT.HashMapWriter.class); + return config; + } + + @Override + protected void configureClient(ClientConfig config) { + super.configureClient(config); + config.register(WebViewsIT.HashMapWriter.class); + } + + /** + * Expect to fail the constraint validation on the REST endpoint. The constraint is the pre-defined word character class Pattern so passing a table name with + * punctuation will cause a 400 response code. + */ + @Test + public void testGetTablesConstraintViolations() { + Response output = target("tables/f+o*o").request().get(); + Assert.assertEquals("should return status 400", 400, output.getStatus()); + } + + /** + * Test path tables/{tableID} which passes constraints. On passing constraints underlying logic will be executed so we need to mock a certain amount of it. + * Note: If you get test failures here due to 500 server response, it's likely an underlying class or method call was added/modified and needs mocking. Note: + * To get the proper response code back, you need to make sure jersey has a registered MessageBodyWriter capable of serializing/writing the object returned + * from your endpoint. We're using a simple stubbed out inner class HashMapWriter for this. + * + * @throws Exception + * not expected + */ + @Test + public void testGetTablesConstraintPassing() throws Exception { + Instance instanceMock = EasyMock.createMock(Instance.class); + expect(instanceMock.getInstanceID()).andReturn("foo").anyTimes(); + AccumuloServerContext contextMock = EasyMock.createMock(AccumuloServerContext.class); + expect(contextMock.getInstance()).andReturn(instanceMock).anyTimes(); + expect(contextMock.getConfiguration()).andReturn(DefaultConfiguration.getInstance()).anyTimes(); + + PowerMock.mockStatic(Monitor.class); + expect(Monitor.getContext()).andReturn(contextMock).anyTimes(); + + PowerMock.mockStatic(Tables.class); + expect(Tables.getTableName(instanceMock, Table.ID.of("foo"))).andReturn("bar"); + PowerMock.replayAll(); + org.easymock.EasyMock.replay(instanceMock, contextMock); + + // Using the mocks we can verify that the getModel method gets called via debugger + // however it's difficult to continue to mock through the jersey MVC code for the properly built response. + // Our silly HashMapWriter registered in the configure method gets wired in and used here. + Response output = target("tables/foo").request().get(); + Assert.assertEquals("should return status 200", 200, output.getStatus()); + String responseBody = output.readEntity(String.class); + Assert.assertTrue(responseBody.contains("tableID=foo") && responseBody.contains("table=bar")); + } + + /** + * Test minutes parameter constraints. When outside range we should get a 400 response. + */ + @Test + public void testGetTracesSummaryValidationConstraint() { + // Test upper bounds of constraint + Response output = target("trace/summary").queryParam("minutes", 5000000).request().get(); + Assert.assertEquals("should return status 400", 400, output.getStatus()); + + // Test lower bounds of constraint + output = target("trace/summary").queryParam("minutes", -27).request().get(); + Assert.assertEquals("should return status 400", 400, output.getStatus()); + } + + /** + * Silly stub to handle MessageBodyWriter for Hashmap. Registered in configure method and auto-wired by Jersey. + */ + public static class HashMapWriter implements MessageBodyWriter<HashMap<?,?>> { + @Override + public boolean isWriteable(Class<?> type, Type genericType, Annotation[] annotations, MediaType mediaType) { + return true; + } + + @Override + public long getSize(HashMap<?,?> hashMap, Class<?> type, Type genericType, Annotation[] annotations, MediaType mediaType) { + return 0; + } + + @Override + public void writeTo(HashMap<?,?> hashMap, Class<?> type, Type genericType, Annotation[] annotations, MediaType mediaType, + MultivaluedMap<String,Object> httpHeaders, OutputStream entityStream) throws IOException, WebApplicationException { + String s = hashMap.toString(); + entityStream.write(s.getBytes()); + } + } +} -- To stop receiving notification emails like this one, please contact ['"commits@accumulo.apache.org" <commits@accumulo.apache.org>'].