Merge pull request #463 from Hornwitser/player-time-autosave-bugfixes

Player time autosave bugfixes

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.

docs/jsdoc.json

@@ -13,7 +13,9 @@
 		"include": [
 			"packages/master/src/",
 			"packages/master/master.js",
+			"packages/slave/src/",
 			"packages/slave/slave.js",
+			"packages/ctl/src/",
 			"packages/ctl/ctl.js",
 			"packages/lib/",
 			"plugins/"

packages/lib/plugin.js

@@ -39,7 +39,7 @@ class BaseInstancePlugin {
 
 		/**
 		 * Instance the plugin started for
-		 * @type {module:slave/slave~Instance}
+		 * @type {module:slave/src/Instance}
 		 */
 		this.instance = instance;
 
@@ -49,7 +49,7 @@ class BaseInstancePlugin {
 		 * With the exepction of accessing the slave's config you should
 		 * avoid ineracting with the slave object directly.
 		 *
-		 * @type {module:slave/slave~Slave}
+		 * @type {module:slave/src/Slave}
 		 */
 		this.slave = slave;
 

packages/slave/src/Instance.js

@@ -108,6 +108,7 @@ const serverSettingsActions = {
  *
  * @param {Instance} instance - Instance to link mods for
  * @param {string} sharedMods - Path to folder to link mods from.
+ * @private
  */
 async function symlinkMods(instance, sharedMods) {
 	await fs.ensureDir(instance.path("mods"));
@@ -450,7 +451,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,43 @@ 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
+ * @private
+ */
+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);
@@ -83,6 +120,7 @@ function checkRequestSaveName(name) {
  * @param {string} instancesDir - Directory containing instances
  * @returns {Map<integer, Object>}
  *     mapping between instance id and information about this instance.
+ * @private
  */
 async function discoverInstances(instancesDir) {
 	let instanceInfos = new Map();
@@ -123,6 +161,7 @@ async function discoverInstances(instancesDir) {
  * Handles running the slave
  *
  * Connects to the master server over the WebSocket and manages intsances.
+ * @alias module:slave/src/Slave
  */
 class Slave extends libLink.Link {
 	// I don't like God classes, but the alternative of putting all this state
@@ -203,15 +242,28 @@ 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");
-		return path.join(instancesDir, await libFileOps.findUnusedName(instancesDir, name));
+		for (let i = 0; i < 10; i++) { // Limit attempts in case this is somehow an infinite loop
+			let candidateDir = path.join(instancesDir, await libFileOps.findUnusedName(instancesDir, name));
+			try {
+				await fs.mkdir(candidateDir);
+			} catch (err) {
+				if (err.code === "EEXIST") {
+					continue;
+				}
+				throw err;
+			}
+			return candidateDir;
+		}
+		throw Error("Unable to create instance dir, retry threshold reached");
 	}
 
 	async forwardRequestToInstance(message, request) {
@@ -343,8 +395,7 @@ class Slave extends libLink.Link {
 				let instanceConfig = new libConfig.InstanceConfig("slave");
 				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 +838,7 @@ module.exports = Slave;
 
 // For testing only
 module.exports._checkFilename = checkFilename;
+module.exports._cleanFilename = cleanFilename;
 module.exports._discoverInstances = discoverInstances;
 module.exports._Slave = Slave;
 

packages/web_ui/src/components/SiteLayout.jsx

@@ -30,14 +30,14 @@ export default function SiteLayout(props) {
 		onClick={({ key }) => {
 			if (key === "user") {
 				history.push(`/users/${account.name}/view`);
-			} else if (key === "logout") {
-				account.logout();
+			} else if (key === "logOut") {
+				account.logOut();
 			}
 		}}
 		items={[
 			{ label: account.name, key: "user" },
 			{ type: "divider" },
-			{ label: "Log out", danger: true, key: "logout" },
+			{ label: "Log out", danger: true, key: "logOut" },
 		]}
 	/>;
 

packages/web_ui/src/model/account.jsx

@@ -9,7 +9,7 @@ import ControlContext from "../components/ControlContext";
  * @property {?Array<object>} roles - Roles of the corrently logged in account.
  * @property {function(string)} hasPermission -
  *     Check if the currently logged in account has the given permission.
- * @property {function()} logout - Logs out of the web interface.
+ * @property {function()} logOut - Logs out of the web interface.
  */
 
 export function useAccount() {

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");