From 5bd435974079583d6b28b6875d88004cf4375076 Mon Sep 17 00:00:00 2001 From: markt Date: Thu, 3 Mar 2011 11:16:51 +0000 Subject: [PATCH] [SECURITY] Start of fix for issue reported on users list that @ServletSecurity annotations were ignored. This fix is not yet complete. This first part: - Triggers the loading of the Wrapper before the constraints are processed to ensure that any @ServletSecurity annotations are taken account of - Makes sure the constraints collection is thread-safe given new usage - Adds scanning for @ServletSecurity when a Servlet is loaded - Ensure there is always an authenticator when using the embedded Tomcat class so that @ServletSecurity will have an effect - Adds a simple unit test to check @ServletSecurity annotations are processed Further commits will add additional test cases and any changes required for those test cases to pass git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1076586 13f79535-47bb-0310-9956-ffa450edef68 --- .../catalina/authenticator/AuthenticatorBase.java | 8 +++ java/org/apache/catalina/core/StandardContext.java | 3 +- java/org/apache/catalina/core/StandardWrapper.java | 14 ++++- java/org/apache/catalina/startup/Tomcat.java | 9 +++ .../apache/catalina/core/TestStandardWrapper.java | 73 ++++++++++++++++++++++ 5 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 test/org/apache/catalina/core/TestStandardWrapper.java diff --git a/java/org/apache/catalina/authenticator/AuthenticatorBase.java b/java/org/apache/catalina/authenticator/AuthenticatorBase.java index 877d2698a..8f33ce289 100644 --- a/java/org/apache/catalina/authenticator/AuthenticatorBase.java +++ b/java/org/apache/catalina/authenticator/AuthenticatorBase.java @@ -37,6 +37,7 @@ import org.apache.catalina.Manager; import org.apache.catalina.Realm; import org.apache.catalina.Session; import org.apache.catalina.Valve; +import org.apache.catalina.Wrapper; import org.apache.catalina.connector.Request; import org.apache.catalina.connector.Response; import org.apache.catalina.deploy.LoginConfig; @@ -478,6 +479,13 @@ public abstract class AuthenticatorBase extends ValveBase } } + // The Servlet may specify security constraints through annotations. + // Ensure that they have been processed before constraints are checked + Wrapper wrapper = (Wrapper) request.getMappingData().wrapper; + if (wrapper.getServlet() != null) { + wrapper.load(); + } + Realm realm = this.context.getRealm(); // Is this request URI subject to a security constraint? SecurityConstraint [] constraints diff --git a/java/org/apache/catalina/core/StandardContext.java b/java/org/apache/catalina/core/StandardContext.java index 4291d3978..b0da03fa9 100644 --- a/java/org/apache/catalina/core/StandardContext.java +++ b/java/org/apache/catalina/core/StandardContext.java @@ -298,7 +298,8 @@ public class StandardContext extends ContainerBase /** * The security constraints for this web application. */ - private SecurityConstraint constraints[] = new SecurityConstraint[0]; + private volatile SecurityConstraint constraints[] = + new SecurityConstraint[0]; private final Object constraintsLock = new Object(); diff --git a/java/org/apache/catalina/core/StandardWrapper.java b/java/org/apache/catalina/core/StandardWrapper.java index 657d84fcb..5e57643f8 100644 --- a/java/org/apache/catalina/core/StandardWrapper.java +++ b/java/org/apache/catalina/core/StandardWrapper.java @@ -42,9 +42,11 @@ import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; +import javax.servlet.ServletSecurityElement; import javax.servlet.SingleThreadModel; import javax.servlet.UnavailableException; import javax.servlet.annotation.MultipartConfig; +import javax.servlet.annotation.ServletSecurity; import org.apache.catalina.Container; import org.apache.catalina.ContainerServlet; @@ -1075,10 +1077,20 @@ public class StandardWrapper extends ContainerBase } } + ServletSecurity secAnnotation = + servlet.getClass().getAnnotation(ServletSecurity.class); + Context ctxt = (Context) getParent(); + if (secAnnotation != null) { + ctxt.addServletSecurity( + new ApplicationServletRegistration(this, ctxt), + new ServletSecurityElement(secAnnotation)); + } + + // Special handling for ContainerServlet instances if ((servlet instanceof ContainerServlet) && (isContainerProvidedServlet(servletClass) || - ((Context)getParent()).getPrivileged() )) { + ctxt.getPrivileged() )) { ((ContainerServlet) servlet).setWrapper(this); } diff --git a/java/org/apache/catalina/startup/Tomcat.java b/java/org/apache/catalina/startup/Tomcat.java index 9ad208198..84f6490ad 100644 --- a/java/org/apache/catalina/startup/Tomcat.java +++ b/java/org/apache/catalina/startup/Tomcat.java @@ -42,6 +42,7 @@ import org.apache.catalina.Realm; import org.apache.catalina.Server; import org.apache.catalina.Service; import org.apache.catalina.Wrapper; +import org.apache.catalina.authenticator.NonLoginAuthenticator; import org.apache.catalina.connector.Connector; import org.apache.catalina.core.NamingContextListener; import org.apache.catalina.core.StandardContext; @@ -50,6 +51,7 @@ import org.apache.catalina.core.StandardHost; import org.apache.catalina.core.StandardServer; import org.apache.catalina.core.StandardService; import org.apache.catalina.core.StandardWrapper; +import org.apache.catalina.deploy.LoginConfig; import org.apache.catalina.realm.GenericPrincipal; import org.apache.catalina.realm.RealmBase; import org.apache.catalina.session.StandardManager; @@ -698,6 +700,13 @@ public class Tomcat { if (event.getType().equals(Lifecycle.CONFIGURE_START_EVENT)) { context.setConfigured(true); } + // LoginConfig is required to process @ServletSecurity + // annotations + if (context.getLoginConfig() == null) { + context.setLoginConfig( + new LoginConfig("NONE", null, null, null)); + context.getPipeline().addValve(new NonLoginAuthenticator()); + } } catch (ClassCastException e) { return; } diff --git a/test/org/apache/catalina/core/TestStandardWrapper.java b/test/org/apache/catalina/core/TestStandardWrapper.java new file mode 100644 index 000000000..45537cd35 --- /dev/null +++ b/test/org/apache/catalina/core/TestStandardWrapper.java @@ -0,0 +1,73 @@ +/* + * 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.catalina.core; + +import java.io.IOException; + +import javax.servlet.ServletException; +import javax.servlet.annotation.HttpConstraint; +import javax.servlet.annotation.ServletSecurity; +import javax.servlet.annotation.ServletSecurity.EmptyRoleSemantic; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.apache.catalina.Context; +import org.apache.catalina.Wrapper; +import org.apache.catalina.startup.Tomcat; +import org.apache.catalina.startup.TomcatBaseTest; +import org.apache.tomcat.util.buf.ByteChunk; + +public class TestStandardWrapper extends TomcatBaseTest { + + public void testSecurityAnnotations1() throws Exception { + // Setup Tomcat instance + Tomcat tomcat = getTomcatInstance(); + + // Must have a real docBase - just use temp + Context ctx = + tomcat.addContext("", System.getProperty("java.io.tmpdir")); + + Wrapper wrapper = Tomcat.addServlet(ctx, "servlet", + "org.apache.catalina.core.TestStandardWrapper$DenyServlet"); + wrapper.setAsyncSupported(true); + ctx.addServletMapping("/", "servlet"); + + tomcat.start(); + + // Call the servlet once + ByteChunk bc = new ByteChunk(); + int rc = getUrl("http://localhost:" + getPort() + "/", bc, null); + + assertNull(bc.toString()); + assertEquals(403, rc); + } + + @ServletSecurity(@HttpConstraint(EmptyRoleSemantic.DENY)) + public static class DenyServlet extends HttpServlet { + private static final long serialVersionUID = 1L; + + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) + throws ServletException, IOException { + + resp.setContentType("text/plain"); + resp.getWriter().print("FAIL"); + } + } +} -- 2.11.0