Issue #6497 - Improve comments, re-implement the hasSymbolicLink method.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java

@@ -90,6 +90,18 @@ public boolean check(String uri, Resource resource)
         return true;
     }
 
+    /**
+     * <p>Determines whether the provided resource path is protected.</p>
+     *
+     * <p>The resource path is protected if it is under one of the protected targets defined by
+     * {@link ContextHandler#isProtectedTarget(String)} in which case the alias should not be allowed.
+     * The resource path may also attempt to traverse above the root path and should be denied.</p>
+     *
+     * @param resourcePath the resource {@link Path} to be tested.
+     * @param linkOptions an array of {@link LinkOption} to be provided to the {@link Path#toRealPath(LinkOption...)} method.
+     * @return true if the resource path is protected and the alias should not be allowed.
+     * @throws IOException if an I/O error occurs.
+     */
     protected boolean isProtectedPath(Path resourcePath, LinkOption[] linkOptions) throws IOException
     {
         String basePath = Objects.requireNonNull(getPath(_contextHandler.getBaseResource())).toRealPath(linkOptions).toString();
@@ -125,16 +137,14 @@ protected boolean isProtectedPath(Path resourcePath, LinkOption[] linkOptions) t
 
     protected boolean hasSymbolicLink(Path path)
     {
-        // Is file itself a symlink?
-        if (Files.isSymbolicLink(path))
-            return true;
+        return hasSymbolicLink(path.getRoot(), path);
+    }
 
-        // Lets try each path segment
-        Path base = path.getRoot();
-        for (Path segment : path)
+    protected boolean hasSymbolicLink(Path base, Path path)
+    {
+        for (Path p = path; (p != null) && !p.equals(base); p = p.getParent())
         {
-            base = base.resolve(segment);
-            if (Files.isSymbolicLink(base))
+            if (Files.isSymbolicLink(p))
                 return true;
         }
 

jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java

@@ -28,7 +28,7 @@
 import org.eclipse.jetty.util.resource.Resource;
 
 /**
- * An extension of {@link AllowedResourceAliasChecker} which only allows resources if they are symlinks.
+ * An extension of {@link AllowedResourceAliasChecker} which only allows aliased resources if they are symlinks.
  */
 public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChecker
 {
@@ -51,13 +51,12 @@ public boolean check(String uri, Resource resource)
             if (!super.check(uri, resource))
                 return false;
 
-            // Only approve resource if it is accessed by a symbolic link.
+            // Approve if path is a symbolic link.
             Path resourcePath = resource.getFile().toPath();
             if (Files.isSymbolicLink(resourcePath))
                 return true;
 
-            // TODO: If base resource contains symlink then this will always return true.
-            //  But we don't want to deny all paths if the resource base is symbolically linked.
+            // Approve if path has symlink in under its resource base.
             if (super.hasSymbolicLink(resourcePath))
                 return true;
         }

tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java

@@ -40,7 +40,6 @@
 import org.eclipse.jetty.util.resource.PathResource;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
-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;
@@ -142,12 +141,6 @@ public static void afterAll() throws Exception
         _server.stop();
     }
 
-    @Test
-    public void test() throws Exception
-    {
-        _server.join();
-    }
-
     public static Stream<Arguments> testCases()
     {
         AllowedResourceAliasChecker allowedResource = new AllowedResourceAliasChecker(_context, false);
@@ -176,13 +169,13 @@ public static Stream<Arguments> testCases()
                 Arguments.of(allowedResourceTarget, "/webInfSymlink/web.xml", HttpStatus.NOT_FOUND_404, null),
 
                 // SymlinkAllowedResourceAliasChecker that does not check the target of symlinks, but only approves files obtained through a symlink.
-                Arguments.of(allowedResource, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."),
-                Arguments.of(allowedResource, "/symlinkExternalFile", HttpStatus.OK_200, "This file is outside webroot."),
-                Arguments.of(allowedResource, "/symlinkDir/file", HttpStatus.OK_200, "This file is inside webroot/documents."),
-                Arguments.of(allowedResource, "/symlinkParentDir/webroot/file", HttpStatus.OK_200, "This file is inside webroot."),
-                Arguments.of(allowedResource, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.OK_200, "This is the web.xml file."),
-                Arguments.of(allowedResource, "/symlinkSiblingDir/file", HttpStatus.OK_200, "This file is inside a sibling dir to webroot."),
-                Arguments.of(allowedResource, "/webInfSymlink/web.xml", HttpStatus.OK_200, "This is the web.xml file."),
+                Arguments.of(symlinkAllowedResource, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."),
+                Arguments.of(symlinkAllowedResource, "/symlinkExternalFile", HttpStatus.OK_200, "This file is outside webroot."),
+                Arguments.of(symlinkAllowedResource, "/symlinkDir/file", HttpStatus.OK_200, "This file is inside webroot/documents."),
+                Arguments.of(symlinkAllowedResource, "/symlinkParentDir/webroot/file", HttpStatus.OK_200, "This file is inside webroot."),
+                Arguments.of(symlinkAllowedResource, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.OK_200, "This is the web.xml file."),
+                Arguments.of(symlinkAllowedResource, "/symlinkSiblingDir/file", HttpStatus.OK_200, "This file is inside a sibling dir to webroot."),
+                Arguments.of(symlinkAllowedResource, "/webInfSymlink/web.xml", HttpStatus.OK_200, "This is the web.xml file."),
 
                 // The AllowSymLinkAliasChecker.
                 Arguments.of(allowSymlinks, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."),