diff --git a/docs/source/tutorials/options.md b/docs/source/tutorials/options.md index b1c2315b3..ecb039898 100644 --- a/docs/source/tutorials/options.md +++ b/docs/source/tutorials/options.md @@ -138,9 +138,7 @@ It defaults to `false`. For example, when set to `true`, a blank string would ev **lenientIf** modifies the behavior of `strictVariables` to allow handling optional variables. If set to `true`, an undefined variable will *not* cause an exception in the following two situations: a) it is the condition to an `if`, `elsif`, or `unless` tag; b) it occurs right before a `default` filter. Irrelevant if `strictVariables` is not set. Defaults to `false`. -**ownPropertyOnly** hides scope variables from prototypes, useful when you're passing a not sanitized object into LiquidJS or need to hide prototypes from templates. Defaults to `true`. - -Built-in DoS limits and host isolation guidance are documented in [Security Model](./security-model.html). +**ownPropertyOnly** limits template property reads on plain scope objects to own properties. Defaults to `true`. See [Security Model](./security-model.html). {% note info Nonexistent Tags %} Nonexistent tags always throw errors during parsing and this behavior cannot be customized. diff --git a/docs/source/tutorials/security-model.md b/docs/source/tutorials/security-model.md index 3c6b4aa74..454bdf57a 100644 --- a/docs/source/tutorials/security-model.md +++ b/docs/source/tutorials/security-model.md @@ -50,7 +50,11 @@ The `memoryLimit` option was removed in v11; enforce memory limits at the host o ## `ownPropertyOnly` and scope data -With [`ownPropertyOnly`][ownPropertyOnly] `true`, plain scope objects only expose **own** properties (no inherited / `Object.prototype` keys). Default `false` follows normal JS property access. Use `true` for untrusted or polluted objects; add [`strictVariables`][strictVariables] if missing paths should error. Override per render via [`RenderOptions`][renderOwnPropertyOnly]. This is a read policy for scope data—not a sandbox for filters, tags, or your code. +With [`ownPropertyOnly`][ownPropertyOnly] `true` (default), plain scope objects only expose **own** properties (no inherited / `Object.prototype` keys), and reads of `__proto__`, `constructor`, and `prototype` are blocked (own and inherited) as a prototype-pollution defense. With `false`, inherited properties and those keys are allowed—sanitize untrusted scope data (e.g. with [bourne](https://www.npmjs.com/package/bourne)) before passing it as scope. LiquidJS also uses null-prototype objects for managed scope frames (e.g. `{% capture %}`, `{% assign %}`) so internal frames do not inherit from `Object.prototype`. + +Not restricted: [`Drop`][drop] values, iteration via `Symbol.iterator`, `.size`/`.first`/`.last`, filters, and custom tags. + +Use `true` for untrusted objects; add [`strictVariables`][strictVariables] if missing paths should error. Override per render via [`RenderOptions`][renderOwnPropertyOnly]. This is a read policy for scope data—not a sandbox for filters, tags, or your code. ## Custom `Drop` classes diff --git a/src/context/context.spec.ts b/src/context/context.spec.ts index d8eeac978..b26575f82 100644 --- a/src/context/context.spec.ts +++ b/src/context/context.spec.ts @@ -58,6 +58,9 @@ describe('Context', function () { it('should return map size as size', async function () { expect(ctx.get(['map', 'size'])).toEqual(1) }) + it('should return own size property', async function () { + expect(ctx.get(['zoo', 'size'])).toEqual(4) + }) it('should return undefined if not have a size', async function () { expect(ctx.get(['one', 'size'])).toBeUndefined() expect(ctx.get(['non-exist', 'size'])).toBeUndefined() @@ -130,6 +133,10 @@ describe('Context', function () { ctx = new Context({ foo: Object.create({ bar: 'BAR' }) }, { ownPropertyOnly: false } as any) return expect(ctx.getSync(['foo', 'bar'])).toEqual('BAR') }) + it('should read inherited size when ownPropertyOnly=false', function () { + ctx = new Context({ foo: Object.create({ size: 99 }) }, { ownPropertyOnly: false } as any) + return expect(ctx.getSync(['foo', 'size'])).toEqual(99) + }) it('renderOptions.ownPropertyOnly should override options.ownPropertyOnly', function () { ctx = new Context({ foo: Object.create({ bar: 'BAR' }) }, { ownPropertyOnly: false } as any, { ownPropertyOnly: true }) return expect(ctx.getSync(['foo', 'bar'])).toEqual(undefined) @@ -198,6 +205,36 @@ describe('Context', function () { delete (Array.prototype as any)[0] } }) + it('should allow own blocked keys when ownPropertyOnly=false', function () { + ctx = new Context({ + foo: { + ...JSON.parse('{"__proto__": {"bar": "BAR"}}'), + constructor: { name: 'Custom' }, + prototype: { x: 1 } + } + }, { ownPropertyOnly: false } as any) + expect(ctx.getSync(['foo', '__proto__', 'bar'])).toEqual('BAR') + expect(ctx.getSync(['foo', 'constructor', 'name'])).toEqual('Custom') + expect(ctx.getSync(['foo', 'prototype', 'x'])).toEqual(1) + }) + it('should allow inherited properties when ownPropertyOnly=false', function () { + ctx = new Context({ foo: Object.create({ __proto__: { bar: 'BAR' }, constructor: { name: 'Evil' } }) }, { ownPropertyOnly: false } as any) + expect(ctx.getSync(['foo', '__proto__', '__proto__', 'bar'])).toEqual('BAR') + expect(ctx.getSync(['foo', 'constructor', 'name'])).toEqual('Evil') + }) + it('should block own constructor when ownPropertyOnly=true', function () { + ctx.push({ foo: { constructor: { name: 'Evil' } } }) + expect(ctx.getSync(['foo', 'constructor'])).toEqual(undefined) + }) + it('should block own prototype when ownPropertyOnly=true', function () { + ctx.push({ foo: { prototype: { bar: 'BAR' } } }) + expect(ctx.getSync(['foo', 'prototype'])).toEqual(undefined) + }) + it('should block own top-level __proto__ variable when ownPropertyOnly=true', function () { + ctx = new Context(JSON.parse('{"__proto__": {"bar": "BAR"}, "bar": "BAR"}')) + expect(ctx.getSync(['__proto__'])).toEqual(undefined) + expect(ctx.getSync(['bar'])).toEqual('BAR') + }) }) describe('.getAll()', function () { @@ -221,6 +258,11 @@ describe('Context', function () { expect(ctx.getSync(['bar', 'foo'])).toEqual('foo') expect(ctx.getSync(['bar', 'bar'])).toEqual(undefined) }) + it('should return pushed scope for in-place mutation', function () { + const scope = ctx.push({}) + scope.item = 'ITEM' + expect(ctx.getSync(['item'])).toEqual('ITEM') + }) }) describe('.pop()', function () { it('should pop scope', async function () { diff --git a/src/context/context.ts b/src/context/context.ts index fbc17f8d8..94a8301c8 100644 --- a/src/context/context.ts +++ b/src/context/context.ts @@ -6,6 +6,8 @@ import { hasOwnProperty, isArray, isNil, isUndefined, isString, isFunction, isNu type PropertyKey = string | number; +const BLOCKED_SCOPE_KEYS: ReadonlySet = new Set(['__proto__', 'constructor', 'prototype']) + export class Context { /** * insert a Context-level empty scope, @@ -94,8 +96,10 @@ export class Context { } return scope } - public push (ctx: object) { - return this.scopes.push(ctx) + public push (ctx: Scope): Scope { + const scope = createScope(ctx) + this.scopes.push(scope) + return scope } public pop () { return this.scopes.pop() @@ -118,9 +122,9 @@ export class Context { private findScope (key: string | number) { for (let i = this.scopes.length - 1; i >= 0; i--) { const candidate = this.scopes[i] - if (key in candidate) return candidate + if (this.ownPropertyOnly ? hasOwnProperty.call(candidate, key) : key in candidate) return candidate } - if (key in this.environments) return this.environments + if (this.ownPropertyOnly ? hasOwnProperty.call(this.environments, key) : key in this.environments) return this.environments return this.globals } readProperty (obj: Scope, key: (PropertyKey | Drop)) { @@ -131,30 +135,30 @@ export class Context { const value = readJSProperty(obj, key, this.ownPropertyOnly) if (value === undefined && obj instanceof Drop) return obj.liquidMethodMissing(key, this) if (isFunction(value)) return value.call(obj) - if (key === 'size') return readSize(obj) - else if (key === 'first') return readFirst(obj, this.ownPropertyOnly) - else if (key === 'last') return readLast(obj, this.ownPropertyOnly) + if (key === 'size') return this.readSize(obj) + else if (key === 'first') return this.readFirst(obj) + else if (key === 'last') return this.readLast(obj) return value } + private readFirst (obj: Scope) { + if (isArray(obj)) return readArrayElement(obj, 0, this.ownPropertyOnly) + return readJSProperty(obj, 'first', this.ownPropertyOnly) + } + private readLast (obj: Scope) { + if (isArray(obj)) return readArrayElement(obj, -1, this.ownPropertyOnly) + return readJSProperty(obj, 'last', this.ownPropertyOnly) + } + private readSize (obj: Scope) { + if (hasOwnProperty.call(obj, 'size')) return obj['size'] + if (!this.ownPropertyOnly && obj['size'] !== undefined) return obj['size'] + if (isArray(obj) || isString(obj)) return obj.length + if (obj instanceof Map || obj instanceof Set) return obj.size + if (typeof obj === 'object') return Object.keys(obj).length + } } export function readJSProperty (obj: Scope, key: PropertyKey, ownPropertyOnly: boolean) { + if (BLOCKED_SCOPE_KEYS.has(key) && ownPropertyOnly) return undefined if (ownPropertyOnly && !hasOwnProperty.call(obj, key) && !(obj instanceof Drop)) return undefined return obj[key] } - -function readFirst (obj: Scope, ownPropertyOnly: boolean) { - if (isArray(obj)) return readArrayElement(obj, 0, ownPropertyOnly) - return readJSProperty(obj, 'first', ownPropertyOnly) -} - -function readLast (obj: Scope, ownPropertyOnly: boolean) { - if (isArray(obj)) return readArrayElement(obj, -1, ownPropertyOnly) - return readJSProperty(obj, 'last', ownPropertyOnly) -} - -function readSize (obj: Scope) { - if (hasOwnProperty.call(obj, 'size') || obj['size'] !== undefined) return obj['size'] - if (isArray(obj) || isString(obj)) return obj.length - if (typeof obj === 'object') return Object.keys(obj).length -} diff --git a/src/context/scope.ts b/src/context/scope.ts index 2cd261588..33603d941 100644 --- a/src/context/scope.ts +++ b/src/context/scope.ts @@ -6,8 +6,7 @@ export interface ScopeObject extends Record { export type Scope = ScopeObject | Drop -export function createScope (from?: ScopeObject): ScopeObject { - const scope = Object.create(null) - if (from) Object.assign(scope, from) - return scope +export function createScope (from?: Scope): Scope { + if (from instanceof Drop) return from + return Object.assign(Object.create(null), from) } diff --git a/src/liquid-options.ts b/src/liquid-options.ts index f0fc1bff6..cbc8b9489 100644 --- a/src/liquid-options.ts +++ b/src/liquid-options.ts @@ -38,10 +38,7 @@ export interface LiquidOptions { strictVariables?: boolean; /** Catch all errors instead of exit upon one. Please note that render errors won't be reached when parse fails. */ catchAllErrors?: boolean; - /** - * Hide scope variables from prototypes, useful when you're passing a not sanitized object into LiquidJS or need to hide prototypes from templates. - * This only applies to property/index access on scope objects. Filter transforms and iteration operate on the resolved value with standard JavaScript semantics, so prototype-inherited array indices may still be surfaced by them. - */ + /** Limit template property reads on plain scope objects to own properties. Defaults to `true`. See https://liquidjs.com/tutorials/security-model.html */ ownPropertyOnly?: boolean; /** Modifies the behavior of `strictVariables`. If set, a single undefined variable will *not* cause an exception in the context of the `if`/`elsif`/`unless` tag and the `default` filter. Instead, it will evaluate to `false` and `null`, respectively. Irrelevant if `strictVariables` is not set. Defaults to `false`. **/ lenientIf?: boolean; diff --git a/src/tags/block.ts b/src/tags/block.ts index 56dca80af..947f0b3e5 100644 --- a/src/tags/block.ts +++ b/src/tags/block.ts @@ -1,4 +1,4 @@ -import { BlockMode, createScope } from '../context' +import { BlockMode } from '../context' import { isTagToken } from '../util' import { BlockDrop } from '../drop' import { Liquid, TagToken, TopLevelToken, Template, Context, Emitter, Tag } from '..' @@ -38,7 +38,7 @@ export default class extends Tag { if (stack.includes(self)) throw new Error('block tag cannot be nested') stack.push(self) - ctx.push(createScope({ block: superBlock })) + ctx.push({ block: superBlock }) yield liquid.renderer.renderTemplates(templates, ctx, emitter) ctx.pop() stack.pop() diff --git a/src/tags/for.ts b/src/tags/for.ts index ec964ac6a..c338a4018 100644 --- a/src/tags/for.ts +++ b/src/tags/for.ts @@ -1,6 +1,5 @@ import { Hash, ValueToken, Liquid, Tag, evalToken, Emitter, TagToken, TopLevelToken, Context, Template, ParseStream } from '..' import { assertEmpty, isValueToken, toEnumerable } from '../util' -import { createScope } from '../context/scope' import { ForloopDrop } from '../drop/forloop-drop' import { Parser } from '../parser' import { Arguments } from '../template' @@ -44,7 +43,7 @@ export default class extends Tag { * render (ctx: Context, emitter: Emitter): Generator { const r = this.liquid.renderer const continueKey = 'continue-' + this.variable + '-' + this.collection.getText() - ctx.push(createScope({ continue: ctx.getRegister(continueKey, {}) })) + ctx.push({ continue: ctx.getRegister(continueKey, {}) }) const hash = (yield this.hash.render(ctx)) as Record ctx.pop() @@ -68,8 +67,7 @@ export default class extends Tag { if (!this.templates.length) return - const scope = createScope({ forloop: new ForloopDrop(collection.length, this.collection.getText(), this.variable) }) - ctx.push(scope) + const scope = ctx.push({ forloop: new ForloopDrop(collection.length, this.collection.getText(), this.variable) }) for (const item of collection) { scope[this.variable] = item ctx.continueCalled = ctx.breakCalled = false diff --git a/src/tags/include.ts b/src/tags/include.ts index 41507757d..a0965a617 100644 --- a/src/tags/include.ts +++ b/src/tags/include.ts @@ -1,5 +1,5 @@ import { Template, ValueToken, TopLevelToken, Liquid, Tag, assert, evalToken, Hash, Emitter, TagToken, Context } from '..' -import { BlockMode, createScope, Scope } from '../context' +import { BlockMode, Scope } from '../context' import { Parser } from '../parser' import { Argument, Arguments, PartialScope } from '../template' import { isString, isValueToken } from '../util' @@ -37,10 +37,10 @@ export default class extends Tag { const saved = ctx.saveRegister('blocks', 'blockMode') ctx.setRegister('blocks', {}) ctx.setRegister('blockMode', BlockMode.OUTPUT) - const scope = createScope((yield hash.render(ctx)) as Scope) + const scope = (yield hash.render(ctx)) as Scope if (withVar) scope[filepath] = yield evalToken(withVar, ctx) const templates = (yield liquid._parsePartialFile(filepath, ctx.sync, this.currentFile)) as Template[] - ctx.push(ctx.opts.jekyllInclude ? createScope({ include: scope }) : scope) + ctx.push(ctx.opts.jekyllInclude ? { include: scope } : scope) yield renderer.renderTemplates(templates, ctx, emitter) ctx.pop() ctx.restoreRegister(saved) diff --git a/src/tags/layout.ts b/src/tags/layout.ts index 91e512ecb..ca8ddf017 100644 --- a/src/tags/layout.ts +++ b/src/tags/layout.ts @@ -1,5 +1,5 @@ import { Scope, Template, Liquid, Tag, assert, Emitter, Hash, TagToken, TopLevelToken, Context } from '..' -import { BlockMode, createScope } from '../context' +import { BlockMode } from '../context' import { parseFilePath, renderFilePath, ParsedFileName } from './render' import { BlankDrop } from '../drop' import { Parser } from '../parser' @@ -41,7 +41,7 @@ export default class extends Tag { ctx.setRegister('blockMode', BlockMode.OUTPUT) // render the layout file use stored blocks - ctx.push(createScope((yield args.render(ctx)) as Scope)) + ctx.push((yield args.render(ctx)) as Scope) yield renderer.renderTemplates(templates, ctx, emitter) ctx.pop() ctx.depthLimit.release(1) diff --git a/src/tags/tablerow.ts b/src/tags/tablerow.ts index 563b44392..22352b698 100644 --- a/src/tags/tablerow.ts +++ b/src/tags/tablerow.ts @@ -1,5 +1,4 @@ import { isValueToken, toEnumerable } from '../util' -import { createScope } from '../context/scope' import { ValueToken, Liquid, Tag, evalToken, Emitter, Hash, TagToken, TopLevelToken, Context, Template, ParseStream } from '..' import { TablerowloopDrop } from '../drop/tablerowloop-drop' import { Parser } from '../parser' @@ -53,8 +52,7 @@ export default class extends Tag { const r = this.liquid.renderer const tablerowloop = new TablerowloopDrop(collection.length, cols, this.collection.getText(), this.variable) - const scope = createScope({ tablerowloop }) - ctx.push(scope) + const scope = ctx.push({ tablerowloop }) for (let idx = 0; idx < collection.length; idx++, tablerowloop.next()) { scope[this.variable] = collection[idx] diff --git a/test/integration/liquid/scope-security.spec.ts b/test/integration/liquid/scope-security.spec.ts new file mode 100644 index 000000000..bb1eb474f --- /dev/null +++ b/test/integration/liquid/scope-security.spec.ts @@ -0,0 +1,61 @@ +import { Liquid } from '../../../src/liquid' +import { Drop } from '../../../src/drop/drop' + +describe('scope security', function () { + let liquid: Liquid + + beforeEach(function () { + liquid = new Liquid() + }) + + it('should iterate plain objects via inherited Symbol.iterator (ownPropertyOnly exception)', async function () { + // eslint-disable-next-line no-extend-native + (Object.prototype as any)[Symbol.iterator] = function * () { yield 'inherited' } + try { + await expect(liquid.parseAndRender( + '{% for x in obj %}{{ x }}{% endfor %}', + { obj: {} } + )).resolves.toBe('inherited') + } finally { + delete (Object.prototype as any)[Symbol.iterator] + } + }) + + it('should not read inherited size on plain objects', async function () { + const obj = Object.create({ size: 99 }) + obj.own = 'yes' + await expect(liquid.parseAndRender('{{ obj.size }}', { obj })).resolves.toBe('1') + }) + + it('should read inherited size when ownPropertyOnly=false', async function () { + liquid = new Liquid({ ownPropertyOnly: false }) + const obj = Object.create({ size: 99 }) + await expect(liquid.parseAndRender('{{ obj.size }}', { obj })).resolves.toBe('99') + }) + + it('should still iterate Drop with Symbol.iterator', async function () { + class IterableDrop extends Drop { + * [Symbol.iterator] () { + yield 'a' + yield 'b' + } + } + await expect(liquid.parseAndRender( + '{% for x in drop %}{{ x }}{% endfor %}', + { drop: new IterableDrop() } + )).resolves.toBe('ab') + }) + + it('should block own blocked keys when ownPropertyOnly=true', async function () { + const scope = JSON.parse('{"__proto__": {"polluted": true}, "constructor": {"name": "Custom"}, "name": "Alice"}') + await expect(liquid.parseAndRender('{{ __proto__.polluted }}', scope)).resolves.toBe('') + await expect(liquid.parseAndRender('{{ constructor.name }}', scope)).resolves.toBe('') + await expect(liquid.parseAndRender('{{ name }}', scope)).resolves.toBe('Alice') + }) + + it('should block inherited properties when ownPropertyOnly=true', async function () { + const scope = { foo: Object.create({ __proto__: { bar: 'BAR' }, constructor: { name: 'Evil' } }) } + await expect(liquid.parseAndRender('{{ foo.__proto__ }}', scope)).resolves.toBe('') + await expect(liquid.parseAndRender('{{ foo.constructor }}', scope)).resolves.toBe('') + }) +})