Issue #5129 - WebAppContext.setExtraClasspath(String) cleanup

+ More tests for both relative and absolute path references
+ More testing that will trigger quirks on Windows builds
  so that we can catch regressions faster
+ Reworked WebInfConfiguration to be glob aware in a way
  similar to how WebAppClassLoader behaves.
+ Reworked Resource.newResource(String) to delegate
  canonical path resolution to PathResource
+ Guarded PathResource's usage of Path.toAbsolutePath()
  to ignore valid conditions where the Path cannot be
  resolved to an absolute path (yet)
+ Normalize resolved paths in PathResource

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java

@@ -19,6 +19,7 @@
 package org.eclipse.jetty.util.resource;
 
 import java.io.File;
+import java.io.IOError;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -184,7 +185,7 @@ public class PathResource extends Resource
             // different number of segments
             return false;
         }
-        
+
         // compare each segment of path, backwards
         for (int i = bCount; i-- > 0; )
         {
@@ -193,7 +194,7 @@ public class PathResource extends Resource
                 return false;
             }
         }
-        
+
         return true;
     }
 
@@ -226,7 +227,23 @@ public class PathResource extends Resource
      */
     public PathResource(Path path)
     {
-        this.path = path.toAbsolutePath();
+        Path absPath = path;
+        try
+        {
+            absPath = path.toAbsolutePath();
+        }
+        catch (IOError ioError)
+        {
+            // Not able to resolve absolute path from provided path
+            // This could be due to a glob reference, or a reference
+            // to a path that doesn't exist (yet)
+            if (LOG.isDebugEnabled())
+                LOG.debug("Unable to get absolute path for {}", path, ioError);
+        }
+
+        // cleanup any lingering relative path nonsense (like "/./" and "/../")
+        this.path = absPath.normalize();
+
         assertValidPath(path);
         this.uri = this.path.toUri();
         this.alias = checkAliasPath();

jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java

@@ -29,6 +29,7 @@ import java.net.URI;
 import java.net.URL;
 import java.nio.channels.ReadableByteChannel;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.text.DateFormat;
 import java.util.ArrayList;
 import java.util.Base64;
@@ -174,19 +175,8 @@ public abstract class Resource implements ResourceFactory, Closeable
                 !resource.startsWith("file:") &&
                 !resource.startsWith("jar:"))
             {
-                try
-                {
-                    // It's a file.
-                    if (resource.startsWith("./"))
-                        resource = resource.substring(2);
-                    File file = new File(resource).getCanonicalFile();
-                    return new PathResource(file);
-                }
-                catch (IOException e2)
-                {
-                    e2.addSuppressed(e);
-                    throw e2;
-                }
+                // It's likely a file/path reference.
+                return new PathResource(Paths.get(resource));
             }
             else
             {

jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java

@@ -19,9 +19,11 @@
 package org.eclipse.jetty.util.resource;
 
 import java.io.File;
+import java.io.IOException;
 import java.io.InputStream;
 import java.net.URI;
 import java.net.URL;
+import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.stream.Stream;
 
@@ -30,6 +32,7 @@ import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
 import org.eclipse.jetty.util.IO;
 import org.hamcrest.Matchers;
 import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
@@ -279,4 +282,14 @@ public class ResourceTest
         String c = IO.toString(in);
         assertThat("Content: " + data.test, c, startsWith(data.content));
     }
+
+    @Test
+    public void testGlobPath() throws IOException
+    {
+        Path testDir = MavenTestingUtils.getTargetTestingPath("testGlobPath");
+        FS.ensureEmpty(testDir);
+
+        String globReference = testDir.toAbsolutePath().toString() + File.separator + '*';
+        Resource globResource = Resource.newResource(globReference);
+    }
 }

jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java

@@ -947,13 +947,47 @@ public class WebInfConfiguration extends AbstractConfiguration
         StringTokenizer tokenizer = new StringTokenizer(context.getExtraClasspath(), ",;");
         while (tokenizer.hasMoreTokens())
         {
-            Resource resource = context.newResource(tokenizer.nextToken().trim());
-            String fnlc = resource.getName().toLowerCase(Locale.ENGLISH);
-            int dot = fnlc.lastIndexOf('.');
-            String extension = (dot < 0 ? null : fnlc.substring(dot));
-            if (extension != null && (extension.equals(".jar") || extension.equals(".zip")))
+            String token = tokenizer.nextToken().trim();
+
+            // Is this a Glob Reference?
+            if (isGlobReference(token))
             {
-                jarResources.add(resource);
+                String dir = token.substring(0, token.length() - 2);
+                // Use directory
+                Resource dirResource = context.newResource(dir);
+                if (dirResource.exists() && dirResource.isDirectory())
+                {
+                    // To obtain the list of files
+                    String[] files = dirResource.list();
+                    if (files != null)
+                    {
+                        Arrays.sort(files);
+                        for (String file : files)
+                        {
+                            try
+                            {
+                                Resource fileResource = dirResource.addPath(file);
+                                if (isFileSupported(fileResource))
+                                {
+                                    jarResources.add(fileResource);
+                                }
+                            }
+                            catch (Exception ex)
+                            {
+                                LOG.warn(Log.EXCEPTION, ex);
+                            }
+                        }
+                    }
+                }
+            }
+            else
+            {
+                // Simple reference, add as-is
+                Resource resource = context.newResource(token);
+                if (isFileSupported(resource))
+                {
+                    jarResources.add(resource);
+                }
             }
         }
 
@@ -1003,11 +1037,28 @@ public class WebInfConfiguration extends AbstractConfiguration
         StringTokenizer tokenizer = new StringTokenizer(context.getExtraClasspath(), ",;");
         while (tokenizer.hasMoreTokens())
         {
-            Resource resource = context.newResource(tokenizer.nextToken().trim());
-            if (resource.exists() && resource.isDirectory())
-                dirResources.add(resource);
+            String token = tokenizer.nextToken().trim();
+            if (!isGlobReference(token))
+            {
+                Resource resource = context.newResource(token);
+                if (resource.exists() && resource.isDirectory())
+                    dirResources.add(resource);
+            }
         }
 
         return dirResources;
     }
+
+    private boolean isGlobReference(String token)
+    {
+        return token.endsWith("/*") || token.endsWith("\\*");
+    }
+
+    private boolean isFileSupported(Resource resource)
+    {
+        String filenameLowercase = resource.getName().toLowerCase(Locale.ENGLISH);
+        int dot = filenameLowercase.lastIndexOf('.');
+        String extension = (dot < 0 ? null : filenameLowercase.substring(dot));
+        return (extension != null && (extension.equals(".jar") || extension.equals(".zip")));
+    }
 }

jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextExtraClasspathTest.java

@@ -0,0 +1,215 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.webapp;
+
+import java.io.File;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
+import org.eclipse.jetty.util.component.LifeCycle;
+import org.eclipse.jetty.util.resource.PathResource;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class WebAppContextExtraClasspathTest
+{
+    private Server server;
+
+    private Server newServer()
+    {
+        server = new Server();
+        ServerConnector connector = new ServerConnector(server);
+        connector.setPort(0);
+        server.addConnector(connector);
+        return server;
+    }
+
+    @AfterEach
+    public void tearDown()
+    {
+        LifeCycle.stop(server);
+    }
+
+    @Test
+    public void testBaseResourceAbsolutePath() throws Exception
+    {
+        Server server = newServer();
+
+        WebAppContext context = new WebAppContext();
+        context.setContextPath("/");
+
+        Path warPath = MavenTestingUtils.getTestResourcePathFile("wars/dump.war");
+        warPath = warPath.toAbsolutePath();
+        assertTrue(warPath.isAbsolute(), "Path should be absolute: " + warPath);
+        // Use String reference to war
+        // On Unix / Linux this should have no issue.
+        // On Windows with fully qualified paths such as "E:\mybase\webapps\dump.war" the
+        // resolution of the Resource can trigger various URI issues with the "E:" portion of the provided String.
+        context.setResourceBase(warPath.toString());
+
+        server.setHandler(context);
+        server.start();
+
+        assertTrue(context.isAvailable(), "WebAppContext should be available");
+    }
+
+    public static Stream<Arguments> extraClasspathGlob()
+    {
+        List<Arguments> references = new ArrayList<>();
+
+        Path extLibs = MavenTestingUtils.getTestResourcePathDir("ext");
+        extLibs = extLibs.toAbsolutePath();
+
+        // Absolute reference with trailing slash and glob
+        references.add(Arguments.of(extLibs.toString() + File.separator + "*"));
+
+        // Establish a relative extraClassPath reference
+        String relativeExtLibsDir = MavenTestingUtils.getBasePath().relativize(extLibs).toString();
+
+        // This will be in the String form similar to "src/test/resources/ext/*" (with trailing slash and glob)
+        references.add(Arguments.of(relativeExtLibsDir + File.separator + "*"));
+
+        return references.stream();
+    }
+
+    /**
+     * Test using WebAppContext.setExtraClassPath(String) with a reference to a glob
+     */
+    @ParameterizedTest
+    @MethodSource("extraClasspathGlob")
+    public void testExtraClasspathGlob(String extraClasspathGlobReference) throws Exception
+    {
+        Server server = newServer();
+
+        WebAppContext context = new WebAppContext();
+        context.setContextPath("/");
+        Path warPath = MavenTestingUtils.getTestResourcePathFile("wars/dump.war");
+        context.setBaseResource(new PathResource(warPath));
+        context.setExtraClasspath(extraClasspathGlobReference);
+
+        server.setHandler(context);
+        server.start();
+
+        // Should not have failed the start of the WebAppContext
+        assertTrue(context.isAvailable(), "WebAppContext should be available");
+
+        // Test WebAppClassLoader contents for expected jars
+        ClassLoader contextClassLoader = context.getClassLoader();
+        assertThat(contextClassLoader, instanceOf(WebAppClassLoader.class));
+        WebAppClassLoader webAppClassLoader = (WebAppClassLoader)contextClassLoader;
+        Path extLibsDir = MavenTestingUtils.getTestResourcePathDir("ext");
+        extLibsDir = extLibsDir.toAbsolutePath();
+        List<Path> expectedPaths = Files.list(extLibsDir)
+            .filter((path) -> path.toString().endsWith(".jar"))
+            .collect(Collectors.toList());
+        List<Path> actualPaths = new ArrayList<>();
+        for (URL url : webAppClassLoader.getURLs())
+        {
+            actualPaths.add(Paths.get(url.toURI()));
+        }
+        assertThat("WebAppClassLoader.urls.length", actualPaths.size(), is(expectedPaths.size()));
+        for (Path expectedPath : expectedPaths)
+        {
+            boolean found = false;
+            for (Path actualPath : actualPaths)
+            {
+                if (Files.isSameFile(actualPath, expectedPath))
+                {
+                    found = true;
+                }
+            }
+            assertTrue(found, "Not able to find expected jar in WebAppClassLoader: " + expectedPath);
+        }
+    }
+
+    public static Stream<Arguments> extraClasspathDir()
+    {
+        List<Arguments> references = new ArrayList<>();
+
+        Path extLibs = MavenTestingUtils.getTestResourcePathDir("ext");
+        extLibs = extLibs.toAbsolutePath();
+
+        // Absolute reference with trailing slash
+        references.add(Arguments.of(extLibs.toString() + File.separator));
+
+        // Absolute reference without trailing slash
+        references.add(Arguments.of(extLibs.toString()));
+
+        // Establish a relative extraClassPath reference
+        String relativeExtLibsDir = MavenTestingUtils.getBasePath().relativize(extLibs).toString();
+
+        // This will be in the String form similar to "src/test/resources/ext/" (with trailing slash)
+        references.add(Arguments.of(relativeExtLibsDir + File.separator));
+
+        // This will be in the String form similar to "src/test/resources/ext/" (without trailing slash)
+        references.add(Arguments.of(relativeExtLibsDir));
+
+        return references.stream();
+    }
+
+    /**
+     * Test using WebAppContext.setExtraClassPath(String) with a reference to a directory
+     */
+    @ParameterizedTest
+    @MethodSource("extraClasspathDir")
+    public void testExtraClasspathDir(String extraClassPathReference) throws Exception
+    {
+        Server server = newServer();
+
+        WebAppContext context = new WebAppContext();
+        context.setContextPath("/");
+        Path warPath = MavenTestingUtils.getTestResourcePathFile("wars/dump.war");
+        context.setBaseResource(new PathResource(warPath));
+
+        context.setExtraClasspath(extraClassPathReference);
+
+        server.setHandler(context);
+        server.start();
+
+        // Should not have failed the start of the WebAppContext
+        assertTrue(context.isAvailable(), "WebAppContext should be available");
+
+        // Test WebAppClassLoader contents for expected directory reference
+        ClassLoader contextClassLoader = context.getClassLoader();
+        assertThat(contextClassLoader, instanceOf(WebAppClassLoader.class));
+        WebAppClassLoader webAppClassLoader = (WebAppClassLoader)contextClassLoader;
+        URL[] urls = webAppClassLoader.getURLs();
+        assertThat("URLs", urls.length, is(1));
+        Path extLibs = MavenTestingUtils.getTestResourcePathDir("ext");
+        extLibs = extLibs.toAbsolutePath();
+        assertThat("URL[0]", urls[0].toURI(), is(extLibs.toUri()));
+    }
+}

jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java

@@ -52,6 +52,7 @@ import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsString;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -256,7 +257,7 @@ public class WebAppContextTest
         try
         {
             String response = connector.getResponse("GET http://localhost:8080 HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n");
-            assertTrue(response.indexOf("200 OK") >= 0);
+            assertThat("Response OK", response, containsString("200 OK"));
         }
         finally
         {