You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by ct...@apache.org on 2019/11/06 05:16:46 UTC
[accumulo] branch 1.9 updated: Fix #1401,
trace servlet parameter handling (#1409)
This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch 1.9
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/1.9 by this push:
new f3b3590 Fix #1401, trace servlet parameter handling (#1409)
f3b3590 is described below
commit f3b3590a5b4f8adf80f0acffe768a8509d409c51
Author: Richard W. Eggert II <ri...@gmail.com>
AuthorDate: Wed Nov 6 00:16:40 2019 -0500
Fix #1401, trace servlet parameter handling (#1409)
* corrected servlet parameter handling so that it does not break things
This resolves #1401.
I removed the previous implementation of string sanitization, replacing it with more specific checks at the places where the parameters are used.
BasicTest was deleted because it was exclusively testing the broken
sanitization mechanism.
* Update server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ShowTrace.java
Added null check to avoid NPE
Co-Authored-By: Christopher Tubbs <ct...@apache.org>
* automatically adjusted formatting
---
.../accumulo/server/util/TabletIterator.java | 3 +-
.../accumulo/monitor/servlets/BasicServlet.java | 5 +-
.../accumulo/monitor/servlets/trace/Basic.java | 2 +-
.../accumulo/monitor/servlets/trace/ListType.java | 4 +-
.../accumulo/monitor/servlets/trace/ShowTrace.java | 8 +++-
.../accumulo/monitor/servlets/trace/BasicTest.java | 55 ----------------------
6 files changed, 16 insertions(+), 61 deletions(-)
diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/TabletIterator.java b/server/base/src/main/java/org/apache/accumulo/server/util/TabletIterator.java
index 83ec344..b9a5a71 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/util/TabletIterator.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/util/TabletIterator.java
@@ -102,7 +102,8 @@ public class TabletIterator implements Iterator<Map<Key,Value>> {
currentTabletKeys = scanToPrevEndRow();
if (currentTabletKeys.size() == 0) {
- // Always expect the default tablet to exist for a table. The following checks for the case when
+ // Always expect the default tablet to exist for a table. The following checks for the case
+ // when
// the default tablet was not seen when it should have been seen.
if (lastTablet != null) {
KeyExtent lastExtent = new KeyExtent(lastTablet, (Text) null);
diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java
index 75e485c..dfc81de 100644
--- a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java
+++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java
@@ -17,6 +17,7 @@
package org.apache.accumulo.monitor.servlets;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.commons.lang.StringEscapeUtils.escapeHtml;
import java.io.IOException;
import java.io.PrintWriter;
@@ -133,8 +134,8 @@ abstract public class BasicServlet extends HttpServlet {
// BEGIN HEADER
sb.append("<head>\n");
- sb.append("<title>").append(getTitle(req)).append(" - Accumulo ").append(Constants.VERSION)
- .append("</title>\n");
+ sb.append("<title>").append(escapeHtml(getTitle(req))).append(" - Accumulo ")
+ .append(Constants.VERSION).append("</title>\n");
if ((refresh > 0) && (req.getRequestURI().startsWith("/vis") == false)
&& (req.getRequestURI().startsWith("/shell") == false))
sb.append("<meta http-equiv='refresh' content='" + refresh + "' />\n");
diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/Basic.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/Basic.java
index ec3c8e6..09b7ccf 100644
--- a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/Basic.java
+++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/Basic.java
@@ -51,7 +51,7 @@ abstract class Basic extends BasicServlet {
public static String getStringParameter(HttpServletRequest req, String name,
String defaultValue) {
- String result = req.getParameter(name).replaceAll("[^A-Za-z]", "");
+ final String result = req.getParameter(name);
if (result == null) {
return defaultValue;
}
diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ListType.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ListType.java
index e80aef1..babeb02 100644
--- a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ListType.java
+++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ListType.java
@@ -16,6 +16,8 @@
*/
package org.apache.accumulo.monitor.servlets.trace;
+import static org.apache.commons.lang.StringEscapeUtils.escapeHtml;
+
import java.security.PrivilegedAction;
import java.util.Map.Entry;
@@ -61,7 +63,7 @@ public class ListType extends Basic {
Range range = new Range(new Text("start:" + Long.toHexString(startTime)),
new Text("start:" + Long.toHexString(endTime)));
scanner.setRange(range);
- final Table trace = new Table("trace", "Traces for " + getType(req));
+ final Table trace = new Table("trace", "Traces for " + escapeHtml(type));
trace.addSortableColumn("Start", new ShowTraceLinkType(), "Start Time");
trace.addSortableColumn("ms", new DurationType(), "Span time");
trace.addUnsortableColumn("Source", new StringType<String>(), "Service and location");
diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ShowTrace.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ShowTrace.java
index 8970dd0..55a09a4 100644
--- a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ShowTrace.java
+++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ShowTrace.java
@@ -22,6 +22,7 @@ import java.security.PrivilegedAction;
import java.util.Collection;
import java.util.Map.Entry;
import java.util.Set;
+import java.util.regex.Pattern;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
@@ -45,9 +46,14 @@ public class ShowTrace extends Basic {
private static final long serialVersionUID = 1L;
private static final String checkboxIdSuffix = "_checkbox";
private static final String pageLoadFunctionName = "pageload";
+ private static final Pattern TRACE_ID_PATTERN = Pattern.compile("\\p{XDigit}{16}");
String getTraceId(HttpServletRequest req) {
- return getStringParameter(req, "id", null);
+ final String stringValue = getStringParameter(req, "id", null);
+ if (stringValue == null) {
+ return null;
+ }
+ return TRACE_ID_PATTERN.matcher(stringValue).matches() ? stringValue : null;
}
@Override
diff --git a/server/monitor/src/test/java/org/apache/accumulo/monitor/servlets/trace/BasicTest.java b/server/monitor/src/test/java/org/apache/accumulo/monitor/servlets/trace/BasicTest.java
deleted file mode 100644
index c917473..0000000
--- a/server/monitor/src/test/java/org/apache/accumulo/monitor/servlets/trace/BasicTest.java
+++ /dev/null
@@ -1,55 +0,0 @@
-/*
- * 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.servlets.trace;
-
-import static org.junit.Assert.assertEquals;
-
-import javax.servlet.http.HttpServletRequest;
-
-import org.easymock.EasyMock;
-import org.junit.Before;
-import org.junit.Test;
-
-/**
- * Test class for {@link Basic}.
- */
-public class BasicTest {
- private HttpServletRequest request;
-
- @Before
- public void setupMocks() {
- request = EasyMock.createMock(HttpServletRequest.class);
- }
-
- @Test
- public void testSanitizedStringParameters() {
- final String safeKey = "safeKey";
- final String bogusKey = "bogusKey";
- final String safeValue = "value";
- final String bogusValue = "value<script>alert('ohnoes');</script>";
- final String sanitizedBogusValue = "valuescriptalertohnoesscript";
-
- EasyMock.expect(request.getParameter(safeKey)).andReturn(safeValue).atLeastOnce();
- EasyMock.expect(request.getParameter(bogusKey)).andReturn(bogusValue).atLeastOnce();
- EasyMock.replay(request);
- String actualValue = Basic.getStringParameter(request, safeKey, null);
- assertEquals(safeValue, actualValue);
- actualValue = Basic.getStringParameter(request, bogusKey, null);
- assertEquals(sanitizedBogusValue, actualValue);
- EasyMock.verify(request);
- }
-}