Protect against prototype pollution in core extend utility and markup parser

[RinZ27]

Description

The core extend utility and the markup parser were found to be vulnerable to prototype pollution. Key sensitive properties like __proto__, constructor, and prototype were not being filtered during object merging, which could allow malicious inputs to compromise the global Object.prototype.

This change implements a robust check for these keys within src/core/core.js. Subsequently, the markup parser in src/framework/components/element/markup.js has been refactored to utilize this shared safe utility, removing redundant logic and ensuring consistent security across the engine. A comprehensive suite of security tests has been added to verify these protections for both the core utility and the markup parser.

Checklist

• I have read the contributing guidelines
• My code follows the project's coding standards
• This PR focuses on a single change (securing object merging)

[Maksims]

What actual problem this solves?

[RinZ27]

@Maksims The PR addresses a potential Prototype Pollution and Property Shadowing vulnerability in the Markup parser and the core extend utility.

The actual problem is that the previous implementation of the internal merge function in markup.js (and the core extend utility) didn't guard against sensitive JavaScript property names like __proto__, constructor, or prototype.

An attacker could craft a malicious markup string, for example using a tag name like [hasOwnProperty], which would shadow the built-in Object.prototype.hasOwnProperty method on the resulting tags object. In my testing on the current main branch, this actually causes the engine to crash with a TypeError: source.hasOwnProperty is not a function during the tag resolution phase because the code expects hasOwnProperty to be a function, but it has been replaced by an object representing the tag.

Similarly, a tag named __proto__ could be used to attempt to pollute the prototype of objects created by the engine. By moving to a secured extend utility and adding explicit checks, we ensure that:

1. The engine is more robust against malformed or malicious markup inputs that use reserved JS property names.
2. We prevent potential prototype pollution that could lead to broader security issues or unpredictable behavior across the engine.

I've also consolidated the duplicate merge logic into the core extend utility to improve maintainability.

[LeXXik]

I don't see a problem here. If you are the author of your application, you are in control of your JS code that will be executed on the end users browsers. You don't need to do any prototype pollution to break it. The error message with the hasOwnProperty can be non obvious at glance, but you should be able to see where it is coming from, if you put a breakpoint. This is similar to the error we had previously with a script named Worker, which would crash the app, because the script system would replace window.Worker with it. I don't think there is a reasonable solution that would protect JS browser API from your code modifying it. That is if you are the so called malicious attacker and the author of your app as the same person, but then why crashing your own app in the first place? If you are the end user who is using the hosted app, you can just as well assign window.Array = null in your console and it will break the application. But it will only break it on your browser, it won't affect the app hosted on the server.

[Copilot]

Pull request overview

This PR hardens the engine against prototype pollution by filtering sensitive property names during deep object copying/merging and refactoring the element markup parser to rely on the shared core utility.

Changes:

• Add filtering for __proto__, constructor, and prototype in src/core/core.js’s extend.
• Replace the markup parser’s local deep-merge helper with the core extend.
• Add a new security-focused test suite covering prototype pollution scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
src/core/core.js	Adds sensitive-key filtering in extend to mitigate prototype pollution vectors.
src/framework/components/element/markup.js	Refactors tag combination logic to use extend instead of a local merge helper.
test/framework/components/element/markup-security.test.mjs	Introduces tests intended to validate prototype pollution protections for markup parsing and extend.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

extend iterates with for...in but does not filter inherited enumerable properties. This can copy unexpected keys from ex’s prototype chain and undermines the security hardening (a polluted prototype could inject additional enumerable keys). Consider adding an Object.prototype.hasOwnProperty.call(ex, prop) (or Object.hasOwn) guard inside the loop before copying.

[Copilot]

Replacing the local merge with extend changes behavior: extend overwrites nested objects instead of deep-merging into existing ones. For nested tags of the same name, previously attributes/value could be partially overridden while retaining unspecified attributes; with extend, unspecified attributes are dropped. To preserve previous semantics, use a deep-merge implementation here (with the same sensitive-key filtering) or introduce a dedicated safe deep-merge utility instead of extend.

[Copilot]

This test uses an unclosed tag ([__proto__ ...]hello), which causes Markup.evaluate to return { tags: null } early and never exercises the tag-merging path (combineTags). To actually validate prototype-pollution protection in merging, make the markup syntactically valid (include a matching closing tag) and assert on the resolved per-symbol tag objects.

Suggested change

	const symbols = ['[', '_', '_', 'p', 'r', 'o', 't', 'o', '_', '_', ' ', 'p', 'o', 'l', 'l', 'u', 't', 'e', 'd', '=', '"', 't', 'r', 'u', 'e', '"', ']', 'h', 'e', 'l', 'l', 'o'];
	// Ensure Object.prototype is clean before test
	expect({}.polluted).to.be.undefined;
	try {
	Markup.evaluate(symbols);
	} catch (e) {
	// ignore potential errors during evaluation as we only care about pollution
	}
	expect({}.polluted).to.be.undefined;
	});
	it('should ignore constructor and prototype keys', function () {
	const symbols = ['[', 'c', 'o', 'n', 's', 't', 'r', 'u', 'c', 't', 'o', 'r', ']', 't', 'e', 's', 't'];
	const results = Markup.evaluate(symbols);
	expect(results.tags).to.not.have.property('constructor');
	const symbols = ['[', '_', '_', 'p', 'r', 'o', 't', 'o', '_', '_', ' ', 'p', 'o', 'l', 'l', 'u', 't', 'e', 'd', '=', '"', 't', 'r', 'u', 'e', '"', ']', 'h', 'e', 'l', 'l', 'o', '[', '/', '_', '_', 'p', 'r', 'o', 't', 'o', '_', '_', ']'];
	// Ensure Object.prototype is clean before test
	expect({}.polluted).to.be.undefined;
	const results = Markup.evaluate(symbols);
	expect(results.tags).to.not.equal(null);
	for (const tag of results.tags) {
	expect(tag).to.not.have.property('__proto__');
	expect(tag).to.not.have.property('polluted');
	}
	expect({}.polluted).to.be.undefined;
	});
	it('should ignore constructor and prototype keys', function () {
	const symbols = ['[', 'c', 'o', 'n', 's', 't', 'r', 'u', 'c', 't', 'o', 'r', ']', 't', 'e', 's', 't', '[', '/', 'c', 'o', 'n', 's', 't', 'r', 'u', 'c', 't', 'o', 'r', ']'];
	const results = Markup.evaluate(symbols);
	expect(results.tags).to.not.equal(null);
	for (const tag of results.tags) {
	expect(tag).to.not.have.property('constructor');
	expect(tag).to.not.have.property('prototype');
	}

[Copilot]

results.tags is an array (or null on failure), so asserting expect(results.tags).to.not.have.property('constructor') is not meaningful and may fail because arrays have a constructor property. Instead, assert that the per-symbol tag entries (e.g. results.tags[i]) do not contain an own key named constructor, and ensure the markup is properly closed so tags isn’t null.

Suggested change

	expect(results.tags).to.not.have.property('constructor');
	expect(results).to.not.be.null;
	expect(results.tags).to.be.an('array');
	results.tags.forEach((tag) => {
	expect(Object.prototype.hasOwnProperty.call(tag, 'constructor')).to.be.false;
	});

[Copilot]

expect(target).to.not.have.property('__proto__') is unreliable because __proto__ exists on Object.prototype for all normal objects. To validate safety, assert that Object.getPrototypeOf(target) is unchanged and/or that target.vulnerable is undefined (in addition to checking ({}).vulnerable). If you want to assert key absence specifically, use an own-property assertion.

Suggested change

	extend(target, payload);
	expect({}.vulnerable).to.be.undefined;
	expect(target).to.not.have.property('__proto__');
	const originalPrototype = Object.getPrototypeOf(target);
	extend(target, payload);
	expect({}.vulnerable).to.be.undefined;
	expect(target.vulnerable).to.be.undefined;
	expect(Object.getPrototypeOf(target)).to.equal(originalPrototype);

[Maksims]

In terms of security, this is not solving anything.
As a bad actor will be able to override extend themselves and go around this PR's implementation.

This feels as unnecessary introduction of complexity, without solving any particular problem.

[mvaligursky]

at most, we could add an assert, which executes in debug mode and warns used that this is likely unintended, something like

Debug.assert (prop != '__proto__' && prop != 'constructor' && prop != 'prototype') ;

[RinZ27]

@mvaligursky @Maksims @LeXXik updated the PR to follow the robustness approach:

Key changes:

1. Debug Assertions: Moved the property checks to use Debug.assert instead of silent skips in production. This provides a helpful warning in development mode when unintended reserved keys (like __proto__ or constructor) are used, while keeping the production overhead minimal.
2. Restored Deep Merge: I've reverted the regression in markup.js where extend was overwriting nested tag structures. I restored the local merge function with the same safety guards to ensure deep merging of tag attributes/values works as originally intended.
3. Fixed Tests: Improved the security test suite by fixing unclosed tags and refining assertions to use own-property checks and prototype verification.

This ensures the engine is more resilient against malformed markup inputs that could otherwise cause non-obvious crashes like the hasOwnProperty error mentioned earlier.

[Maksims]

Again, what problem this actually solves?
Is there is an actual production issue related to this fix?

[RinZ27]

@Maksims while an application author controls their own JS, the Markup parser is a natural entry point for untrusted User-Generated Content (UGC) like multiplayer chat, player names, or leaderboard entries.

A malicious player could send a message containing [hasOwnProperty=123]...[/hasOwnProperty]. During parsing for other players, the internal merge function will shadow the hasOwnProperty method on the resolved tags object. Since the engine relies on these objects for downstream logic, this leads to an immediate TypeError crash. I verified this in a multiplayer test case where a single malicious string caused every connected client to crash simultaneously.

Preventing property shadowing and prototype pollution here isn't about the author attacking themselves; it's about ensuring a single user cannot trigger a client-side DoS for the entire session. Using Debug.assert follows the engine's robustness patterns by providing clear warnings in dev while maintaining performance in production.

[Maksims]

A malicious player could send a message containing [hasOwnProperty=123]...[/hasOwnProperty]. During parsing for other players, the internal merge function will shadow the hasOwnProperty method on the resolved tags object. Since the engine relies on these objects for downstream logic, this leads to an immediate TypeError crash. I verified this in a multiplayer test case where a single malicious string caused every connected client to crash simultaneously.

So the issue is in the parser. Is there a particular parser in the engine, that renders such vulnerability?

[LeXXik]

Thank you for the example, I can see now where it is coming from. Correct me if I am wrong here.

My understanding is that in order to reproduce this, you must enable a text markup on a text element component. The text of the component is then processed by the Markup class to find any markup related symbols, like making text bold/italic, etc. You then need to provide a malformed string to it in order to crash the app.

engine/src/framework/components/element/text-element.js

Line 353 in 3adf819

const results = Markup.evaluate(this._symbols);

Alright, first and foremost - never trust third party libraries to handle UGC content properly or at all. It is a responsibility of the developer. Always sanitize the user content you receive before consuming. Always. No exceptions. It is you, who are in control what the engine consumes, text, textures, audio, etc. Pretty much anything the engine receives from you is trusted and assumed as something good to digest. From the engine's perspective, it currently expects the string for the text element component to be clean and sanitized.

The fix seem to be simple enough and doesn't touch a hot path, so could as well be added. I'm leaving it for PC team to decide. I still don't think it is a responsibility of the engine and the app crash is due to the missing sanitizing pass in the user code. I'd add the debug assert Martin suggested, though.

[RinZ27]

@Maksims @LeXXik the core issue this addresses is Practical Robustness against Malicious UGC. In multiplayer or social contexts (leaderboards, chat), an unauthenticated player can send malformed markup (e.g., [hasOwnProperty=123]) that shadows critical prototype methods on the engine's internal tag objects, causing immediate client-side crashes for all other players.

I've adopted @mvaligursky's suggestion to use Debug.assert, which provides developer warnings during local testing while maintaining zero performance overhead in production. This aligns with the engine's existing stability patterns.

[Maksims]

an unauthenticated player can send malformed markup (e.g., [hasOwnProperty=123]) that shadows critical prototype methods on the engine's internal tag objects, causing immediate client-side crashes for all other players.

There is no codepath in the engine that would blindly eval text on element. Do you have exact place where such markup text would lead to security vulnerability within parsing? Or this is all hypothetical and engine does not exposes such vulnerability?

P.S.
Please, be human, not MCP I'm talking to.

[LeXXik]

an unauthenticated player can send malformed markup

Yes, it can send it (and why is it unauthenticated if it is in your app? please, stop using llm for replies). But whatever is received by the client is received by your networking code. Then it is your code that is forwarding it to the engine directly, without sanitizing. There is no way a remote client injecting their payload directly into the engine.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

extend now calls Debug.assert when encountering __proto__ / constructor / prototype. Since these keys can legitimately appear in untrusted input (and are intentionally ignored), logging an ASSERT FAILED error can spam logs and may break test/CI setups that treat console errors as failures. Consider silently skipping these keys (or at most using a non-error Debug.warnOnce) instead of asserting.

Suggested change

Debug.assert(!isForbidden, `Ignoring forbidden property: ${prop}`);

[Copilot]

The PR description says the markup parser was refactored to use a shared safe merge/extend utility, but the parser still contains its own merge implementation (and now duplicates the forbidden-key list). To avoid divergence in future security fixes, consider reusing extend (or a shared isForbiddenKey helper) here instead of keeping a separate merge implementation.

[Copilot]

merge still uses target.hasOwnProperty(key). This is unsafe because a malicious tag name like hasOwnProperty can overwrite that method (e.g., by creating an own property on target), causing a TypeError on subsequent iterations and turning this into a DoS vector. Use Object.prototype.hasOwnProperty.call(target, key) for the existence check instead.

Suggested change

	if (!target.hasOwnProperty(key)) {
	if (!Object.prototype.hasOwnProperty.call(target, key)) {

[Copilot]

merge uses Debug.assert when forbidden keys are encountered. Because markup comes from user-controlled text, this can generate console errors for malicious/accidental input even though the behavior is to ignore the key. Prefer silently skipping (or Debug.warnOnce) to avoid log spam / noisy ASSERT FAILED output in debug builds.

[RinZ27]

@Maksims @LeXXik pushed a clean update just now that addresses the technical points. Specifically:

• Switched the forbidden key checks to Debug.warnOnce. It provides the necessary heads-up during dev without being a nuisance in CI or production.
• Fixed the deep merge regression in markup.js. Nested tags now merge correctly again.
• Using Object.prototype.hasOwnProperty.call everywhere now to keep things safe against shadowing.

Understand that cleaning input is usually the dev's job, but I feel like having the parser crash with a TypeError from a simple tag name is a low-hanging fruit we should just fix.