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.

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 - Issue #8474 - return null on resolve() if resource doesn't exist #8597

Closed
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
20f4b5c
Issue #8474 - return null on resolve() if resource doesn't exist
joakime Sep 16, 2022
bd29121
In process change to resolve is null
joakime Sep 19, 2022
8482ce3
Fixing jetty-util expectations of resolve() returning null
joakime Sep 21, 2022
74acb62
Initial fixing of resolve(non-existent) returning null
joakime Sep 21, 2022
7f86ec1
Resource.resolve is now abstract
joakime Sep 22, 2022
d2e57e4
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
joakime Sep 23, 2022
1812737
Rolling back changes that are now in other PRs
joakime Sep 23, 2022
9ad61ec
Fixing compilation
joakime Sep 23, 2022
0c05840
More resolve() == null checks
joakime Sep 23, 2022
1a49f7e
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
joakime Sep 27, 2022
cb795ab
Minor tweak to show full URI of resource in logging (helps if it's a …
joakime Sep 28, 2022
5b811f5
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
joakime Sep 28, 2022
459ea92
newResource(URI) can now return null if the URI would result in a non…
joakime Sep 28, 2022
72abb2a
More protections on blank/null inputs
joakime Sep 28, 2022
7edb795
Better error handling for non-existent keystore/truststore
joakime Sep 28, 2022
1f2b73d
Don't add non-existent web fragments to cache
joakime Sep 28, 2022
1e43680
Continued work on resolve() returns null on non-existent
joakime Sep 28, 2022
2335007
Remove no longer used class
joakime Sep 28, 2022
fdba6ed
test-keystore should occur before ssl
joakime Sep 29, 2022
c549a97
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
joakime Oct 5, 2022
dedf53d
Fix CachedContentFactory
joakime Oct 5, 2022
7369e11
Fixing ServerListener
joakime Oct 5, 2022
ebe0ce4
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
joakime Oct 5, 2022
4438232
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
joakime Oct 5, 2022
ade5345
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
joakime Oct 7, 2022
6fa6199
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…
joakime Oct 8, 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
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ protected void doStart() throws Exception
List<Path> files = new ArrayList<>();
for (Resource resource : _monitored)
{
if (resource.exists() && Files.isReadable(resource.getPath()))
if (resource != null && Files.isReadable(resource.getPath()))
Copy link
Contributor

Choose a reason for hiding this comment

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

We are not adding nulls into _monitored for non-existing resources, so this null check can go away.

files.add(resource.getPath());
else
LOG.warn("Does not exist: {}", resource);
Expand Down Expand Up @@ -339,7 +339,11 @@ public void setDeploymentManager(DeploymentManager deploymentManager)
public void setMonitoredResources(List<Resource> resources)
{
_monitored.clear();
_monitored.addAll(resources);
for (Resource resource: resources)
{
if (resource != null)
_monitored.addAll(resources);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why adding the resources collection for each resource?

}
}

public List<Resource> getMonitoredResources()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ DO NOT USE IN PRODUCTION!!!
demo
ssl

[depend]
[before]
ssl

[files]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,18 @@ public void setBaseResource(Resource resourceBase)

public void setBaseResource(Path path)
{
setBaseResource(path == null ? null : ResourceFactory.root().newResource(path));
if (path == null)
{
// allow user to unset variable
setBaseResource((Resource)null);
return;
}

Resource resource = ResourceFactory.root().newResource(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

The Path may require mounting, right? So this may lead to a mount leak if I'm not mistaken.

if (resource == null)
throw new IllegalArgumentException("Base Resource does not exist: " + path);

setBaseResource(resource);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -695,17 +695,7 @@ public static URI getClassLoaderLocation(Class<?> clazz, ClassLoader loader)
if (url != null)
{
URI uri = url.toURI();
String uriStr = uri.toASCIIString();
if (uriStr.startsWith("jar:file:"))
{
uriStr = uriStr.substring(4);
int idx = uriStr.indexOf("!/");
if (idx > 0)
{
return URI.create(uriStr.substring(0, idx));
}
}
return uri;
return URIUtil.unwrapContainer(uri);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ public long length()
return _bytes.length;
}

@Override
public Resource resolve(String subUriPath)
{
return null;
}

@Override
public InputStream newInputStream() throws IOException
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
import java.io.IOException;
import java.net.URI;
import java.nio.file.FileSystem;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;

import org.eclipse.jetty.util.URIUtil;

Expand All @@ -26,6 +28,14 @@
*/
public class MountedPathResource extends PathResource
{
public static MountedPathResource of(URI uri) throws IOException
Copy link
Contributor

Choose a reason for hiding this comment

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

Is public needed?

{
Path path = Paths.get(uri.normalize());
if (!Files.exists(path))
return null;
return new MountedPathResource(path, uri);
}

private final URI containerUri;

MountedPathResource(URI uri) throws IOException
Expand All @@ -34,6 +44,18 @@ public class MountedPathResource extends PathResource
containerUri = URIUtil.unwrapContainer(getURI());
}

MountedPathResource(Path path, URI uri)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the other constructor still needed?

{
super(path, uri, true);
containerUri = URIUtil.unwrapContainer(getURI());
}

@Override
protected Resource newResource(Path path, URI uri)
{
return new MountedPathResource(path, uri);
}

@Override
public boolean isContainedIn(Resource r)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ public class PathResource extends Resource
.with("jrt")
.build();

public static PathResource of(URI uri) throws IOException
Copy link
Contributor

Choose a reason for hiding this comment

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

Is public needed?

{
Path path = Paths.get(uri.normalize());
if (!Files.exists(path))
return null;
return new PathResource(path, uri, false);
}

private final Path path;
private final URI uri;
private boolean targetResolved = false;
Expand Down Expand Up @@ -247,9 +255,53 @@ public int hashCode()
return Objects.hashCode(path);
}

@Override
public Resource resolve(String subUriPath)
joakime marked this conversation as resolved.
Show resolved Hide resolved
{
// Check that the path is within the root,
// but use the original path to create the
// resource, to preserve aliasing.
// TODO do a URI safe encoding?
if (URIUtil.isNotNormalWithinSelf(subUriPath))
throw new IllegalArgumentException(subUriPath);

if (URIUtil.SLASH.equals(subUriPath))
return this;

// Sub-paths are always resolved under the given URI,
// we compensate for input sub-paths like "/subdir"
// where default resolve behavior would be to treat
// that like an absolute path.
while (subUriPath.startsWith(URIUtil.SLASH))
{
// TODO XXX this appears entirely unnecessary and inefficient. We already have utilities
// to handle appending path strings with/without slashes.
subUriPath = subUriPath.substring(1);
}
joakime marked this conversation as resolved.
Show resolved Hide resolved

URI uri = getURI();
URI resolvedUri = URIUtil.addPath(uri, subUriPath);
Path path = Paths.get(resolvedUri.normalize());
joakime marked this conversation as resolved.
Show resolved Hide resolved
if (Files.exists(path))
return newResource(path, resolvedUri);

return null;
}

/**
* Internal override for creating a new PathResource.
* Used by MountedPathResource (eg)
*/
protected Resource newResource(Path path, URI uri)
{
return new PathResource(path, uri, true);
}

@Override
public boolean isContainedIn(Resource r)
{
if (r == null)
return false;
return r.getClass() == PathResource.class && path.startsWith(r.getPath());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,9 @@ static Resource create(URI uri)

// If the scheme is allowed by PathResource, we can build a non-mounted PathResource.
if (PathResource.ALLOWED_SCHEMES.contains(uri.getScheme()))
return new PathResource(uri);
return PathResource.of(uri);

return new MountedPathResource(uri);
return MountedPathResource.of(uri);
}
catch (URISyntaxException | ProviderNotFoundException | IOException ex)
{
Expand Down Expand Up @@ -292,33 +292,7 @@ public List<Resource> list()
* @return a Resource representing the subUriPath
* @throws IllegalArgumentException if subUriPath is invalid
*/
public Resource resolve(String subUriPath)
{
// Check that the path is within the root,
// but use the original path to create the
// resource, to preserve aliasing.
// TODO do a URI safe encoding?
if (URIUtil.isNotNormalWithinSelf(subUriPath))
throw new IllegalArgumentException(subUriPath);

if (URIUtil.SLASH.equals(subUriPath))
return this;

// Sub-paths are always resolved under the given URI,
// we compensate for input sub-paths like "/subdir"
// where default resolve behavior would be to treat
// that like an absolute path.
while (subUriPath.startsWith(URIUtil.SLASH))
{
// TODO XXX this appears entirely unnecessary and inefficient. We already have utilities
// to handle appending path strings with/without slashes.
subUriPath = subUriPath.substring(1);
}

URI uri = getURI();
URI resolvedUri = URIUtil.addPath(uri, subUriPath);
return create(resolvedUri);
}
public abstract Resource resolve(String subUriPath);

/**
* @return true if this Resource is an alias to another real Resource
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,8 @@ public Resource resolve(String subUriPath)
for (Resource res : _resources)
{
addedResource = res.resolve(subUriPath);
if (!addedResource.exists())
continue;
if (addedResource == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think more changes below are needed for code clarity.

specifically if resources == null, can we just explicitly return null?

continue; // skip, doesn't exist
if (!addedResource.isDirectory())
return addedResource; // Return simple (non-directory) Resource
if (resources == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import org.eclipse.jetty.util.FileID;
import org.eclipse.jetty.util.Loader;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.URIUtil;
import org.eclipse.jetty.util.component.Container;
import org.eclipse.jetty.util.component.Dumpable;
Expand Down Expand Up @@ -49,6 +50,9 @@ public interface ResourceFactory
*/
default Resource newSystemResource(String resource)
{
if (StringUtil.isBlank(resource))
return null;

URL url = null;
// Try to format as a URL?
ClassLoader loader = Thread.currentThread().getContextClassLoader();
Expand Down Expand Up @@ -110,6 +114,9 @@ default Resource newSystemResource(String resource)
*/
default Resource newClassPathResource(String resource)
{
if (StringUtil.isBlank(resource))
return null;

URL url = ResourceFactory.class.getResource(resource);

if (url == null)
Expand All @@ -135,6 +142,9 @@ default Resource newClassPathResource(String resource)
*/
default Resource newMemoryResource(URL url)
{
if (url == null)
return null;

return new MemoryResource(url);
}

Expand All @@ -146,6 +156,9 @@ default Resource newMemoryResource(URL url)
*/
default Resource newResource(String resource)
{
if (StringUtil.isBlank(resource))
return null;

return newResource(URIUtil.toURI(resource));
}

Expand All @@ -157,6 +170,9 @@ default Resource newResource(String resource)
*/
default Resource newResource(Path path)
{
if (path == null)
return null;

return newResource(path.toUri());
}

Expand All @@ -168,11 +184,17 @@ default Resource newResource(Path path)
*/
default ResourceCollection newResource(List<URI> uris)
{
if ((uris == null) || (uris.isEmpty()))
return null;

return Resource.combine(uris.stream().map(this::newResource).toList());
}

default Resource newResource(URL url)
{
if (url == null)
return null;

try
{
return newResource(url.toURI());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,20 @@ public String getKeyStorePath()
*/
public void setKeyStorePath(String keyStorePath)
{
_keyStoreResource = ResourceFactory.of(this).newResource(keyStorePath);
if (StringUtil.isBlank(keyStorePath))
{
// allow user to unset variable
_keyStoreResource = null;
return;
}

Resource res = ResourceFactory.of(this).newResource(keyStorePath);
if (res == null)
{
_keyStoreResource = null;
throw new IllegalArgumentException("KeyStore Path does not exist: " + keyStorePath);
}
_keyStoreResource = res;
}

/**
Expand Down Expand Up @@ -703,7 +716,20 @@ public String getTrustStorePath()
*/
public void setTrustStorePath(String trustStorePath)
{
_trustStoreResource = StringUtil.isEmpty(trustStorePath) ? null : ResourceFactory.of(this).newResource(trustStorePath);
if (StringUtil.isBlank(trustStorePath))
{
// allow user to unset variable
_trustStoreResource = null;
return;
}

Resource res = ResourceFactory.of(this).newResource(trustStorePath);
if (res == null)
{
_trustStoreResource = null;
throw new IllegalArgumentException("TrustStore Path does not exist: " + trustStorePath);
}
_trustStoreResource = res;
}

/**
Expand Down
Loading