Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jetty 12 - Simplification of aliases in PathResource #8631

Closed
wants to merge 14 commits into from
Closed

Jetty 12 - Simplification of aliases in PathResource #8631

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
97739f9
simplify the PathResource.resolveTargetPath code
lachlan-roberts Sep 28, 2022
9e29c95
changes to how PathResource handles aliases
lachlan-roberts Sep 28, 2022
fcd6e56
fix usages of Resource.getTargetUri()
lachlan-roberts Sep 28, 2022
a32aa83
fixes for FileSystemResourceTest
lachlan-roberts Sep 28, 2022
db4a44c
update javadoc for Resource.getTargetURI()
lachlan-roberts Sep 30, 2022
882cdd9
rename getTargetURI to getCanonicalURI
lachlan-roberts Oct 12, 2022
a3d9efb
let resolveCanonicalPath return null if resource does not exist
lachlan-roberts Oct 13, 2022
4d39c8a
add test in PathResourceTest for broken symlinks
lachlan-roberts Oct 13, 2022
4411d10
some changes from review + optimization for exists()
lachlan-roberts Oct 13, 2022
48eaf49
Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…
lachlan-roberts Oct 13, 2022
98f25cb
restore name to getTargetUri in Resource
lachlan-roberts Oct 13, 2022
5fb4e65
fix some tests related to PathResource changes
lachlan-roberts Oct 14, 2022
bbc261c
revert changes to PathResource equals and hashcode
lachlan-roberts Oct 14, 2022
79cd37d
also compare URI in PathResource
lachlan-roberts Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ public boolean checkAlias(String pathInContext, Resource resource)
if (resource.isAlias())
{
if (LOG.isDebugEnabled())
LOG.debug("Aliased resource: {} -> {}", resource, resource.getTargetURI());
LOG.debug("Aliased resource: {} -> {}", resource, resource.getCanonicalURI());

// alias checks
for (AliasCheck check : _aliasChecks)
Expand Down
119 changes: 47 additions & 72 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ public class PathResource extends Resource

private final Path path;
private final URI uri;
private boolean targetResolved = false;
private Path targetPath;
private boolean alias = false;
private Path canonicalPath;
joakime marked this conversation as resolved.
Show resolved Hide resolved

/**
* Test if the paths are the same name.
Expand Down Expand Up @@ -142,8 +142,8 @@ public static boolean isSameName(Path pathA, Path pathB)

PathResource(URI uri, boolean bypassAllowedSchemeCheck)
{
// normalize to referenced location, Paths.get() doesn't like "/bar/../foo/text.txt" style references
// and will return a Path that will not be found with `Files.exists()` or `Files.isDirectory()`
// Normalize to referenced location, Paths.get() doesn't like "/bar/../foo/text.txt" style references
// and will return a Path that will not be found with `Files.exists()` or `Files.isDirectory()`.
this(Paths.get(uri.normalize()), uri, bypassAllowedSchemeCheck);
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
joakime marked this conversation as resolved.
Show resolved Hide resolved
}

Expand All @@ -170,7 +170,11 @@ public static boolean isSameName(Path pathA, Path pathB)
@Override
public boolean exists()
{
return Files.exists(targetPath != null ? targetPath : path);
if (Files.exists(path))
return true;
if (isAlias())
return Files.exists(canonicalPath);
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));

Copy link
Contributor

@joakime joakime Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@joakime joakime Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. We have all Resource implementations do the URI normalization, removing segments like /../ before creating the Path object.
  2. 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)

}

@Override
Expand All @@ -192,14 +196,24 @@ public boolean equals(Object obj)
return Objects.equals(path, other.path);
}

/**
* @return the {@link Path} of the resource
*/
@Override
public Path getPath()
{
return path;
}

public Path getCanonicalPath()
{
checkAlias();
return canonicalPath;
}

@Override
public URI getCanonicalURI()
{
return getCanonicalPath().toUri();
}

public List<Resource> list()
{
if (!isDirectory())
Expand All @@ -220,6 +234,13 @@ public List<Resource> list()
return List.of(); // empty
}

@Override
public boolean isAlias()
{
checkAlias();
return alias;
}

@Override
public String getName()
{
Expand Down Expand Up @@ -253,78 +274,31 @@ public boolean isContainedIn(Resource r)
return r.getClass() == PathResource.class && path.startsWith(r.getPath());
}

@Override
public URI getTargetURI()
private void checkAlias()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to resolveAlias().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
if (!targetResolved)
if (canonicalPath == null)
{
targetPath = resolveTargetPath();
targetResolved = true;
canonicalPath = resolveTargetPath();

/* If the path and canonicalPath are the same also check
* the Path class has already normalized in the constructor
* from the URI e.g. input path "aa./foo.txt"
* from an #resolve(String) is normalized away during
* the creation of a Path object reference.
* If the URI is different from the Path.toUri() then
* we will just use the original URI to construct the
* alias reference Path.
*/
alias = !isSameName(path, canonicalPath) || !URIUtil.equalsIgnoreEncodings(uri, toUri(path));
}
if (targetPath == null)
return null;
return targetPath.toUri();
}

private Path resolveTargetPath()
{
Path abs = path;

// TODO: is this a valid shortcut?
// If the path doesn't exist, then there's no alias to reference
if (!Files.exists(path))
return null;

/* Catch situation where the Path class has already normalized
* the URI eg. input path "aa./foo.txt"
* from an #resolve(String) is normalized away during
* the creation of a Path object reference.
* If the URI is different from the Path.toUri() then
* we will just use the original URI to construct the
* alias reference Path.
*
* We use the method `toUri(Path)` here, instead of
* Path.toUri() to ensure that the path contains
* a trailing slash if it's a directory, (something
* not all FileSystems seem to support)
*/
if (!URIUtil.equalsIgnoreEncodings(uri, toUri(path)))
{
try
{
// Use normalized path to get past navigational references like "/bar/../foo/test.txt"
Path ref = Paths.get(uri.normalize());
return ref.toRealPath();
}
catch (IOException ioe)
{
// If the toRealPath() call fails, then let
// the alias checking routines continue on
// to other techniques.
LOG.trace("IGNORED", ioe);
}
}

if (!abs.isAbsolute())
abs = path.toAbsolutePath();

// Any normalization difference means it's an alias,
// and we don't want to bother further to follow
// symlinks as it's an alias anyway.
Path normal = path.normalize();
if (!isSameName(abs, normal))
return normal;

Path normalized = path.normalize();
try
{
if (Files.isSymbolicLink(path))
return path.getParent().resolve(Files.readSymbolicLink(path));
if (Files.exists(path))
{
Path real = abs.toRealPath();
if (!isSameName(abs, real))
return real;
}
return normalized.toRealPath();
}
catch (IOException e)
{
Expand All @@ -334,7 +308,8 @@ private Path resolveTargetPath()
{
LOG.warn("bad alias ({} {}) for {}", e.getClass().getName(), e.getMessage(), path);
}
return null;

return normalized;
}

@Override
Expand Down
13 changes: 6 additions & 7 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,19 +325,18 @@ public Resource resolve(String subUriPath)
*/
public boolean isAlias()
{
return getTargetURI() != null;
return false;
}

/**
* If this Resource is an alias pointing to a different location,
* return the target location as URI.
* The canonical URI of the resource. If this Resource is an alias pointing to a different location,
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
* this will resolve the alias to return the true target URI of the resource.
*
* @return The target URI location of this resource,
* or null if there is no target URI location (eg: not an alias, or a symlink)
* @return The canonical URI location of this resource, with any aliases resolved.
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
*/
public URI getTargetURI()
public URI getCanonicalURI()
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
{
return null;
return getURI();
}

/**
Expand Down
6 changes: 2 additions & 4 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/KeyStoreScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

package org.eclipse.jetty.util.ssl;

import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand Down Expand Up @@ -55,9 +54,8 @@ public KeyStoreScanner(SslContextFactory sslContextFactory)
throw new IllegalArgumentException("expected keystore file not directory");

// Use real location of keystore (if different), so that change monitoring can work properly
URI targetURI = keystoreResource.getTargetURI();
if (targetURI != null)
monitoredFile = Paths.get(targetURI);
if (keystoreResource.isAlias())
monitoredFile = Paths.get(keystoreResource.getCanonicalURI());

keystoreFile = monitoredFile;
if (LOG.isDebugEnabled())
Expand Down
Loading