googlearchive / caja

Caja is a tool for safely embedding third party HTML, CSS and JavaScript in your website.
Apache License 2.0
1.13k stars 127 forks source link

Subjective vs Record inheritance #953

Closed kpreid closed 9 years ago

kpreid commented 9 years ago

Original issue 953 created by erights on 2008-12-06T00:57:42.000Z:

Currently, the read() function valija-cajita.js (also known as $v.r()) has the following alarming bit of code:

// BUG TODO(erights): figure out why things break when the
// following line (which really shouldn't be there) is deleted.
if (name in obj) { return obj[name];}

The presence of this line had not been a problem till now, and (since it's in teh Valija runtime), can't cause a security problem. But Ihab just ran into a case where it is causing the cajoling of Prototype to fail. I just figured out why things break if it is removed -- it suppresses record inheritance as seen from Valija. More later...

kpreid commented 9 years ago

Comment #1 originally posted by erights on 2008-12-06T21:54:09.000Z:

The presence of that alarming line

// BUG TODO(erights): figure out why things break when the
// following line (which really shouldn't be there) is deleted.
if (name in obj) { return obj[name];}

in Valija's read() function meant that (for non-function objects) the objective prototype chain would be checked first, i.e., the inherited properties visible from Cajita. Not only does this make non-additive monkey patching ineffective, it prevents a Valija monkey patching of a sub-prototype from overriding a property genuinely inherited from a super-prototype -- as in the case that Ihab ran into, involving setStyle IIRC.

Clearly, the above code allows too much visibility of objective properties, since we currently intend to allow non-additive monkey patching, and will certainly allow subjective overriding. However, removing this line leaves as objective only own properties of instances, which turns out to be too little. To explain this precisely, we first need some definitions.

        Relevant Cajita & Valija definitions and background

Cajita traffics in two kinds of prototype inheritance -- classical inheritance and record inheritance. Cajita code itself can only create record inheritance chains, by use of cajita.beget(). Uncajoled code exports to Cajita (by taming) objects obeying the classical inheritance pattern, which the Cajita runtime assumes is well formed. In the classical pattern, an instance inherits directly from a prototypical object, which is defined to be an object whose actual 'constructor' property points at a function whose actual 'prototype' property points back at it. That function is the instance's direct constructor, and is returned by the query cajita.directConstructor(). Prototypical objects themselves are not first-class in Cajita -- they can be queried indirectly (cajita.getProtoPropertyNames() and cajita.getProtoPropertyValue()), but one cannot obtain a direct reference to one.

An object whose direct constructor is Object is a record. An object whose direct constructor is Array is an array. (Unlike full ES, in Cajita, only genuine arrays can inherit from Array.prototype.) A JSON Container is a record or array. All other Cajita-accessible non-function objects are constructed objects.

If x is a record, "var y = cajita.beget(x);" creates a record y that inherits from x, but whose direct constructor remains Object. x and y are both first class and may both be independently mutable. The Valija runtime uses record inheritance to simulate EcmaScript's mutable classical inheritance -- both to monkey patch tamed classical objects and to define new objects by code written in Valija.

       Finally, the real problem and proposed solution

For constructed objects and arrays, only their own properties need be objective in Valija, since they directly inherit from prototypical objects which are subjectively shadowed. However, For records, all the properties they inherit from parent records should also be objective. With the above line deleted, Valija does not recognize the record inheritance it uses, since it treats only own properties as objective, and consults its subjective shadows only for functions and prototypical objects. Properties inherited from intermediate records like x appear to be missing.

The cajita.readOwn() fastpath provided for use by Valija should instead be cajita.readObjective(). For records, it should recognize both own properties and any property inherited from any parent below Object.prototype.

               Possible future enhancement

Ihab, Metaweta, Jas and I discussed a possible future enhancement that would enable one to mark a frozen object as one that should be treated as subjectively mutable rather than objectively frozen. For example, if a particular Valija plugin does $v.monkey(Math), then Math, which is objectively frozen, becomes subjectively mutable as seen from that $v. That Valija plugin is effectively saying "I want to (pretend to) monkey around with Math." I'm not planning to do this as part of this bug fix but will keep it in mind.

kpreid commented 9 years ago

Comment #2 originally posted by erights on 2008-12-09T02:14:56.000Z:

Related to issue 956

kpreid commented 9 years ago

Comment #3 originally posted by erights on 2008-12-09T02:23:44.000Z:

Valija's "in" operator is not correct in the presence of non-additive monkey patching

delete Array.prototype.push;
'push' in [];

in Valija, for example in the testbed, should be false but is currently true. The presence of this bug is consistent with the read() bug reported above, but occurs through a distinct mechanism. Since "in" and read() should give consistent answers, they should be fixed together.

kpreid commented 9 years ago

Comment #4 originally posted by erights on 2009-02-25T22:09:45.000Z:

Related to issue 997

kpreid commented 9 years ago

Comment #5 originally posted by erights on 2009-02-25T22:32:22.000Z:

Related to issue 998

kpreid commented 9 years ago

Comment #6 originally posted by erights on 2009-02-25T22:37:22.000Z:

<empty>

kpreid commented 9 years ago

Comment #7 originally posted by erights on 2010-06-20T05:35:24.000Z:

<empty>

kpreid commented 9 years ago

Comment #8 originally posted by erights@google.com on 2010-09-19T03:39:46.000Z:

This thorn in our side is now fixed, as expected, by ES5/3. I can't test the "push" example code above because the current property virtualization prevents tamed primordial methods from being deleted. Once we transition to objective replacement, I'll be able to test this case as well.

kpreid commented 9 years ago

Comment #9 originally posted by michaeljohnthethird on 2010-09-19T13:34:17.000Z:

Then shouldn't this issue remain open until that push example is both testable and working? We don't want this forgotten about just so we can have less tickets open, right?

kpreid commented 9 years ago

Comment #10 originally posted by erights@google.com on 2010-09-19T15:31:54.000Z:

Good point. Reverting to Postponed.

kpreid commented 9 years ago

Comment #11 originally posted by metaweta on 2012-02-10T17:11:33.000Z:

We're not going to move away from virtualization in ES5/3.