From 7d135cb4953244b5941fe48b320f80092cf15386 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20G=C3=B6rler?= Date: Mon, 12 May 2014 16:10:52 +0200 Subject: [PATCH] Fix the request wrapper for http requests served from plugins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A request wrapper is used to provide servlets contributed by plugins with a context that is rooted in the plugins path segment of the request URI. For example a plugin myPlugin that is invoked under '/a/plugins/myPlugin' should be provided with the context path '/a/plugins/myPlugin'. The request '/a/plugins/myPlugin/myResource' should give the following info for the wrapped context: contextPath: '/a/plugins/myPlugin' servletPath: '' pathInfo: '/myResource' requestURI: '/a/plugins/myPlugin/myResource' The request wrapper failed to comply with the servlet specification (servlet specification 3.1, ยง3.5). For example the invariant requestURI = contextPath + servletPath + pathInfo was violated. The request mapping mechanism is now extracted to a ContextMapper class. Change-Id: Ie8d9a9ac151162efb080a6e676a121a53bc11852 --- .../gerrit/httpd/plugins/ContextMapper.java | 74 +++++++++++++++++ .../httpd/plugins/HttpPluginServlet.java | 57 +++---------- .../httpd/plugins/ContextMapperTest.java | 79 +++++++++++++++++++ 3 files changed, 162 insertions(+), 48 deletions(-) create mode 100644 gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/ContextMapper.java create mode 100644 gerrit-httpd/src/test/java/com/google/gerrit/httpd/plugins/ContextMapperTest.java diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/ContextMapper.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/ContextMapper.java new file mode 100644 index 0000000000..47ef52026c --- /dev/null +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/ContextMapper.java @@ -0,0 +1,74 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed 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 com.google.gerrit.httpd.plugins; + +package com.google.gerrit.httpd.plugins; + +import com.google.common.base.Strings; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletRequestWrapper; + +class ContextMapper { + private static final String PLUGINS_PREFIX = "/plugins/"; + private static final String AUTHORIZED_PREFIX = "/a" + PLUGINS_PREFIX; + private final String base; + private final String authorizedBase; + + public ContextMapper(String contextPath) { + base = Strings.nullToEmpty(contextPath) + PLUGINS_PREFIX; + authorizedBase = Strings.nullToEmpty(contextPath) + AUTHORIZED_PREFIX; + } + + private static boolean isAuthorizedCall(HttpServletRequest req) { + return !Strings.isNullOrEmpty(req.getServletPath()) + && req.getServletPath().startsWith(AUTHORIZED_PREFIX); + } + + HttpServletRequest create(HttpServletRequest req, String name) { + String contextPath = (isAuthorizedCall(req) ? authorizedBase : base) + name; + + return new WrappedRequest(req, contextPath); + } + + public String getFullPath(String name) { + return base + name; + } + + private class WrappedRequest extends HttpServletRequestWrapper { + private final String contextPath; + private final String pathInfo; + + private WrappedRequest(HttpServletRequest req, String contextPath) { + super(req); + this.contextPath = contextPath; + this.pathInfo = getRequestURI().substring(contextPath.length()); + } + + @Override + public String getServletPath() { + return ""; + } + + @Override + public String getContextPath() { + return contextPath; + } + + @Override + public String getPathInfo() { + return pathInfo; + } + } + +} diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java index 8f66b0558f..4dc6f1eb5c 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java @@ -73,7 +73,6 @@ import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletRequestWrapper; import javax.servlet.http.HttpServletResponse; @Singleton @@ -83,8 +82,6 @@ class HttpPluginServlet extends HttpServlet private static final long serialVersionUID = 1L; private static final Logger log = LoggerFactory.getLogger(HttpPluginServlet.class); - private static final String PLUGINS_PREFIX = "/plugins/"; - private static final String AUTHORIZED_PREFIX = "/a" + PLUGINS_PREFIX; private final MimeUtilFileTypeRegistry mimeUtil; private final Provider webUrl; @@ -94,8 +91,7 @@ class HttpPluginServlet extends HttpServlet private final RestApiServlet managerApi; private List pending = Lists.newArrayList(); - private String base; - private String authorizedBase; + private ContextMapper wrapper; private final ConcurrentMap plugins = Maps.newConcurrentMap(); @@ -134,9 +130,7 @@ class HttpPluginServlet extends HttpServlet public synchronized void init(ServletConfig config) throws ServletException { super.init(config); - String path = config.getServletContext().getContextPath(); - base = Strings.nullToEmpty(path) + PLUGINS_PREFIX; - authorizedBase = Strings.nullToEmpty(path) + AUTHORIZED_PREFIX; + wrapper = new ContextMapper(config.getServletContext().getContextPath()); for (Plugin plugin : pending) { install(plugin); } @@ -182,7 +176,8 @@ class HttpPluginServlet extends HttpServlet } try { - ServletContext ctx = PluginServletContext.create(plugin, base + name); + ServletContext ctx = + PluginServletContext.create(plugin, wrapper.getFullPath(name)); filter.init(new WrappedFilterConfig(ctx)); } catch (ServletException e) { log.warn(String.format("Plugin %s failed to initialize HTTP", name), e); @@ -220,8 +215,7 @@ class HttpPluginServlet extends HttpServlet return; } - WrappedRequest wr = new WrappedRequest(req, - (isAuthorizedCall(req) ? authorizedBase : base) + name); + HttpServletRequest wr = wrapper.create(req, name); FilterChain chain = new FilterChain() { @Override public void doFilter(ServletRequest req, ServletResponse res) @@ -236,11 +230,6 @@ class HttpPluginServlet extends HttpServlet } } - private boolean isAuthorizedCall(HttpServletRequest req) { - return !Strings.isNullOrEmpty(req.getServletPath()) - && req.getServletPath().startsWith(AUTHORIZED_PREFIX); - } - private static boolean isApiCall(HttpServletRequest req, List parts) { String method = req.getMethod(); int cnt = parts.size(); @@ -258,14 +247,13 @@ class HttpPluginServlet extends HttpServlet return; } - String uri = req.getRequestURI(); - String ctx = req.getContextPath(); - if (uri.length() <= ctx.length()) { + String pathInfo = req.getPathInfo(); + if (pathInfo.length() < 1) { Resource.NOT_FOUND.send(req, res); return; } - String file = uri.substring(ctx.length() + 1); + String file = pathInfo.substring(1); ResourceKey key = new ResourceKey(holder.plugin, file); Resource rsc = resourceCache.getIfPresent(key); if (rsc != null) { @@ -273,6 +261,7 @@ class HttpPluginServlet extends HttpServlet return; } + String uri = req.getRequestURI(); if ("".equals(file)) { res.sendRedirect(uri + holder.docPrefix + "index.html"); return; @@ -675,32 +664,4 @@ class HttpPluginServlet extends HttpServlet } } } - - private static class WrappedRequest extends HttpServletRequestWrapper { - private final String contextPath; - - WrappedRequest(HttpServletRequest req, String contextPath) { - super(req); - this.contextPath = contextPath; - } - - @Override - public String getContextPath() { - return contextPath; - } - - @Override - public String getServletPath() { - return getRequestURI().substring(contextPath.length()); - } - - @Override - public String getRequestURI() { - String uri = super.getRequestURI(); - if (uri.startsWith("/a/")) { - uri = uri.substring(2); - } - return uri; - } - } } diff --git a/gerrit-httpd/src/test/java/com/google/gerrit/httpd/plugins/ContextMapperTest.java b/gerrit-httpd/src/test/java/com/google/gerrit/httpd/plugins/ContextMapperTest.java new file mode 100644 index 0000000000..e032823cf1 --- /dev/null +++ b/gerrit-httpd/src/test/java/com/google/gerrit/httpd/plugins/ContextMapperTest.java @@ -0,0 +1,79 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed 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 com.google.gerrit.httpd.plugins; + +package com.google.gerrit.httpd.plugins; + +import static org.easymock.EasyMock.createNiceMock; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.replay; +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +import javax.servlet.http.HttpServletRequest; + +public class ContextMapperTest { + + private static final String CONTEXT = "/context"; + private static final String PLUGIN_NAME = "my-plugin"; + private static final String RESOURCE = "my-resource"; + + @Test + public void testUnauthorized() throws Exception { + ContextMapper classUnderTest = new ContextMapper(CONTEXT); + + HttpServletRequest originalRequest = + createMockRequest("/plugins/", PLUGIN_NAME + "/" + RESOURCE); + + HttpServletRequest result = + classUnderTest.create(originalRequest, PLUGIN_NAME); + + assertEquals(CONTEXT + "/plugins/" + PLUGIN_NAME, result.getContextPath()); + assertEquals("", result.getServletPath()); + assertEquals("/" + RESOURCE, result.getPathInfo()); + assertEquals(CONTEXT + "/plugins/" + PLUGIN_NAME + "/" + RESOURCE, + result.getRequestURI()); + } + + @Test + public void testAuthorized() throws Exception { + ContextMapper classUnderTest = new ContextMapper(CONTEXT); + + HttpServletRequest originalRequest = + createMockRequest("/a/plugins/", PLUGIN_NAME + "/" + RESOURCE); + + HttpServletRequest result = + classUnderTest.create(originalRequest, PLUGIN_NAME); + + assertEquals(CONTEXT + "/a/plugins/" + PLUGIN_NAME, + result.getContextPath()); + assertEquals("", result.getServletPath()); + assertEquals("/" + RESOURCE, result.getPathInfo()); + assertEquals(CONTEXT + "/a/plugins/" + PLUGIN_NAME + "/" + RESOURCE, + result.getRequestURI()); + } + + private static HttpServletRequest createMockRequest(String servletPath, + String pathInfo) { + HttpServletRequest req = createNiceMock(HttpServletRequest.class); + expect(req.getContextPath()).andStubReturn(CONTEXT); + expect(req.getServletPath()).andStubReturn(servletPath); + expect(req.getPathInfo()).andStubReturn(pathInfo); + String uri = CONTEXT + servletPath + pathInfo; + expect(req.getRequestURI()).andStubReturn(uri); + replay(req); + + return req; + } +}