Skip to content
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.

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

Guard against prototype pollution #630

Merged
merged 1 commit into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"document": false
},
"root": true,
"ignorePatterns": ["dist/"],
"rules": {
"comma-style": [
2,
Expand Down
2 changes: 2 additions & 0 deletions src/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { Page, type PageBase } from './Page';
import { PageParams, routes } from './routes';
import * as derbyTemplates from './templates';
import { type Views } from './templates/templates';
import { checkKeyIsSafe } from './templates/util';

const { templates } = derbyTemplates;

Expand Down Expand Up @@ -122,6 +123,7 @@ export abstract class AppBase<T = object> extends EventEmitter {

// TODO: DRY. This is copy-pasted from ./templates
const mapName = viewName.replace(/:index$/, '');
checkKeyIsSafe(mapName);
const currentView = this.views.nameMap[mapName];
const currentConstructor = (currentView && currentView.componentFactory) ?
currentView.componentFactory.constructorFn :
Expand Down
4 changes: 2 additions & 2 deletions src/PageForServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,11 @@ function stringifyBundle(bundle) {

// TODO: Cleanup; copied from tracks
function pageParams(req) {
const params = {
const params = Object.create(null, {
url: req.url,
body: req.body,
query: req.query,
};
});
for (const key in req.params) {
params[key] = req.params[key];
}
Expand Down
4 changes: 4 additions & 0 deletions src/components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import derbyTemplates = require('./templates');
import { Context } from './templates/contexts';
import { Expression } from './templates/expressions';
import { Attribute, Binding } from './templates/templates';
import { checkKeyIsSafe } from './templates/util';
const { expressions, templates } = derbyTemplates;
const slice = [].slice;

Expand Down Expand Up @@ -332,10 +333,12 @@ export abstract class Component<T = object> extends Controller<T> {
}

setAttribute(key: string, value: Attribute) {
checkKeyIsSafe(key);
this.context.parent.attributes[key] = value;
}

setNullAttribute(key: string, value: Attribute) {
checkKeyIsSafe(key);
const attributes = this.context.parent.attributes;
if (attributes[key] == null) attributes[key] = value;
}
Expand All @@ -354,6 +357,7 @@ export class ComponentAttribute<T = object> {
key: string;

constructor(expression: Expression, model: ChildModel<T>, key: string) {
checkKeyIsSafe(key);
this.expression = expression;
this.model = model;
this.key = key;
Expand Down
2 changes: 2 additions & 0 deletions src/eventmodel.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var expressions = require('./templates').expressions;
var checkKeyIsSafe = require('./templates/util').checkKeyIsSafe;

// The many trees of bindings:
//
Expand Down Expand Up @@ -183,6 +184,7 @@ EventModel.prototype.child = function(segment) {
segment = segment.item;
}

checkKeyIsSafe(segment);
return container[segment] || (container[segment] = new EventModel());
};

Expand Down
2 changes: 2 additions & 0 deletions src/parsing/createPathExpression.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as esprima from 'esprima-derby';
import type * as estree from 'estree';

import { expressions, operatorFns } from '../templates';
import { checkKeyIsSafe } from '../templates/util';
const { Syntax } = esprima;

export function createPathExpression(source: string) {
Expand Down Expand Up @@ -221,6 +222,7 @@ function reduceObjectExpression(node: estree.ObjectExpression) {
if (isProperty(property)) {
const key = getKeyName(property.key);
const expression = reduce(property.value);
checkKeyIsSafe(key);
properties[key] = expression;
if (isLiteral && expression instanceof expressions.LiteralExpression) {
literal[key] = expression.value;
Expand Down
7 changes: 7 additions & 0 deletions src/parsing/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { App, AppBase } from '../App';
import { templates, expressions } from '../templates';
import { Expression } from '../templates/expressions';
import { MarkupHook, View } from '../templates/templates';
import { checkKeyIsSafe } from '../templates/util';

export { createPathExpression } from './createPathExpression';
export { markup } from './markup';
Expand Down Expand Up @@ -122,6 +123,7 @@ function parseHtmlStart(tag: string, tagName: string, attributes: Record<string,
function parseAttributes(attributes: Record<string, string>) {
let attributesMap;
for (const key in attributes) {
checkKeyIsSafe(key);
if (!attributesMap) attributesMap = {};

const value = attributes[key];
Expand Down Expand Up @@ -384,6 +386,7 @@ function attributeValueFromContent(content, isWithin) {
function viewAttributesFromElement(element) {
const viewAttributes = {};
for (const key in element.attributes) {
checkKeyIsSafe(key);
const attribute = element.attributes[key];
const camelCased = dashToCamelCase(key);
viewAttributes[camelCased] =
Expand Down Expand Up @@ -555,6 +558,7 @@ function parseNameAttribute(element) {

function parseAttributeElement(element, name, viewAttributes) {
const camelName = dashToCamelCase(name);
checkKeyIsSafe(camelName);
const isWithin = element.attributes && element.attributes.within;
viewAttributes[camelName] = attributeValueFromContent(element.content, isWithin);
}
Expand All @@ -564,6 +568,7 @@ function createAttributesExpression(attributes) {
const literalAttributes = {};
let isLiteral = true;
for (const key in attributes) {
checkKeyIsSafe(key);
const attribute = attributes[key];
if (attribute instanceof expressions.Expression) {
dynamicAttributes[key] = attribute;
Expand All @@ -588,6 +593,7 @@ function parseArrayElement(element, name, viewAttributes) {
delete attributes.within;
const expression = createAttributesExpression(attributes);
const camelName = dashToCamelCase(name);
checkKeyIsSafe(camelName);
const viewAttribute = viewAttributes[camelName];

// If viewAttribute is already an ArrayExpression, push the expression for
Expand Down Expand Up @@ -662,6 +668,7 @@ function viewAttributesFromExpression(expression) {

const viewAttributes = {};
for (const key in object) {
checkKeyIsSafe(key);
const value = object[key];
viewAttributes[key] =
(value instanceof expressions.LiteralExpression) ? value.value :
Expand Down
4 changes: 3 additions & 1 deletion src/templates/expressions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { type Context } from './contexts';
import { DependencyOptions } from './dependencyOptions';
import * as operatorFns from './operatorFns';
import { ContextClosure, Dependency, Template } from './templates';
import { concat } from './util';
import { checkKeyIsSafe, concat } from './util';
import { Component } from '../components';

type SegmentOrContext = string | number | { item: number } | Context;
Expand Down Expand Up @@ -88,6 +88,7 @@ function renderArrayProperties(array: Renderable[], context: Context) {
function renderObjectProperties(object: Record<string, Renderable>, context: Context): Record<string, any> {
const out = {};
for (const key in object) {
checkKeyIsSafe(key);
out[key] = renderValue(object[key], context);
}
return out;
Expand Down Expand Up @@ -547,6 +548,7 @@ export class ObjectExpression extends Expression {
get(context: Context) {
const object = {};
for (const key in this.properties) {
checkKeyIsSafe(key);
const value = this.properties[key].get(context);
object[key] = value;
}
Expand Down
12 changes: 11 additions & 1 deletion src/templates/templates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ if (typeof require === 'function') {
import { type Context } from './contexts';
import { DependencyOptions } from './dependencyOptions';
import { type Expression } from './expressions';
import { concat, hasKeys, traverseAndCreate } from './util';
import { checkKeyIsSafe, concat, hasKeys, traverseAndCreate } from './util';
import { Component } from '../components';
import { Controller } from '../Controller';

Expand Down Expand Up @@ -1369,6 +1369,7 @@ function emitRemoved(context: Context, node: Node, ignore: Binding) {
}

function emitRemovedBinding(context: Context, ignore: Binding, node: Node, property: string) {
checkKeyIsSafe(property);
const binding = node[property];
if (binding) {
node[property] = null;
Expand Down Expand Up @@ -1444,6 +1445,7 @@ export class AttributeBinding extends Binding {
name: string;

constructor(template: DynamicAttribute, context: Context, element: globalThis.Element, name: string) {
checkKeyIsSafe(name);
super();
this.template = template;
this.context = context;
Expand Down Expand Up @@ -1728,6 +1730,7 @@ export class Marker extends Comment {
function ViewAttributesMap(source: string) {
const items = source.split(/\s+/);
for (let i = 0, len = items.length; i < len; i++) {
checkKeyIsSafe(items[i]);
this[items[i]] = true;
}
}
Expand All @@ -1736,6 +1739,7 @@ function ViewArraysMap(source: string) {
const items = source.split(/\s+/);
for (let i = 0, len = items.length; i < len; i++) {
const item = items[i].split('/');
checkKeyIsSafe(item[0]);
this[item[0]] = item[1] || item[0];
}
}
Expand Down Expand Up @@ -2159,6 +2163,7 @@ export class Views {

register(name: string, source: string, options?: ViewOptions) {
const mapName = name.replace(/:index$/, '');
checkKeyIsSafe(mapName);
let view = this.nameMap[mapName];
if (view) {
// Recreate the view if it already exists. We re-apply the constructor
Expand All @@ -2173,6 +2178,7 @@ export class Views {
this.nameMap[mapName] = view;
// TODO: element is deprecated and should be removed with Derby 0.6.0
const tagName = options && (options.tag || options.element);
checkKeyIsSafe(tagName);
if (tagName) this.tagMap[tagName] = view;
return view;
}
Expand Down Expand Up @@ -2323,6 +2329,7 @@ abstract class AsPropertyBase<T> extends MarkupHook<T> {

emit(context: Context, target: T) {
const node = traverseAndCreate(context.controller, this.segments);
checkKeyIsSafe(this.lastSegment);
node[this.lastSegment] = target;
this.addListeners(target, node, this.lastSegment);
}
Expand Down Expand Up @@ -2376,8 +2383,10 @@ export class AsObject extends AsProperty {

emit(context: Context, target: any) {
const node = traverseAndCreate(context.controller, this.segments);
checkKeyIsSafe(this.lastSegment);
const object = node[this.lastSegment] || (node[this.lastSegment] = {});
const key = this.keyExpression.get(context);
checkKeyIsSafe(key);
object[key] = target;
this.addListeners(target, object, key);
}
Expand All @@ -2398,6 +2407,7 @@ abstract class AsArrayBase<T> extends AsPropertyBase<T> {

emit(context: Context, target: any) {
const node = traverseAndCreate(context.controller, this.segments);
checkKeyIsSafe(this.lastSegment);
const array = node[this.lastSegment] || (node[this.lastSegment] = []);

// Iterate backwards, since rendering will usually append
Expand Down
14 changes: 14 additions & 0 deletions src/templates/util.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
const objectProtoPropNames = Object.create(null);
Object.getOwnPropertyNames(Object.prototype).forEach(function(prop) {
if (prop !== '__proto__') {
objectProtoPropNames[prop] = true;
}
});

export function checkKeyIsSafe(key) {
if (key === '__proto__' || objectProtoPropNames[key]) {
throw new Error(`Unsafe key "${key}"`);
}
}

export function concat(a, b) {
if (!a) return b;
if (!b) return a;
Expand All @@ -17,6 +30,7 @@ export function traverseAndCreate(node, segments) {
if (!len) return node;
for (let i = 0; i < len; i++) {
const segment = segments[i];
checkKeyIsSafe(segment);
node = node[segment] || (node[segment] = {});
}
return node;
Expand Down
25 changes: 23 additions & 2 deletions test/dom/bindings.mocha.js
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,29 @@ describe('bindings', function() {
expect(page.box.myDiv).to.not.equal(initialElement);
done();
});
})
})
});

['__proto__', 'constructor'].forEach(function(badKey) {
it(`disallows prototype modification with ${badKey}`, function() {
var harness = runner.createHarness(`
<view is="box"/>
`);
function Box() {}
Box.view = {
is: 'box',
source:`
<index:>
<div as="${badKey}">one</div>
`
};
var app = harness.app;
app.component(Box);
expect(() => harness.renderDom()).to.throw(`Unsafe key "${badKey}"`);
// Rendering to HTML string should still work, as that doesn't process `as` attributes
expect(harness.renderHtml().html).to.equal('<div>one</div>');
});
});
});

function testArray(itemTemplate, itemData) {
it('each on path', function() {
Expand Down
Loading