leaking JS prototype getter functions in evaluation (eg, .length)

[amit777]

I noticed that javascript string and array prototype functions may be getting leaked. I actually like some of them like str.length or arr_var.length rather than using str | size or arr_var | size. This seems to be non-standard liquid templating.

I'm just asking because I would like to keep the .length since it is more ergonomic when used in {% if str.length > 5 %} ... {% endif %}

I'm worried it may be removed in a future "fix". Is this a bug or a feature that getters like .length or .toUpperCase work?

[amit777]

And if you do plan on removing access to the getter functions, I wonder if there could be flag to keep the existing functionality. something like

const liquid = new Liquid({
  jsTruthy: true,
  allowedJsGetters: ['length', <etc>.. ]
});

[harttle]

Related to #254 but this is more specific. I'll treat this as a feature request and leave this open for further discussion.

I don't have a good idea to restrict prototype access since we had discussion on #254, because we cannot tell whether a property is from prototype or not. Currently we rely on users only pass sanitized plain object into LiquidJS as scope and modifying builtin prototype is considered bad practice.

Consider this case:

let proto = { foo: 'FOO' }
let obj1 = Object.create(proto)
let obj2 = Object.create(proto)
obj2.foo = 'FOO'

isFromProto(obj1, 'foo') // expect true
isFromProto(obj2, 'foo') // expect false

// we cannot implement such a `isFromProto` for both of following are true
obj1.__proto__.foo === obj1.foo
obj2.__proto__.foo === obj2.foo

A white list like allowedJsGetters will affect all objects and that property would be nolonger valid as variable/property name. Need more discussion. Maybe removing __proto__ recursively on scope is also a solution?

[amit777]

I’ll ask one of JS experts his thoughts on this. My initial thought would be to use the hasOwnProperty() method to check if the key belongs to the object or is it inherited. I’m not sure if you have considered this? Would you be able to point me to the code that executes the inherited method?

…

On Sat, Jan 22, 2022 at 7:10 PM Jun Yang ***@***.***> wrote: Related to #254 <#254> but this is more specific. I'll treat this as a feature request and leave this open for further discussion. I don't have a good idea to restrict prototype access since we had discussion on #254 <#254>, because we cannot tell whether a property is from prototype or not. Currently we rely on users only pass sanitized plain object into LiquidJS as scope and modifying builtin prototype is considered bad practice. Consider this case: let proto = { foo: 'FOO' }let obj1 = Object.create(proto)let obj2 = Object.create(proto)obj2.foo = 'FOO' isFromProto(obj1, 'foo') // expect trueisFromProto(obj2, 'foo') // expect false // we cannot implement such a `isFromProto` for both of following are trueobj1.__proto__.foo === obj1.fooobj2.__proto__.foo === obj2.foo A white list like allowedJsGetters will affect all objects and that property would be nolonger valid as variable/property name. Need more discussion. Maybe removing __proto__ recursively on scope is also a solution? — Reply to this email directly, view it on GitHub <#454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUT7TLDOF52GSQYQQZ2RVTUXNWTPANCNFSM5MSPZ7FQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[harttle]

use the hasOwnProperty() method to check if the key belongs to the
object

Good idea. It works fine with the above the above case I posted, and also works for Array.prototype.length.

Object.hasOwnProperty(Object.create({foo: 'foo'}), 'foo') // false
Object.hasOwnProperty([1,2], 'length') // false
class Foo { foo() {} }
Object.hasOwnProperty(new Foo(), 'foo') // false, does this make sense?

Would
you be able to point me to the code that executes the inherited method?

liquidjs/src/context/context.ts

Line 90 in 8db6c7b

export function readProperty (obj: Scope, key: string) {

[jg-rp]

I don't think commit 7e99efc is solving this issue as intended.

const { Liquid } = require("liquidjs");
const engine = new Liquid({ ownPropertyOnly: true });
engine.parseAndRender("{{ a.length }}", { a: ["a", "b", "c"] }).then(console.log);

Output:

3

Notice from your examples that Object.hasOwnProperty([1,2], 'length') (without .call) is equal to false, but Object.hasOwnProperty.call([1,2], 'length') is equal to true.

Perhaps a solution using Object.propertyIsEnumerable would be more appropriate.

Object.propertyIsEnumerable.call(Object.create({foo: 'foo'}), 'foo') // false
Object.propertyIsEnumerable.call([1,2], 'length') // false
Object.propertyIsEnumerable.call("", 'length') // false

[harttle]

I don't think commit 7e99efc is solving this issue as intended.

Yes, that is not solving the "eg, .length" isssue, but it does solve "reading from prototype" issue. By definition, use hasOwnProperty can prevent from reading from prototype chain (chained __proto__ property in most implementations).

Actually .length is not on the prototype, as you can see in:

// own property
> [1,2].length
2
// prototype also has this property, but not the intended one
> [1,2].__proto__.length

Here's a better explanation on stackoverflow about why it's not on prototype. And I don't think reading array length in template is a potential security issue, it's an Array after all, otherwise why not use object.

BTW, Object.hasOwnProperty([1,2], 'length') is a misuse, the [1,2] will be interperated as property name and length will be ignored. This call will always return false for any object. Further explained:

> a = {foo: 'bar'}
{ foo: 'bar' }
// aparently
> a.hasOwnProperty('foo')
true
// Object don't have a a property
> Object.hasOwnProperty(a, 'foo')
false
// Object.create is its own property
> Object.hasOwnProperty('create')
true