From 393652c1d958225ec6263e48c4942480e7f5bd49 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Thu, 23 Jun 2022 00:08:23 +0200 Subject: [PATCH 1/4] 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"); From 826d89c7dc86e4d6f9fa6c4da38f4e8c8a96afe5 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Thu, 23 Jun 2022 00:09:28 +0200 Subject: [PATCH 2/4] Fix race condition on creating instance directory --- packages/slave/src/Slave.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/slave/src/Slave.js b/packages/slave/src/Slave.js index 174c4783..c894b745 100644 --- a/packages/slave/src/Slave.js +++ b/packages/slave/src/Slave.js @@ -248,7 +248,19 @@ class Slave extends libLink.Link { } 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) { @@ -380,7 +392,6 @@ 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._createNewInstanceDir(instanceConfig.get("instance.name")); await Instance.create(instanceDir, this.config.get("slave.factorio_directory")); From 51c025c79b7a19c34aeaa6089d78445ffc29d889 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Thu, 23 Jun 2022 00:15:59 +0200 Subject: [PATCH 3/4] Fix Log out button not logging out --- packages/web_ui/src/components/SiteLayout.jsx | 6 +++--- packages/web_ui/src/model/account.jsx | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/web_ui/src/components/SiteLayout.jsx b/packages/web_ui/src/components/SiteLayout.jsx index f6c192fa..61606449 100644 --- a/packages/web_ui/src/components/SiteLayout.jsx +++ b/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" }, ]} />; diff --git a/packages/web_ui/src/model/account.jsx b/packages/web_ui/src/model/account.jsx index 165a50d1..c991aa6e 100644 --- a/packages/web_ui/src/model/account.jsx +++ b/packages/web_ui/src/model/account.jsx @@ -9,7 +9,7 @@ import ControlContext from "../components/ControlContext"; * @property {?Array} 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() { From ef74436cde355b63d6b6ceff643f2de150f8f18d Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Thu, 23 Jun 2022 16:56:23 +0200 Subject: [PATCH 4/4] Fix jsdoc for #462 Fix references and missing files from the jsdoc output after the refactor in #462. --- docs/jsdoc.json | 2 ++ packages/lib/plugin.js | 4 ++-- packages/slave/src/Instance.js | 1 + packages/slave/src/Slave.js | 3 +++ 4 files changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/jsdoc.json b/docs/jsdoc.json index 4a90978a..d2b4df6a 100644 --- a/docs/jsdoc.json +++ b/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/" diff --git a/packages/lib/plugin.js b/packages/lib/plugin.js index 72c6dbc1..b9810254 100644 --- a/packages/lib/plugin.js +++ b/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; diff --git a/packages/slave/src/Instance.js b/packages/slave/src/Instance.js index a7169ec2..99d7aebd 100644 --- a/packages/slave/src/Instance.js +++ b/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")); diff --git a/packages/slave/src/Slave.js b/packages/slave/src/Slave.js index c894b745..4c28b605 100644 --- a/packages/slave/src/Slave.js +++ b/packages/slave/src/Slave.js @@ -70,6 +70,7 @@ function checkFilename(name) { * * @param {string} name - Arbitrary name string * @returns {string} Filename suitable to use in the filesystem + * @private */ function cleanFilename(name) { // copied from checkFilename @@ -119,6 +120,7 @@ function checkRequestSaveName(name) { * @param {string} instancesDir - Directory containing instances * @returns {Map} * mapping between instance id and information about this instance. + * @private */ async function discoverInstances(instancesDir) { let instanceInfos = new Map(); @@ -159,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