From 393652c1d958225ec6263e48c4942480e7f5bd49 Mon Sep 17 00:00:00 2001
From: Hornwitser <github@hornwitser.no>
Date: Thu, 23 Jun 2022 00:08:23 +0200
Subject: [PATCH] Allow instance names to be any string

Replace bad characters with underscore instead of rejecting bad instance
names.
---
 CHANGELOG.md                   |  1 -
 packages/slave/src/Instance.js |  1 -
 packages/slave/src/Slave.js    | 44 ++++++++++++++++++++++++++++--
 test/slave.js                  | 50 ++++++++++++++++++++++++++++++++++
 4 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 39386312..424c643e 100644
--- a/CHANGELOG.md
+++ b/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.
diff --git a/packages/slave/src/Instance.js b/packages/slave/src/Instance.js
index 1b97ded0..a7169ec2 100644
--- a/packages/slave/src/Instance.js
+++ b/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"));
 	}
diff --git a/packages/slave/src/Slave.js b/packages/slave/src/Slave.js
index 6cc88d1f..174c4783 100644
--- a/packages/slave/src/Slave.js
+++ b/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;
 
diff --git a/test/slave.js b/test/slave.js
index 7603f803..a1fe4247 100644
--- a/test/slave.js
+++ b/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");