diff --git a/scm-core/src/main/java/sonia/scm/plugin/SmpArchive.java b/scm-core/src/main/java/sonia/scm/plugin/SmpArchive.java index bb750fdfd7..9620cbe156 100644 --- a/scm-core/src/main/java/sonia/scm/plugin/SmpArchive.java +++ b/scm-core/src/main/java/sonia/scm/plugin/SmpArchive.java @@ -21,53 +21,57 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.plugin; //~--- non-JDK imports -------------------------------------------------------- -import com.google.common.base.Charsets; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.common.io.ByteSource; import com.google.common.io.ByteStreams; import com.google.common.io.Files; import com.google.common.io.Resources; - import sonia.scm.util.IOUtil; -//~--- JDK imports ------------------------------------------------------------ - import java.io.File; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; - import java.net.URL; - import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.nio.file.Path; - import java.util.zip.ZipEntry; import java.util.zip.ZipInputStream; +//~--- JDK imports ------------------------------------------------------------ + /** * Smp plugin archive. * * @author Sebastian Sdorra * @since 2.0.0 */ -public final class SmpArchive -{ +public final class SmpArchive { + + private final ByteSource archive; + private final ZipInputStreamFactory zipInputStreamFactory; + private InstalledPluginDescriptor plugin; /** * Constructs ... * - * * @param archive */ - public SmpArchive(ByteSource archive) - { + public SmpArchive(ByteSource archive) { + this(archive, source -> new ZipInputStream(archive.openStream(), StandardCharsets.UTF_8)); + } + + @VisibleForTesting + SmpArchive(ByteSource archive, ZipInputStreamFactory zipInputStreamFactory) { this.archive = archive; + this.zipInputStreamFactory = zipInputStreamFactory; } //~--- methods -------------------------------------------------------------- @@ -75,52 +79,40 @@ public final class SmpArchive /** * Method description * - * * @param archive - * * @return */ - public static SmpArchive create(ByteSource archive) - { + public static SmpArchive create(ByteSource archive) { return new SmpArchive(archive); } /** * Method description * - * * @param archive - * * @return */ - public static SmpArchive create(URL archive) - { + public static SmpArchive create(URL archive) { return create(Resources.asByteSource(archive)); } /** * Method description * - * * @param archive - * * @return */ - public static SmpArchive create(Path archive) - { + public static SmpArchive create(Path archive) { return create(archive.toFile()); } /** * Method description * - * * @param archive - * * @return */ - public static SmpArchive create(File archive) - { + public static SmpArchive create(File archive) { return create(Files.asByteSource(archive)); } @@ -129,17 +121,13 @@ public final class SmpArchive /** * Method description * - * * @param entry - * * @return */ - private static String getPath(ZipEntry entry) - { + private static String getPath(ZipEntry entry) { String path = entry.getName().replace("\\", "/"); - if (!path.startsWith("/")) - { + if (!path.startsWith("/")) { path = "/".concat(path); } @@ -151,33 +139,27 @@ public final class SmpArchive /** * Method description * - * * @param target - * * @throws IOException */ - public void extract(File target) throws IOException - { - try (ZipInputStream zis = open()) - { + public void extract(File target) throws IOException { + try (ZipInputStream zis = open()) { ZipEntry ze = zis.getNextEntry(); - while (ze != null) - { + while (ze != null) { String fileName = ze.getName(); File file = new File(target, fileName); + if (!IOUtil.isChild(target, file)) { + throw new PluginException("smp contains illegal path, which tries to escape from the plugin directory: " + fileName); + } IOUtil.mkdirs(file.getParentFile()); - if (ze.isDirectory()) - { + if (ze.isDirectory()) { IOUtil.mkdirs(file); - } - else - { - try (FileOutputStream fos = new FileOutputStream(file)) - { + } else { + try (FileOutputStream fos = new FileOutputStream(file)) { ByteStreams.copy(zis, fos); } } @@ -194,32 +176,25 @@ public final class SmpArchive /** * Method description * - * * @return - * * @throws IOException */ - public InstalledPluginDescriptor getPlugin() throws IOException - { - if (plugin == null) - { + public InstalledPluginDescriptor getPlugin() throws IOException { + if (plugin == null) { plugin = createPlugin(); PluginInformation info = plugin.getInformation(); - if (info == null) - { + if (info == null) { throw new PluginException("could not find information section"); } - if (Strings.isNullOrEmpty(info.getName())) - { + if (Strings.isNullOrEmpty(info.getName())) { throw new PluginException( "could not find name in plugin descriptor"); } - if (Strings.isNullOrEmpty(info.getVersion())) - { + if (Strings.isNullOrEmpty(info.getVersion())) { throw new PluginException( "could not find version in plugin descriptor"); } @@ -233,26 +208,20 @@ public final class SmpArchive /** * Method description * - * * @return - * * @throws IOException */ - private InstalledPluginDescriptor createPlugin() throws IOException - { + private InstalledPluginDescriptor createPlugin() throws IOException { InstalledPluginDescriptor p = null; NonClosingZipInputStream zis = null; - try - { + try { zis = openNonClosing(); ZipEntry entry = zis.getNextEntry(); - while (entry != null) - { - if (PluginConstants.PATH_DESCRIPTOR.equals(getPath(entry))) - { + while (entry != null) { + if (PluginConstants.PATH_DESCRIPTOR.equals(getPath(entry))) { p = Plugins.parsePluginDescriptor(new InputStreamByteSource(zis)); } @@ -260,17 +229,13 @@ public final class SmpArchive } zis.closeEntry(); - } - finally - { - if (zis != null) - { + } finally { + if (zis != null) { zis.reallyClose(); } } - if (p == null) - { + if (p == null) { throw new PluginLoadException("could not find plugin descriptor"); } @@ -280,27 +245,21 @@ public final class SmpArchive /** * Method description * - * * @return - * * @throws IOException */ - private ZipInputStream open() throws IOException - { - return new ZipInputStream(archive.openStream(), Charsets.UTF_8); + private ZipInputStream open() throws IOException { + return zipInputStreamFactory.open(archive); } /** * Method description * - * * @return - * * @throws IOException */ - private NonClosingZipInputStream openNonClosing() throws IOException - { - return new NonClosingZipInputStream(archive.openStream(), Charsets.UTF_8); + private NonClosingZipInputStream openNonClosing() throws IOException { + return new NonClosingZipInputStream(archive.openStream(), StandardCharsets.UTF_8); } //~--- inner classes -------------------------------------------------------- @@ -308,21 +267,17 @@ public final class SmpArchive /** * Class description * - * - * @version Enter version here..., 14/07/13 - * @author Enter your name here... + * @author Enter your name here... + * @version Enter version here..., 14/07/13 */ - private static class InputStreamByteSource extends ByteSource - { + private static class InputStreamByteSource extends ByteSource { /** * Constructs ... * - * * @param input */ - public InputStreamByteSource(InputStream input) - { + public InputStreamByteSource(InputStream input) { this.input = input; } @@ -331,20 +286,19 @@ public final class SmpArchive /** * Method description * - * * @return - * * @throws IOException */ @Override - public InputStream openStream() throws IOException - { + public InputStream openStream() throws IOException { return input; } //~--- fields ------------------------------------------------------------- - /** Field description */ + /** + * Field description + */ private final InputStream input; } @@ -352,22 +306,18 @@ public final class SmpArchive /** * Class description * - * - * @version Enter version here..., 14/07/12 - * @author Enter your name here... + * @author Enter your name here... + * @version Enter version here..., 14/07/12 */ - private static class NonClosingZipInputStream extends ZipInputStream - { + private static class NonClosingZipInputStream extends ZipInputStream { /** * Constructs ... * - * * @param in * @param charset */ - public NonClosingZipInputStream(InputStream in, Charset charset) - { + public NonClosingZipInputStream(InputStream in, Charset charset) { super(in, charset); } @@ -376,12 +326,10 @@ public final class SmpArchive /** * Method description * - * * @throws IOException */ @Override - public void close() throws IOException - { + public void close() throws IOException { // do nothing } @@ -389,21 +337,17 @@ public final class SmpArchive /** * Method description * - * * @throws IOException */ - public void reallyClose() throws IOException - { + public void reallyClose() throws IOException { super.close(); } } + @FunctionalInterface + interface ZipInputStreamFactory { - //~--- fields --------------------------------------------------------------- + ZipInputStream open(ByteSource source) throws IOException; - /** Field description */ - private final ByteSource archive; - - /** Field description */ - private InstalledPluginDescriptor plugin; + } } diff --git a/scm-core/src/test/java/sonia/scm/plugin/SmpArchiveTest.java b/scm-core/src/test/java/sonia/scm/plugin/SmpArchiveTest.java index e536272e89..c732f5c313 100644 --- a/scm-core/src/test/java/sonia/scm/plugin/SmpArchiveTest.java +++ b/scm-core/src/test/java/sonia/scm/plugin/SmpArchiveTest.java @@ -21,259 +21,154 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - -package sonia.scm.plugin; -//~--- non-JDK imports -------------------------------------------------------- +package sonia.scm.plugin; import com.google.common.base.Charsets; import com.google.common.base.Strings; -import com.google.common.base.Throwables; +import com.google.common.io.ByteSource; import com.google.common.io.Files; - -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; - +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.w3c.dom.Document; - import org.xml.sax.SAXException; - import sonia.scm.util.IOUtil; import sonia.scm.util.XmlUtil; -import static org.junit.Assert.*; - -//~--- JDK imports ------------------------------------------------------------ - -import java.io.File; -import java.io.FileInputStream; -import java.io.FileOutputStream; -import java.io.IOException; - -import java.util.zip.ZipEntry; -import java.util.zip.ZipOutputStream; - import javax.xml.parsers.ParserConfigurationException; import javax.xml.stream.XMLOutputFactory; import javax.xml.stream.XMLStreamException; import javax.xml.stream.XMLStreamWriter; +import java.io.File; +import java.io.FileInputStream; +import java.io.FileOutputStream; +import java.io.IOException; +import java.nio.file.Path; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; +import java.util.zip.ZipOutputStream; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; /** - * * @author Sebastian Sdorra */ -public class SmpArchiveTest -{ +class SmpArchiveTest { - /** - * Method description - * - * - * @throws IOException - * @throws ParserConfigurationException - * @throws SAXException - */ @Test - public void testExtract() - throws IOException, ParserConfigurationException, SAXException - { - File archive = createArchive("sonia.sample", "1.0"); - File target = tempFolder.newFolder(); + void shouldExtractArchive(@TempDir Path tempDir) throws IOException, ParserConfigurationException, SAXException, XMLStreamException { + File archive = createArchive(tempDir, "sonia.sample", "1.0"); + File target = tempDir.toFile(); IOUtil.mkdirs(target); SmpArchive.create(archive).extract(target); File descriptor = new File(target, PluginConstants.FILE_DESCRIPTOR); - assertTrue(descriptor.exists()); + assertThat(descriptor).exists(); - try (FileInputStream fis = new FileInputStream(descriptor)) - { + try (FileInputStream fis = new FileInputStream(descriptor)) { Document doc = XmlUtil.createDocument(fis); - - assertEquals("plugin", doc.getDocumentElement().getNodeName()); + assertThat(doc.getDocumentElement().getNodeName()).isEqualTo("plugin"); } } - /** - * Method description - * - * - * @throws IOException - */ @Test - public void testGetPlugin() throws IOException - { - File archive = createArchive("sonia.sample", "1.0"); + void shouldReturnPluginDescriptor(@TempDir Path tempDir) throws IOException, XMLStreamException { + File archive = createArchive(tempDir, "sonia.sample", "1.0"); InstalledPluginDescriptor plugin = SmpArchive.create(archive).getPlugin(); - assertNotNull(plugin); + assertThat(plugin).isNotNull(); PluginInformation info = plugin.getInformation(); - assertNotNull(info); + assertThat(info).isNotNull(); - assertEquals("sonia.sample", info.getName()); - assertEquals("1.0", info.getVersion()); + assertThat(info.getName()).isEqualTo("sonia.sample"); + assertThat(info.getVersion()).isEqualTo("1.0"); } - /** - * Method description - * - * @throws IOException - */ - @Test(expected = PluginException.class) - public void testWithMissingName() throws IOException - { - File archive = createArchive( null, "1.0"); + @Test + void shouldFailOnMissingName(@TempDir Path tempDir) throws IOException, XMLStreamException { + File archive = createArchive(tempDir, null, "1.0"); - SmpArchive.create(archive).getPlugin(); + SmpArchive smp = SmpArchive.create(archive); + assertThrows(PluginException.class, smp::getPlugin); } - /** - * Method description - * - * @throws IOException - */ - @Test(expected = PluginException.class) - public void testWithMissingVersion() throws IOException - { - File archive = createArchive("sonia.sample", null); - - SmpArchive.create(archive).getPlugin(); + @Test + void shouldFailOnMissingVersion(@TempDir Path tempDir) throws IOException, XMLStreamException { + File archive = createArchive(tempDir, "sonia.sample", null); + SmpArchive smp = SmpArchive.create(archive); + assertThrows(PluginException.class, smp::getPlugin); } - /** - * Method description - * - * - * @param name - * @param version - * - * @return - */ - private File createArchive(String name, String version) - { - File archiveFile; + @Test + void shouldFailOnZipEntriesWhichCreateFilesOutsideOfThePluginFolder(@TempDir Path tempDir) throws IOException { + ZipInputStream zis = mock(ZipInputStream.class); + ZipEntry entry = mock(ZipEntry.class); + when(zis.getNextEntry()).thenReturn(entry); + when(entry.getName()).thenReturn("../../plugin.xml"); + SmpArchive smp = new SmpArchive(ByteSource.empty(), source -> zis); + File directory = tempDir.resolve("one").resolve("two").resolve("three").toFile(); + assertThat(directory.mkdirs()).isTrue(); + assertThrows(PluginException.class, () -> smp.extract(directory)); + } - try - { - File descriptor = tempFolder.newFile(); + private File createArchive(Path tempDir, String name, String version) throws IOException, XMLStreamException { + File descriptor = tempDir.resolve("descriptor.xml").toFile(); - writeDescriptor(descriptor, name, version); - archiveFile = tempFolder.newFile(); + writeDescriptor(descriptor, name, version); + File archiveFile = tempDir.resolve("archive.smp").toFile(); - try (ZipOutputStream zos = - new ZipOutputStream(new FileOutputStream(archiveFile), Charsets.UTF_8)) - { - zos.putNextEntry(new ZipEntry(PluginConstants.PATH_DESCRIPTOR)); - Files.copy(descriptor, zos); - zos.closeEntry(); - zos.putNextEntry(new ZipEntry("/META-INF/")); - zos.putNextEntry(new ZipEntry("/META-INF/somefile.txt")); - zos.write("some text".getBytes(Charsets.UTF_8)); - zos.closeEntry(); - } - } - catch (IOException ex) - { - throw Throwables.propagate(ex); + try (ZipOutputStream zos = new ZipOutputStream(new FileOutputStream(archiveFile), Charsets.UTF_8)) { + zos.putNextEntry(new ZipEntry(PluginConstants.PATH_DESCRIPTOR)); + Files.copy(descriptor, zos); + zos.closeEntry(); + zos.putNextEntry(new ZipEntry("/META-INF/")); + zos.putNextEntry(new ZipEntry("/META-INF/somefile.txt")); + zos.write("some text".getBytes(Charsets.UTF_8)); + zos.closeEntry(); } return archiveFile; } - /** - * Method description - * - * - * @param file - * - * @return - * - * @throws IOException - * @throws XMLStreamException - */ - private XMLStreamWriter createStreamWriter(File file) - throws IOException, XMLStreamException - { - return XMLOutputFactory.newFactory().createXMLStreamWriter( - new FileOutputStream(file)); + private XMLStreamWriter createStreamWriter(File file) throws IOException, XMLStreamException { + return XMLOutputFactory.newFactory().createXMLStreamWriter(new FileOutputStream(file)); } - /** - * Method description - * - * - * @param descriptor - * @param name - * @param version - * - * @throws IOException - */ - private void writeDescriptor(File descriptor, String name, - String version) - throws IOException - { - try - { + private void writeDescriptor(File descriptor, String name, String version) throws IOException, XMLStreamException { + IOUtil.mkdirs(descriptor.getParentFile()); - IOUtil.mkdirs(descriptor.getParentFile()); + XMLStreamWriter writer = null; - XMLStreamWriter writer = null; + try { + writer = createStreamWriter(descriptor); + writer.writeStartDocument(); + writer.writeStartElement("plugin"); + writer.writeStartElement("information"); + writeElement(writer, "name", name); + writeElement(writer, "version", version); - try - { - writer = createStreamWriter(descriptor); - writer.writeStartDocument(); - writer.writeStartElement("plugin"); - writer.writeStartElement("information"); - writeElement(writer, "name", name); - writeElement(writer, "version", version); - - writer.writeEndElement(); - writer.writeEndElement(); - writer.writeEndDocument(); + writer.writeEndElement(); + writer.writeEndElement(); + writer.writeEndDocument(); + } finally { + if (writer != null) { + writer.close(); } - finally - { - if (writer != null) - { - writer.close(); - } - } - } - catch (XMLStreamException ex) - { - throw Throwables.propagate(ex); } } - /** - * Method description - * - * - * @param writer - * @param name - * @param value - * - * @throws XMLStreamException - */ - private void writeElement(XMLStreamWriter writer, String name, String value) - throws XMLStreamException - { - if (!Strings.isNullOrEmpty(value)) - { + private void writeElement(XMLStreamWriter writer, String name, String value) throws XMLStreamException { + if (!Strings.isNullOrEmpty(value)) { writer.writeStartElement(name); writer.writeCharacters(value); writer.writeEndElement(); } } - - //~--- fields --------------------------------------------------------------- - - /** Field description */ - @Rule - public TemporaryFolder tempFolder = new TemporaryFolder(); } diff --git a/scm-test/src/main/java/sonia/scm/repository/spi/ZippedRepositoryTestBase.java b/scm-test/src/main/java/sonia/scm/repository/spi/ZippedRepositoryTestBase.java index 0ea8887ff6..6994667056 100644 --- a/scm-test/src/main/java/sonia/scm/repository/spi/ZippedRepositoryTestBase.java +++ b/scm-test/src/main/java/sonia/scm/repository/spi/ZippedRepositoryTestBase.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.repository.spi; //~--- non-JDK imports -------------------------------------------------------- @@ -150,18 +150,18 @@ public abstract class ZippedRepositoryTestBase extends AbstractTestBase public static void extract(File targetFolder, String zippedRepositoryResource) throws IOException { URL url = Resources.getResource(zippedRepositoryResource); - ZipInputStream zip = null; - try + try (ZipInputStream zip = new ZipInputStream(url.openStream());) { - zip = new ZipInputStream(url.openStream()); - ZipEntry entry = zip.getNextEntry(); while (entry != null) { File file = new File(targetFolder, entry.getName()); File parent = file.getParentFile(); + if (!IOUtil.isChild(parent, file)) { + throw new IOException("invalid zip entry name"); + } if (!parent.exists()) { @@ -174,27 +174,16 @@ public abstract class ZippedRepositoryTestBase extends AbstractTestBase } else { - OutputStream output = null; - - try + try (OutputStream output = new FileOutputStream(file)) { - output = new FileOutputStream(file); IOUtil.copy(zip, output); } - finally - { - IOUtil.close(output); - } } zip.closeEntry(); entry = zip.getNextEntry(); } } - finally - { - IOUtil.close(zip); - } } //~--- fields ---------------------------------------------------------------