Skip to content

Commit efd980d

Browse files
mthmuldersmthmulders
authored
[MDEP-651] Warn on duplicate archive entries (#128)
1 parent 36bf6bc commit efd980d

File tree

3 files changed

+275
-12
lines changed

3 files changed

+275
-12
lines changed

‎src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java

+60-10
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import java.io.IOException;
2323
import java.io.InputStream;
2424
import java.io.OutputStream;
25+
import java.nio.file.Files;
26+
import java.nio.file.Paths;
2527
import java.util.ArrayList;
2628
import java.util.Date;
2729
import java.util.List;
@@ -330,11 +332,11 @@ protected void extractFile( final File srcF, final File dir, final InputStream c
330332
}
331333

332334
// Hmm. Symlinks re-evaluate back to the original file here. Unsure if this is a good thing...
333-
final File f = FileUtils.resolveFile( dir, entryName );
335+
final File targetFileName = FileUtils.resolveFile( dir, entryName );
334336

335337
// Make sure that the resolved path of the extracted file doesn't escape the destination directory
336338
String canonicalDirPath = dir.getCanonicalPath();
337-
String canonicalDestPath = f.getCanonicalPath();
339+
String canonicalDestPath = targetFileName.getCanonicalPath();
338340

339341
if ( !canonicalDestPath.startsWith( canonicalDirPath ) )
340342
{
@@ -343,45 +345,93 @@ protected void extractFile( final File srcF, final File dir, final InputStream c
343345

344346
try
345347
{
346-
if ( !isOverwrite() && f.exists() && ( f.lastModified() >= entryDate.getTime() ) )
348+
if ( !shouldExtractEntry( dir, targetFileName, entryName, entryDate ) )
347349
{
348350
return;
349351
}
350352

351353
// create intermediary directories - sometimes zip don't add them
352-
final File dirF = f.getParentFile();
354+
final File dirF = targetFileName.getParentFile();
353355
if ( dirF != null )
354356
{
355357
dirF.mkdirs();
356358
}
357359

358360
if ( !StringUtils.isEmpty( symlinkDestination ) )
359361
{
360-
SymlinkUtils.createSymbolicLink( f, new File( symlinkDestination ) );
362+
SymlinkUtils.createSymbolicLink( targetFileName, new File( symlinkDestination ) );
361363
}
362364
else if ( isDirectory )
363365
{
364-
f.mkdirs();
366+
targetFileName.mkdirs();
365367
}
366368
else
367369
{
368-
try ( OutputStream out = new FileOutputStream( f ) )
370+
try ( OutputStream out = new FileOutputStream( targetFileName ) )
369371
{
370372
IOUtil.copy( compressedInputStream, out );
371373
}
372374
}
373375

374-
f.setLastModified( entryDate.getTime() );
376+
targetFileName.setLastModified( entryDate.getTime() );
375377

376378
if ( !isIgnorePermissions() && mode != null && !isDirectory )
377379
{
378-
ArchiveEntryUtils.chmod( f, mode );
380+
ArchiveEntryUtils.chmod( targetFileName, mode );
379381
}
380382
}
381383
catch ( final FileNotFoundException ex )
382384
{
383-
getLogger().warn( "Unable to expand to file " + f.getPath() );
385+
getLogger().warn( "Unable to expand to file " + targetFileName.getPath() );
384386
}
385387
}
386388

389+
// Visible for testing
390+
protected boolean shouldExtractEntry( File targetDirectory, File targetFileName, String entryName, Date entryDate ) throws IOException
391+
{
392+
// entryname | entrydate | filename | filedate | behavior
393+
// (1) readme.txt | 1970 | readme.txt | 2020 | never overwrite
394+
// (2) readme.txt | 2020 | readme.txt | 1970 | only overwrite when isOverWrite()
395+
// (3) README.txt | 1970 | readme.txt | 2020 | case-insensitive filesystem: warn + never overwrite
396+
// case-sensitive filesystem: extract without warning
397+
// (4) README.txt | 2020 | readme.txt | 1970 | case-insensitive filesystem: warn + only overwrite when isOverWrite()
398+
// case-sensitive filesystem: extract without warning
399+
400+
// The canonical file name follows the name of the archive entry, but takes into account the case-
401+
// sensitivity of the filesystem. So on a case-sensitive file system, file.exists() returns false for
402+
// scenario (3) and (4).
403+
if ( !targetFileName.exists() )
404+
{
405+
return true;
406+
}
407+
408+
String canonicalDestPath = targetFileName.getCanonicalPath();
409+
String relativeCanonicalDestPath = canonicalDestPath.replace( targetDirectory.getCanonicalPath() + File.separatorChar, "" );
410+
boolean fileOnDiskIsNewerThanEntry = targetFileName.lastModified() >= entryDate.getTime();
411+
boolean differentCasing = !entryName.equals( relativeCanonicalDestPath );
412+
413+
String casingMessage = String.format( "Archive entry '%s' and existing file '%s' names differ only by case."
414+
+ " This may lead to an unexpected outcome on case-insensitive filesystems.", entryName, canonicalDestPath );
415+
416+
// (1)
417+
if ( fileOnDiskIsNewerThanEntry )
418+
{
419+
// (3)
420+
if ( differentCasing )
421+
{
422+
getLogger().warn( casingMessage );
423+
}
424+
return false;
425+
}
426+
427+
// (4)
428+
if ( differentCasing )
429+
{
430+
getLogger().warn( casingMessage );
431+
}
432+
433+
// (2)
434+
return isOverwrite();
435+
}
436+
387437
}

‎src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java

+116-2
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,22 @@
1818

1919
import java.io.File;
2020
import java.io.IOException;
21-
import java.nio.file.Files;
21+
import java.util.Date;
2222

2323
import org.codehaus.plexus.components.io.filemappers.FileMapper;
24+
import org.hamcrest.BaseMatcher;
25+
import org.hamcrest.Description;
26+
import org.hamcrest.core.StringContains;
2427
import org.junit.After;
2528
import org.junit.Before;
2629
import org.junit.Rule;
2730
import org.junit.Test;
2831
import org.junit.rules.ExpectedException;
32+
import org.junit.rules.TemporaryFolder;
33+
34+
import static org.hamcrest.CoreMatchers.hasItem;
35+
import static org.hamcrest.CoreMatchers.is;
36+
import static org.junit.Assert.assertThat;
2937

3038
/**
3139
* Unit test for {@link AbstractUnArchiver}
@@ -37,7 +45,11 @@ public class AbstractUnArchiverTest
3745
@Rule
3846
public ExpectedException thrown = ExpectedException.none();
3947

48+
@Rule
49+
public TemporaryFolder temporaryFolder = new TemporaryFolder();
50+
4051
private AbstractUnArchiver abstractUnArchiver;
52+
private CapturingLog log = new CapturingLog( 0 /*debug*/, "AbstractUnArchiver" );
4153

4254
@Before
4355
public void setUp()
@@ -58,6 +70,7 @@ protected void execute()
5870
// unused
5971
}
6072
};
73+
this.abstractUnArchiver.enableLogging( log );
6174
}
6275

6376
@After
@@ -72,7 +85,7 @@ public void shouldThrowExceptionBecauseRewrittenPathIsOutOfDirectory()
7285
{
7386
// given
7487
this.thrown.expectMessage( "Entry is outside of the target directory (../PREFIX/ENTRYNAME.SUFFIX)" );
75-
final File targetFolder = Files.createTempDirectory( null ).toFile();
88+
final File targetFolder = temporaryFolder.newFolder();
7689
final FileMapper[] fileMappers = new FileMapper[] { new FileMapper()
7790
{
7891
@Override
@@ -97,4 +110,105 @@ public String getMappedFileName( String pName )
97110
// ArchiverException is thrown providing the rewritten path
98111
}
99112

113+
@Test
114+
public void shouldExtractWhenFileOnDiskDoesNotExist() throws IOException
115+
{
116+
// given
117+
File file = new File( temporaryFolder.getRoot(), "whatever.txt" ); // does not create the file!
118+
String entryname = file.getName();
119+
Date entryDate = new Date();
120+
121+
// when & then
122+
assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is ( true ) );
123+
}
124+
125+
@Test
126+
public void shouldNotExtractWhenFileOnDiskIsNewerThanEntryInArchive() throws IOException
127+
{
128+
// given
129+
File file = temporaryFolder.newFile();
130+
file.setLastModified( System.currentTimeMillis() );
131+
String entryname = file.getName();
132+
Date entryDate = new Date( 0 );
133+
134+
// when & then
135+
assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is ( false ) );
136+
}
137+
138+
@Test
139+
public void shouldNotExtractWhenFileOnDiskIsNewerThanEntryInArchive_andWarnAboutDifferentCasing() throws IOException
140+
{
141+
// given
142+
File file = temporaryFolder.newFile();
143+
file.setLastModified( System.currentTimeMillis() );
144+
String entryname = file.getName().toUpperCase();
145+
Date entryDate = new Date( 0 );
146+
147+
// when & then
148+
assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is ( false ) );
149+
assertThat( this.log.getWarns(), hasItem( new LogMessageMatcher( "names differ only by case" ) ) );
150+
}
151+
152+
@Test
153+
public void shouldExtractWhenEntryInArchiveIsNewerThanFileOnDisk() throws IOException
154+
{
155+
// given
156+
File file = temporaryFolder.newFile();
157+
file.setLastModified( 0 );
158+
String entryname = file.getName().toUpperCase();
159+
Date entryDate = new Date( System.currentTimeMillis() );
160+
161+
// when & then
162+
this.abstractUnArchiver.setOverwrite( true );
163+
assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is( true ) );
164+
165+
// when & then
166+
this.abstractUnArchiver.setOverwrite( false );
167+
assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is( false ) );
168+
}
169+
170+
@Test
171+
public void shouldExtractWhenEntryInArchiveIsNewerThanFileOnDiskAndWarnAboutDifferentCasing() throws IOException
172+
{
173+
// given
174+
File file = temporaryFolder.newFile();
175+
file.setLastModified( 0 );
176+
String entryname = file.getName().toUpperCase();
177+
Date entryDate = new Date( System.currentTimeMillis() );
178+
179+
// when & then
180+
this.abstractUnArchiver.setOverwrite( true );
181+
assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is( true ) );
182+
this.abstractUnArchiver.setOverwrite( false );
183+
assertThat( this.abstractUnArchiver.shouldExtractEntry( temporaryFolder.getRoot(), file, entryname, entryDate ), is( false ) );
184+
assertThat( this.log.getWarns(), hasItem( new LogMessageMatcher( "names differ only by case" ) ) );
185+
}
186+
187+
static class LogMessageMatcher extends BaseMatcher<CapturingLog.Message> {
188+
private final StringContains delegateMatcher;
189+
190+
LogMessageMatcher( String needle )
191+
{
192+
this.delegateMatcher = new StringContains( needle );
193+
}
194+
195+
@Override
196+
public void describeTo( Description description )
197+
{
198+
description.appendText( "a log message with " );
199+
delegateMatcher.describeTo( description );
200+
}
201+
202+
@Override
203+
public boolean matches( Object item )
204+
{
205+
if ( item instanceof CapturingLog.Message )
206+
{
207+
CapturingLog.Message message = (CapturingLog.Message) item;
208+
String haystack = message.message;
209+
return delegateMatcher.matches( haystack );
210+
}
211+
return false;
212+
}
213+
}
100214
}

‎src/test/java/org/codehaus/plexus/archiver/CapturingLog.java

+99
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
package org.codehaus.plexus.archiver;
2+
3+
import org.codehaus.plexus.logging.AbstractLogger;
4+
import org.codehaus.plexus.logging.Logger;
5+
6+
import java.util.ArrayList;
7+
import java.util.List;
8+
9+
public class CapturingLog extends AbstractLogger
10+
{
11+
public CapturingLog( int threshold, String name )
12+
{
13+
super( threshold, name );
14+
}
15+
16+
static class Message {
17+
public final String message;
18+
public final Throwable throwable;
19+
20+
public Message( String message, Throwable throwable )
21+
{
22+
this.message = message;
23+
this.throwable = throwable;
24+
}
25+
26+
@Override
27+
public String toString()
28+
{
29+
return "Message{" + "message='" + message + '\'' + ", throwable=" + throwable + '}';
30+
}
31+
}
32+
33+
private final List<Message> debugs = new ArrayList<>();
34+
@Override
35+
public void debug( String s, Throwable throwable )
36+
{
37+
debugs.add( new Message( s, throwable ) );
38+
}
39+
40+
public List<Message> getDebugs()
41+
{
42+
return debugs;
43+
}
44+
45+
46+
private final List<Message> infos = new ArrayList<>();
47+
@Override
48+
public void info( String s, Throwable throwable )
49+
{
50+
infos.add( new Message( s, throwable ) );
51+
}
52+
53+
public List<Message> getInfos()
54+
{
55+
return infos;
56+
}
57+
58+
private final List<Message> warns = new ArrayList<>();
59+
@Override
60+
public void warn( String s, Throwable throwable )
61+
{
62+
warns.add( new Message( s, throwable ) );
63+
}
64+
65+
public List<Message> getWarns()
66+
{
67+
return warns;
68+
}
69+
70+
private final List<Message> errors = new ArrayList<>();
71+
@Override
72+
public void error( String s, Throwable throwable )
73+
{
74+
errors.add( new Message( s, throwable ) );
75+
}
76+
77+
public List<Message> getErors()
78+
{
79+
return errors;
80+
}
81+
82+
private final List<Message> fatals = new ArrayList<>();
83+
@Override
84+
public void fatalError( String s, Throwable throwable )
85+
{
86+
fatals.add( new Message( s, throwable ) );
87+
}
88+
89+
public List<Message> getFatals()
90+
{
91+
return fatals;
92+
}
93+
94+
@Override
95+
public Logger getChildLogger( String s )
96+
{
97+
return null;
98+
}
99+
}

0 commit comments

Comments
 (0)