diff --git a/docs/source/tutorials/options.md b/docs/source/tutorials/options.md index b1c2315b3..73832eb28 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`. Blocked keys (`__proto__`, `constructor`, `prototype`) always apply. [`Drop`][drop] values, iteration, `.size`/`.first`/`.last`, filters, and custom tags follow separate rules—see [Security Model](./security-model.html). {% note info Nonexistent Tags %} Nonexistent tags always throw errors during parsing and this behavior cannot be customized. @@ -163,3 +161,4 @@ Parameter orders are ignored by default, for example `{% for i in (1..8) reverse [jekyllInclude]: /api/interfaces/LiquidOptions.html#jekyllInclude [raw]: ../filters/raw.html [outputEscape]: /api/interfaces/LiquidOptions.html#outputEscape +[drop]: /api/classes/Drop.html diff --git a/docs/source/tutorials/security-model.md b/docs/source/tutorials/security-model.md index f797d7143..6e90430dd 100644 --- a/docs/source/tutorials/security-model.md +++ b/docs/source/tutorials/security-model.md @@ -50,9 +50,18 @@ 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 `true`. 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. +[`ownPropertyOnly`][ownPropertyOnly] controls **template property reads on plain scope objects** (objects whose prototype is `null` or `Object.prototype`). Default `true`. When enabled, only own enumerable properties are visible to variable lookup; inherited keys from `Object.prototype` or other prototypes are hidden. -LiquidJS also blocks template access to the property names `__proto__`, `constructor`, and `prototype` at any depth, and omits those keys when building null-prototype managed scopes (for example loop and `{% render %}` locals). For deeply untrusted input, pre-sanitize scope objects before passing them to `render()` (for example with [@hapi/bourne](https://www.npmjs.com/package/@hapi/bourne)). +**Always blocked** (regardless of `ownPropertyOnly`): template access to the property names `__proto__`, `constructor`, and `prototype`, and writes to those names via `{% assign %}`, `{% capture %}`, `{% increment %}`, and `{% decrement %}`. Managed scopes built with null prototypes (loop locals, `{% render %}` bindings, filter iteration scopes) omit those keys when created from user data. + +**Exceptions** — `ownPropertyOnly` does not restrict: + +- [`Drop`][drop] values: prototype chain and [`liquidMethodMissing`][liquidMethodMissing] still apply; audit custom drops like privileged code. +- Iteration (`{% for %}`, `{% tablerow %}`, `{% render for %}`): class instances and drops keep their iterators; plain objects only iterate via an own `Symbol.iterator`. +- Liquid pseudo-properties `.size`, `.first`, and `.last`: arrays and strings use length/index rules; `Map`/`Set` use their native size; plain objects with an own `size` property use that value (inherited `size` on plain objects is ignored when `ownPropertyOnly` is `true`). +- Filters and custom tags: operate on resolved values with their own semantics. + +Use `true` for untrusted or polluted objects; add [`strictVariables`][strictVariables] if missing paths should error. Override per render via [`RenderOptions`][renderOwnPropertyOnly]. For deeply untrusted input, pre-sanitize scope objects before `render()` (for example with [@hapi/bourne](https://www.npmjs.com/package/@hapi/bourne)). 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.ts b/src/context/context.ts index ae76d838d..9ab03798c 100644 --- a/src/context/context.ts +++ b/src/context/context.ts @@ -139,7 +139,7 @@ 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) + if (key === 'size') return readSize(obj, this.ownPropertyOnly) else if (key === 'first') return readFirst(obj, this.ownPropertyOnly) else if (key === 'last') return readLast(obj, this.ownPropertyOnly) return value @@ -162,8 +162,10 @@ function readLast (obj: Scope, ownPropertyOnly: boolean) { return readJSProperty(obj, 'last', ownPropertyOnly) } -function readSize (obj: Scope) { - if (hasOwnProperty.call(obj, 'size') || obj['size'] !== undefined) return obj['size'] +function readSize (obj: Scope, ownPropertyOnly: boolean) { + if (hasOwnProperty.call(obj, 'size')) return obj['size'] + if (!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 } diff --git a/src/filters/array.ts b/src/filters/array.ts index 2a5cb8a0e..5b452dd97 100644 --- a/src/filters/array.ts +++ b/src/filters/array.ts @@ -3,6 +3,7 @@ import { arrayIncludes, equals, evalToken, isTruthy } from '../render' import { Value, FilterImpl } from '../template' import { Tokenizer } from '../parser' import type { Scope } from '../context' +import { createScope } from '../context/scope' import { EmptyDrop } from '../drop' export const join = argumentsToValue(function (this: FilterImpl, v: any[], arg: string) { @@ -134,7 +135,7 @@ function * filter_exp (this: FilterImpl, include: boolean, arr const keyTemplate = new Value(stringify(exp), this.liquid) const array = toArray(arr) for (const item of array) { - this.context.push({ [itemName]: item }) + this.context.push(createScope({ [itemName]: item })) const value = yield keyTemplate.value(this.context) this.context.pop() if (value === include) filtered.push(item) @@ -160,7 +161,7 @@ export function * reject_exp (this: FilterImpl, arr: T[], item export function * group_by (this: FilterImpl, arr: T[], property: string): IterableIterator { const map = new Map() - arr = toEnumerable(arr) + arr = toEnumerable(arr, this.context.ownPropertyOnly) const token = new Tokenizer(stringify(property)).readScopeValue() for (const item of arr) { const key = yield evalToken(token, this.context.spawn(item)) @@ -173,9 +174,9 @@ export function * group_by (this: FilterImpl, arr: T[], proper export function * group_by_exp (this: FilterImpl, arr: T[], itemName: string, exp: string): IterableIterator { const map = new Map() const keyTemplate = new Value(stringify(exp), this.liquid) - arr = toEnumerable(arr) + arr = toEnumerable(arr, this.context.ownPropertyOnly) for (const item of arr) { - this.context.push({ [itemName]: item }) + this.context.push(createScope({ [itemName]: item })) const key = yield keyTemplate.value(this.context) this.context.pop() if (!map.has(key)) map.set(key, []) diff --git a/src/tags/assign.ts b/src/tags/assign.ts index da6114120..020563d69 100644 --- a/src/tags/assign.ts +++ b/src/tags/assign.ts @@ -1,4 +1,5 @@ import { Value, Liquid, TopLevelToken, TagToken, Context, Tag } from '..' +import { isBlockedScopeKey } from '../context/scope' import { Arguments } from '../template' import { IdentifierToken } from '../tokens' @@ -20,6 +21,7 @@ export default class extends Tag { this.value = new Value(this.tokenizer.readFilteredValue(), this.liquid) } * render (ctx: Context): Generator { + if (isBlockedScopeKey(this.key)) return ctx.bottom()[this.key] = yield this.value.value(ctx, this.liquid.options.lenientIf) } diff --git a/src/tags/capture.ts b/src/tags/capture.ts index d82f14a62..cdf34051d 100644 --- a/src/tags/capture.ts +++ b/src/tags/capture.ts @@ -1,4 +1,5 @@ import { Liquid, Tag, Template, Context, TagToken, TopLevelToken } from '..' +import { isBlockedScopeKey } from '../context/scope' import { Parser } from '../parser' import { IdentifierToken, QuotedToken } from '../tokens' import { isTagToken } from '../util' @@ -31,6 +32,7 @@ export default class extends Tag { * render (ctx: Context): Generator { const r = this.liquid.renderer const html = yield r.renderTemplates(this.templates, ctx) + if (isBlockedScopeKey(this.variable)) return ctx.bottom()[this.variable] = html } diff --git a/src/tags/decrement.ts b/src/tags/decrement.ts index c854e2481..a0ba81a21 100644 --- a/src/tags/decrement.ts +++ b/src/tags/decrement.ts @@ -1,4 +1,5 @@ import { Tag, Liquid, TopLevelToken, Emitter, TagToken, Context } from '..' +import { isBlockedScopeKey } from '../context/scope' import { IdentifierToken } from '../tokens' import { isNumber, stringify } from '../util' @@ -11,6 +12,7 @@ export default class extends Tag { this.variable = this.identifier.content } render (context: Context, emitter: Emitter) { + if (isBlockedScopeKey(this.variable)) return const scope = context.environments if (!isNumber(scope[this.variable])) { scope[this.variable] = 0 diff --git a/src/tags/for.ts b/src/tags/for.ts index ec964ac6a..e37e51900 100644 --- a/src/tags/for.ts +++ b/src/tags/for.ts @@ -52,7 +52,7 @@ export default class extends Tag { ? Object.keys(hash).filter(x => MODIFIERS.includes(x)) : MODIFIERS.filter(x => hash[x] !== undefined) - let collection = toEnumerable(yield evalToken(this.collection, ctx)) + let collection = toEnumerable(yield evalToken(this.collection, ctx), ctx.ownPropertyOnly) collection = modifiers.reduce((collection, modifier: valueOf) => { if (modifier === 'offset') return offset(collection, hash['offset']) if (modifier === 'limit') return limit(collection, hash['limit']) diff --git a/src/tags/increment.ts b/src/tags/increment.ts index 948faf0ce..72da3711b 100644 --- a/src/tags/increment.ts +++ b/src/tags/increment.ts @@ -1,4 +1,5 @@ import { isNumber, stringify } from '../util' +import { isBlockedScopeKey } from '../context/scope' import { Tag, Liquid, TopLevelToken, Emitter, TagToken, Context } from '..' import { IdentifierToken } from '../tokens' @@ -11,6 +12,7 @@ export default class extends Tag { this.variable = this.identifier.content } render (context: Context, emitter: Emitter) { + if (isBlockedScopeKey(this.variable)) return const scope = context.environments if (!isNumber(scope[this.variable])) { scope[this.variable] = 0 diff --git a/src/tags/render.ts b/src/tags/render.ts index 8ac279e94..dbac5b981 100644 --- a/src/tags/render.ts +++ b/src/tags/render.ts @@ -70,7 +70,7 @@ export default class extends Tag { if (this.forBinding) { const { value, alias } = this.forBinding - const collection = toEnumerable(yield evalToken(value, ctx)) + const collection = toEnumerable(yield evalToken(value, ctx), ctx.ownPropertyOnly) scope['forloop'] = new ForloopDrop(collection.length, value.getText(), alias as string) for (const item of collection) { scope[alias as string] = item diff --git a/src/tags/tablerow.ts b/src/tags/tablerow.ts index 563b44392..556d94749 100644 --- a/src/tags/tablerow.ts +++ b/src/tags/tablerow.ts @@ -39,7 +39,7 @@ export default class extends Tag { } * render (ctx: Context, emitter: Emitter): Generator { - let collection = toEnumerable(yield evalToken(this.collection, ctx)) + let collection = toEnumerable(yield evalToken(this.collection, ctx), ctx.ownPropertyOnly) const args = (yield this.args.render(ctx)) as Record const offset = args.offset || 0 const limit = (args.limit === undefined) ? collection.length : args.limit diff --git a/src/util/underscore.ts b/src/util/underscore.ts index 4ee8d14b0..15a8d93cb 100644 --- a/src/util/underscore.ts +++ b/src/util/underscore.ts @@ -47,11 +47,11 @@ export function readArrayElement (arr: any[], index: number, ownPropertyOnly: bo return arr[index] } -export function toEnumerable (val: any): T[] { +export function toEnumerable (val: any, ownPropertyOnly = false): T[] { val = toValue(val) if (isArray(val)) return val if (isString(val) && val.length > 0) return [val] as unknown as T[] - if (isIterable(val)) return Array.from(val) + if (isIterable(val, ownPropertyOnly)) return Array.from(val) if (isObject(val)) return Object.keys(val).map((key) => [key, val[key]]) as unknown as T[] return [] } @@ -96,8 +96,17 @@ export function isArrayLike (value: any): value is any[] { return value && isNumber(value.length) } -export function isIterable (value: any): value is Iterable { - return isObject(value) && Symbol.iterator in value +export function isIterable (value: any, ownPropertyOnly = false): value is Iterable { + value = toValue(value) + if (!isObject(value)) return false + if (isArray(value)) return true + if (value instanceof Drop) return Symbol.iterator in value + if (ownPropertyOnly) { + const proto = Object.getPrototypeOf(value) + const isPlain = proto === null || proto === Object.prototype + if (isPlain) return hasOwnProperty.call(value, Symbol.iterator) + } + return Symbol.iterator in value } /* diff --git a/test/integration/liquid/scope-security.spec.ts b/test/integration/liquid/scope-security.spec.ts index c8d10472f..aa85511fb 100644 --- a/test/integration/liquid/scope-security.spec.ts +++ b/test/integration/liquid/scope-security.spec.ts @@ -1,4 +1,5 @@ import { Liquid } from '../../../src/liquid' +import { Drop } from '../../../src/drop/drop' describe('scope security', function () { let liquid: Liquid @@ -55,4 +56,52 @@ describe('scope security', function () { const scope = { foo: { __proto__: { bar: 'BAR' } } } await expect(liquid.parseAndRender('{{ foo.__proto__.bar }}', scope, { ownPropertyOnly: false })).resolves.toBe('') }) + + it('should not write increment to __proto__ on user scope', async function () { + const scope = Object.create(null) as Record + await expect(liquid.parseAndRender('{% increment __proto__ %}', scope)).resolves.toBe('') + expect(Object.prototype).toEqual(Object.prototype) + expect(scope).toEqual({}) + }) + + it('should not write assign to __proto__ on user scope', async function () { + const scope = { safe: 'ok' } + await expect(liquid.parseAndRender( + '{% assign __proto__ = obj %}', + { ...scope, obj: { polluted: true } } + )).resolves.toBe('') + expect((Object.prototype as any).polluted).toBeUndefined() + }) + + it('should not iterate plain objects via inherited Symbol.iterator', 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('') + } 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') + }) })