Skip to content

Commit f776e71

Browse files
Copilotslachiewicz
Copilot
and
slachiewicz
committed
Fix symlink overwrite issue in archive extraction
- Changed AbstractUnArchiver to use `new File(dir, entryName)` instead of `FileUtils.resolveFile()` which was following symlinks - Updated shouldExtractEntry to always extract symlinks to allow overwriting - Updated canonicalDestPath calculation to not follow symlinks - Added deletion of existing symlink before creating new one - Added tests to verify symlinks are properly overwritten Co-authored-by: slachiewicz <[email protected]>
1 parent 24207d5 commit f776e71

File tree

2 files changed

+177
-5
lines changed

2 files changed

+177
-5
lines changed

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

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import org.codehaus.plexus.components.io.filemappers.FileMapper;
3434
import org.codehaus.plexus.components.io.fileselectors.FileSelector;
3535
import org.codehaus.plexus.components.io.resources.PlexusIoResource;
36-
import org.codehaus.plexus.util.FileUtils;
3736
import org.codehaus.plexus.util.StringUtils;
3837
import org.slf4j.Logger;
3938
import org.slf4j.LoggerFactory;
@@ -290,21 +289,32 @@ protected void extractFile(
290289
}
291290
}
292291

293-
// Hmm. Symlinks re-evaluate back to the original file here. Unsure if this is a good thing...
294-
final File targetFileName = FileUtils.resolveFile(dir, entryName);
292+
// Don't use FileUtils.resolveFile as it follows symlinks, which would cause issues
293+
// when trying to overwrite an existing symlink
294+
final File targetFileName = new File(dir, entryName);
295295

296296
// Make sure that the resolved path of the extracted file doesn't escape the destination directory
297297
// getCanonicalFile().toPath() is used instead of getCanonicalPath() (returns String),
298298
// because "/opt/directory".startsWith("/opt/dir") would return false negative.
299299
Path canonicalDirPath = dir.getCanonicalFile().toPath();
300-
Path canonicalDestPath = targetFileName.getCanonicalFile().toPath();
300+
// Don't follow symlinks for the target file, to avoid issues with symlink handling
301+
Path targetPath = targetFileName.toPath();
302+
// For security check, we need the canonical path but without following the last symlink
303+
Path canonicalDestPath;
304+
if (Files.isSymbolicLink(targetPath)) {
305+
// If it's a symlink, get the canonical path of the parent and append the file name
306+
canonicalDestPath =
307+
targetFileName.getParentFile().getCanonicalFile().toPath().resolve(targetFileName.getName());
308+
} else {
309+
canonicalDestPath = targetFileName.getCanonicalFile().toPath();
310+
}
301311

302312
if (!canonicalDestPath.startsWith(canonicalDirPath)) {
303313
throw new ArchiverException("Entry is outside of the target directory (" + entryName + ")");
304314
}
305315

306316
// don't allow override target symlink by standard file
307-
if (StringUtils.isEmpty(symlinkDestination) && Files.isSymbolicLink(canonicalDestPath)) {
317+
if (StringUtils.isEmpty(symlinkDestination) && Files.isSymbolicLink(targetPath)) {
308318
throw new ArchiverException("Entry is outside of the target directory (" + entryName + ")");
309319
}
310320

@@ -320,6 +330,10 @@ protected void extractFile(
320330
}
321331

322332
if (!StringUtils.isEmpty(symlinkDestination)) {
333+
// Delete existing symlink if it exists
334+
if (Files.isSymbolicLink(targetFileName.toPath())) {
335+
Files.delete(targetFileName.toPath());
336+
}
323337
SymlinkUtils.createSymbolicLink(targetFileName, new File(symlinkDestination));
324338
} else if (isDirectory) {
325339
targetFileName.mkdirs();
@@ -362,6 +376,11 @@ protected boolean shouldExtractEntry(File targetDirectory, File targetFileName,
362376
// scenario (4) and (5).
363377
// No matter the case sensitivity of the file system, file.exists() returns false when there is no file with the
364378
// same name (1).
379+
// Check if it's a symlink first, as exists() follows symlinks and may give incorrect results
380+
if (Files.isSymbolicLink(targetFileName.toPath())) {
381+
// For symlinks, always extract to overwrite the existing symlink
382+
return true;
383+
}
365384
if (!targetFileName.exists()) {
366385
return true;
367386
}

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

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.junit.jupiter.api.condition.DisabledOnOs;
1515
import org.junit.jupiter.api.condition.OS;
1616

17+
import static org.junit.jupiter.api.Assertions.assertEquals;
1718
import static org.junit.jupiter.api.Assertions.assertFalse;
1819
import static org.junit.jupiter.api.Assertions.assertTrue;
1920

@@ -107,4 +108,156 @@ void testSymlinkDirArchiver() throws Exception {
107108
symbolicLink = new File("target/output/dirarchiver-symlink/aDirWithALink/backOutsideToFileX");
108109
assertTrue(Files.isSymbolicLink(symbolicLink.toPath()));
109110
}
111+
112+
@Test
113+
@DisabledOnOs(OS.WINDOWS)
114+
void testSymlinkOverwriteZip() throws Exception {
115+
// Create temporary directory structure for testing
116+
File tempDir = new File("target/test-symlink-overwrite");
117+
// Clean up from any previous test runs
118+
if (tempDir.exists()) {
119+
org.codehaus.plexus.util.FileUtils.deleteDirectory(tempDir);
120+
}
121+
tempDir.mkdirs();
122+
123+
// Create two target files
124+
File target1 = new File(tempDir, "target1.txt");
125+
File target2 = new File(tempDir, "target2.txt");
126+
Files.write(target1.toPath(), "content1".getBytes());
127+
Files.write(target2.toPath(), "content2".getBytes());
128+
129+
// Create first archive with symlink pointing to target1
130+
File archive1Dir = new File(tempDir, "archive1");
131+
archive1Dir.mkdirs();
132+
File archive1Target1 = new File(archive1Dir, "target1.txt");
133+
Files.write(archive1Target1.toPath(), "content1".getBytes());
134+
Files.createSymbolicLink(
135+
new File(archive1Dir, "link.txt").toPath(),
136+
archive1Target1.toPath().getFileName());
137+
138+
ZipArchiver archiver1 = (ZipArchiver) lookup(Archiver.class, "zip");
139+
archiver1.addDirectory(archive1Dir);
140+
File zipFile1 = new File(tempDir, "archive1.zip");
141+
archiver1.setDestFile(zipFile1);
142+
archiver1.createArchive();
143+
144+
// Extract first archive
145+
File outputDir = new File(tempDir, "output");
146+
outputDir.mkdirs();
147+
ZipUnArchiver unarchiver1 = (ZipUnArchiver) lookup(UnArchiver.class, "zip");
148+
unarchiver1.setSourceFile(zipFile1);
149+
unarchiver1.setDestFile(outputDir);
150+
unarchiver1.extract();
151+
152+
// Verify symlink points to target1.txt
153+
File extractedLink = new File(outputDir, "link.txt");
154+
assertTrue(Files.isSymbolicLink(extractedLink.toPath()));
155+
assertEquals(
156+
"target1.txt", Files.readSymbolicLink(extractedLink.toPath()).toString());
157+
158+
// Create second archive with symlink pointing to target2
159+
File archive2Dir = new File(tempDir, "archive2");
160+
archive2Dir.mkdirs();
161+
File archive2Target2 = new File(archive2Dir, "target2.txt");
162+
Files.write(archive2Target2.toPath(), "content2".getBytes());
163+
Files.createSymbolicLink(
164+
new File(archive2Dir, "link.txt").toPath(),
165+
archive2Target2.toPath().getFileName());
166+
167+
ZipArchiver archiver2 = (ZipArchiver) lookup(Archiver.class, "zip");
168+
archiver2.addDirectory(archive2Dir);
169+
File zipFile2 = new File(tempDir, "archive2.zip");
170+
archiver2.setDestFile(zipFile2);
171+
archiver2.createArchive();
172+
173+
// Extract second archive (should overwrite the symlink)
174+
ZipUnArchiver unarchiver2 = (ZipUnArchiver) lookup(UnArchiver.class, "zip");
175+
unarchiver2.setSourceFile(zipFile2);
176+
unarchiver2.setDestFile(outputDir);
177+
unarchiver2.extract();
178+
179+
// Verify symlink now points to target2.txt (THIS IS THE KEY TEST)
180+
assertTrue(Files.isSymbolicLink(extractedLink.toPath()), "link.txt should still be a symlink");
181+
assertEquals(
182+
"target2.txt",
183+
Files.readSymbolicLink(extractedLink.toPath()).toString(),
184+
"Symlink should be updated to point to target2.txt");
185+
}
186+
187+
@Test
188+
@DisabledOnOs(OS.WINDOWS)
189+
void testSymlinkOverwriteTar() throws Exception {
190+
// Create temporary directory structure for testing
191+
File tempDir = new File("target/test-symlink-overwrite-tar");
192+
// Clean up from any previous test runs
193+
if (tempDir.exists()) {
194+
org.codehaus.plexus.util.FileUtils.deleteDirectory(tempDir);
195+
}
196+
tempDir.mkdirs();
197+
198+
// Create two target files
199+
File target1 = new File(tempDir, "target1.txt");
200+
File target2 = new File(tempDir, "target2.txt");
201+
Files.write(target1.toPath(), "content1".getBytes());
202+
Files.write(target2.toPath(), "content2".getBytes());
203+
204+
// Create first archive with symlink pointing to target1
205+
File archive1Dir = new File(tempDir, "archive1");
206+
archive1Dir.mkdirs();
207+
File archive1Target1 = new File(archive1Dir, "target1.txt");
208+
Files.write(archive1Target1.toPath(), "content1".getBytes());
209+
Files.createSymbolicLink(
210+
new File(archive1Dir, "link.txt").toPath(),
211+
archive1Target1.toPath().getFileName());
212+
213+
TarArchiver archiver1 = (TarArchiver) lookup(Archiver.class, "tar");
214+
archiver1.setLongfile(TarLongFileMode.posix);
215+
archiver1.addDirectory(archive1Dir);
216+
File tarFile1 = new File(tempDir, "archive1.tar");
217+
archiver1.setDestFile(tarFile1);
218+
archiver1.createArchive();
219+
220+
// Extract first archive
221+
File outputDir = new File(tempDir, "output");
222+
outputDir.mkdirs();
223+
TarUnArchiver unarchiver1 = (TarUnArchiver) lookup(UnArchiver.class, "tar");
224+
unarchiver1.setSourceFile(tarFile1);
225+
unarchiver1.setDestFile(outputDir);
226+
unarchiver1.extract();
227+
228+
// Verify symlink points to target1.txt
229+
File extractedLink = new File(outputDir, "link.txt");
230+
assertTrue(Files.isSymbolicLink(extractedLink.toPath()));
231+
assertEquals(
232+
"target1.txt", Files.readSymbolicLink(extractedLink.toPath()).toString());
233+
234+
// Create second archive with symlink pointing to target2
235+
File archive2Dir = new File(tempDir, "archive2");
236+
archive2Dir.mkdirs();
237+
File archive2Target2 = new File(archive2Dir, "target2.txt");
238+
Files.write(archive2Target2.toPath(), "content2".getBytes());
239+
Files.createSymbolicLink(
240+
new File(archive2Dir, "link.txt").toPath(),
241+
archive2Target2.toPath().getFileName());
242+
243+
TarArchiver archiver2 = (TarArchiver) lookup(Archiver.class, "tar");
244+
archiver2.setLongfile(TarLongFileMode.posix);
245+
archiver2.addDirectory(archive2Dir);
246+
File tarFile2 = new File(tempDir, "archive2.tar");
247+
archiver2.setDestFile(tarFile2);
248+
archiver2.createArchive();
249+
250+
// Extract second archive (should overwrite the symlink)
251+
TarUnArchiver unarchiver2 = (TarUnArchiver) lookup(UnArchiver.class, "tar");
252+
unarchiver2.setSourceFile(tarFile2);
253+
unarchiver2.setDestFile(outputDir);
254+
unarchiver2.extract();
255+
256+
// Verify symlink now points to target2.txt (THIS IS THE KEY TEST)
257+
assertTrue(Files.isSymbolicLink(extractedLink.toPath()), "link.txt should still be a symlink");
258+
assertEquals(
259+
"target2.txt",
260+
Files.readSymbolicLink(extractedLink.toPath()).toString(),
261+
"Symlink should be updated to point to target2.txt");
262+
}
110263
}

0 commit comments

Comments
 (0)