diff --git a/docs/source/tutorials/options.md b/docs/source/tutorials/options.md index b1c2315b32..e4386c7bf0 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 (no inherited prototype keys). Defaults to `true`. Proto keys (`__proto__`, `constructor`, `prototype`) are blocked when `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 3c6b4aa746..c054752647 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). Proto keys (`__proto__`, `constructor`, `prototype`) are blocked when `true`, even as own properties; when `false`, own properties with those names are allowed but inherited access to those names is still blocked. + +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 d8eeac9784..6cac6d84b3 100644 --- a/src/context/context.spec.ts +++ b/src/context/context.spec.ts @@ -198,6 +198,35 @@ 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 still block inherited blocked keys when ownPropertyOnly=false', function () { + ctx = new Context({ foo: Object.create({ __proto__: { bar: 'BAR' } }) }, { ownPropertyOnly: false } as any) + expect(ctx.getSync(['foo', '__proto__'])).toEqual(undefined) + }) + it('should block constructor access', function () { + ctx.push({ foo: { constructor: { name: 'Evil' } } }) + expect(ctx.getSync(['foo', 'constructor'])).toEqual(undefined) + }) + it('should block prototype access', function () { + ctx.push({ foo: { prototype: { bar: 'BAR' } } }) + expect(ctx.getSync(['foo', 'prototype'])).toEqual(undefined) + }) + it('should block top-level __proto__ variable', function () { + ctx = new Context({ __proto__: { bar: 'BAR' }, bar: 'BAR' } as any) + expect(ctx.getSync(['__proto__'])).toEqual(undefined) + expect(ctx.getSync(['bar'])).toEqual('BAR') + }) }) describe('.getAll()', function () { @@ -221,6 +250,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 fbc17f8d8c..c85323f396 100644 --- a/src/context/context.ts +++ b/src/context/context.ts @@ -94,8 +94,10 @@ export class Context { } return scope } - public push (ctx: object) { - return this.scopes.push(ctx) + public push (ctx: Scope): Scope { + const scope = ctx instanceof Drop ? ctx : createScope(ctx) + this.scopes.push(scope) + return scope } public pop () { return this.scopes.pop() @@ -118,9 +120,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 +133,34 @@ 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 + } } +const BLOCKED_SCOPE_KEYS = new Set(['__proto__', 'constructor', 'prototype']) + export function readJSProperty (obj: Scope, key: PropertyKey, ownPropertyOnly: boolean) { + if (typeof key === 'string' && BLOCKED_SCOPE_KEYS.has(key)) { + if (ownPropertyOnly || !hasOwnProperty.call(obj, key)) 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 2cd2615882..e5fd79943b 100644 --- a/src/context/scope.ts +++ b/src/context/scope.ts @@ -7,7 +7,5 @@ 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 + return Object.assign(Object.create(null), from) } diff --git a/src/liquid-options.ts b/src/liquid-options.ts index f0fc1bff68..b911c5c435 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`. Proto keys (`__proto__`, `constructor`, `prototype`) are blocked when `true`. */ 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 56dca80af8..947f0b3e59 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 ec964ac6ac..c338a40183 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 41507757d7..a0965a617e 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 91e512ecb0..ca8ddf0174 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 563b44392b..22352b6985 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 0000000000..2c3ae8973f --- /dev/null +++ b/test/integration/liquid/scope-security.spec.ts @@ -0,0 +1,42 @@ +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 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') + }) +})