[MDEP-651] Warn on duplicate archive entries

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,45 +345,93 @@ 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();
             }
 
             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();
+    }
+
 }

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<CapturingLog.Message> {
+        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;
+        }
+    }
 }

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<Message> debugs = new ArrayList<>();
+    @Override
+    public void debug( String s, Throwable throwable )
+    {
+        debugs.add( new Message( s, throwable ) );
+    }
+
+    public List<Message> getDebugs()
+    {
+        return debugs;
+    }
+
+
+    private final List<Message> infos = new ArrayList<>();
+    @Override
+    public void info( String s, Throwable throwable )
+    {
+        infos.add( new Message( s, throwable ) );
+    }
+
+    public List<Message> getInfos()
+    {
+        return infos;
+    }
+
+    private final List<Message> warns = new ArrayList<>();
+    @Override
+    public void warn( String s, Throwable throwable )
+    {
+        warns.add( new Message( s, throwable ) );
+    }
+
+    public List<Message> getWarns()
+    {
+        return warns;
+    }
+
+    private final List<Message> errors = new ArrayList<>();
+    @Override
+    public void error( String s, Throwable throwable )
+    {
+        errors.add( new Message( s, throwable ) );
+    }
+
+    public List<Message> getErors()
+    {
+        return errors;
+    }
+
+    private final List<Message> fatals = new ArrayList<>();
+    @Override
+    public void fatalError( String s, Throwable throwable )
+    {
+        fatals.add( new Message( s, throwable ) );
+    }
+
+    public List<Message> getFatals()
+    {
+        return fatals;
+    }
+
+    @Override
+    public Logger getChildLogger( String s )
+    {
+        return null;
+    }
+}