diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index 7abdf90f01f..738c28ec5a4 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -23,6 +23,7 @@ import org.jetbrains.annotations.NotNull; import org.json.JSONObject; import org.labkey.api.action.ApiQueryResponse; +import org.labkey.api.action.ApiResponseWriter; import org.labkey.api.action.ApiXmlWriter; import org.labkey.api.action.ConcurrencyLimiter; import org.labkey.api.action.SpringActionController; @@ -414,6 +415,7 @@ public void registerServlets(ServletContext servletCtx) { return Set.of( Aggregate.TestCase.class, + ApiResponseWriter.TestCase.class, ApiXmlWriter.TestCase.class, ArrayListMap.TestCase.class, AssayResultsFileWriter.TestCase.class, diff --git a/api/src/org/labkey/api/action/ApiResponseWriter.java b/api/src/org/labkey/api/action/ApiResponseWriter.java index ed483c37b25..44e578ed845 100644 --- a/api/src/org/labkey/api/action/ApiResponseWriter.java +++ b/api/src/org/labkey/api/action/ApiResponseWriter.java @@ -21,6 +21,8 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.query.BatchValidationException; import org.labkey.api.query.PropertyValidationError; import org.labkey.api.query.SimpleValidationError; @@ -28,6 +30,7 @@ import org.labkey.api.query.ValidationException; import org.labkey.api.util.ExceptionUtil; import org.labkey.api.util.ExpectedException; +import org.labkey.api.util.FileUtil; import org.labkey.api.util.JsonUtil; import org.labkey.api.util.Pair; import org.labkey.api.util.UnexpectedException; @@ -40,6 +43,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; +import java.io.StringWriter; import java.io.Writer; import java.util.List; import java.util.Stack; @@ -232,6 +236,9 @@ public int getErrorResponseStatus(Throwable ex) if (ex instanceof HttpStatusException statusEx) return statusEx.getStatus(); + if (ex instanceof FileUtil.InvalidPathReferenceException) + return HttpServletResponse.SC_NOT_FOUND; + if (ExceptionUtil.isRequestTimeout(ex)) return HttpServletResponse.SC_SERVICE_UNAVAILABLE; @@ -611,4 +618,15 @@ public void setSerializeViaJacksonAnnotations(boolean serializeViaJacksonAnnotat { this.serializeViaJacksonAnnotations = serializeViaJacksonAnnotations; } + + public static class TestCase extends Assert + { + @Test + public void testErrorResponseStatus() throws IOException + { + ApiResponseWriter writer = new ApiJsonWriter(new StringWriter()); + assertEquals(HttpServletResponse.SC_NOT_FOUND, writer.getErrorResponseStatus(new FileUtil.InvalidPathReferenceException("../x", "Path to parent not allowed"))); + assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, writer.getErrorResponseStatus(new IllegalArgumentException())); + } + } } diff --git a/api/src/org/labkey/api/docker/DockerService.java b/api/src/org/labkey/api/docker/DockerService.java index aa337a38a85..254ad2a85be 100644 --- a/api/src/org/labkey/api/docker/DockerService.java +++ b/api/src/org/labkey/api/docker/DockerService.java @@ -25,7 +25,6 @@ import org.labkey.vfs.FileLike; import java.io.File; -import java.io.FileFilter; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -36,6 +35,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; // Moved from git, see history at https://github.com/LabKey/docker/commits/release18.1/api-src/org/labkey/api/docker/DockerService.java @@ -463,7 +463,7 @@ public DockerContainer( void start(String containerId); - void stop(String containerId, Map archivesFromContainer, boolean delete); + void stop(String containerId, Map archivesFromContainer, boolean delete); void stop(DockerContainer dc); @@ -481,7 +481,7 @@ public DockerContainer( boolean isUseDockerVolumes(); - default void executeR(DockerImage dockerImage, FileLike scriptFile, String localWorkingDir, String remoteWorkingDir, FileFilter inputScripts) throws IOException + default void executeR(DockerImage dockerImage, FileLike scriptFile, FileLike localWorkingDir, String remoteWorkingDir, Predicate inputScripts) throws IOException { throw new UnsupportedOperationException(NO_DOCKER); } diff --git a/api/src/org/labkey/api/reports/report/r/RDockerScriptEngine.java b/api/src/org/labkey/api/reports/report/r/RDockerScriptEngine.java index 7211d9b5ef4..fbec9c4102c 100644 --- a/api/src/org/labkey/api/reports/report/r/RDockerScriptEngine.java +++ b/api/src/org/labkey/api/reports/report/r/RDockerScriptEngine.java @@ -29,14 +29,15 @@ import org.labkey.api.util.FileUtil; import org.labkey.api.util.UnexpectedException; import org.labkey.vfs.FileLike; +import org.labkey.vfs.FileSystemLike; import javax.script.ScriptContext; import javax.script.ScriptException; import java.io.File; -import java.io.FileFilter; import java.net.URI; import java.net.URISyntaxException; import java.util.Collections; +import java.util.function.Predicate; /** * Created by matthew on 7/14/17. @@ -100,7 +101,9 @@ protected Object eval(FileLike scriptFile, ScriptContext context) throws ScriptE { try (CustomTiming t = MiniProfiler.custom("docker", "execute r in docker container")) { - _ds.executeR(_dockerImage, scriptFile, getRWorkingDir(context), _remoteWorkingDir, InputFiles()); + // root the file system at the working dir so files copied back from the container can't escape it + FileLike workingDir = new FileSystemLike.Builder(getWorkingDir(context).toNioPathForWrite()).readwrite().root(); + _ds.executeR(_dockerImage, scriptFile, workingDir, _remoteWorkingDir, InputFiles()); appendConsoleOutput(context, output); } catch (Exception e) @@ -122,7 +125,7 @@ protected Object eval(FileLike scriptFile, ScriptContext context) throws ScriptE /** * Filter for the input script files in the report temp folder to copy to the Docker container which will run R */ - private static FileFilter InputFiles() + private static Predicate InputFiles() { return pathname -> pathname.isFile() && diff --git a/api/src/org/labkey/api/util/FileUtil.java b/api/src/org/labkey/api/util/FileUtil.java index 0257783fd6b..6d530cf6314 100644 --- a/api/src/org/labkey/api/util/FileUtil.java +++ b/api/src/org/labkey/api/util/FileUtil.java @@ -885,12 +885,15 @@ public static File appendPath(File dir, org.labkey.api.util.Path originalPath) /** Only returns a child path */ - public static FileLike appendPath(FileLike dir, org.labkey.api.util.Path path) + public static FileLike appendPath(FileLike dir, org.labkey.api.util.Path originalPath) { - path = path.normalize(); - if (!path.isEmpty() && "..".equals(path.get(0))) - throw new InvalidPathReferenceException(path.toString(), "Path to parent not allowed"); - return dir.resolveFile(path); + // normalize() returns null rather than a path with a leading ".." + org.labkey.api.util.Path path = originalPath.normalize(); + assert path == null || path.isEmpty() || !path.get(0).equals(".."); + if (path == null || (!path.isEmpty() && "..".equals(path.get(0)))) + throw new InvalidPathReferenceException(originalPath.toString(), "Path to parent not allowed"); + // resolveFile() resolves an absolute path from the file system root, not from dir + return dir.resolveFile(path.relative()); } @@ -2480,6 +2483,18 @@ public void testNoAcceptableExtensions() assertNull("Numeric extension should be allowed", checkExtension("test.1", mockProps)); } + @Test + public void testAppendPathFileLike() + { + FileLike sub = new FileSystemLike.Builder(ROOT).root().resolveChild("sub"); + FileLike x = sub.resolveChild("x"); + assertEquals(x, appendPath(sub, org.labkey.api.util.Path.parse("a/../x"))); + assertEquals(x, appendPath(sub, org.labkey.api.util.Path.parse("/x"))); + assertEquals(sub, appendPath(sub, org.labkey.api.util.Path.parse("/"))); + assertThrows(InvalidPathReferenceException.class, () -> appendPath(sub, org.labkey.api.util.Path.parse("../x"))); + assertThrows(InvalidPathReferenceException.class, () -> appendPath(sub, org.labkey.api.util.Path.parse("a/../../x"))); + } + @Test public void testGetAppendedFileName() { diff --git a/api/src/org/labkey/api/util/Path.java b/api/src/org/labkey/api/util/Path.java index 7789a4db842..d2642ff1338 100644 --- a/api/src/org/labkey/api/util/Path.java +++ b/api/src/org/labkey/api/util/Path.java @@ -395,6 +395,11 @@ public Path getParent() return _parent.get(); } + + /** + * Create a path that does not contain parts "." or "..". + * NOTE: returns null if the normalized path would need to start with "../" + */ public Path normalize() { for (int i = 0; i < _length; i++) diff --git a/api/src/org/labkey/vfs/AbstractFileLike.java b/api/src/org/labkey/vfs/AbstractFileLike.java index dadce6d94f0..eabfd56ba47 100644 --- a/api/src/org/labkey/vfs/AbstractFileLike.java +++ b/api/src/org/labkey/vfs/AbstractFileLike.java @@ -18,6 +18,7 @@ import com.fasterxml.jackson.core.JsonGenerator; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; +import org.labkey.api.util.FileUtil; import org.labkey.api.util.Path; import org.labkey.api.view.UnauthorizedException; @@ -117,7 +118,7 @@ static Path resolvePath(Path base, Path path) var resolved = base.resolve(path); resolved = resolved.normalize(); if (null == resolved) - throw new IllegalArgumentException("Illegal path " + path + " in " + base); + throw new FileUtil.InvalidPathReferenceException(path.toString(), "Illegal path in " + base); assert resolved.isAbsolute(); return resolved; } diff --git a/api/src/org/labkey/vfs/FileLike.java b/api/src/org/labkey/vfs/FileLike.java index de3e2a42f1b..c4e8aa9f1e4 100644 --- a/api/src/org/labkey/vfs/FileLike.java +++ b/api/src/org/labkey/vfs/FileLike.java @@ -37,7 +37,6 @@ import java.io.OutputStream; import java.net.URI; import java.nio.file.Files; -import java.nio.file.InvalidPathException; import java.util.List; import java.util.function.Predicate; @@ -116,10 +115,10 @@ default FileLike resolveChild(Path.Part name) default FileLike resolveChild(String name) { if (".".equals(name) || "..".equals(name)) - throw new InvalidPathException(name, "Cannot resolve child"); + throw new FileUtil.InvalidPathReferenceException(name, "Cannot resolve child"); Path path = Path.parse(name); if (1 != path.size()) - throw new InvalidPathException(name, "Cannot resolve child"); + throw new FileUtil.InvalidPathReferenceException(name, "Cannot resolve child"); return resolveFile(path); } diff --git a/api/src/org/labkey/vfs/FileSystemLike.java b/api/src/org/labkey/vfs/FileSystemLike.java index 432fb08e065..f945165fa29 100644 --- a/api/src/org/labkey/vfs/FileSystemLike.java +++ b/api/src/org/labkey/vfs/FileSystemLike.java @@ -293,11 +293,7 @@ static FileLike wrapFile(java.nio.file.Path p) /** Helper for partially converted code. root must exist. */ static FileLike wrapFile(File root, File f) throws IOException { - if (!root.isDirectory()) - throw new FileNotFoundException(root.getPath()); - FileSystemLike fs = new Builder(root.toURI()).build(); - String rel = FileUtil.relativize(root, f, true); - return fs.getRoot().resolveFile(Path.parse(rel)); + return wrapFile(FileUtil.getAbsoluteCaseSensitiveFile(root).toPath(), FileUtil.getAbsoluteCaseSensitiveFile(f).toPath()); } /** Helper for partially converted code. root must exist. */ @@ -311,7 +307,7 @@ static FileLike wrapFile(java.nio.file.Path root, java.nio.file.Path f) throws I { throw new IOException("File '" + f.toUri().getPath() + "' is outside the allowed root '" + root.toUri().getPath() + "'"); } - return fs.resolveFile(new Path(relative.getPath())); + return fs.resolveFile(Path.parse(relative.getPath())); } /** Helper for partially converted code. May throw if the FileLike does not wrap a local file system. */ @@ -408,7 +404,7 @@ static FileLike getVerifiedFileLike(Container container, String filePath) } // if root = /a/b/c/ and file = /a/b/c/d/e/f.xlsx - among other things, this essentially checks if '/a/b/c/d/e/f.xlsx' starts with '/a/b/c/' - return allowedRoot.resolveFile(new Path(relativeURI.getPath())); + return allowedRoot.resolveFile(Path.parse(relativeURI.getPath())); } } diff --git a/api/src/org/labkey/vfs/FileSystemLocal.java b/api/src/org/labkey/vfs/FileSystemLocal.java index 7a175af6596..cc1f749f2b1 100644 --- a/api/src/org/labkey/vfs/FileSystemLocal.java +++ b/api/src/org/labkey/vfs/FileSystemLocal.java @@ -91,13 +91,32 @@ public FileLike getRoot() public FileLike resolveFile(Path path) { LOG.debug("Resolving {} against {}", path, nioRoot); - path = path.absolute().normalize(); - if (null == path) - throw new IllegalArgumentException("Path could not be resolved"); + var normalized = path.absolute().normalize(); + if (null == normalized) + throw new FileUtil.InvalidPathReferenceException(path.toString(), "Path could not be resolved"); var nioPath = nioRoot; - for (var s : path) - nioPath = nioPath.resolve(s); - return createFileLike(path, nioPath.toFile()); + for (var s : normalized) + nioPath = nioPath.resolve(toNioName(s)); + if (!nioPath.normalize().startsWith(nioRoot.normalize())) + throw new FileUtil.InvalidPathReferenceException(path.toString(), "Path is outside the file system root"); + return createFileLike(normalized, nioPath.toFile()); + } + + // Each Path part must map to exactly one nio name; reject a part nio would split or treat as a root (e.g. "..\x" or "C:x" on Windows) + private java.nio.file.Path toNioName(String part) + { + java.nio.file.Path name; + try + { + name = nioRoot.getFileSystem().getPath(part); + } + catch (InvalidPathException x) + { + throw new FileUtil.InvalidPathReferenceException(part, x.getReason()); + } + if (name.getRoot() != null || name.getNameCount() != 1 || ".".equals(part) || "..".equals(part)) + throw new FileUtil.InvalidPathReferenceException(part, "Not a valid file name"); + return name; } diff --git a/api/src/org/labkey/vfs/vfsTestCase.jsp b/api/src/org/labkey/vfs/vfsTestCase.jsp index 25fd562bedc..d9c72e6ac04 100644 --- a/api/src/org/labkey/vfs/vfsTestCase.jsp +++ b/api/src/org/labkey/vfs/vfsTestCase.jsp @@ -17,6 +17,9 @@ <%@ page import="org.junit.Before" %> <%@ page import="org.junit.Test" %> <%@ page import="java.io.File" %> +<%@ page import="java.io.IOException" %> +<%@ page import="java.io.FileNotFoundException" %> +<%@ page import="java.nio.file.Files" %> <%@ page import="java.net.URI" %> <%@ page import="org.labkey.api.util.FileUtil" %> <%@ page import="org.labkey.vfs.FileSystemLike" %> @@ -69,14 +72,20 @@ FileLike tempDir = new FileSystemLike.Builder(tempPath.toFile()).root(); assertNotNull(tempDir); - FileLike t1 = tempDir.resolveFile(Path.parse(tempPath + "\\b\\t1.txt")); + FileLike t1 = tempDir.resolveFile(Path.parse("b/t1.txt")); assertNotNull(t1); - FileLike td1 = tempDir.resolveFile(Path.parse(tempPath + "\\b\\")); + FileLike td1 = tempDir.resolveFile(Path.parse("b/")); assertNotNull(td1); FileLike tc1 = td1.resolveChild("t1.txt"); assertNotNull(tc1); assertEquals(t1, tc1); + for (String name : new String[] {tempPath + "\\b\\t1.txt", "..\\x", "a\\..\\..\\x", "a\\b", "C:\\x", "C:x", "\\x", "\\\\host\\share\\x", "a tempDir.resolveFile(new Path(name))); + assertThrows(name, FileUtil.InvalidPathReferenceException.class, () -> tempDir.resolveChild(name)); + } + FileLike root = new FileSystemLike.Builder(new File("C:\\")).root(); assertNotNull(root); FileLike r1 = root.resolveFile(Path.parse("r1.txt")); @@ -203,9 +212,65 @@ } - void resolve(FileSystemLike fs) + @Test + public void testWrapFile() throws Exception { + File root = tempPath.toFile(); + java.nio.file.Path nested = Files.createFile(Files.createDirectory(tempPath.resolve("sub")).resolve("f.txt")); + + FileLike fromFile = FileSystemLike.wrapFile(root, nested.toFile()); + assertEquals(2, fromFile.getPath().size()); + assertEquals("sub", fromFile.getPath().get(0)); + assertEquals("f.txt", fromFile.getName()); + assertTrue(fromFile.isFile()); + + FileLike fromNio = FileSystemLike.wrapFile(tempPath, nested); + assertEquals(fromFile, fromNio); + assertTrue(fromNio.isFile()); + + assertThrows(IOException.class, () -> FileSystemLike.wrapFile(root, new File(root.getParentFile(), "x.txt"))); + assertThrows(IOException.class, () -> FileSystemLike.wrapFile(root, new File(new File(root.getParentFile(), root.getName() + "foo"), "x.txt"))); + assertThrows(IOException.class, () -> FileSystemLike.wrapFile(root, new File(root, "sub/../../x.txt"))); + assertEquals(fromFile, FileSystemLike.wrapFile(root, new File(root, "sub/../sub/f.txt"))); + + FileLike self = FileSystemLike.wrapFile(root, root); + assertTrue(self.getPath().isEmpty()); + assertTrue(self.isDirectory()); + + // URI round trip must not decode the name twice + java.nio.file.Path encoded = Files.createFile(tempPath.resolve("a b%20c.txt")); + FileLike e = FileSystemLike.wrapFile(root, encoded.toFile()); + assertEquals(1, e.getPath().size()); + assertEquals("a b%20c.txt", e.getName()); + assertTrue(e.isFile()); + + assertThrows(FileNotFoundException.class, () -> FileSystemLike.wrapFile(nested.toFile(), nested.toFile())); + + if (!System.getProperty("os.name").toLowerCase().startsWith("windows")) + { + // '\' is an ordinary file name character here + java.nio.file.Path backslash = Files.createFile(tempPath.resolve("a\\b.txt")); + FileLike f = FileSystemLike.wrapFile(root, backslash.toFile()); + assertEquals(1, f.getPath().size()); + assertEquals("a\\b.txt", f.getName()); + assertTrue(f.isFile()); + } + } + + void resolve(FileSystemLike fs) + { + FileLike root = fs.getRoot(); + FileLike a = root.resolveChild("a"); + assertEquals(a, root.resolveFile(Path.parse("b/../a"))); + assertEquals("b.txt", a.resolveFile(new Path("b.txt")).getName()); + + // a Path part is a single file name, never a path + for (String name : new String[] {"../x", "a/../../x", "a/b", "/x"}) + assertThrows(name, FileUtil.InvalidPathReferenceException.class, () -> a.resolveFile(new Path(name))); + for (String name : new String[] {"..", "../x", "a/b"}) + assertThrows(name, FileUtil.InvalidPathReferenceException.class, () -> a.resolveChild(name)); + assertThrows(FileUtil.InvalidPathReferenceException.class, () -> a.resolveFile(Path.parse("../../x"))); } void list(FileSystemLike fs)