CrossOriginFilter: performance: allowedMethods

Avoid recreating for each request the value of the header Access-Control-Allow-Method. Cheaper matching: Previously an array list was iterated to try to match the method. Now a single Pattern is used. Related to: jetty#1053 (Review CrossOriginFilter) Might conflict with: jetty#1952 (CrossOriginFilter performace: reuse matcher) Signed-off-by: Juan F. Codagnone <[email protected]>
jcodagnone · Nov 8, 2017 · 87f7a07 · 87f7a07
1 parent d1e5883
commit 87f7a07
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 8 deletions.
diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/CrossOriginFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/CrossOriginFilter.java
@@ -159,7 +159,8 @@ public class CrossOriginFilter implements Filter
     private boolean anyHeadersAllowed;
     private List<String> allowedOrigins = new ArrayList<String>();
     private List<String> allowedTimingOrigins = new ArrayList<String>();
-    private List<String> allowedMethods = new ArrayList<String>();
+    private String  allowedMethodsCommify;
+    private Pattern allowedMethodsPattern;
     private List<String> allowedHeaders = new ArrayList<String>();
     private List<String> exposedHeaders = new ArrayList<String>();
     private int preflightMaxAge;
@@ -175,10 +176,13 @@ public void init(FilterConfig config) throws ServletException
         anyTimingOriginAllowed = generateAllowedOrigins(allowedTimingOrigins, allowedTimingOriginsConfig, DEFAULT_ALLOWED_TIMING_ORIGINS);
 
         String allowedMethodsConfig = config.getInitParameter(ALLOWED_METHODS_PARAM);
+        List<String> allowedMethods;
         if (allowedMethodsConfig == null)
-            allowedMethods.addAll(DEFAULT_ALLOWED_METHODS);
+            allowedMethods = DEFAULT_ALLOWED_METHODS;
         else
-            allowedMethods.addAll(Arrays.asList(StringUtil.csvSplit(allowedMethodsConfig)));
+            allowedMethods = Arrays.asList(StringUtil.csvSplit(allowedMethodsConfig));
+        allowedMethodsCommify = commify(allowedMethods);
+        allowedMethodsPattern = patternize(allowedMethods);
 
         String allowedHeadersConfig = config.getInitParameter(ALLOWED_HEADERS_PARAM);
         if (allowedHeadersConfig == null)
@@ -423,7 +427,7 @@ private void handlePreflightResponse(HttpServletRequest request, HttpServletResp
             response.setHeader(ACCESS_CONTROL_ALLOW_CREDENTIALS_HEADER, "true");
         if (preflightMaxAge > 0)
             response.setHeader(ACCESS_CONTROL_MAX_AGE_HEADER, String.valueOf(preflightMaxAge));
-        response.setHeader(ACCESS_CONTROL_ALLOW_METHODS_HEADER, commify(allowedMethods));
+        response.setHeader(ACCESS_CONTROL_ALLOW_METHODS_HEADER, allowedMethodsCommify);
         if (anyHeadersAllowed)
             response.setHeader(ACCESS_CONTROL_ALLOW_HEADERS_HEADER, commify(headersRequested));
         else
@@ -434,10 +438,16 @@ private boolean isMethodAllowed(HttpServletRequest request)
     {
         String accessControlRequestMethod = request.getHeader(ACCESS_CONTROL_REQUEST_METHOD_HEADER);
         LOG.debug("{} is {}", ACCESS_CONTROL_REQUEST_METHOD_HEADER, accessControlRequestMethod);
+        boolean result = isMethodAllowed(allowedMethodsPattern, accessControlRequestMethod);
+        LOG.debug("Method {} is" + (result ? "" : " not") + " among allowed methods {}", accessControlRequestMethod, allowedMethodsCommify);
+        return result;
+    }
+
+    static boolean isMethodAllowed(final Pattern allowedMethodsPattern, final String method) 
+    {
         boolean result = false;
-        if (accessControlRequestMethod != null)
-            result = allowedMethods.contains(accessControlRequestMethod);
-        LOG.debug("Method {} is" + (result ? "" : " not") + " among allowed methods {}", accessControlRequestMethod, allowedMethods);
+        if (allowedMethodsPattern != null && method != null)
+            result = allowedMethodsPattern.matcher(method).matches();
         return result;
     }
 
@@ -501,11 +511,41 @@ private String commify(List<String> strings)
         return builder.toString();
     }
 
+    /** 
+     * creates a pattern to match any of a list of  string
+     * If the input list is ["foo, "bar"] it will generate
+     * a Pattern for the regex "(\\Efoo\\E|\\Ebar\\E) 
+     */
+    static Pattern patternize(final List<String> strings) 
+    {
+        final StringBuilder builder = new StringBuilder();
+        for (int i = 0; i < strings.size(); ++i)
+        {
+            final String string = strings.get(i);
+            if (string.trim().isEmpty()) break;
+
+            if (builder.length() == 0) 
+                builder.append('(');
+            else
+                builder.append('|');
+
+            builder.append(Pattern.quote(string));
+        }
+        Pattern ret;
+        if (builder.length() > 0) 
+            ret = Pattern.compile(builder.append(")").toString());
+        else 
+            ret = null;
+
+        return ret;
+    }
+
     public void destroy()
     {
         anyOriginAllowed = false;
         allowedOrigins.clear();
-        allowedMethods.clear();
+        allowedMethodsCommify = null;
+        allowedMethodsPattern = null;
         allowedHeaders.clear();
         preflightMaxAge = 0;
         allowCredentials = false;

diff --git a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/CrossOriginFilterTest.java b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/CrossOriginFilterTest.java
@@ -19,9 +19,11 @@
 package org.eclipse.jetty.servlets;
 
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.regex.Pattern;
 
 import javax.servlet.DispatcherType;
 import javax.servlet.ServletException;
@@ -565,6 +567,25 @@ public void testChainPreflightRequest() throws Exception
         Assert.assertFalse(latch.await(1, TimeUnit.SECONDS));
     }
 
+
+    @Test
+    public void testPatternize()
+    {
+        Pattern p = CrossOriginFilter.patternize(Arrays.asList("GET", "POST", "   "));
+        Assert.assertTrue (CrossOriginFilter.isMethodAllowed(p, "GET"));
+        Assert.assertTrue (CrossOriginFilter.isMethodAllowed(p, "POST"));
+        Assert.assertFalse(CrossOriginFilter.isMethodAllowed(p, "PUT"));
+        Assert.assertFalse(CrossOriginFilter.isMethodAllowed(p, "GETA"));
+        Assert.assertFalse(CrossOriginFilter.isMethodAllowed(p, "AGET"));
+        Assert.assertFalse(CrossOriginFilter.isMethodAllowed(p, ""));
+
+
+        p = CrossOriginFilter.patternize(Arrays.asList("  ", "   "));
+        Assert.assertNull(p);
+        Assert.assertFalse(CrossOriginFilter.isMethodAllowed(p, "AGET"));
+        Assert.assertFalse(CrossOriginFilter.isMethodAllowed(p, ""));
+    }
+
     public static class ResourceServlet extends HttpServlet
     {
         private static final long serialVersionUID = 1L;