commit 2aac69f694d42d9219eb27018b3da0ae1bdd73ab

Author: Mark Thomas <markt@apache.org>

Date:   Tue Feb 6 12:49:49 2018 +0000

    Process all ServletSecurity annotations at web application start rather

    than at servlet load time to ensure constraints are applied

    consistently.

    git-svn-id: https://svn.apache.org/repos/asf/tomcat/tc7.0.x/trunk@1823322 13f79535-47bb-0310-9956-ffa450edef68

diff -up java/org/apache/catalina/authenticator/AuthenticatorBase.java.orig java/org/apache/catalina/authenticator/AuthenticatorBase.java

--- java/org/apache/catalina/authenticator/AuthenticatorBase.java.orig	2019-02-28 15:41:52.561346049 -0500

+++ java/org/apache/catalina/authenticator/AuthenticatorBase.java	2019-02-28 15:44:09.129930251 -0500

@@ -40,7 +40,6 @@ 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;

@@ -488,13 +487,6 @@ public abstract class AuthenticatorBase

             }

         }

-        // 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 != null) {

-            wrapper.servletSecurityAnnotationScan();

-        }

-

         Realm realm = this.context.getRealm();

         // Is this request URI subject to a security constraint?

         SecurityConstraint [] constraints

diff -up java/org/apache/catalina/core/ApplicationContext.java.orig java/org/apache/catalina/core/ApplicationContext.java

--- java/org/apache/catalina/core/ApplicationContext.java.orig	2017-03-09 08:51:39.000000000 -0500

+++ java/org/apache/catalina/core/ApplicationContext.java	2019-02-28 15:44:09.130930248 -0500

@@ -53,8 +53,10 @@ import javax.servlet.ServletException;

 import javax.servlet.ServletRegistration;

 import javax.servlet.ServletRequestAttributeListener;

 import javax.servlet.ServletRequestListener;

+import javax.servlet.ServletSecurityElement;

 import javax.servlet.SessionCookieConfig;

 import javax.servlet.SessionTrackingMode;

+import javax.servlet.annotation.ServletSecurity;

 import javax.servlet.descriptor.JspConfigDescriptor;

 import javax.servlet.http.HttpSessionAttributeListener;

 import javax.servlet.http.HttpSessionListener;

@@ -69,6 +71,7 @@ import org.apache.catalina.Wrapper;

 import org.apache.catalina.connector.Connector;

 import org.apache.catalina.deploy.FilterDef;

 import org.apache.catalina.util.ResourceSet;

+import org.apache.catalina.util.Introspection;

 import org.apache.catalina.util.ServerInfo;

 import org.apache.catalina.util.URLEncoder;

 import org.apache.naming.resources.DirContextURLStreamHandler;

@@ -1186,14 +1189,27 @@ public class ApplicationContext implemen

             }

         }

+        ServletSecurity annotation = null;

         if (servlet == null) {

             wrapper.setServletClass(servletClass);

+            Class clazz = Introspection.loadClass(context, servletClass);

+            if (clazz != null) {

+                annotation = clazz.getAnnotation(ServletSecurity.class);

+            }

         } else {

             wrapper.setServletClass(servlet.getClass().getName());

             wrapper.setServlet(servlet);

+            if (context.wasCreatedDynamicServlet(servlet)) {

+                annotation = servlet.getClass().getAnnotation(ServletSecurity.class);

+            }

         }

-        return context.dynamicServletAdded(wrapper);

+        ServletRegistration.Dynamic registration =

+                new ApplicationServletRegistration(wrapper, context);

+        if (annotation != null) {

+            registration.setServletSecurity(new ServletSecurityElement(annotation));

+        }

+        return registration;

     }

diff -up java/org/apache/catalina/core/ApplicationServletRegistration.java.orig java/org/apache/catalina/core/ApplicationServletRegistration.java

--- java/org/apache/catalina/core/ApplicationServletRegistration.java.orig	2019-02-28 15:41:52.568346028 -0500

+++ java/org/apache/catalina/core/ApplicationServletRegistration.java	2019-02-28 15:44:09.130930248 -0500

@@ -5,9 +5,9 @@

  * 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.

@@ -41,15 +41,16 @@ public class ApplicationServletRegistrat

      */

     private static final StringManager sm =

       StringManager.getManager(Constants.Package);

-    

-    private Wrapper wrapper;

-    private Context context;

-    

+

+    private final Wrapper wrapper;

+    private final Context context;

+    private ServletSecurityElement constraint;

+

     public ApplicationServletRegistration(Wrapper wrapper,

             Context context) {

         this.wrapper = wrapper;

         this.context = context;

-        

+

     }

     @Override

@@ -65,9 +66,9 @@ public class ApplicationServletRegistrat

     @Override

     public Map<String, String> getInitParameters() {

         ParameterMap<String,String> result = new ParameterMap<String,String>();

-        

+

         String[] parameterNames = wrapper.findInitParameters();

-        

+

         for (String parameterName : parameterNames) {

             result.put(parameterName, wrapper.findInitParameter(parameterName));

         }

@@ -91,7 +92,7 @@ public class ApplicationServletRegistrat

         if (getInitParameter(name) != null) {

             return false;

         }

-        

+

         wrapper.addInitParameter(name, value);

         return true;

@@ -99,9 +100,9 @@ public class ApplicationServletRegistrat

     @Override

     public Set<String> setInitParameters(Map<String, String> initParameters) {

-        

+

         Set<String> conflicts = new HashSet<String>();

-        

+

         for (Map.Entry<String, String> entry : initParameters.entrySet()) {

             if (entry.getKey() == null || entry.getValue() == null) {

                 throw new IllegalArgumentException(sm.getString(

@@ -151,13 +152,14 @@ public class ApplicationServletRegistrat

                     "applicationServletRegistration.setServletSecurity.iae",

                     getName(), context.getName()));

         }

-        

+

         if (!context.getState().equals(LifecycleState.STARTING_PREP)) {

             throw new IllegalStateException(sm.getString(

                     "applicationServletRegistration.setServletSecurity.ise",

                     getName(), context.getName()));

         }

+        this.constraint = constraint;

         return context.addServletSecurity(this, constraint);

     }

@@ -167,9 +169,9 @@ public class ApplicationServletRegistrat

         if (urlPatterns == null) {

             return Collections.emptySet();

         }

-        

+

         Set<String> conflicts = new HashSet<String>();

-        

+

         for (String urlPattern : urlPatterns) {

             String wrapperName = context.findServletMapping(urlPattern);

             if (wrapperName != null) {

@@ -187,10 +189,15 @@ public class ApplicationServletRegistrat

         if (!conflicts.isEmpty()) {

             return conflicts;

         }

-        

+

         for (String urlPattern : urlPatterns) {

             context.addServletMapping(urlPattern, wrapper.getName());

         }

+

+        if (constraint != null) {

+            context.addServletSecurity(this, constraint);

+        }

+

         return Collections.emptySet();

     }

@@ -199,7 +206,7 @@ public class ApplicationServletRegistrat

         Set<String> result = new HashSet<String>();

         String servletName = wrapper.getName();

-        

+

         String[] urlPatterns = context.findServletMappings();

         for (String urlPattern : urlPatterns) {

             String name = context.findServletMapping(urlPattern);

@@ -214,5 +221,5 @@ public class ApplicationServletRegistrat

     public String getRunAsRole() {

         return wrapper.getRunAs();

     }

-    

+

 }

diff -up java/org/apache/catalina/core/StandardContext.java.orig java/org/apache/catalina/core/StandardContext.java

--- java/org/apache/catalina/core/StandardContext.java.orig	2017-03-09 08:51:39.000000000 -0500

+++ java/org/apache/catalina/core/StandardContext.java	2019-02-28 15:44:09.132930242 -0500

@@ -4813,27 +4813,36 @@ public class StandardContext extends Con

     }

     /**

-     * hook to register that we need to scan for security annotations.

-     * @param wrapper   The wrapper for the Servlet that was added

+     * Create a servlet registration.

+     *

+     * @param wrapper The wrapper for which the registration should be created.

+     *

+     * @return An appropriate registration

+     *

+     * @deprecated This will be removed in Tomcat 9. The registration should be

+     *             created directly.

      */

+    @Deprecated

     public ServletRegistration.Dynamic dynamicServletAdded(Wrapper wrapper) {

-        Servlet s = wrapper.getServlet();

-        if (s != null && createdServlets.contains(s)) {

-            // Mark the wrapper to indicate annotations need to be scanned

-            wrapper.setServletSecurityAnnotationScanRequired(true);

-        }

         return new ApplicationServletRegistration(wrapper, this);

     }

     /**

-     * hook to track which registrations need annotation scanning

-     * @param servlet

+     * Hook to track which Servlets were created via

+     * {@link ServletContext#createServlet(Class)}.

+     *

+     * @param servlet the created Servlet

      */

     public void dynamicServletCreated(Servlet servlet) {

         createdServlets.add(servlet);

     }

+    public boolean wasCreatedDynamicServlet(Servlet servlet) {

+        return createdServlets.contains(servlet);

+    }

+

+

     /**

      * A helper class to manage the filter mappings in a Context.

      */

diff -up java/org/apache/catalina/core/StandardWrapper.java.orig java/org/apache/catalina/core/StandardWrapper.java

--- java/org/apache/catalina/core/StandardWrapper.java.orig	2019-02-28 15:41:52.576346004 -0500

+++ java/org/apache/catalina/core/StandardWrapper.java	2019-02-28 15:44:09.133930239 -0500

@@ -5,17 +5,15 @@

  * 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.PrintStream;

@@ -44,11 +42,9 @@ 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;

@@ -114,12 +110,12 @@ public class StandardWrapper extends Con

      * servlet is considered permanent.

      */

     protected long available = 0L;

-    

+

     /**

-     * The broadcaster that sends j2ee notifications. 

+     * The broadcaster that sends j2ee notifications.

      */

     protected NotificationBroadcasterSupport broadcaster = null;

-    

+

     /**

      * The count of allocations that are currently active (even if they

      * are for the same instance, as will be true on a non-STM servlet).

@@ -231,12 +227,12 @@ public class StandardWrapper extends Con

      */

     protected Stack<Servlet> instancePool = null;

-    

+

     /**

      * Wait time for servlet unload in ms.

      */

     protected long unloadDelay = 2000;

-    

+

     /**

      * True if this StandardWrapper is for the JspServlet

@@ -259,12 +255,12 @@ public class StandardWrapper extends Con

     protected StandardWrapperValve swValve;

     protected long loadTime=0;

     protected int classLoadTime=0;

-    

+

     /**

      * Multipart config

      */

     protected MultipartConfigElement multipartConfigElement = null;

-    

+

     /**

      * Async support

      */

@@ -275,26 +271,24 @@ public class StandardWrapper extends Con

      */

     protected boolean enabled = true;

-    protected volatile boolean servletSecurityAnnotationScanRequired = false;

-

     private boolean overridable = false;

-    

+

     /**

-     * Static class array used when the SecurityManager is turned on and 

+     * Static class array used when the SecurityManager is turned on and

      * Servlet.init is invoked.

      */

     protected static Class[] classType = new Class[]{ServletConfig.class};

-    

-    

+

+

     /**

-     * Static class array used when the SecurityManager is turned on and 

+     * Static class array used when the SecurityManager is turned on and

      * Servlet.service  is invoked.

-     */                                                 

+     */

     @Deprecated

     protected static Class[] classTypeUsedInService = new Class[]{

                                                          ServletRequest.class,

                                                          ServletResponse.class};

-    

+

     private final ReentrantReadWriteLock parametersLock =

             new ReentrantReadWriteLock();

@@ -625,7 +619,7 @@ public class StandardWrapper extends Con

     public String[] getServletMethods() throws ServletException {

         instance = loadServlet();

-        

+

         Class servletClazz = instance.getClass();

         if (!javax.servlet.http.HttpServlet.class.isAssignableFrom(

                                                         servletClazz)) {

@@ -665,8 +659,8 @@ public class StandardWrapper extends Con

     public Servlet getServlet() {

         return instance;

     }

-    

-    

+

+

     /**

      * Set the associated servlet instance.

      */

@@ -675,13 +669,13 @@ public class StandardWrapper extends Con

         instance = servlet;

     }

-    

+

     /**

      * {@inheritDoc}

      */

     @Override

     public void setServletSecurityAnnotationScanRequired(boolean b) {

-        this.servletSecurityAnnotationScanRequired = b;

+        // NO-OP

     }

     // --------------------------------------------------------- Public Methods

@@ -695,19 +689,19 @@ public class StandardWrapper extends Con

     @Override

     public void backgroundProcess() {

         super.backgroundProcess();

-        

+

         if (!getState().isAvailable())

             return;

-        

+

         if (getServlet() != null && (getServlet() instanceof PeriodicEventListener)) {

             ((PeriodicEventListener) getServlet()).periodicEvent();

         }

     }

-    

-    

+

+

     /**

      * Extract the root cause from a servlet exception.

-     * 

+     *

      * @param e The servlet exception

      */

     public static Throwable getRootCause(ServletException e) {

@@ -836,7 +830,7 @@ public class StandardWrapper extends Con

         }

         boolean newInstance = false;

-        

+

         // If not SingleThreadedModel, return the same instance every time

         if (!singleThreadModel) {

             // Load and initialize our instance if necessary

@@ -1070,7 +1064,7 @@ public class StandardWrapper extends Con

     @Override

     public synchronized void load() throws ServletException {

         instance = loadServlet();

-        

+

         if (!instanceInitialized) {

             initServlet(instance);

         }

@@ -1078,12 +1072,12 @@ public class StandardWrapper extends Con

         if (isJspServlet) {

             StringBuilder oname =

                 new StringBuilder(MBeanUtils.getDomain(getParent()));

-            

+

             oname.append(":type=JspMonitor,name=");

             oname.append(getName());

-            

+

             oname.append(getWebModuleKeyProperties());

-            

+

             try {

                 jspMonitorON = new ObjectName(oname.toString());

                 Registry.getRegistry(null, null)

@@ -1161,8 +1155,6 @@ public class StandardWrapper extends Con

                 }

             }

-            processServletSecurityAnnotation(servlet.getClass());

-

             // Special handling for ContainerServlet instances

             if ((servlet instanceof ContainerServlet) &&

                     (isContainerProvidedServlet(servletClass) ||

@@ -1205,44 +1197,13 @@ public class StandardWrapper extends Con

      */

     @Override

     public void servletSecurityAnnotationScan() throws ServletException {

-        if (getServlet() == null) {

-            Class clazz = null;

-            try {

-                clazz = getParent().getLoader().getClassLoader().loadClass(

-                        getServletClass());

-                processServletSecurityAnnotation(clazz);

-            } catch (ClassNotFoundException e) {

-                // Safe to ignore. No class means no annotations to process

-            }

-        } else {

-            if (servletSecurityAnnotationScanRequired) {

-                processServletSecurityAnnotation(getServlet().getClass());

-            }

-        }

+        // NO-OP

     }

-    private void processServletSecurityAnnotation(Class clazz) {

-        // Calling this twice isn't harmful so no syncs

-        servletSecurityAnnotationScanRequired = false;

-

-        Context ctxt = (Context) getParent();

-        

-        if (ctxt.getIgnoreAnnotations()) {

-            return;

-        }

-

-        ServletSecurity secAnnotation =

-            clazz.getAnnotation(ServletSecurity.class);

-        if (secAnnotation != null) {

-            ctxt.addServletSecurity(

-                    new ApplicationServletRegistration(this, ctxt),

-                    new ServletSecurityElement(secAnnotation));

-        }

-    }

     private synchronized void initServlet(Servlet servlet)

             throws ServletException {

-        

+

         if (instanceInitialized && !singleThreadModel) return;

         // Call the initialization method of this servlet

@@ -1454,12 +1415,12 @@ public class StandardWrapper extends Con

             if (swallowOutput) {

                 SystemLogHandler.startCapture();

             }

-    

+

             // Call the servlet destroy() method

             try {

                 instanceSupport.fireInstanceEvent

                   (InstanceEvent.BEFORE_DESTROY_EVENT, instance);

-    

+

                 if( Globals.IS_SECURITY_ENABLED) {

                     try {

                         SecurityUtil.doAsPrivilege("destroy",

@@ -1470,7 +1431,7 @@ public class StandardWrapper extends Con

                 } else {

                     instance.destroy();

                 }

-                

+

                 instanceSupport.fireInstanceEvent

                   (InstanceEvent.AFTER_DESTROY_EVENT, instance);

@@ -1701,7 +1662,7 @@ public class StandardWrapper extends Con

     public boolean isAsyncSupported() {

         return asyncSupported;

     }

-    

+

     @Override

     public void setAsyncSupported(boolean asyncSupported) {

         this.asyncSupported = asyncSupported;

@@ -1711,7 +1672,7 @@ public class StandardWrapper extends Con

     public boolean isEnabled() {

         return enabled;

     }

-    

+

     @Override

     public void setEnabled(boolean enabled) {

         this.enabled = enabled;

@@ -1787,24 +1748,24 @@ public class StandardWrapper extends Con

      */

     @Override

     protected synchronized void startInternal() throws LifecycleException {

-    

-        // Send j2ee.state.starting notification 

+

+        // Send j2ee.state.starting notification

         if (this.getObjectName() != null) {

-            Notification notification = new Notification("j2ee.state.starting", 

-                                                        this.getObjectName(), 

+            Notification notification = new Notification("j2ee.state.starting",

+                                                        this.getObjectName(),

                                                         sequenceNumber++);

             broadcaster.sendNotification(notification);

         }

-        

+

         // Start up this component

         super.startInternal();

         setAvailable(0L);

-        // Send j2ee.state.running notification 

+        // Send j2ee.state.running notification

         if (this.getObjectName() != null) {

-            Notification notification = 

-                new Notification("j2ee.state.running", this.getObjectName(), 

+            Notification notification =

+                new Notification("j2ee.state.running", this.getObjectName(),

                                 sequenceNumber++);

             broadcaster.sendNotification(notification);

         }

@@ -1823,15 +1784,15 @@ public class StandardWrapper extends Con

     protected synchronized void stopInternal() throws LifecycleException {

         setAvailable(Long.MAX_VALUE);

-        

-        // Send j2ee.state.stopping notification 

+

+        // Send j2ee.state.stopping notification

         if (this.getObjectName() != null) {

-            Notification notification = 

-                new Notification("j2ee.state.stopping", this.getObjectName(), 

+            Notification notification =

+                new Notification("j2ee.state.stopping", this.getObjectName(),

                                 sequenceNumber++);

             broadcaster.sendNotification(notification);

         }

-        

+

         // Shut down our servlet instance (if it has been initialized)

         try {

             unload();

@@ -1843,43 +1804,43 @@ public class StandardWrapper extends Con

         // Shut down this component

         super.stopInternal();

-        // Send j2ee.state.stopped notification 

+        // Send j2ee.state.stopped notification

         if (this.getObjectName() != null) {

-            Notification notification = 

-                new Notification("j2ee.state.stopped", this.getObjectName(), 

+            Notification notification =

+                new Notification("j2ee.state.stopped", this.getObjectName(),

                                 sequenceNumber++);

             broadcaster.sendNotification(notification);

         }

-        

-        // Send j2ee.object.deleted notification 

-        Notification notification = 

-            new Notification("j2ee.object.deleted", this.getObjectName(), 

+

+        // Send j2ee.object.deleted notification

+        Notification notification =

+            new Notification("j2ee.object.deleted", this.getObjectName(),

                             sequenceNumber++);

         broadcaster.sendNotification(notification);

     }

-    

+

     @Override

     protected String getObjectNameKeyProperties() {

         StringBuilder keyProperties =

             new StringBuilder("j2eeType=Servlet,name=");

-        

+

         String name = getName();

         if (Util.objectNameValueNeedsQuote(name)) {

             name = ObjectName.quote(name);

         }

         keyProperties.append(name);

-        

+

         keyProperties.append(getWebModuleKeyProperties());

         return keyProperties.toString();

     }

-        

+

     private String getWebModuleKeyProperties() {

-        

+

         StringBuilder keyProperties = new StringBuilder(",WebModule=//");

         String hostName = getParent().getParent().getName();

         if (hostName == null) {

@@ -1887,7 +1848,7 @@ public class StandardWrapper extends Con

         } else {

             keyProperties.append(hostName);

         }

-        

+

         String contextName = ((Context) getParent()).getName();

         if (!contextName.startsWith("/")) {

             keyProperties.append('/');

@@ -1898,7 +1859,7 @@ public class StandardWrapper extends Con

         if (parent instanceof StandardContext) {

             ctx = (StandardContext) getParent();

         }

-        

+

         keyProperties.append(",J2EEApplication=");

         if (ctx == null) {

             keyProperties.append("none");

@@ -1911,10 +1872,10 @@ public class StandardWrapper extends Con

         } else {

             keyProperties.append(ctx.getJ2EEServer());

         }

-        

+

         return keyProperties.toString();

     }

-    

+

     /**

      * JSR 77. Always return false.

@@ -1922,19 +1883,19 @@ public class StandardWrapper extends Con

     public boolean isStateManageable() {

         return false;

     }