diff --git a/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java b/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java index 41f6c8cbe..bfb4b8fa4 100644 --- a/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java +++ b/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java @@ -22,6 +22,8 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.Paths; import java.util.ArrayList; import java.util.Date; import java.util.List; @@ -330,11 +332,11 @@ protected void extractFile( final File srcF, final File dir, final InputStream c } // Hmm. Symlinks re-evaluate back to the original file here. Unsure if this is a good thing... - final File f = FileUtils.resolveFile( dir, entryName ); + final File targetFileName = FileUtils.resolveFile( dir, entryName ); // Make sure that the resolved path of the extracted file doesn't escape the destination directory String canonicalDirPath = dir.getCanonicalPath(); - String canonicalDestPath = f.getCanonicalPath(); + String canonicalDestPath = targetFileName.getCanonicalPath(); if ( !canonicalDestPath.startsWith( canonicalDirPath ) ) { @@ -343,13 +345,13 @@ protected void extractFile( final File srcF, final File dir, final InputStream c try { - if ( !isOverwrite() && f.exists() && ( f.lastModified() >= entryDate.getTime() ) ) + if ( !shouldExtractEntry( dir, targetFileName, entryName, entryDate ) ) { return; } // create intermediary directories - sometimes zip don't add them - final File dirF = f.getParentFile(); + final File dirF = targetFileName.getParentFile(); if ( dirF != null ) { dirF.mkdirs(); @@ -357,31 +359,79 @@ protected void extractFile( final File srcF, final File dir, final InputStream c if ( !StringUtils.isEmpty( symlinkDestination ) ) { - SymlinkUtils.createSymbolicLink( f, new File( symlinkDestination ) ); + SymlinkUtils.createSymbolicLink( targetFileName, new File( symlinkDestination ) ); } else if ( isDirectory ) { - f.mkdirs(); + targetFileName.mkdirs(); } else { - try ( OutputStream out = new FileOutputStream( f ) ) + try ( OutputStream out = new FileOutputStream( targetFileName ) ) { IOUtil.copy( compressedInputStream, out ); } } - f.setLastModified( entryDate.getTime() ); + targetFileName.setLastModified( entryDate.getTime() ); if ( !isIgnorePermissions() && mode != null && !isDirectory ) { - ArchiveEntryUtils.chmod( f, mode ); + ArchiveEntryUtils.chmod( targetFileName, mode ); } } catch ( final FileNotFoundException ex ) { - getLogger().warn( "Unable to expand to file " + f.getPath() ); + getLogger().warn( "Unable to expand to file " + targetFileName.getPath() ); } } + // Visible for testing + protected boolean shouldExtractEntry( File targetDirectory, File targetFileName, String entryName, Date entryDate ) throws IOException + { + // entryname | entrydate | filename | filedate | behavior + // (1) readme.txt | 1970 | readme.txt | 2020 | never overwrite + // (2) readme.txt | 2020 | readme.txt | 1970 | only overwrite when isOverWrite() + // (3) README.txt | 1970 | readme.txt | 2020 | case-insensitive filesystem: warn + never overwrite + // case-sensitive filesystem: extract without warning + // (4) README.txt | 2020 | readme.txt | 1970 | case-insensitive filesystem: warn + only overwrite when isOverWrite() + // case-sensitive filesystem: extract without warning + + // The canonical file name follows the name of the archive entry, but takes into account the case- + // sensitivity of the filesystem. So on a case-sensitive file system, file.exists() returns false for + // scenario (3) and (4). + if ( !targetFileName.exists() ) + { + return true; + } + + String canonicalDestPath = targetFileName.getCanonicalPath(); + String relativeCanonicalDestPath = canonicalDestPath.replace( targetDirectory.getCanonicalPath() + File.separatorChar, "" ); + boolean fileOnDiskIsNewerThanEntry = targetFileName.lastModified() >= entryDate.getTime(); + boolean differentCasing = !entryName.equals( relativeCanonicalDestPath ); + + String casingMessage = String.format( "Archive entry '%s' and existing file '%s' names differ only by case." + + " This may lead to an unexpected outcome on case-insensitive filesystems.", entryName, canonicalDestPath ); + + // (1) + if ( fileOnDiskIsNewerThanEntry ) + { + // (3) + if ( differentCasing ) + { + getLogger().warn( casingMessage ); + } + return false; + } + + // (4) + if ( differentCasing ) + { + getLogger().warn( casingMessage ); + } + + // (2) + return isOverwrite(); + } + } diff --git a/src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java b/src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java index 0fd224ba5..b7870adb5 100644 --- a/src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java +++ b/src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java @@ -18,14 +18,22 @@ import java.io.File; import java.io.IOException; -import java.nio.file.Files; +import java.util.Date; import org.codehaus.plexus.components.io.filemappers.FileMapper; +import org.hamcrest.BaseMatcher; +import org.hamcrest.Description; +import org.hamcrest.core.StringContains; import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.junit.rules.TemporaryFolder; + +import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertThat; /** * Unit test for {@link AbstractUnArchiver} @@ -37,7 +45,11 @@ public class AbstractUnArchiverTest @Rule public ExpectedException thrown = ExpectedException.none(); + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(); + private AbstractUnArchiver abstractUnArchiver; + private CapturingLog log = new CapturingLog( 0 /*debug*/, "AbstractUnArchiver" ); @Before public void setUp() @@ -58,6 +70,7 @@ protected void execute() // unused } }; + this.abstractUnArchiver.enableLogging( log ); } @After @@ -72,7 +85,7 @@ public void shouldThrowExceptionBecauseRewrittenPathIsOutOfDirectory() { // given this.thrown.expectMessage( "Entry is outside of the target directory (../PREFIX/ENTRYNAME.SUFFIX)" ); - final File targetFolder = Files.createTempDirectory( null ).toFile(); + final File targetFolder = temporaryFolder.newFolder(); final FileMapper[] fileMappers = new FileMapper[] { new FileMapper() { @Override @@ -97,4 +110,105 @@ public String getMappedFileName( String pName ) // ArchiverException is thrown providing the rewritten path } + @Test + public void shouldExtractWhenFileOnDiskDoesNotExist() throws IOException + { + // given + File file = new File( temporaryFolder.getRoot(), "whatever.txt" ); // does not create the file! + String entryname = file.getName(); + Date entryDate = new Date(); + + // when & then + assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is ( true ) ); + } + + @Test + public void shouldNotExtractWhenFileOnDiskIsNewerThanEntryInArchive() throws IOException + { + // given + File file = temporaryFolder.newFile(); + file.setLastModified( System.currentTimeMillis() ); + String entryname = file.getName(); + Date entryDate = new Date( 0 ); + + // when & then + assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is ( false ) ); + } + + @Test + public void shouldNotExtractWhenFileOnDiskIsNewerThanEntryInArchive_andWarnAboutDifferentCasing() throws IOException + { + // given + File file = temporaryFolder.newFile(); + file.setLastModified( System.currentTimeMillis() ); + String entryname = file.getName().toUpperCase(); + Date entryDate = new Date( 0 ); + + // when & then + assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is ( false ) ); + assertThat( this.log.getWarns(), hasItem( new LogMessageMatcher( "names differ only by case" ) ) ); + } + + @Test + public void shouldExtractWhenEntryInArchiveIsNewerThanFileOnDisk() throws IOException + { + // given + File file = temporaryFolder.newFile(); + file.setLastModified( 0 ); + String entryname = file.getName().toUpperCase(); + Date entryDate = new Date( System.currentTimeMillis() ); + + // when & then + this.abstractUnArchiver.setOverwrite( true ); + assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is( true ) ); + + // when & then + this.abstractUnArchiver.setOverwrite( false ); + assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is( false ) ); + } + + @Test + public void shouldExtractWhenEntryInArchiveIsNewerThanFileOnDiskAndWarnAboutDifferentCasing() throws IOException + { + // given + File file = temporaryFolder.newFile(); + file.setLastModified( 0 ); + String entryname = file.getName().toUpperCase(); + Date entryDate = new Date( System.currentTimeMillis() ); + + // when & then + this.abstractUnArchiver.setOverwrite( true ); + assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is( true ) ); + this.abstractUnArchiver.setOverwrite( false ); + assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is( false ) ); + assertThat( this.log.getWarns(), hasItem( new LogMessageMatcher( "names differ only by case" ) ) ); + } + + static class LogMessageMatcher extends BaseMatcher { + private final StringContains delegateMatcher; + + LogMessageMatcher( String needle ) + { + this.delegateMatcher = new StringContains( needle ); + } + + @Override + public void describeTo( Description description ) + { + description.appendText( "a log message with " ); + delegateMatcher.describeTo( description ); + } + + @Override + public boolean matches( Object item ) + { + if ( item instanceof CapturingLog.Message ) + { + CapturingLog.Message message = (CapturingLog.Message) item; + String haystack = message.message; + return delegateMatcher.matches( haystack ); + } + return false; + } + } } diff --git a/src/test/java/org/codehaus/plexus/archiver/CapturingLog.java b/src/test/java/org/codehaus/plexus/archiver/CapturingLog.java new file mode 100644 index 000000000..320566d3e --- /dev/null +++ b/src/test/java/org/codehaus/plexus/archiver/CapturingLog.java @@ -0,0 +1,99 @@ +package org.codehaus.plexus.archiver; + +import org.codehaus.plexus.logging.AbstractLogger; +import org.codehaus.plexus.logging.Logger; + +import java.util.ArrayList; +import java.util.List; + +public class CapturingLog extends AbstractLogger +{ + public CapturingLog( int threshold, String name ) + { + super( threshold, name ); + } + + static class Message { + public final String message; + public final Throwable throwable; + + public Message( String message, Throwable throwable ) + { + this.message = message; + this.throwable = throwable; + } + + @Override + public String toString() + { + return "Message{" + "message='" + message + '\'' + ", throwable=" + throwable + '}'; + } + } + + private final List debugs = new ArrayList<>(); + @Override + public void debug( String s, Throwable throwable ) + { + debugs.add( new Message( s, throwable ) ); + } + + public List getDebugs() + { + return debugs; + } + + + private final List infos = new ArrayList<>(); + @Override + public void info( String s, Throwable throwable ) + { + infos.add( new Message( s, throwable ) ); + } + + public List getInfos() + { + return infos; + } + + private final List warns = new ArrayList<>(); + @Override + public void warn( String s, Throwable throwable ) + { + warns.add( new Message( s, throwable ) ); + } + + public List getWarns() + { + return warns; + } + + private final List errors = new ArrayList<>(); + @Override + public void error( String s, Throwable throwable ) + { + errors.add( new Message( s, throwable ) ); + } + + public List getErors() + { + return errors; + } + + private final List fatals = new ArrayList<>(); + @Override + public void fatalError( String s, Throwable throwable ) + { + fatals.add( new Message( s, throwable ) ); + } + + public List getFatals() + { + return fatals; + } + + @Override + public Logger getChildLogger( String s ) + { + return null; + } +}