Skip to content

Protect against prototype pollution in core extend utility and markup parser #8578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RinZ27 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Protect against prototype pollution in core extend utility and markup parser #8578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions src/core/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,23 @@ const version = '$_CURRENT_SDK_VERSION';
*/
const revision = '$_CURRENT_SDK_REVISION';

import { Debug } from './debug.js';

/**
* Merge the contents of two objects into a single object.
*
* @param {object} target - The target object of the merge.
* @param {object} ex - The object that is merged with target.
* @param {object} ex - The object to be merged into the target.
* @returns {object} The target object.
* @example
* const A = {
* var A = {
* a: function () {
* console.log(this.a);
* console.log('a');
* }
* };
* const B = {
* var B = {
* b: function () {
* console.log(this.b);
* console.log('b');
* }
* };
*
Expand All @@ -36,12 +38,28 @@ const revision = '$_CURRENT_SDK_REVISION';
*/
function extend(target, ex) {
for (const prop in ex) {
if (!Object.prototype.hasOwnProperty.call(ex, prop)) {
continue;
}

const isForbidden = prop === '__proto__' || prop === 'constructor' || prop === 'prototype';
if (isForbidden) {
Debug.warnOnce(`Ignoring forbidden property: ${prop}`);
continue;
}

const copy = ex[prop];

if (Array.isArray(copy)) {
Comment on lines 39 to 53
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
target[prop] = extend([], copy);
if (!Array.isArray(target[prop])) {
target[prop] = [];
}
extend(target[prop], copy);
} else if (copy && typeof copy === 'object') {
target[prop] = extend({}, copy);
if (!target[prop] || typeof target[prop] !== 'object') {
target[prop] = {};
}
extend(target[prop], copy);
} else {
target[prop] = copy;
}
Expand All @@ -50,4 +68,5 @@ function extend(target, ex) {
return target;
}


export { extend, revision, version };
17 changes: 13 additions & 4 deletions src/framework/components/element/markup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { Debug } from '../../../core/debug.js';

// markup scanner

// list of scanner tokens
Expand Down Expand Up @@ -334,12 +336,19 @@ class Parser {
// of assign)
function merge(target, source) {
for (const key in source) {
if (!source.hasOwnProperty(key)) {
if (!Object.prototype.hasOwnProperty.call(source, key)) {
continue;
}
Comment on lines 337 to 341
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

const isForbidden = key === '__proto__' || key === 'constructor' || key === 'prototype';
Debug.assert(!isForbidden, `Ignoring forbidden property: ${key}`);
if (isForbidden) {
continue;
Comment on lines +343 to +346
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

const value = source[key];
if (value instanceof Object) {
if (!target.hasOwnProperty(key)) {
if (!Object.prototype.hasOwnProperty.call(target, key)) {
target[key] = { };
}
merge(target[key], source[key]);
Expand Down Expand Up @@ -380,7 +389,7 @@ function resolveMarkupTags(tags, numSymbols) {
const edges = { };
for (let index = 0; index < tags.length; ++index) {
const tag = tags[index];
if (!edges.hasOwnProperty(tag.start)) {
if (!Object.prototype.hasOwnProperty.call(edges, tag.start)) {
edges[tag.start] = { open: [tag], close: null };
} else {
if (edges[tag.start].open === null) {
Expand All @@ -390,7 +399,7 @@ function resolveMarkupTags(tags, numSymbols) {
}
}

if (!edges.hasOwnProperty(tag.end)) {
if (!Object.prototype.hasOwnProperty.call(edges, tag.end)) {
edges[tag.end] = { open: null, close: [tag] };
} else {
if (edges[tag.end].close === null) {
Expand Down