diff options
10 files changed, 80 insertions, 13 deletions
diff --git a/config-application-package/src/main/java/com/yahoo/config/application/FileSystemWrapper.java b/config-application-package/src/main/java/com/yahoo/config/application/FileSystemWrapper.java index b9dd4943149..04b4de10fc9 100644 --- a/config-application-package/src/main/java/com/yahoo/config/application/FileSystemWrapper.java +++ b/config-application-package/src/main/java/com/yahoo/config/application/FileSystemWrapper.java @@ -16,20 +16,22 @@ import java.util.function.Predicate; */ public class FileSystemWrapper { - Predicate<Path> existence; - ThrowingFunction<Path, byte[], IOException> reader; + final Path root; + final Predicate<Path> existence; + final ThrowingFunction<Path, byte[], IOException> reader; - private FileSystemWrapper(Predicate<Path> existence, ThrowingFunction<Path, byte[], IOException> reader) { + private FileSystemWrapper(Path root, Predicate<Path> existence, ThrowingFunction<Path, byte[], IOException> reader) { + this.root = root; this.existence = existence; this.reader = reader; } - public static FileSystemWrapper ofFiles(Predicate<Path> existence, ThrowingFunction<Path, byte[], IOException> reader) { - return new FileSystemWrapper(existence, reader); + public static FileSystemWrapper ofFiles(Path root, Predicate<Path> existence, ThrowingFunction<Path, byte[], IOException> reader) { + return new FileSystemWrapper(root, existence, reader); } - public static FileSystemWrapper getDefault() { - return ofFiles(Files::exists, Files::readAllBytes); + public static FileSystemWrapper getDefault(Path root) { + return ofFiles(root, Files::exists, Files::readAllBytes); } public FileWrapper wrap(Path path) { @@ -39,7 +41,13 @@ public class FileSystemWrapper { public class FileWrapper { private final Path path; - private FileWrapper(Path path) { this.path = path; } + private FileWrapper(Path path) { + Path relative = root.relativize(path).normalize(); + if (relative.isAbsolute() || relative.startsWith("..")) + throw new IllegalArgumentException(path + " is not a descendant of " + root); + + this.path = path; + } public Path path() { return path; } public boolean exists() { return existence.test(path); } diff --git a/config-application-package/src/main/java/com/yahoo/config/application/IncludeProcessor.java b/config-application-package/src/main/java/com/yahoo/config/application/IncludeProcessor.java index 51f9aecd812..cf6f339fa74 100644 --- a/config-application-package/src/main/java/com/yahoo/config/application/IncludeProcessor.java +++ b/config-application-package/src/main/java/com/yahoo/config/application/IncludeProcessor.java @@ -29,7 +29,7 @@ class IncludeProcessor implements PreProcessor { private final FileWrapper application; public IncludeProcessor(File application) { - this(FileSystemWrapper.getDefault().wrap(application.toPath())); + this(FileSystemWrapper.getDefault(application.toPath()).wrap(application.toPath())); } public IncludeProcessor(FileWrapper application) { diff --git a/config-application-package/src/main/java/com/yahoo/config/application/XmlPreProcessor.java b/config-application-package/src/main/java/com/yahoo/config/application/XmlPreProcessor.java index 36ab5e86266..640cc7bcfa7 100644 --- a/config-application-package/src/main/java/com/yahoo/config/application/XmlPreProcessor.java +++ b/config-application-package/src/main/java/com/yahoo/config/application/XmlPreProcessor.java @@ -44,7 +44,7 @@ public class XmlPreProcessor { } public XmlPreProcessor(File applicationDir, Reader xmlInput, InstanceName instance, Environment environment, RegionName region) { - this(FileSystemWrapper.getDefault().wrap(applicationDir.toPath()), xmlInput, instance, environment, region); + this(FileSystemWrapper.getDefault(applicationDir.toPath()).wrap(applicationDir.toPath()), xmlInput, instance, environment, region); } public XmlPreProcessor(FileWrapper applicationDir, Reader xmlInput, InstanceName instance, Environment environment, RegionName region) { diff --git a/config-application-package/src/test/java/com/yahoo/config/application/IncludeProcessorTest.java b/config-application-package/src/test/java/com/yahoo/config/application/IncludeProcessorTest.java index 56ba4f01048..3de624c78ac 100644 --- a/config-application-package/src/test/java/com/yahoo/config/application/IncludeProcessorTest.java +++ b/config-application-package/src/test/java/com/yahoo/config/application/IncludeProcessorTest.java @@ -2,6 +2,7 @@ package com.yahoo.config.application; import com.yahoo.config.application.api.ApplicationPackage; +import org.junit.Assert; import org.junit.Test; import org.w3c.dom.Document; import org.xml.sax.SAXException; @@ -13,6 +14,9 @@ import java.io.File; import java.io.IOException; import java.nio.file.NoSuchFileException; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + /** * @author Ulf Lilleengen */ @@ -78,11 +82,33 @@ public class IncludeProcessorTest { TestBase.assertDocument(expected, doc); } + @Test + public void testIllegalParent() throws ParserConfigurationException, IOException, SAXException, TransformerException { + try { + File app = new File("src/test/resources/multienvapp_fail_parent"); + DocumentBuilder docBuilder = Xml.getPreprocessDocumentBuilder(); + new IncludeProcessor(app).process(docBuilder.parse(getServices(app))); + fail("sibling to package should not be allowed"); + } + catch (IllegalArgumentException e) { + assertEquals("src/test/resources/multienvapp_fail_parent/../multienvapp/services.xml is not a descendant of src/test/resources/multienvapp_fail_parent", e.getMessage()); + } + } + + @Test(expected = IllegalArgumentException.class) + public void testIllegalParent2() throws ParserConfigurationException, IOException, SAXException, TransformerException { + File app = new File("src/test/resources/multienvapp_fail_parent2"); + DocumentBuilder docBuilder = Xml.getPreprocessDocumentBuilder(); + new IncludeProcessor(app).process(docBuilder.parse(getServices(app))); + fail("absolute include path should not be allowed"); + } + @Test(expected = NoSuchFileException.class) public void testRequiredIncludeIsDefault() throws ParserConfigurationException, IOException, SAXException, TransformerException { File app = new File("src/test/resources/multienvapp_failrequired"); DocumentBuilder docBuilder = Xml.getPreprocessDocumentBuilder(); new IncludeProcessor(app).process(docBuilder.parse(getServices(app))); + fail("should fail by default to include a non-existent file"); } static File getServices(File app) { diff --git a/config-application-package/src/test/resources/multienvapp_fail_parent/services.xml b/config-application-package/src/test/resources/multienvapp_fail_parent/services.xml new file mode 100644 index 00000000000..b68e304e4c9 --- /dev/null +++ b/config-application-package/src/test/resources/multienvapp_fail_parent/services.xml @@ -0,0 +1,4 @@ +<!-- Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. --> +<services version='1.0' xmlns:deploy="vespa" xmlns:preprocess="properties"> + <preprocess:include file='../multienvapp/services.xml' /> +</services> diff --git a/config-application-package/src/test/resources/multienvapp_fail_parent2/services.xml b/config-application-package/src/test/resources/multienvapp_fail_parent2/services.xml new file mode 100644 index 00000000000..e3f6bc3bd4c --- /dev/null +++ b/config-application-package/src/test/resources/multienvapp_fail_parent2/services.xml @@ -0,0 +1,4 @@ +<!-- Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. --> +<services version='1.0' xmlns:deploy="vespa" xmlns:preprocess="properties"> + <preprocess:include file='/absolute/path' /> +</services> diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java index 8e0c029aeab..f58dce3d861 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java @@ -110,6 +110,8 @@ public class ApplicationPackage { this.trustedCertificates = files.get(trustedCertificatesFile).map(bytes -> X509CertificateUtils.certificateListFromPem(new String(bytes, UTF_8))).orElse(List.of()); this.bundleHash = calculateBundleHash(); + + preProcessAndPopulateCache(); } /** Returns a copy of this with the given certificate appended. */ @@ -177,7 +179,6 @@ public class ApplicationPackage { /** Returns a zip containing meta data about deployments of this package by the given job. */ public byte[] metaDataZip() { - preProcessAndPopulateCache(); return cacheZip(); } @@ -276,7 +277,8 @@ public class ApplicationPackage { } public FileSystemWrapper wrapper() { - return FileSystemWrapper.ofFiles(path -> get(path).isPresent(), // Assume content asked for will also be read ... + return FileSystemWrapper.ofFiles(Path.of("./"), // zip archive root + path -> get(path).isPresent(), // Assume content asked for will also be read ... path -> get(path).orElseThrow(() -> new NoSuchFileException(path.toString()))); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java index 9a831b27cb3..2b407dd597b 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java @@ -18,6 +18,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * @author valerijf @@ -104,7 +105,7 @@ public class ApplicationPackageTest { try { new ApplicationPackage(zip, false).metaDataZip(); - Assert.fail("Should fail on missing include file"); + fail("Should fail on missing include file"); } catch (RuntimeException e) { assertEquals("./jdisc.xml", e.getCause().getMessage()); @@ -112,6 +113,28 @@ public class ApplicationPackageTest { } @Test + public void testAbsoluteInclude() throws Exception { + try { + getApplicationZip("include-absolute.zip"); + fail("Should fail on include file outside zip"); + } + catch (RuntimeException e) { + assertEquals(IllegalArgumentException.class, e.getCause().getClass()); + } + } + + @Test + public void testParentInclude() throws Exception { + try { + getApplicationZip("include-parent.zip"); + fail("Should fail on include file outside zip"); + } + catch (RuntimeException e) { + assertEquals("java.lang.IllegalArgumentException: ./../not_found.xml is not a descendant of .", e.getCause().toString()); + } + } + + @Test public void testBundleHashesAreSameWithDifferentDeploymentXml() throws Exception { var originalPackage = getApplicationZip("original.zip"); var changedServices = getApplicationZip("changed-services-xml.zip"); diff --git a/controller-server/src/test/resources/application-packages/include-absolute.zip b/controller-server/src/test/resources/application-packages/include-absolute.zip Binary files differnew file mode 100644 index 00000000000..3b30cd8265a --- /dev/null +++ b/controller-server/src/test/resources/application-packages/include-absolute.zip diff --git a/controller-server/src/test/resources/application-packages/include-parent.zip b/controller-server/src/test/resources/application-packages/include-parent.zip Binary files differnew file mode 100644 index 00000000000..18c1b0f5e37 --- /dev/null +++ b/controller-server/src/test/resources/application-packages/include-parent.zip |