Skip to content

Commit

Permalink
More validation of maps on upload, reworked file name sanitization
Browse files Browse the repository at this point in the history
  • Loading branch information
Brutus5000 committed Oct 8, 2018
1 parent b5c59a8 commit f2c68c2
Show file tree
Hide file tree
Showing 9 changed files with 100 additions and 58 deletions.
4 changes: 2 additions & 2 deletions src/main/java/com/faforever/api/avatar/AvatarService.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
import com.faforever.api.error.ErrorCode;
import com.faforever.api.error.NotFoundApiException;
import com.faforever.api.error.ProgrammingError;
import com.faforever.api.utils.FileNameUtil;
import com.faforever.api.utils.FilePermissionUtil;
import com.faforever.api.utils.NameUtil;
import lombok.SneakyThrows;
import lombok.extern.slf4j.Slf4j;
import org.jetbrains.annotations.NotNull;
Expand Down Expand Up @@ -44,7 +44,7 @@ public AvatarService(AvatarRepository avatarRepository, FafApiProperties propert
@Transactional
public void createAvatar(AvatarMetadata avatarMetadata, String originalFilename, InputStream imageDataInputStream, long avatarImageFileSize) {
final Avatar avatarToCreate = new Avatar();
final String normalizedAvatarFileName = FileNameUtil.normalizeFileName(originalFilename);
final String normalizedAvatarFileName = NameUtil.normalizeFileName(originalFilename);
String url = String.format(properties.getAvatar().getDownloadUrlFormat(), normalizedAvatarFileName);
avatarRepository.findOneByUrl(url).ifPresent(existingAvatar -> {
throw new ApiException(new Error(ErrorCode.AVATAR_NAME_CONFLICT, normalizedAvatarFileName));
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/com/faforever/api/error/ErrorCode.java
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ public enum ErrorCode {
AVATAR_NAME_CONFLICT(168, "Invalid avatar file name", "Avatar file name ''{0}'' already exists."),
AVATAR_IN_USE(169, "Avatar in use", "Could not delete avatar {0, number}. Avatar still in use."),
ENTITY_NOT_FOUND(170, "Entity not found", "Entity with id: {0} not found."),
INVALID_AVATAR_DIMENSION(171, "Invalid avatar dimensions", "Avatar dimensions must be {0, number}x{1, number}, was: {2, number}x{3, number}.");
INVALID_AVATAR_DIMENSION(171, "Invalid avatar dimensions", "Avatar dimensions must be {0, number}x{1, number}, was: {2, number}x{3, number}."),
MAP_NAME_INVALID(172, "Map name invalid", "The name of the map in the scenario file can only contain printable ASCII characters and blanks."),
MOD_NAME_INVALID(172, "Mod name invalid", "The name of the mod in the scenario file can only contain printable ASCII characters and blanks.");

private final int code;
private final String title;
Expand Down
41 changes: 28 additions & 13 deletions src/main/java/com/faforever/api/map/MapService.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
import com.faforever.api.error.Error;
import com.faforever.api.error.ErrorCode;
import com.faforever.api.error.ProgrammingError;
import com.faforever.api.utils.FileNameUtil;
import com.faforever.api.utils.FilePermissionUtil;
import com.faforever.api.utils.NameUtil;
import com.faforever.commons.io.Unzipper;
import com.faforever.commons.io.Zipper;
import com.faforever.commons.lua.LuaLoader;
Expand Down Expand Up @@ -55,6 +55,7 @@ public class MapService {
"_script.lua"};
private static final Charset MAP_CHARSET = StandardCharsets.ISO_8859_1;
private static final String STUPID_MAP_FOLDER_PREFIX = "maps/";
public static final int MAP_DISPLAY_NAME_MAX_LENGTH = 100;
private final FafApiProperties fafApiProperties;
private final MapRepository mapRepository;
private final ContentService contentService;
Expand All @@ -72,7 +73,7 @@ void uploadMap(byte[] mapData, String mapFilename, Player author, boolean isRank
Assert.notNull(author, "'author' must not be null");
Assert.isTrue(mapData.length > 0, "'mapData' must not be empty");

mapFilename = FileNameUtil.normalizeFileName(mapFilename);
mapFilename = NameUtil.normalizeFileName(mapFilename);

MapUploadData progressData = new MapUploadData()
.setBaseDir(contentService.createTempDir())
Expand All @@ -83,20 +84,23 @@ void uploadMap(byte[] mapData, String mapFilename, Player author, boolean isRank
progressData.setUploadedFile(progressData.getBaseDir().resolve(mapFilename));
copyToTemporaryDirectory(mapData, progressData);

unzipFile(progressData);
postProcessZipFiles(progressData);
try {
unzipFile(progressData);
postProcessZipFiles(progressData);

parseScenarioLua(progressData);
checkLua(progressData);
postProcessLuaFile(progressData);
parseScenarioLua(progressData);
checkLua(progressData);
postProcessLuaFile(progressData);

updateMapEntities(progressData);
updateMapEntities(progressData);

renameFolderNameAndCorrectPathInLuaFiles(progressData);
generatePreview(progressData);
renameFolderNameAndCorrectPathInLuaFiles(progressData);
generatePreview(progressData);

zipMapData(progressData);
assert cleanup(progressData);
zipMapData(progressData);
} finally {
cleanup(progressData);
}
}

@SneakyThrows
Expand Down Expand Up @@ -165,6 +169,14 @@ private void checkLua(MapUploadData progressData) {
if (scenarioInfo.get(ScenarioMapInfo.NAME) == LuaValue.NIL) {
errors.add(new Error(ErrorCode.MAP_NAME_MISSING));
}
String displayName = scenarioInfo.get(ScenarioMapInfo.NAME).tojstring();
if (displayName.length() > MAP_DISPLAY_NAME_MAX_LENGTH) {
throw new ApiException(new Error(ErrorCode.MAP_NAME_TOO_LONG, MAP_DISPLAY_NAME_MAX_LENGTH, displayName.length()));
}
if (!NameUtil.isPrintableAsciiString(displayName)) {
throw new ApiException(new Error(ErrorCode.MAP_NAME_INVALID));
}

if (scenarioInfo.get(ScenarioMapInfo.DESCRIPTION) == LuaValue.NIL) {
errors.add(new Error(ErrorCode.MAP_DESCRIPTION_MISSING));
}
Expand Down Expand Up @@ -239,7 +251,10 @@ private void updateMapEntities(MapUploadData progressData) {
if (map == null) {
map = new Map();
}
String name = FileNameUtil.normalizeFileName(scenarioInfo.get(ScenarioMapInfo.NAME).toString());

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)
Expand Down
13 changes: 9 additions & 4 deletions src/main/java/com/faforever/api/mod/ModService.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
import com.faforever.api.error.ApiException;
import com.faforever.api.error.Error;
import com.faforever.api.error.ErrorCode;
import com.faforever.api.utils.FileNameUtil;
import com.faforever.api.utils.FilePermissionUtil;
import com.faforever.api.utils.NameUtil;
import com.faforever.commons.mod.ModReader;
import com.google.common.primitives.Ints;
import lombok.SneakyThrows;
Expand Down Expand Up @@ -146,8 +146,13 @@ private void validateModInfo(com.faforever.commons.mod.Mod modInfo) {
String name = modInfo.getName();
if (name == null) {
errors.add(new Error(ErrorCode.MOD_NAME_MISSING));
} else if (name.length() > properties.getMod().getMaxNameLength()) {
errors.add(new Error(ErrorCode.MOD_NAME_TOO_LONG));
} else {
if (name.length() > properties.getMod().getMaxNameLength()) {
errors.add(new Error(ErrorCode.MOD_NAME_TOO_LONG, properties.getMod().getMaxNameLength(), name.length()));
}
if (!NameUtil.isPrintableAsciiString(name)) {
errors.add(new Error(ErrorCode.MOD_NAME_INVALID));
}
}
if (modInfo.getUid() == null) {
errors.add(new Error(ErrorCode.MOD_UID_MISSING));
Expand Down Expand Up @@ -205,7 +210,7 @@ private String generateZipFileName(String displayName, short version) {
}

private String generateFolderName(String displayName, short version) {
return String.format("%s.v%04d", FileNameUtil.normalizeFileName(displayName), version);
return String.format("%s.v%04d", NameUtil.normalizeFileName(displayName), version);
}

private void store(com.faforever.commons.mod.Mod modInfo, Optional<Path> thumbnailPath, Player uploader, String zipFileName) {
Expand Down
23 changes: 0 additions & 23 deletions src/main/java/com/faforever/api/utils/FileNameUtil.java

This file was deleted.

34 changes: 34 additions & 0 deletions src/main/java/com/faforever/api/utils/NameUtil.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.faforever.api.utils;

import org.apache.commons.lang3.StringUtils;

import java.nio.file.Paths;
import java.text.Normalizer;
import java.text.Normalizer.Form;
import java.util.Locale;

public final class NameUtil {
private NameUtil() {
// Utility class
}

/**
* Returns true if the string only consists of all printable ASCII characters (see ASCII table from SP to ~ [SP being
* space])
*/
public static boolean isPrintableAsciiString(String name) {
return !name.matches(".*[^\\x20-\\x7E]+.*");
}

public static String normalizeFileName(String originalFileName) {
return StringUtils.strip(
Normalizer.normalize(
Paths.get(originalFileName
.replaceAll("[/\\\\ :*?<>|\"]", "_")
.toLowerCase(Locale.US))
.getFileName()
.toString(),
Form.NFKC),
"_");
}
}
2 changes: 1 addition & 1 deletion src/test/java/com/faforever/api/map/MapServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,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());
Expand Down
14 changes: 0 additions & 14 deletions src/test/java/com/faforever/api/utils/FileNameUtilTest.java

This file was deleted.

23 changes: 23 additions & 0 deletions src/test/java/com/faforever/api/utils/NameUtilTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package com.faforever.api.utils;

import org.junit.Test;

import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;

public class NameUtilTest {
@Test
public void testIsPrintableAsciiString() {
final boolean positiveResult = NameUtil.isPrintableAsciiString(" !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~");
assertThat(positiveResult, is(true));

final boolean negativeResult = NameUtil.isPrintableAsciiString("Hello this is ä test.");
assertThat(negativeResult, is(false));
}

@Test
public void generateFileName() throws Exception {
final String fileName = NameUtil.normalizeFileName(" :*?<>|\"{}:[]la/ \\la??");
assertThat(fileName, is("{}_[]la___la"));
}
}

0 comments on commit f2c68c2

Please sign in to comment.