Jetty 12 - Simplification of aliases in `PathResource` #8631

lachlan-roberts · 2022-09-30T01:09:54Z

The resolveTargetPath() method has been simplified.
The method getTargetPath() now always returns the target resource with all aliases resolved.
The check isAlias() should be used to determine whether it is an alias instead of getTargetPath() != null.

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

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

lachlan-roberts · 2022-09-30T01:12:52Z

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileSystemResourceTest.java

            if (org.junit.jupiter.api.condition.OS.WINDOWS.isCurrentOs())
            {
+                assertThat("getURI()", r.getPath().toString(), containsString("aa\\foo.txt"));


This test was already failing on 12.0.x branch when run on windows.
So this isn't directly related to the other changes.

Why is it getURI() and then using path to validate the string?

This was fixed in PR #8706 btw (you should merge from origin/jetty-12.0.x again into this branch)

joakime

Questions about normalize, and some nits about consistency in test cases.

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileSystemResourceTest.java

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

sbordet · 2022-10-04T17:26:32Z

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

+            return true;
+        if (isAlias())
+            return Files.exists(targetPath);
+        return false;


Files.exists() implicitly take a FOLLOW_SYMLINKS, so the first at line 173 check should be enough.

Not if the path has unresolved .. segments, then Files.exists() returns false even if it does exist.

I agree with sbordet

If this.path was created with one of the URI based methods for creating a Path (eg: Paths.of(URI) or Paths.get(URI)) then your statement is true.
As there is an expectation that the URI it is given is absolute, canonical, and normalized.
But if this.path was created from a String or other Path object, even the /../ segments have no impact on the Files.exists() logic and will return true.

But if this.path was created from a String or other Path object, even the /../ segments have no impact on the Files.exists() logic and will return true.

This is not correct.

The following test passes and shows that Files.exists does not work with /../ segments. And since you can create PathResource with a Path directly we cannot assume that the path does not contain /../ segments.

Path resourcePath = testDir.resolve("resource.txt"); assertTrue(Files.exists(resourcePath)); Path aliasPath = testDir.resolve("foo/../resource.txt"); assertFalse(Files.exists(aliasPath)); assertTrue(Files.exists(aliasPath.normalize()));

The Files.exists() does work with /../ if each element of the path exists.
In your case foo does not exist on your filesystem.

This is part of the filesystem specific normalization techniques.
Some filesystems will find it (some will not).
Don't write your code assuming that all filesystems behave the same in this regard.

Example code (tested on Linux with ext4 filesystem)

package fs; import java.io.IOException; import java.net.URI; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import org.eclipse.jetty.toolchain.test.FS; public class PathsGetExample { public static void main(String[] args) throws IOException { boolean withFoo = true; System.out.printf("withFoo = %b%n", withFoo); // Create some content Path testDir = Paths.get("/tmp/paths-get-examples"); FS.ensureDirExists(testDir); Path fooDir = testDir.resolve("foo"); if (withFoo) FS.ensureDirExists(testDir.resolve("foo")); else Files.deleteIfExists(fooDir); FS.touch(testDir.resolve("test.txt")); URI uri = URI.create(testDir.toUri().toASCIIString() + "foo/../test.txt"); Path a = Paths.get(uri); System.out.printf("%s -> %s -> %b%n", uri, a, Files.exists(a)); URI urin = uri.normalize(); Path b = Paths.get(urin); System.out.printf("%s -> %s -> %b%n", urin, b, Files.exists(b)); String str = "/tmp/paths-get-examples/foo/../test.txt"; Path c = Paths.get(str); System.out.printf("%s -> %s -> %b%n", str, c, Files.exists(c)); } }

Executing with "foo"

withFoo = true file:///tmp/paths-get-examples/foo/../test.txt -> /tmp/paths-get-examples/foo/../test.txt -> true file:/tmp/paths-get-examples/test.txt -> /tmp/paths-get-examples/test.txt -> true /tmp/paths-get-examples/foo/../test.txt -> /tmp/paths-get-examples/foo/../test.txt -> true

Executing without "foo"

withFoo = false file:///tmp/paths-get-examples/foo/../test.txt -> /tmp/paths-get-examples/foo/../test.txt -> false file:/tmp/paths-get-examples/test.txt -> /tmp/paths-get-examples/test.txt -> true /tmp/paths-get-examples/foo/../test.txt -> /tmp/paths-get-examples/foo/../test.txt -> false

This is the same behavior you get on linux with ls ...

[joakim@hyperion ~]$ ls -la /tmp/paths-get-examples/ total 512 drwxrwxr-x 2 joakim joakim 4096 Oct 14 07:45 ./ drwxrwxrwt 221 root root 516096 Oct 14 07:42 ../ -rw-rw-r-- 1 joakim joakim 0 Oct 14 07:42 test.txt [joakim@hyperion ~]$ ls -la /tmp/paths-get-examples/foo/../test.txt ls: cannot access '/tmp/paths-get-examples/foo/../test.txt': No such file or directory [joakim@hyperion ~]$ mkdir /tmp/paths-get-examples/foo [joakim@hyperion ~]$ ls -la /tmp/paths-get-examples/ total 516 drwxrwxr-x 3 joakim joakim 4096 Oct 14 07:45 ./ drwxrwxrwt 221 root root 516096 Oct 14 07:42 ../ drwxrwxr-x 2 joakim joakim 4096 Oct 14 07:45 foo/ -rw-rw-r-- 1 joakim joakim 0 Oct 14 07:42 test.txt [joakim@hyperion ~]$ ls -la /tmp/paths-get-examples/foo/../test.txt -rw-rw-r-- 1 joakim joakim 0 Oct 14 07:42 /tmp/paths-get-examples/foo/../test.txt

The way I see it we have a few options here.

We have all Resource implementations do the URI normalization, removing segments like /../ before creating the Path object.

We have all Resource implementations honor the as-submitted URI, with no Resource implementation normalization, and let the FileSystem suss out if the content can be referenced or not.

We are doing 1 currently, but should we?
Is it such a terrible idea to reject requests for content like dir-does-not-exist/../test.dat and allow dir-that-exists/../test.dat?
I could easily be convinced that the Resource implementations should not perform URI normalization.
It just means a few types of torturous requests we used to receive in the past we no longer support.
I wonder, is supporting /dir-does-not-exist/ segments hiding or revealing extra information about the filesystem we shouldn't be doing? (need more time to ponder this)
What does supporting /dir-does-not-exist/ segments mean for ResourceCollection too? (if anything)

Or, if we want to support these requests still, it means that the normalization occurs at the layer above Resource.resolve(String) (which kinda makes sense, and I suspect it would simplify the protected paths issues too)

sbordet · 2022-10-04T17:26:48Z

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

        return targetPath.toUri();
    }

-    private Path resolveTargetPath()
+    private void checkAlias()


Rename to resolveAlias().

I agree with @sbordet, this is a resolveAlias() or resolveTarget() name.

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

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

joakime

I disagree with the name change from target to canonical as that's not appropriate for Path (or FileSystem or FileStore).

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

joakime · 2022-10-12T18:17:27Z

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

+            return true;
+        if (isAlias())
+            return Files.exists(targetPath);
+        return false;


I agree with sbordet

If this.path was created with one of the URI based methods for creating a Path (eg: Paths.of(URI) or Paths.get(URI)) then your statement is true.
As there is an expectation that the URI it is given is absolute, canonical, and normalized.
But if this.path was created from a String or other Path object, even the /../ segments have no impact on the Files.exists() logic and will return true.

joakime · 2022-10-12T18:20:09Z

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

        return targetPath.toUri();
    }

-    private Path resolveTargetPath()
+    private void checkAlias()


I agree with @sbordet, this is a resolveAlias() or resolveTarget() name.

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

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

…PathResource-resolveTargetPath

joakime

Canonical Path is the wrong name for multiple reasons.

Either correct the name to be what it represents (a real name, or target), or correct the API usage to use the old school canonicalization techniques that you apparently want to stick with.

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

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

joakime · 2022-10-14T11:43:51Z

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

+        if (getClass() != obj.getClass())
+            return false;
+        PathResource other = (PathResource)obj;
+        return Objects.equals(path, other.path) && Objects.equals(uri, other.uri);


Why do we need equals/hashCode at all?

The comparison on uri is odd, that's a user-space provided field.

I'm not sure why we need to compare PathResource, I know there are tests for the equals() but not sure if we actually use it anywhere. But to compare these correctly you need to take the uri into account.

You can come up with examples where two different PathResources have the same path field but one is classified as an alias and the other is not. So comparing only on path isn't quite correct.

This will impact the ResourceService cache though.

The same Resource will exist on multiple locations once this new equals/hashcode exists.
Is that an acceptable behavior?

joakime

The new equals/hashcode logic is not needed, nor is is critical for any step you have in this PR.
Also, if a Path doesn't exist, then it cannot have an alias.

joakime · 2022-10-14T11:45:50Z

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileSystemResourceTest.java

-        assertThat("uri.targetURI", ResourceFactory.root().newResource(resBar.getURI()), isTargetFor(resFoo));
-        assertThat("file.targetURI", ResourceFactory.root().newResource(resBar.getPath()), isTargetFor(resFoo));
+        // This is an alias because the file does not exist.
+        assertFalse(resFoo.exists());


What!?
If the file doesn't exist, it can never be an alias.

joakime · 2022-10-14T11:46:42Z

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileSystemResourceTest.java

-        assertThat("file.targetURI", ResourceFactory.root().newResource(resBar.getPath()), isTargetFor(resFoo));
+        // This is an alias because the file does not exist.
+        assertFalse(resFoo.exists());
+        assertTrue(resFoo.isAlias());


This should be false. Something is wrong here.

joakime · 2022-10-14T11:47:43Z

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileSystemResourceTest.java

@@ -1067,10 +1066,14 @@ public void testSingleQuoteInFileName() throws Exception
            "URI[b] = " + refB;
        assertThat(msg, refA.equals(refB), is(false));

-        // now show that Resource.equals() does work
+        // These resources are not equal because they have different URIs.


This feels dangerous for our mounting logic and also our caches.
The user-space provided URI should not be part of our equals/hashcode behaviors.
I don't see a use case in this PR that makes this change necessary.

joakime · 2022-10-14T11:48:31Z

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileSystemResourceTest.java

            if (org.junit.jupiter.api.condition.OS.WINDOWS.isCurrentOs())
            {
+                assertThat("getURI()", r.getPath().toString(), containsString("aa\\foo.txt"));


This was fixed in PR #8706 btw (you should merge from origin/jetty-12.0.x again into this branch)

joakime · 2022-10-14T11:49:54Z

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/PathResourceTest.java

+        Path testDir = workDir.getEmptyPathDir();
+        Path resourcePath = testDir.resolve("resource.txt");
+        IO.copy(MavenTestingUtils.getTestResourcePathFile("resource.txt").toFile(), resourcePath.toFile());
+        Path symlinkPath = Files.createSymbolicLink(testDir.resolve("symlink.txt"), resourcePath);


Need protection for users that execute this test on filesystems that don't support symlink (don't check the OS here, check the success/failure of the createSymbolicLink method instead, like we do elsewhere)

joakime · 2022-10-14T12:00:07Z

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/PathResourceTest.java

+        assertTrue(symlinkResource.exists());
+
+        // After deleting file the Resources do not exist even though symlink file exists.
+        Files.delete(resourcePath);


This delete needs protection for users on filesystems that don't immediately delete.
Test that the resourcePath was actually deleted in an assumption.

joakime · 2022-10-14T12:00:59Z

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

@@ -398,7 +398,7 @@ public void testDotAliasDirDoesNotExist(WorkDir workDir)
        Resource dot = resource.resolve(".");
        assertNotNull(dot);
        assertFalse(dot.exists());
-        assertFalse(dot.isAlias(), "Reference to '.' is not an alias as directory doesn't exist");
+        assertTrue(dot.isAlias(), "Reference to '.' is an alias as directory doesn't exist");


Again, if the resource doesn't exist, it cannot have an alias.

joakime · 2022-10-14T12:01:08Z

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

@@ -428,7 +428,7 @@ public void testDotAliasFileDoesNotExists(WorkDir workDir) throws IOException
        Resource dot = resource.resolve(".");
        assertNotNull(dot);
        assertFalse(dot.exists());
-        assertFalse(dot.isAlias(), "Reference to '.' is not an alias as file doesn't exist");
+        assertTrue(dot.isAlias(), "Reference to '.' is an alias as file doesn't exist");


Again, if the resource doesn't exist, it cannot have an alias.

joakime · 2022-10-14T12:02:08Z

jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java

-            if (checkAlias(pathInContext, resource))
-                return resource;
-            return null;
+            return baseResource.resolve(pathInContext);


Don't think you can remove the alias checking here.

joakime · 2022-10-14T12:03:24Z

jetty-ee9/jetty-ee9-servlet/src/main/java/org/eclipse/jetty/ee9/servlet/DefaultServlet.java

@@ -457,8 +457,6 @@ protected Resource resolve(String subUriPath)
            if (_baseResource != null)
            {
                r = _baseResource.resolve(subUriPath);


Lets move these alias checking changes in ee9 out to a different PR.

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

joakime · 2022-10-19T16:15:45Z

Closing in favor of #8734

lachlan-roberts added 5 commits September 28, 2022 15:09

simplify the PathResource.resolveTargetPath code

97739f9

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

changes to how PathResource handles aliases

9e29c95

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

fix usages of Resource.getTargetUri()

fcd6e56

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

fixes for FileSystemResourceTest

a32aa83

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

update javadoc for Resource.getTargetURI()

db4a44c

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

lachlan-roberts requested review from joakime and sbordet September 30, 2022 01:09

lachlan-roberts commented Sep 30, 2022

View reviewed changes

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java Show resolved Hide resolved

lachlan-roberts commented Sep 30, 2022

View reviewed changes

joakime requested changes Oct 4, 2022

View reviewed changes

joakime reviewed Oct 4, 2022

View reviewed changes

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java Outdated Show resolved Hide resolved

joakime reviewed Oct 4, 2022

View reviewed changes

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java Outdated Show resolved Hide resolved

joakime reviewed Oct 4, 2022

View reviewed changes

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java Outdated Show resolved Hide resolved

sbordet requested changes Oct 4, 2022

View reviewed changes

joakime added the jetty 12 label Oct 5, 2022

joakime mentioned this pull request Oct 7, 2022

Jetty 12 - Issue #8474 - return null on resolve() if resource doesn't exist #8597

Closed

joakime changed the title ~~Simplification of aliases in PathResource (Jetty-12)~~ Jetty 12 - Simplification of aliases in PathResource Oct 7, 2022

rename getTargetURI to getCanonicalURI

882cdd9

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

joakime requested changes Oct 12, 2022

View reviewed changes

joakime mentioned this pull request Oct 12, 2022

Jetty 12 - Resource resolve() and newResource() return null on resources that do not exist #8702

Merged

lachlan-roberts added 4 commits October 13, 2022 13:31

let resolveCanonicalPath return null if resource does not exist

a3d9efb

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

add test in PathResourceTest for broken symlinks

4d39c8a

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

some changes from review + optimization for exists()

4411d10

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

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…

48eaf49

…PathResource-resolveTargetPath

joakime requested changes Oct 13, 2022

View reviewed changes

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java Show resolved Hide resolved

lachlan-roberts added 4 commits October 13, 2022 23:04

restore name to getTargetUri in Resource

98f25cb

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

fix some tests related to PathResource changes

5fb4e65

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

revert changes to PathResource equals and hashcode

bbc261c

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

also compare URI in PathResource

79cd37d

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

joakime reviewed Oct 14, 2022

View reviewed changes

joakime requested changes Oct 14, 2022

View reviewed changes

joakime reviewed Oct 14, 2022

View reviewed changes

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java Show resolved Hide resolved

joakime mentioned this pull request Oct 18, 2022

Jetty 12 - Simplification of aliases in PathResource (Take 2) #8734

Merged

joakime closed this Oct 19, 2022

joakime deleted the jetty-12.0.x-PathResource-resolveTargetPath branch November 21, 2022 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetty 12 - Simplification of aliases in `PathResource` #8631

Jetty 12 - Simplification of aliases in `PathResource` #8631

lachlan-roberts commented Sep 30, 2022

lachlan-roberts Sep 30, 2022

joakime Sep 30, 2022

joakime Oct 14, 2022

joakime left a comment

sbordet Oct 4, 2022

lachlan-roberts Oct 4, 2022

joakime Oct 12, 2022

lachlan-roberts Oct 13, 2022

joakime Oct 14, 2022 •

edited

Loading

joakime Oct 14, 2022

joakime Oct 14, 2022 •

edited

Loading

sbordet Oct 4, 2022

joakime Oct 12, 2022

joakime left a comment

joakime Oct 12, 2022

joakime Oct 12, 2022

joakime left a comment

joakime Oct 14, 2022

lachlan-roberts Oct 14, 2022

joakime Oct 18, 2022

joakime left a comment

joakime Oct 14, 2022

joakime Oct 14, 2022

joakime Oct 14, 2022

joakime Oct 14, 2022

joakime Oct 14, 2022

joakime Oct 14, 2022

joakime Oct 14, 2022

joakime Oct 14, 2022

joakime Oct 14, 2022

joakime Oct 14, 2022

joakime commented Oct 19, 2022

Jetty 12 - Simplification of aliases in PathResource #8631

Jetty 12 - Simplification of aliases in PathResource #8631

Conversation

lachlan-roberts commented Sep 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime commented Oct 19, 2022

Jetty 12 - Simplification of aliases in `PathResource` #8631

Jetty 12 - Simplification of aliases in `PathResource` #8631

joakime Oct 14, 2022 •

edited

Loading

joakime Oct 14, 2022 •

edited

Loading