Bind expressions #6201

dreamofabear · 2016-11-15T19:41:47Z

Partial for #6199.

Implements Bind's JS-like expression grammar with Jison (also used by amp-access) and unit tests. Contains several security features, including:

Disallow access of prototype properties
Only allow invocation of whitelisted String and Array functions
Disallow access to globals, function declaration and non-whitelisted keywords

Other things to note:

Undefined vars, properties, array-index-out-of-bounds return null instead of throwing errors (similar to Angular)
bind-expr-impl.js is just a compilation of bind-expr-impl.jison and can be ignored

/to @dvoytenko @aghassemi
/cc @cramforce

aghassemi · 2016-11-28T18:28:17Z

Not forgotten, will be 👀 at this today.

jridgewell

Man, I hope bind expressions aren't used too much, this code's gonna be slow.

jridgewell · 2016-11-28T20:13:23Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+        var prop = Object.prototype.toString.call($2);
+
+        if (obj === '[object Array]') {
+          if (prop === '[object Number]' && Number.isInteger($2)) {


Number.isInteger is not available everywhere. An imperfect but practical equivalent is $2 === ($2 | 0), or you can go for the full polyfill.

Removed this code block per Ali's comment below.

jridgewell · 2016-11-28T20:15:30Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+          }
+        }
+
+        if (prop === '[object String]') {


Numeric props are valid for objects, too.

cramforce · 2016-11-28T22:40:47Z

Regarding @jridgewell's speed comment: How long does it take to execute 100 typical expressions on a Nexus 5X type device?

cramforce · 2016-11-29T00:58:36Z

extensions/amp-bind/0.1/bind-expr.js

+
+import {parser} from './bind-expr-impl';
+
+export function evaluateBindExpr(expr, data) {


Instead of evaling each time, can we have a two phase approach that parses the expression and produces a function that evaluates the AST?

I don't believe jison has an option to build ast

Jison doesn't support it natively, but I could build a custom one in the grammar. Not sure how much of a speed-up we'd get but I'll try it out.

cramforce · 2016-11-29T01:00:40Z

extensions/amp-bind/0.1/test/test-bind-expr.js

+  });
+
+  it('should NOT allow access to prototype properties', () => {
+    expect(evaluateBindExpr('{}.constructor')).to.be.null;


Also as bare expression: expect(evaluateBindExpr('constructor')).to.be.null;?

Good catch, thanks!

aghassemi · 2016-11-29T06:25:48Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+        }
+
+        if (prop === '[object String]') {
+          if (Object.prototype.hasOwnProperty.call($1, $2)) {


these three lines alone should be able handle all the cases, regardless of whether obj is an array or not. For array access, both arr[0] and arr['0'] are valid. Alsoarr.hasOwnProperty(0) and arr.hasOwnProperty('0') return true.

Good point, thanks!

It might, however, be much faster to keep the special cases since we are paying the price for the toString already. That way the VM can generated specialized code for each if.

aghassemi · 2016-11-29T06:45:14Z

extensions/amp-bind/0.1/test/test-bind-expr.js

+    expect(evaluateBindExpr('2 - 3.5')).to.equal(-1.5);
+    expect(evaluateBindExpr('3 * 4')).to.equal(12);
+    expect(evaluateBindExpr('4 / 5')).to.equal(0.8);
+    expect(evaluateBindExpr('5 % 4')).to.equal(1);


add test for divide by 0.

dreamofabear · 2016-11-29T16:46:45Z

How long does it take to execute 100 typical expressions on a Nexus 5X type device?

~100ms to evaluate 100 complex-ish expressions (8 operations, 1 function call, 2 member access) on Nexus 5X. About half that on iPhone 6S.

cramforce · 2016-11-29T17:42:33Z

Maybe we should design this to run in a Worker from day 1.

…

On Tue, Nov 29, 2016 at 8:46 AM, William Chou ***@***.***> wrote: How long does it take to execute 100 typical expressions on a Nexus 5X type device? ~100ms to evaluate 100 complex-ish expressions (8 operations, 1 function call, 2 member access) on Nexus 5X. About half that on iPhone 6S. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6201 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFeT7WDUU0Hv68sTI1khLOudZBAIa8jks5rDFb2gaJpZM4Ky76Y> .

jridgewell · 2016-11-29T18:35:57Z

How much of that 100ms is just the lexer? Ie, if we take away all the security code you wrote, how long does it take?

jridgewell · 2016-11-29T18:36:13Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+    expr
+      {$$ = [$1];}
+  | array ',' expr
+      {$$ = $1; Array.prototype.push.call($1, $3);}


$1.push($3)?

This is just a safety measure to make sure we call the intended method.

We create the array though? I don't think there's a way for them to set a property on it.

jridgewell · 2016-11-29T18:39:14Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+
+literal:
+    STRING
+      {$$ = yytext.substr(1, yyleng - 2);}


Why the substring?

Ignores the wrapping ' or ".

jridgewell · 2016-11-29T18:39:36Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+
+variable:
+    NAME
+      {$$ = Object.prototype.hasOwnProperty.call(yy, $1) ? yy[$1] : null;}


Since yy can be null, we'll have to guard that.

hasOwnProperty appears to return null in that case, which is correct.

It should throw an error.

Actually we want to return null instead of throwing undefined symbol error to be more forgiving (see PR summary).

jridgewell · 2016-11-29T18:40:41Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+function typeCheckArgs(args) {
+  for (var i = 0; i < args.length; i++) {
+    var arg = args[i];
+    if (Object.prototype.toString.call(arg) === '[object Object]') {


typeof arg === "object" && !Array.isArray(arg);

This isn't equivalent to the existing line. This will also catch null, Date and other objects.

jridgewell · 2016-11-29T18:42:31Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+
+        var obj = Object.prototype.toString.call($1);
+
+        var whitelist = functionWhitelist[obj];


Instead of toString, use an if:

var whitelist; if (typeof obj === "string") { } else if (Array.isArray(obj)) { }

We'll probably need to whitelist functions other than Array and String in the future though, so the flexibility is nice to keep. Also the performance difference seems negligible at N < 1000, so I'd rather do this fine-tuning later if necessary.

Fun fact: underscore.js also thoroughly debated the use of toString for type checking a few years ago.

jridgewell · 2016-11-29T18:43:05Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+        var whitelist = functionWhitelist[obj];
+        if (whitelist) {
+          var fn = $1[$3];
+          if (whitelist.indexOf(fn) >= 0) {


Since they're method on the prototype, names can't conflict. That makes it perfect for an prototype-less object map instead of an array.

whitelist[$3] === $1[$3];

Nice, this will avoid scanning the whitelist array. Done.

jridgewell · 2016-11-29T18:44:55Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+            if (typeCheckArgs($4)) {
+              $$ = fn.apply($1, $4);
+            } else {
+              throw new Error(`Unexpected argument type in {$3}()`);


This throw should be in typeCheckArgs()

Why? I think this is more readable since control flow remains within the parser's control. Note there's another error that can be thrown a few lines below this.

jridgewell · 2016-11-29T18:50:02Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+        }
+
+        var prop = Object.prototype.toString.call($2);
+        if (prop === '[object String]' || prop === '[object Number]') {


These type checks are likely unnecessary. We definitely need to enforce that it is not a Symbol, but we can do that much quicker at a higher level (forbid the key in the yy context object).

Fair, done.

I actually had to revert this change since hasOwnProperty throws a "cannot convert to primitive" error in some cases. Changed to use typeof instead of toString though, which should be faster.

jridgewell · 2016-11-29T18:50:31Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+
+        var prop = Object.prototype.toString.call($2);
+        if (prop === '[object String]' || prop === '[object Number]') {
+          if (Object.prototype.hasOwnProperty.call($1, $2)) {


Hoist Object#hasOwnProperty into a variable to avoid repeated lookups.

dvoytenko · 2016-11-29T18:26:29Z

extensions/amp-bind/0.1/bind-expr-impl.jison

@@ -0,0 +1,288 @@
+/** shared vars and functions */


Should we publish human grammar in a md file right away?

Also, this will need to be reviewed by security very soon.

Good idea. I'll add the documentation in a follow-up PR and ask @bpaduch for a review.

@molnarg reviewed the design and we're planning to have a formal security review once the feature is ready.

dvoytenko · 2016-11-29T18:27:02Z

extensions/amp-bind/0.1/test/test-bind-expr.js

+    expect(evaluateBindExpr('2 - 3.5')).to.equal(-1.5);
+    expect(evaluateBindExpr('3 * 4')).to.equal(12);
+    expect(evaluateBindExpr('4 / 5')).to.equal(0.8);
+    expect(evaluateBindExpr('5 % 4')).to.equal(1);


dvoytenko · 2016-11-29T18:29:34Z

extensions/amp-bind/0.1/test/test-bind-expr.js

+  it('should support strings', () => {
+    expect(evaluateBindExpr('"a"')).to.equal('a');
+    expect(evaluateBindExpr('"a".length')).to.equal(1);
+    expect(evaluateBindExpr('"a" + "b"')).to.equal('ab');


Will we allow interpolation. I still feel like interpolation is the most obvious way to build a string (e.g. URL) within the markup. We can follow JS here as well by using reverse quotes.

Agree that interpolation would be nice. Current plan is to add it after MVP.

I'm ok deferring. But we need to see the most prevalent cases while we are implementing it toward MVP. If it's URL construction, I believe, we need to do it for MVP.

dvoytenko · 2016-11-29T19:06:39Z

extensions/amp-bind/0.1/test/test-bind-expr.js

+
+  /** @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators */
+  it('should NOT allow access to non-whitelisted operators', () => {
+    expect(evaluateBindExpr('this')).to.be.null;


let's also add self and global

Good idea, done.

dvoytenko · 2016-11-29T19:08:26Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+      {$$ = $1 != $3;}
+  | expr '==' expr
+      {$$ = $1 == $3;}
+  | expr '?' expr ':' expr


Is there elvis too?

No since || offers the same functionality, but I don't have a strong opinion. Think we should add it?

Nope. || is good enough.

dvoytenko · 2016-11-29T19:08:54Z

extensions/amp-bind/0.1/test/test-bind-expr.js

+    expect(() => { evaluateBindExpr('delete foo', {foo: 0}); }).to.throw();
+  });
+
+  it('should NOT allow control flow or loops', () => {


Not seeing tests for ?:, but they seem to be allowed?

The grammar currently doesn't support ?: since an expr can't be empty.

Sorry, I meant tests for expr ? expr : expr.

Good point. Added.

dvoytenko · 2016-11-29T19:09:27Z

@choumx Wow! This grammar is way more powerful than I expected. But, I guess, why hide from it :)

dreamofabear · 2016-11-29T19:10:47Z

How much of that 100ms is just the lexer? Ie, if we take away all the security code you wrote, how long does it take?

The security code seems to be only a small fraction (< 5%) of the total runtime. I'm going to experiment with building an AST first -- hopefully that will boost performance substantially.

jridgewell · 2016-11-29T23:32:58Z

extensions/amp-bind/0.1/bind-expr-impl.jison

-      Array.prototype.lastIndexOf,
-      Array.prototype.slice,
-    ],
+    {


These must be prototype-less objects. As is, you've just allowed __defineGetter__, __defineSetter__, hasOwnProperty, isPrototypeOf, etc.

Good for us we have Justin on our side :) Lets also test them.

types.js#map can help with creating such an object from a literal.

Oh, good catch! Added a unit test for this.

I changed the whitelist to be created by a function. This is more readable than using types.j#map IMO and also prevents typos in the function name.

…efined member access

dreamofabear · 2016-11-30T18:09:21Z

Re: performance, I looked around and there are other parser generator options. This comparison suggests that we may get several-X speed up with another generator or by handwriting a parser. @jridgewell also suggested precompiling the AST into a Function (underscore and Angular do this).

I'll continue to investigate expression performance as I work on an end-to-end MVP for Bind.

cramforce · 2016-11-30T18:16:34Z

Lets get this in and then optimize. We can chat offline about details, but I'm pretty bullish about moving all of this into a web worker.

jridgewell · 2016-11-30T18:34:52Z

extensions/amp-bind/0.1/bind-expr-impl.jison

+    const functions = whitelist[type];
+    for (let i = 0; i < functions.length; i++) {
+      const f = functions[i];
+      out[type][f.name] = f;


The name property has spotty support in iOS and Android Browser (non chrome). Instead of that, we could use strings and grab the method off the prototype:

const whitelist = [ { class: '[object Array]', proto: Array.prototype, methods: ['concat', 'includes', ...] }, ];

For this simple usage of Function.name, it appears to be supported.

jridgewell · 2016-11-30T18:35:38Z

extensions/amp-bind/0.1/bind-expr-impl.jison

@@ -2,44 +2,57 @@

 %{
 // Shortcuts for common functions.
-var toString = Object.prototype.toString;
-var hasOwnProperty = Object.prototype.hasOwnProperty;
+const toString = Object.prototype.toString;


Is this compiled into es5?

dvoytenko · 2016-12-01T17:00:40Z

LGTM with one request for an additional test.

dreamofabear · 2016-12-01T17:21:00Z

Thanks for the reviews everyone!

* initial bind expressions commit * fix unary op bug, started unit tests * bug fixes, unit tests for arrays * more array tests * add unary plus, fix !=, add a lot more tests * remove useless comments * initial bind expressions commit * fix unary op bug, started unit tests * bug fixes, unit tests for arrays * more array tests * add unary plus, fix !=, add a lot more tests * remove useless comments * throw error for non-whitelisted functions, return null for nested undefined member access * more unit tests * minor formatting fixes * ignore compiled bind-expr-impl.js during lint * fix lint errors in test-bind-expr.js * fix presubmit: remove ES6 startsWith/endsWith and whitelist test file * add type check for function args and unit test * expect specific error for unsupported function tests * address PR comments * more PR comments * add test for prototype fns, fix other tests * fix broken string templating in parse errors * use const/let instead of var * use a function to create whitelist map * fix lint errors * add test for ternary op

dreamofabear assigned dvoytenko and aghassemi Nov 15, 2016

dreamofabear added the NEEDS REVIEW label Nov 15, 2016

dreamofabear force-pushed the bind-expressions branch 2 times, most recently from 45a3403 to e4e1051 Compare November 16, 2016 21:36

jridgewell approved these changes Nov 28, 2016

View reviewed changes

cramforce reviewed Nov 29, 2016

View reviewed changes

aghassemi approved these changes Nov 29, 2016

View reviewed changes

dreamofabear force-pushed the bind-expressions branch from 78be64d to 8433be5 Compare November 29, 2016 14:23

dreamofabear force-pushed the bind-expressions branch 2 times, most recently from 0af6628 to 4cd66a8 Compare November 29, 2016 17:26

jridgewell reviewed Nov 29, 2016

View reviewed changes

dvoytenko suggested changes Nov 29, 2016

View reviewed changes

jridgewell reviewed Nov 29, 2016

View reviewed changes

William Chou added 8 commits November 30, 2016 11:40

initial bind expressions commit

b60fb11

fix unary op bug, started unit tests

bef964e

bug fixes, unit tests for arrays

a9e3b7c

more array tests

afb373f

add unary plus, fix !=, add a lot more tests

3064b30

remove useless comments

b3c7849

initial bind expressions commit

cea9cf6

fix unary op bug, started unit tests

e589fea

William Chou added 16 commits November 30, 2016 11:42

remove useless comments

265a651

throw error for non-whitelisted functions, return null for nested und…

f8f0053

…efined member access

more unit tests

e7923d8

minor formatting fixes

81f54d8

ignore compiled bind-expr-impl.js during lint

9a4109b

fix lint errors in test-bind-expr.js

b28db51

fix presubmit: remove ES6 startsWith/endsWith and whitelist test file

bfb3765

add type check for function args and unit test

904a595

expect specific error for unsupported function tests

b6ea877

address PR comments

7faa276

more PR comments

6b8691a

add test for prototype fns, fix other tests

e7851fe

fix broken string templating in parse errors

de8d923

use const/let instead of var

150a630

use a function to create whitelist map

29f85be

fix lint errors

b97245d

jridgewell reviewed Nov 30, 2016

View reviewed changes

dvoytenko approved these changes Dec 1, 2016

View reviewed changes

dreamofabear force-pushed the bind-expressions branch from 2f30a57 to 2db7582 Compare December 1, 2016 17:20

add test for ternary op

74f2fb9

dreamofabear force-pushed the bind-expressions branch from 2db7582 to 74f2fb9 Compare December 1, 2016 23:02

dreamofabear removed the NEEDS REVIEW label Dec 2, 2016

dreamofabear merged commit 81d7f96 into ampproject:master Dec 2, 2016

dreamofabear deleted the bind-expressions branch December 2, 2016 00:34


		import {parser} from './bind-expr-impl';

		export function evaluateBindExpr(expr, data) {


		var obj = Object.prototype.toString.call($1);

		var whitelist = functionWhitelist[obj];

Bind expressions #6201

Bind expressions #6201

Conversation

dreamofabear commented Nov 15, 2016

aghassemi commented Nov 28, 2016

jridgewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Nov 29, 2016

cramforce commented Nov 29, 2016 via email

jridgewell commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Nov 29, 2016

dreamofabear commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Nov 30, 2016

cramforce commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Dec 1, 2016

dreamofabear commented Dec 1, 2016

dreamofabear Nov 29, 2016 •

edited

Loading

dreamofabear Nov 30, 2016 •

edited

Loading