Proper file name sanitizing (follow-up fix for #222)

FAForever · Jun 7, 2019 · e4cc6f2 · e4cc6f2
1 parent ccdb884
commit e4cc6f2
Show file tree

Hide file tree

Showing 10 changed files with 14 additions and 22 deletions.
diff --git a/.idea/modules.xml b/.idea/modules.xml
diff --git a/src/main/java/com/faforever/api/avatar/AvatarService.java b/src/main/java/com/faforever/api/avatar/AvatarService.java
@@ -56,7 +56,7 @@ public void createAvatar(AvatarMetadata avatarMetadata, String originalFilename,
 
     final InputStream markSupportedImageInputStream = getMarkSupportedInputStream(imageDataInputStream);
     validateImageFile(originalFilename, avatarImageFileSize);
-    checkImageDimensions(markSupportedImageInputStream, normalizedAvatarFileName);
+    checkImageDimensions(markSupportedImageInputStream, originalFilename);
     final Path imageTargetPath = properties.getAvatar().getTargetDirectory().resolve(normalizedAvatarFileName);
 
     avatarRepository.save(avatarToCreate);

diff --git a/src/main/java/com/faforever/api/map/MapService.java b/src/main/java/com/faforever/api/map/MapService.java
@@ -75,8 +75,6 @@ public void uploadMap(byte[] mapData, String mapFilename, Player author, boolean
     Assert.notNull(author, "'author' must not be null");
     Assert.isTrue(mapData.length > 0, "'mapData' must not be empty");
 
-    mapFilename = NameUtil.normalizeFileName(mapFilename);
-
     MapUploadData progressData = new MapUploadData()
       .setBaseDir(contentService.createTempDir())
       .setUploadFileName(mapFilename)
@@ -256,7 +254,6 @@ private void updateMapEntities(MapUploadData progressData) {
 
     String name = scenarioInfo.get(ScenarioMapInfo.NAME).toString();
 
-
     map.setDisplayName(name)
       .setMapType(scenarioInfo.get(ScenarioMapInfo.TYPE).tojstring())
       .setBattleType(scenarioInfo.get(ScenarioMapInfo.CONFIGURATIONS).get(ScenarioMapInfo.CONFIGURATION_STANDARD).get(ScenarioMapInfo.CONFIGURATION_STANDARD_TEAMS).get(1)
@@ -393,17 +390,13 @@ private LuaValue getLuaScenarioInfo() {
       return scenarioInfo;
     }
 
-    private String normalizeMapName(String mapName) {
-      return Paths.get(mapName.toLowerCase().replaceAll(" ", "_")).normalize().toString();
-    }
-
     private String getNewFolderName() {
       return generateNewMapNameWithVersion("");
     }
 
     private String generateNewMapNameWithVersion(String extension) {
       return Paths.get(String.format("%s.v%04d%s",
-        normalizeMapName(mapEntity.getDisplayName()),
+        NameUtil.normalizeFileName(mapEntity.getDisplayName()),
         mapVersionEntity.getVersion(),
         extension))
         .normalize().toString();

diff --git a/src/main/java/com/faforever/api/utils/NameUtil.java b/src/main/java/com/faforever/api/utils/NameUtil.java
@@ -24,7 +24,7 @@ public static String normalizeFileName(String originalFileName) {
     return StringUtils.strip(
       Normalizer.normalize(
         Paths.get(originalFileName
-          .replaceAll("[/\\\\ :*?<>|\"]", "_")
+          .replaceAll("[^\\w\\d.]", "_")
           .toLowerCase(Locale.US))
           .getFileName()
           .toString(),

diff --git a/src/test/java/com/faforever/api/map/MapServiceTest.java b/src/test/java/com/faforever/api/map/MapServiceTest.java
@@ -85,7 +85,7 @@ public void shutDown() {
 
   @Test
   public void zipFilenamealreadyExists() throws IOException {
-    Path clashedMap = finalDirectory.getRoot().toPath().resolve("sludge_test.v0001.zip");
+    Path clashedMap = finalDirectory.getRoot().toPath().resolve("sludge_test____.___..v0001.zip");
     assertTrue(clashedMap.toFile().createNewFile());
     String zipFilename = "scmp_037.zip";
     when(mapRepository.findOneByDisplayName(any())).thenReturn(Optional.empty());
@@ -251,7 +251,7 @@ public void positiveUploadTest() throws IOException {
 
       ArgumentCaptor<com.faforever.api.data.domain.Map> mapCaptor = ArgumentCaptor.forClass(com.faforever.api.data.domain.Map.class);
       verify(mapRepository, Mockito.times(1)).save(mapCaptor.capture());
-      assertEquals("Sludge_Test", mapCaptor.getValue().getDisplayName());
+      assertEquals("Sludge_Test &!$.#+/.", mapCaptor.getValue().getDisplayName());
       assertEquals("skirmish", mapCaptor.getValue().getMapType());
       assertEquals("FFA", mapCaptor.getValue().getBattleType());
       assertEquals(1, mapCaptor.getValue().getVersions().size());
@@ -262,11 +262,11 @@ public void positiveUploadTest() throws IOException {
       assertEquals(256, mapVersion.getHeight());
       assertEquals(256, mapVersion.getWidth());
       assertEquals(3, mapVersion.getMaxPlayers());
-      assertEquals("maps/sludge_test.v0001.zip", mapVersion.getFilename());
+      assertEquals("maps/sludge_test____.___..v0001.zip", mapVersion.getFilename());
 
       assertFalse(Files.exists(tmpDir));
 
-      Path generatedFile = finalDirectory.getRoot().toPath().resolve("sludge_test.v0001.zip");
+      Path generatedFile = finalDirectory.getRoot().toPath().resolve("sludge_test____.___..v0001.zip");
       assertTrue(Files.exists(generatedFile));
 
       Path generatedFiles = finalDirectory.getRoot().toPath().resolve("generated_files");
@@ -277,25 +277,25 @@ public void positiveUploadTest() throws IOException {
 
       Path expectedFiles = finalDirectory.getRoot().toPath().resolve("expected_files");
       try (ZipInputStream inputStreamOfExpectedFile = new ZipInputStream(new BufferedInputStream(
-        loadMapResourceAsStream("sludge_test.v0001.zip")))) {
+        loadMapResourceAsStream("sludge_test____.___..v0001.zip")))) {
         Unzipper.from(inputStreamOfExpectedFile).to(expectedFiles).unzip();
       }
 
-      expectedFiles = expectedFiles.resolve("sludge_test.v0001");
+      expectedFiles = expectedFiles.resolve("sludge_test____.___..v0001");
       try (Stream<Path> fileStream = Files.list(expectedFiles)) {
         assertEquals(fileStream.count(), (long) 4);
       }
 
       try (Stream<Path> fileStream = Files.list(expectedFiles)) {
-        Path finalGeneratedFile = generatedFiles.resolve("sludge_test.v0001");
+        Path finalGeneratedFile = generatedFiles.resolve("sludge_test____.___..v0001");
         fileStream.forEach(expectedFile ->
           FileAssert.assertEquals("Difference in " + expectedFile.getFileName().toString(),
             expectedFile.toFile(),
             finalGeneratedFile.resolve(expectedFile.getFileName().toString()).toFile())
         );
 
-        assertTrue(Files.exists(mapProperties.getDirectoryPreviewPathLarge().resolve("sludge_test.v0001.png")));
-        assertTrue(Files.exists(mapProperties.getDirectoryPreviewPathSmall().resolve("sludge_test.v0001.png")));
+        assertTrue(Files.exists(mapProperties.getDirectoryPreviewPathLarge().resolve("sludge_test____.___..v0001.png")));
+        assertTrue(Files.exists(mapProperties.getDirectoryPreviewPathSmall().resolve("sludge_test____.___..v0001.png")));
       }
     }
   }

diff --git a/src/test/java/com/faforever/api/utils/NameUtilTest.java b/src/test/java/com/faforever/api/utils/NameUtilTest.java
@@ -17,7 +17,7 @@ public void testIsPrintableAsciiString() {
 
   @Test
   public void generateFileName() throws Exception {
-    final String fileName = NameUtil.normalizeFileName(" :*?<>|\"{}:[]la/ \\la??");
-    assertThat(fileName, is("{}_[]la___la"));
+    final String fileName = NameUtil.normalizeFileName("a :*?<>|\"{}:[]la/ \\la??0");
+    assertThat(fileName, is("a_____________la___la__0"));
   }
 }
diff --git a/src/test/resources/maps/scmp 037.zip b/src/test/resources/maps/scmp 037.zip
diff --git a/src/test/resources/maps/scmp_037.zip b/src/test/resources/maps/scmp_037.zip
diff --git a/src/test/resources/maps/sludge_test.v0001.zip b/src/test/resources/maps/sludge_test.v0001.zip
diff --git a/src/test/resources/maps/sludge_test____.___..v0001.zip b/src/test/resources/maps/sludge_test____.___..v0001.zip