From 6977699520ff2eb89f986ca32cd4a94237f65195 Mon Sep 17 00:00:00 2001 From: Martin Valigursky Date: Wed, 3 Jun 2026 16:06:14 +0100 Subject: [PATCH] Use a Map for the script registry and guard reserved names in add() Store registered scripts in a Map instead of a plain object so that script names colliding with `Object.prototype` members (e.g. `hasOwnProperty`, `toString`, `__proto__`) are stored and looked up safely, rather than shadowing methods on the backing object (which caused a TypeError) or, in the case of `__proto__`, silently failing to store. Also enforce the reserved-name check in `ScriptRegistry.add()` so the public `app.scripts.add()` path is validated (returns false), not only the `registerScript`/`createScript` paths (which still throw, unchanged). The reserved-name set is moved to `constants.js` to avoid an import cycle. Adds tests for collision-prone names and reserved-name rejection via add(). Co-Authored-By: Claude Opus 4.8 --- src/framework/script/constants.js | 19 ++++++++++ src/framework/script/script-create.js | 13 +------ src/framework/script/script-registry.js | 35 +++++++++++++------ .../framework/script/script-registry.test.mjs | 35 +++++++++++++++++++ 4 files changed, 79 insertions(+), 23 deletions(-) diff --git a/src/framework/script/constants.js b/src/framework/script/constants.js index 548b72c6005..37c796ab553 100644 --- a/src/framework/script/constants.js +++ b/src/framework/script/constants.js @@ -3,3 +3,22 @@ export const SCRIPT_POST_INITIALIZE = 'postInitialize'; export const SCRIPT_UPDATE = 'update'; export const SCRIPT_POST_UPDATE = 'postUpdate'; export const SCRIPT_SWAP = 'swap'; + +/** + * Script names that cannot be used, as they would clash with members of {@link ScriptComponent} + * (a script is attached to its component as `component[scriptName]`) or {@link EventHandler}. + * + * @type {Set} + * @ignore + */ +export const reservedScriptNames = new Set([ + 'system', 'entity', 'create', 'destroy', 'swap', 'move', 'data', + 'scripts', '_scripts', '_scriptsIndex', '_scriptsData', + 'enabled', '_oldState', 'onEnable', 'onDisable', 'onPostStateChange', + '_onSetEnabled', '_checkState', '_onBeforeRemove', + '_onInitializeAttributes', '_onInitialize', '_onPostInitialize', + '_onUpdate', '_onPostUpdate', + '_callbacks', '_callbackActive', 'has', 'get', 'on', 'off', 'fire', 'once', 'hasEvent', + // 'worker' is reserved to prevent users from overwriting the native Worker constructor + 'worker' +]); diff --git a/src/framework/script/script-create.js b/src/framework/script/script-create.js index 115cf87e56c..f7701794898 100644 --- a/src/framework/script/script-create.js +++ b/src/framework/script/script-create.js @@ -4,20 +4,9 @@ import { AppBase } from '../app-base.js'; import { ScriptAttributes } from './script-attributes.js'; import { ScriptType } from './script-type.js'; import { ScriptTypes } from './script-types.js'; +import { reservedScriptNames } from './constants.js'; import { Script, getScriptRegistryName } from './script.js'; -const reservedScriptNames = new Set([ - 'system', 'entity', 'create', 'destroy', 'swap', 'move', 'data', - 'scripts', '_scripts', '_scriptsIndex', '_scriptsData', - 'enabled', '_oldState', 'onEnable', 'onDisable', 'onPostStateChange', - '_onSetEnabled', '_checkState', '_onBeforeRemove', - '_onInitializeAttributes', '_onInitialize', '_onPostInitialize', - '_onUpdate', '_onPostUpdate', - '_callbacks', '_callbackActive', 'has', 'get', 'on', 'off', 'fire', 'once', 'hasEvent', - // 'worker' is reserved to prevent users from overwriting the native Worker constructor - 'worker' -]); - function getReservedScriptNames() { return reservedScriptNames; } diff --git a/src/framework/script/script-registry.js b/src/framework/script/script-registry.js index 360d21a5c4a..37af73e6c42 100644 --- a/src/framework/script/script-registry.js +++ b/src/framework/script/script-registry.js @@ -1,5 +1,6 @@ import { Debug } from '../../core/debug.js'; import { EventHandler } from '../../core/event-handler.js'; +import { reservedScriptNames } from './constants.js'; import { getScriptRegistryName } from './script.js'; /** @@ -18,10 +19,14 @@ import { getScriptRegistryName } from './script.js'; */ class ScriptRegistry extends EventHandler { /** - * @type {Object} + * A Map of script names to script classes. A Map is used (rather than a plain object) so that + * script names which collide with `Object.prototype` members - e.g. `hasOwnProperty`, + * `toString`, `__proto__` - are stored and looked up safely. + * + * @type {Map} * @private */ - _scripts = {}; + _scripts = new Map(); /** * @type {typeof ScriptType[]} @@ -116,16 +121,24 @@ class ScriptRegistry extends EventHandler { return false; } + // Reject names that would clash with ScriptComponent/EventHandler members. This is also + // checked (and thrown) by `registerScript`/`createScript`, but is enforced here too so the + // direct `app.scripts.add()` path is guarded. + if (reservedScriptNames.has(scriptName)) { + Debug.error(`script name '${scriptName}' is reserved and cannot be added to the script registry.`); + return false; + } + script.__name = scriptName; - if (this._scripts.hasOwnProperty(scriptName)) { + if (this._scripts.has(scriptName)) { setTimeout(() => { if (script.prototype.swap) { // swapping - const old = this._scripts[scriptName]; + const old = this._scripts.get(scriptName); const ind = this._list.indexOf(old); this._list[ind] = script; - this._scripts[scriptName] = script; + this._scripts.set(scriptName, script); this.fire('swap', scriptName, script); this.fire(`swap:${scriptName}`, script); @@ -136,7 +149,7 @@ class ScriptRegistry extends EventHandler { return false; } - this._scripts[scriptName] = script; + this._scripts.set(scriptName, script); this._list.push(script); this.fire('add', scriptName, script); @@ -145,7 +158,7 @@ class ScriptRegistry extends EventHandler { // for all components awaiting Script Type // create script instance setTimeout(() => { - if (!this._scripts.hasOwnProperty(scriptName)) { + if (!this._scripts.has(scriptName)) { return; } @@ -239,7 +252,7 @@ class ScriptRegistry extends EventHandler { return false; } - delete this._scripts[scriptName]; + this._scripts.delete(scriptName); const ind = this._list.indexOf(scriptType); this._list.splice(ind, 1); @@ -259,7 +272,7 @@ class ScriptRegistry extends EventHandler { * var PlayerController = app.scripts.get('playerController'); */ get(name) { - return this._scripts[name] || null; + return this._scripts.get(name) || null; } /** @@ -275,12 +288,12 @@ class ScriptRegistry extends EventHandler { */ has(nameOrType) { if (typeof nameOrType === 'string') { - return this._scripts.hasOwnProperty(nameOrType); + return this._scripts.has(nameOrType); } if (!nameOrType) return false; const scriptName = nameOrType.__name; - return this._scripts[scriptName] === nameOrType; + return this._scripts.get(scriptName) === nameOrType; } /** diff --git a/test/framework/script/script-registry.test.mjs b/test/framework/script/script-registry.test.mjs index cd0a8d84504..18845871743 100644 --- a/test/framework/script/script-registry.test.mjs +++ b/test/framework/script/script-registry.test.mjs @@ -90,6 +90,41 @@ describe('ScriptRegistry', function () { expect(app.scripts.get('viaRegister')).to.equal(ViaRegister); }); + it('safely handles script names that collide with Object.prototype members', function () { + const names = ['hasOwnProperty', 'toString', 'constructor', '__proto__', 'valueOf']; + const classes = names.map((name) => { + class Collider extends Script {} + Collider.scriptName = name; + return Collider; + }); + + classes.forEach((cls, i) => { + expect(app.scripts.add(cls), names[i]).to.equal(true); + }); + + // each is stored and retrievable by its (collision-prone) name + names.forEach((name, i) => { + expect(app.scripts.has(name), name).to.equal(true); + expect(app.scripts.get(name), name).to.equal(classes[i]); + }); + + // and they all appear in the list without clobbering each other + names.forEach((name, i) => { + expect(app.scripts.list().includes(classes[i]), name).to.equal(true); + }); + }); + + it('rejects a reserved script name via add()', function () { + class Reserved extends Script { + static scriptName = 'destroy'; + } + + const added = app.scripts.add(Reserved); + + expect(added).to.equal(false); + expect(app.scripts.has('destroy')).to.equal(false); + }); + }); // a subclass must register under its own name, not inherit (and overwrite) its base's name