Allow instance names to be any string

Replace bad characters with underscore instead of rejecting bad instance
names.

CHANGELOG.md

@@ -86,7 +86,6 @@
   Breaks playerManager.
 - The masterIP and masterPort config entries has been merged into masterURL.
   Breaks discordChat, playerManager, serverSelect, trainTeleports and clusterioModel.
-- Instance names can no longer be invalid Windows file names.
 - Removed config management from the command line and the server manager.
 - Moved ejs templates into views folder and changed their extension to .ejs.
   Breaks playerManager.

packages/slave/src/Instance.js

@@ -450,7 +450,6 @@ class Instance extends libLink.Link {
 	 */
 	static async create(instanceDir, factorioDir) {
 		logger.info(`Creating ${instanceDir}`);
-		await fs.ensureDir(instanceDir);
 		await fs.ensureDir(path.join(instanceDir, "script-output"));
 		await fs.ensureDir(path.join(instanceDir, "saves"));
 	}

packages/slave/src/Slave.js

@@ -65,6 +65,42 @@ function checkFilename(name) {
 	}
 }
 
+/**
+ * Clean up string to be suitable for use as filename
+ *
+ * @param {string} name - Arbitrary name string
+ * @returns {string} Filename suitable to use in the filesystem
+ */
+function cleanFilename(name) {
+	// copied from checkFilename
+	const badChars = /[<>:"\/\\|?*\x00-\x1f]/g;
+	const badEnd = /[. ]$/;
+
+	const oneToNine = [1, 2, 3, 4, 5, 6, 7, 8, 9];
+	const badNames = [
+		// Relative path components
+		".", "..",
+
+		// Reserved filenames in Windows
+		"CON", "PRN", "AUX", "NUL",
+		...oneToNine.map(n => `COM${n}`),
+		...oneToNine.map(n => `LPT${n}`),
+	];
+
+	if (typeof name !== "string") {
+		throw new Error("name must be a string");
+	}
+
+	if (name === "" || badNames.includes(name.toUpperCase())) {
+		name += "_";
+	}
+
+	name = name.replace(badChars, "_");
+	name = name.replace(badEnd, "_");
+
+	return name;
+}
+
 function checkRequestSaveName(name) {
 	try {
 		checkFilename(name);
@@ -203,11 +239,12 @@ class Slave extends libLink.Link {
 		}
 	}
 
-	async _findNewInstanceDir(name) {
+	async _createNewInstanceDir(name) {
+		name = cleanFilename(name);
 		try {
 			checkFilename(name);
 		} catch (err) {
-			throw new Error(`Instance name ${err.message}`);
+			throw new Error(`Instance folder was unepectedly invalid: name ${err.message}`);
 		}
 
 		let instancesDir = this.config.get("slave.instances_directory");
@@ -344,7 +381,7 @@ class Slave extends libLink.Link {
 				await instanceConfig.load(serialized_config, "master");
 
 				// XXX: race condition on multiple simultanious calls
-				let instanceDir = await this._findNewInstanceDir(instanceConfig.get("instance.name"));
+				let instanceDir = await this._createNewInstanceDir(instanceConfig.get("instance.name"));
 
 				await Instance.create(instanceDir, this.config.get("slave.factorio_directory"));
 				instanceInfo = {
@@ -787,6 +824,7 @@ module.exports = Slave;
 
 // For testing only
 module.exports._checkFilename = checkFilename;
+module.exports._cleanFilename = cleanFilename;
 module.exports._discoverInstances = discoverInstances;
 module.exports._Slave = Slave;
 

test/slave.js

@@ -84,6 +84,56 @@ describe("Slave testing", function() {
 		});
 	});
 
+	describe("cleanFilename()", function() {
+		function clean(item, expected) {
+			assert.equal(Slave._cleanFilename(item), expected);
+		}
+
+		it("should allow a basic name", function() {
+			clean("file", "file");
+		});
+
+		function check(item, msg) {
+			assert.throws(() => Slave._cleanFilename(item), new Error(msg));
+		}
+
+		it("should throw on non-string", function() {
+			check(undefined, "name must be a string");
+			check(null, "name must be a string");
+			check({}, "name must be a string");
+			check([], "name must be a string");
+			check(0, "name must be a string");
+			check(false, "name must be a string");
+		});
+
+		it("should clean empty name", function() {
+			clean("", "_");
+		});
+
+		it("should clean <>:\"\\/|?* \\x00\\r\\n\\t", function() {
+			for (let char of '<>:"\\/|?*\x00\r\n\t') {
+				clean(char, "_");
+			}
+		});
+
+		it("should clean CON, PRN, AUX, NUL, COM1, LPT1", function() {
+			for (let bad of ["CON", "PRN", "AUX", "NUL", "COM1", "LPT1"]) {
+				clean(bad, `${bad}_`);
+			}
+		});
+
+		it("should clean . and ..", function() {
+			for (let bad of [".", ".."]) {
+				clean(bad, `${bad}_`);
+			}
+		});
+
+		it("should clean names ending with . or space", function() {
+			clean("a ", "a_");
+			clean("a.", "a_");
+		});
+	});
+
 	describe("symlinkMods()", function() {
 		let testDir = path.join("temp", "test", "symlink");