From ccc863726b60c9d5bf6e068f639b8786aa5c83ec Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 26 Aug 2020 12:19:22 -0500 Subject: [PATCH] Issue #5133 - Updates based on review Signed-off-by: Joakim Erdfelt --- .../webapp/OSGiWebappClassLoader.java | 3 - .../jetty/server/CachedContentFactory.java | 49 +++----- .../jetty/server/ResourceContentFactory.java | 14 ++- .../eclipse/jetty/server/ResourceService.java | 2 +- .../jetty/server/handler/ResourceHandler.java | 78 ++++++------- .../util/resource/ResourceCollection.java | 105 +++++++++--------- .../jetty/util/resource/ResourceFactory.java | 2 +- .../jetty/util/resource/URLResource.java | 8 +- .../util/resource/ResourceCollectionTest.java | 2 +- 9 files changed, 126 insertions(+), 137 deletions(-) diff --git a/jetty-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/internal/webapp/OSGiWebappClassLoader.java b/jetty-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/internal/webapp/OSGiWebappClassLoader.java index 3a21d5d738b5..636ccc5fd6ad 100644 --- a/jetty-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/internal/webapp/OSGiWebappClassLoader.java +++ b/jetty-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/internal/webapp/OSGiWebappClassLoader.java @@ -20,7 +20,6 @@ import java.io.File; import java.io.IOException; -import java.lang.reflect.Field; import java.net.URL; import java.util.ArrayList; import java.util.Enumeration; @@ -264,6 +263,4 @@ private boolean isAcceptableLibrary(File file, Set pathToClassFiles) } return true; } - - private static Field _contextField; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/CachedContentFactory.java b/jetty-server/src/main/java/org/eclipse/jetty/server/CachedContentFactory.java index 25cd879d620b..de1aae8f96ad 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/CachedContentFactory.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/CachedContentFactory.java @@ -188,7 +188,8 @@ public HttpContent getContent(String pathInContext, int maxBufferSize) throws IO if (_parent != null) { HttpContent httpContent = _parent.getContent(pathInContext, maxBufferSize); - return httpContent; + if (httpContent != null) + return httpContent; } return null; @@ -209,7 +210,7 @@ protected boolean isCacheable(Resource resource) return (len > 0 && (_useFileMappedBuffer || (len < _maxCachedFileSize && len < _maxCacheSize))); } - private HttpContent load(String pathInContext, Resource resource, int maxBufferSize) + private HttpContent load(String pathInContext, Resource resource, int maxBufferSize) throws IOException { if (resource == null || !resource.exists()) return null; @@ -233,26 +234,18 @@ private HttpContent load(String pathInContext, Resource resource, int maxBufferS if (compressedContent == null || compressedContent.isValid()) { compressedContent = null; - try + Resource compressedResource = _factory.getResource(compressedPathInContext); + if (compressedResource.exists() && compressedResource.lastModified() >= resource.lastModified() && + compressedResource.length() < resource.length()) { - Resource compressedResource = _factory.getResource(compressedPathInContext); - if (compressedResource.exists() && compressedResource.lastModified() >= resource.lastModified() && - compressedResource.length() < resource.length()) + compressedContent = new CachedHttpContent(compressedPathInContext, compressedResource, null); + CachedHttpContent added = _cache.putIfAbsent(compressedPathInContext, compressedContent); + if (added != null) { - compressedContent = new CachedHttpContent(compressedPathInContext, compressedResource, null); - CachedHttpContent added = _cache.putIfAbsent(compressedPathInContext, compressedContent); - if (added != null) - { - compressedContent.invalidate(); - compressedContent = added; - } + compressedContent.invalidate(); + compressedContent = added; } } - catch (IOException e) - { - if (LOG.isDebugEnabled()) - LOG.debug("Unable to find compressed path in context: {}", compressedPathInContext, e); - } } if (compressedContent != null) precompresssedContents.put(format, compressedContent); @@ -286,20 +279,12 @@ private HttpContent load(String pathInContext, Resource resource, int maxBufferS if (compressedContent != null && compressedContent.isValid() && compressedContent.getResource().lastModified() >= resource.lastModified()) compressedContents.put(format, compressedContent); - try - { - // Is there a precompressed resource? - Resource compressedResource = _factory.getResource(compressedPathInContext); - if (compressedResource.exists() && compressedResource.lastModified() >= resource.lastModified() && - compressedResource.length() < resource.length()) - compressedContents.put(format, - new ResourceHttpContent(compressedResource, _mimeTypes.getMimeByExtension(compressedPathInContext), maxBufferSize)); - } - catch (IOException e) - { - if (LOG.isDebugEnabled()) - LOG.debug("Unable to find compressed path in context: {}", compressedPathInContext, e); - } + // Is there a precompressed resource? + Resource compressedResource = _factory.getResource(compressedPathInContext); + if (compressedResource.exists() && compressedResource.lastModified() >= resource.lastModified() && + compressedResource.length() < resource.length()) + compressedContents.put(format, + new ResourceHttpContent(compressedResource, _mimeTypes.getMimeByExtension(compressedPathInContext), maxBufferSize)); } if (!compressedContents.isEmpty()) return new ResourceHttpContent(resource, mt, maxBufferSize, compressedContents); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java index a402ec3efcb3..bc80e4d5ae02 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java @@ -50,7 +50,7 @@ public ResourceContentFactory(ResourceFactory factory, MimeTypes mimeTypes, Comp } @Override - public HttpContent getContent(String pathInContext, int maxBufferSize) + public HttpContent getContent(String pathInContext, int maxBufferSize) throws IOException { try { @@ -60,8 +60,16 @@ public HttpContent getContent(String pathInContext, int maxBufferSize) } catch (Throwable t) { - // Any error has potential to reveal fully qualified path - throw (InvalidPathException)new InvalidPathException(pathInContext, "Invalid PathInContext").initCause(t); + // There are many potential Exceptions that can reveal a fully qualified path. + // See Issue #2560 - Always wrap a Throwable here in an InvalidPathException + // that is limited to only the provided pathInContext. + // The cause (which might reveal a fully qualified path) is still available, + // on the Exception and the logging, but is not reported in normal error page situations. + // This specific exception also allows WebApps to specifically hook into a known / reliable + // Exception type for ErrorPageErrorHandling logic. + InvalidPathException saferException = new InvalidPathException(pathInContext, "Invalid PathInContext"); + saferException.initCause(t); + throw saferException; } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java index 0528fb30f494..56e5438ba0e0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java @@ -881,6 +881,6 @@ public interface WelcomeFactory * @param pathInContext the path of the request * @return The path of the matching welcome file in context or null. */ - String getWelcomeFile(String pathInContext); + String getWelcomeFile(String pathInContext) throws IOException; } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java index 9cb9b7487b0c..a0f20c6f2de0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java @@ -18,9 +18,10 @@ package org.eclipse.jetty.server.handler; +import java.io.FileNotFoundException; import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; +import java.util.Arrays; import java.util.List; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; @@ -36,6 +37,7 @@ import org.eclipse.jetty.server.ResourceService; import org.eclipse.jetty.server.ResourceService.WelcomeFactory; import org.eclipse.jetty.server.handler.ContextHandler.Context; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.resource.Resource; import org.eclipse.jetty.util.resource.ResourceFactory; @@ -74,11 +76,11 @@ protected void notFound(HttpServletRequest request, HttpServletResponse response { } }); - _resourceService.setGzipEquivalentFileExtensions(new ArrayList<>(Collections.singletonList(".svgz"))); + _resourceService.setGzipEquivalentFileExtensions(new ArrayList<>(Arrays.asList(new String[]{".svgz"}))); } @Override - public String getWelcomeFile(String pathInContext) + public String getWelcomeFile(String pathInContext) throws IOException { if (_welcomes == null) return null; @@ -86,18 +88,9 @@ public String getWelcomeFile(String pathInContext) for (int i = 0; i < _welcomes.length; i++) { String welcomeInContext = URIUtil.addPaths(pathInContext, _welcomes[i]); - try - { - Resource welcome = getResource(welcomeInContext); - if (welcome.exists()) - return welcomeInContext; - } - catch (IOException e) - { - // this happens on a critical failure of Resource - if (LOG.isDebugEnabled()) - LOG.debug("Failed to resolve welcome file: {}", welcomeInContext); - } + Resource welcome = getResource(welcomeInContext); + if (welcome.exists()) + return welcomeInContext; } // not found return null; @@ -154,37 +147,46 @@ public Resource getResource(String path) throws IOException if (LOG.isDebugEnabled()) LOG.debug("{} getResource({}): baseResource:{}", _context == null ? _baseResource : _context, path, _baseResource); - if (path != null && path.startsWith("/")) + if (StringUtil.isBlank(path)) { - Resource r = null; + throw new IllegalArgumentException("Path is blank"); + } - if (_baseResource != null) - { - path = URIUtil.canonicalPath(path); - r = _baseResource.addPath(path); - - if (r.isAlias() && (_context == null || !_context.checkAlias(path, r))) - { - if (LOG.isDebugEnabled()) - LOG.debug("Rejected alias resource={} alias={}", r, r.getAlias()); - throw new IOException("Rejected (see debug logs)"); - } - } - else if (_context != null) - { - r = _context.getResource(path); - if (r != null) - return r; - } + if (!path.startsWith("/")) + { + throw new IllegalArgumentException("Path reference invalid: " + path); + } - if ((r == null || !r.exists()) && path.endsWith("/jetty-dir.css")) - r = getStylesheet(); + Resource r = null; + if (_baseResource != null) + { + path = URIUtil.canonicalPath(path); + r = _baseResource.addPath(path); + + if (r.isAlias() && (_context == null || !_context.checkAlias(path, r))) + { + if (LOG.isDebugEnabled()) + LOG.debug("Rejected alias resource={} alias={}", r, r.getAlias()); + throw new IllegalStateException("Rejected alias reference: " + path); + } + } + else if (_context != null) + { + r = _context.getResource(path); if (r != null) return r; } - throw new IOException("Unable to find Resource for " + path); + if ((r == null || !r.exists()) && path.endsWith("/jetty-dir.css")) + r = getStylesheet(); + + if (r == null) + { + throw new FileNotFoundException("Resource: " + path); + } + + return r; } /** diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java index 920506e8b2a5..d7da2a2f500e 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java @@ -25,7 +25,7 @@ import java.net.URI; import java.nio.channels.ReadableByteChannel; import java.util.ArrayList; -import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -42,7 +42,7 @@ */ public class ResourceCollection extends Resource { - private Resource[] _resources; + private List _resources; /** * Instantiates an empty resource collection. @@ -51,7 +51,7 @@ public class ResourceCollection extends Resource */ public ResourceCollection() { - _resources = new Resource[0]; + _resources = new ArrayList<>(); } /** @@ -61,7 +61,7 @@ public ResourceCollection() */ public ResourceCollection(Resource... resources) { - List list = new ArrayList<>(); + _resources = new ArrayList<>(); for (Resource r : resources) { if (r == null) @@ -70,15 +70,25 @@ public ResourceCollection(Resource... resources) } if (r instanceof ResourceCollection) { - Collections.addAll(list, ((ResourceCollection)r).getResources()); + _resources.addAll(((ResourceCollection)r).getResources()); } else { assertResourceValid(r); - list.add(r); + _resources.add(r); } } - _resources = list.toArray(new Resource[0]); + } + + /** + * Instantiates a new resource collection. + * + * @param resources the resources to be added to collection + */ + public ResourceCollection(Collection resources) + { + _resources = new ArrayList<>(); + _resources.addAll(resources); } /** @@ -88,14 +98,13 @@ public ResourceCollection(Resource... resources) */ public ResourceCollection(String[] resources) { + _resources = new ArrayList<>(); + if (resources == null || resources.length == 0) { - _resources = null; return; } - ArrayList res = new ArrayList<>(); - try { for (String strResource : resources) @@ -106,16 +115,13 @@ public ResourceCollection(String[] resources) } Resource resource = Resource.newResource(strResource); assertResourceValid(resource); - res.add(resource); + _resources.add(resource); } - if (res.isEmpty()) + if (_resources.isEmpty()) { - _resources = null; - return; + throw new IllegalArgumentException("resources cannot be empty or null"); } - - _resources = res.toArray(new Resource[0]); } catch (RuntimeException e) { @@ -141,9 +147,9 @@ public ResourceCollection(String csvResources) throws IOException /** * Retrieves the resource collection's resources. * - * @return the resource array + * @return the resource collection */ - public Resource[] getResources() + public List getResources() { return _resources; } @@ -155,13 +161,13 @@ public Resource[] getResources() */ public void setResources(List res) { + _resources = new ArrayList<>(); if (res.isEmpty()) { - _resources = null; return; } - _resources = res.toArray(new Resource[0]); + _resources.addAll(res); } /** @@ -235,53 +241,46 @@ public Resource addPath(String path) throws IOException return this; } - Resource resource = null; - ArrayList resources = null; - int i = 0; - for (; i < _resources.length; i++) + // Attempt a simple (single) Resource lookup that exists + for (Resource res : _resources) { - resource = _resources[i].addPath(path); - if (resource.exists()) + Resource fileResource = res.addPath(path); + if (fileResource.exists()) { - if (resource.isDirectory()) + if (!fileResource.isDirectory()) { - break; + return fileResource; } - return resource; } } - for (i++; i < _resources.length; i++) + // Create a list of potential resource for directories of this collection + ArrayList potentialResources = null; + for (Resource res : _resources) { - Resource r = _resources[i].addPath(path); + Resource r = res.addPath(path); if (r.exists() && r.isDirectory()) { - if (resources == null) + if (potentialResources == null) { - resources = new ArrayList<>(); + potentialResources = new ArrayList<>(); } - if (resource != null) - { - resources.add(resource); - resource = null; - } - - resources.add(r); + potentialResources.add(r); } } - if (resource != null) + if (potentialResources == null || potentialResources.isEmpty()) { - return resource; + throw new MalformedURLException("path does not result in Resource: " + path); } - if (resources != null) + if (potentialResources.size() == 1) { - return new ResourceCollection(resources.toArray(new Resource[0])); + return potentialResources.get(0); } - throw new MalformedURLException("path does not result in Resource: " + path); + return new ResourceCollection(potentialResources); } @Override @@ -421,9 +420,8 @@ public String[] list() { Collections.addAll(set, r.list()); } - String[] result = set.toArray(new String[0]); - Arrays.sort(result); - return result; + + return (String[])set.stream().sorted().toArray(); } @Override @@ -449,9 +447,10 @@ public void copyTo(File destination) { assertResourcesSet(); - for (int r = _resources.length; r-- > 0; ) + // Copy in reverse order + for (int r = _resources.size(); r-- > 0; ) { - _resources[r].copyTo(destination); + _resources.get(r).copyTo(destination); } } @@ -461,12 +460,12 @@ public void copyTo(File destination) @Override public String toString() { - if (_resources == null || _resources.length == 0) + if (_resources.isEmpty()) { return "[]"; } - return String.valueOf(Arrays.asList(_resources)); + return String.valueOf(_resources); } @Override @@ -478,7 +477,7 @@ public boolean isContainedIn(Resource r) private void assertResourcesSet() { - if (_resources == null || _resources.length == 0) + if (_resources == null || _resources.isEmpty()) { throw new IllegalStateException("*resources* not set."); } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactory.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactory.java index 0b70e6dd597b..6797d5bb0c71 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactory.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactory.java @@ -36,7 +36,7 @@ public interface ResourceFactory *

* * @param path The path to the resource - * @return The resource + * @return The resource, that might not actually exist (yet). * @throws IOException if unable to create Resource */ Resource getResource(String path) throws IOException; diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java index 58e65bc0f998..3dbd48ffa025 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java @@ -27,6 +27,7 @@ import java.net.URL; import java.net.URLConnection; import java.nio.channels.ReadableByteChannel; +import java.util.Objects; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.thread.AutoLock; @@ -287,12 +288,9 @@ public String[] list() */ @Override public Resource addPath(String path) - throws IOException, MalformedURLException + throws IOException { - if (path == null) - { - throw new MalformedURLException("null path"); - } + Objects.requireNonNull(path, "Path may not be null"); path = URIUtil.canonicalPath(path); diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceCollectionTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceCollectionTest.java index e09a441418f7..32d9c643ea03 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceCollectionTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceCollectionTest.java @@ -152,7 +152,7 @@ public void testSetResourceAllNullsThrowsISE() assertThrows(IllegalStateException.class, () -> coll.setResources(new Resource[]{null, null, null})); // Ensure not modified. - assertThat(coll.getResources().length, is(1)); + assertThat(coll.getResources().size(), is(1)); } private void assertThrowIllegalStateException(ResourceCollection coll)