Use a Map for the script registry and guard reserved names in add()#8835
Merged
Conversation
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 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the framework script registry against script-name collisions by switching the backing store from a plain object to a Map, and it extends reserved-name enforcement to the now-documented app.scripts.add() registration path.
Changes:
- Replace
ScriptRegistry._scriptswith aMapand migrate all reads/writes to.get()/.set()/.has()/.delete()to safely support keys like__proto__andhasOwnProperty. - Add a reserved-name guard in
ScriptRegistry.add()(log + returnfalse) to match the reserved-name protections already present increateScript/registerScript. - Centralize
reservedScriptNamesinsrc/framework/script/constants.jsand add tests covering both prototype-collision names and reserved-name rejection viaadd().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/framework/script/script-registry.test.mjs | Adds regression tests for Object.prototype-colliding script names and reserved-name rejection via app.scripts.add(). |
| src/framework/script/script-registry.js | Switches registry storage from object to Map and adds reserved-name guard in add(). |
| src/framework/script/script-create.js | Moves reserved-name set to shared constants and reuses it for createScript / registerScript. |
| src/framework/script/constants.js | Introduces shared reservedScriptNames set used across script registration paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mvaligursky
added a commit
that referenced
this pull request
Jun 4, 2026
…8835) 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: Martin Valigursky <mvaligursky@snapchat.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #8831 hardening the script registry against name collisions.
Map-backed registry
ScriptRegistry._scriptsis now aMapinstead of a plain object. With a plain object, a script name that collides with anObject.prototypemember breaks the registry:hasOwnProperty/toStringetc. shadow the method on the backing object — the next internal_scripts.hasOwnProperty(...)call then invokes a class constructor and throws aTypeError.__proto__is worse:_scripts['__proto__'] = clspokes the prototype rather than storing a key, so the script is silently lost.A
Maptreats all of these as ordinary keys. All access sites (add, including the swap path and the deferred awaiting-component check, plusremove/get/has) now use.get/.set/.has/.delete._listis unchanged, so registration order andlist()are unaffected. No public API changes (_scriptsis private;get/has/listsignatures are identical).Reserved-name guard on the
add()pathThe reserved-name check was previously only enforced in
registerScript/createScript. Since #8831 madeapp.scripts.add()a documented registration path, it's now guarded there too (logs and returnsfalse).registerScript/createScriptstill throw on their own paths — unchanged. The reserved-name set is moved toconstants.jsto avoid ascript-create → AppBase → script-registryimport cycle.Tests
hasOwnProperty,toString,constructor,__proto__,valueOf— all stored and retrievable viaadd/has/get/listwithout collision.add()rejects a reserved name (destroy) and does not register it.Full suite passes; types and lint clean.