Skip to content

Commit

Permalink
[MDEP-651] Warn on duplicate archive entries (#128)
Browse files Browse the repository at this point in the history
  • Loading branch information
mthmulders committed Oct 12, 2020
1 parent 36bf6bc commit efd980d
Show file tree
Hide file tree
Showing 3 changed files with 275 additions and 12 deletions.
70 changes: 60 additions & 10 deletions src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java
Expand Up @@ -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;
Expand Down Expand Up @@ -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 ) )
{
Expand All @@ -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();
}

}
118 changes: 116 additions & 2 deletions src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java
Expand Up @@ -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}
Expand All @@ -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()
Expand All @@ -58,6 +70,7 @@ protected void execute()
// unused
}
};
this.abstractUnArchiver.enableLogging( log );
}

@After
Expand All @@ -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
Expand All @@ -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;
}
}
}
99 changes: 99 additions & 0 deletions 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;
}
}

0 comments on commit efd980d

Please sign in to comment.