Idea: assignment on array using indices (a[idx] = value)

[anderium]

It's possible to extend an array using || as the docs say, but either I'm dumb, or it's not yet possible to use index assignment on arrays like a[1] = value. I think it would be nice if this was possible too, perhaps as an option that can be turned off.

I would try to create a PR, but I haven't any experience with good JavaScript development. I also haven't worked on software before, so I only got here through an app I'm using.

Research I did, see below for a better explanation and possible solution

I did look into the parser and I can see that it should be possible to use . expressions for variable assignment. The check for IMEMBER here does allow this because the IMEMBER type is added here.
The type added for [i] indexing is IOP as can be seen from the binaryInstruction() function called here.

If I look at the evaluation() function I believe it might be kinda difficult to support this, as both ways of indexing actually push the value of the variable unto the nstack before the assignment. Check

expr-eval/src/evaluate.js

Lines 101 to 103 in a556e27

	} else if (type === IMEMBER) {
	n1 = nstack.pop();
	nstack.push(n1[item.value]);

for . indexing and

expr-eval/src/evaluate.js

Lines 29 to 31 in a556e27

	} else {
	f = expr.binaryOps[item.value];
	nstack.push(f(resolveExpression(n1, values), resolveExpression(n2, values)));

for [i] indexing.
After these instructions, which don't generate references but values, have been evaluated the assignment is done:

expr-eval/src/evaluate.js

Lines 19 to 28 in a556e27

	} else if (type === IOP2) {
	n2 = nstack.pop();
	n1 = nstack.pop();
	if (item.value === 'and') {
	nstack.push(n1 ? !!evaluate(n2, expr, values) : false);
	} else if (item.value === 'or') {
	nstack.push(n1 ? true : !!evaluate(n2, expr, values));
	} else if (item.value === '=') {
	f = expr.binaryOps[item.value];
	nstack.push(f(n1, evaluate(n2, expr, values), values));

The function executed in this loop is setVar:

expr-eval/src/functions.js

Lines 248 to 251 in a556e27

	export function setVar(name, value, variables) {
	if (variables) variables[name] = value;
	return value;
	}

As far as I can see this function actually only changes the list of variables, which means even using a . assignment wouldn't work. If I look at the console, I think I actually see which errors appear when trying to do this impossible assignment. For the string "a=[0,1];a[1]=2" I get the error expected variable for assignment as a[1] is neither an IFUNCALL, an IVAR or an IMEMBER. For the string "a=[0,1];a.foo=2" I get the error invalid Expression (parity) which indicates the nstack has more than one element at the end of the evaluation of the instruction.^[1]

---

I totally didn't write this issue as I was figuring out the code :). So I might have missed some things or it's not clear here.
However, having looked at all this, I believe it would be very difficult to add support for assignment on array indices OR attributes. It would at least require a rework of variable assignment and perhaps of indexing too.

[anderium]

Oh, I just realised that there's a slight workaround for the array index assignment. The following expression changes only one element of an array:

a = [0, 1];
f(y, x, i) = y || [(i==idx) ? value : x];
fold(f, [], a);

Where idx is the index you want to set and value to what you want to set it.
Of course, this workaround doesn't work for setting attributes. It actually removes all attributes from the array as it creates an entirely new one, it doesn't change the array.
The fact that it creates a new array also means that it won't work if you need the changed array in another expression unless the changed array is stored as well.

[anderium]

Possible solution

Alright, I decided to try and make something that solves this, and I think I've got something that should work for most cases!
I'm not sure if it solves all problems, but it at least makes array/object manipulation possible.

I've attached two patch files that can change the code because I'm not sure of the quality of my edits. With npm you can add the following to your package.json file:

{
  "scripts": {
    "postinstall": "patch --forward node_modules/expr-eval/bundle.js < obj-assignment-bundle.patch && patch --forward node_modules/expr-eval/index.mjs < obj-assignment-index.patch"
  }
}

(This does not include a patch for the minimised version in dist/bundle.min.js.)

The patch files are obj-assignment-bundle.txt and obj-assignment-index.txt. You might want to change the extension to .patch to get syntax highlighting if it's supported. Github didn't allow me to upload with such an extension.

What was the problem?

When setting a variable it's added to an object, often referenced by values or variables, in which the values for each variable are searched. This works when you want to create a normal variable, but when changing properties on an object what happens internally is equivalent to objectValue = new_value instead of object[key] = new_value. It doesn't change the variable but tries to assign the new value to a literal instead. I believe that it would actually create a variable with the value of this literal as a name!

How I tried to make it work

Instead of expanding indexed variables all the way, we need to stop before the final expansion. Where I use expansion to refer to a change from object.property or object[key] to objectValue. When setting a variable we then have to check whether we're dealing with a variable of which the value has to be set, or if we're dealing with an object of which we want to change a property.

To prevent the final expansion I added a new instruction type, IMEMBEREXPR. When parsing expressions an encountered = removes the last instruction from the internal stack. This instruction can be one of 4 things for a correct assignment:

1. IFUNCALL when defining a function. This is unrelated to our problem.
2. IVAR when setting the value of a single variable. This was already possible.
3. IMEMBER when setting the property on an object. This was checked for but had problems when actually evaluating the assignment.
4. IOP2, specifically with value '[', a "member expression". This was not checked for which made object[key]=value a syntax error (parity).

The first two assignments are already handled correctly. IFUNCALL changes to an IFUNDEF and IVAR becomes IVARNAME. The last two assignments require the prevention of an expansion, which is where the IMEMBEREXPR comes in.^Note Both of these are solved mostly during evaluation. During parsing the IMEMBER instruction is replaced with IMEMBEREXPR in the same way how IVAR is replaced by IVARNAME. The '[' instruction is replaced by an IMEMBEREXPR with a single dot as value. Properties accessed with a dot can't start with a dot, so a single dot is 100% an indication of the IOP2 member expression.

In the IMEMBER case, the property name is added to the stack with a . prepended. Variables can't start with a dot, so when an assignment on a string starting with . is found, we know we are actually dealing with an assignment to the property of an object. The object now at the top of the stack is the one on which this assignment should be done.

In the IOP2 member expression case, we don't need to put everything within the brackets into its own subexpression, because the object name will be resolved correctly.^Note After this has been evaluated, a dot is prepended to its value for the same reason as before. The property can actually start with a dot here too and that is also handled correctly.

The actual assignment then has to check if the string starts with a . and in those cases fetches the actual object from the stack as well as the already fetched property. Within the setVar function we then do some checks to execute the correct assignment. There are probably only two possible combinations: nameOrObj is a string and property is undefined or nameOrObj is an object and property is defined. Even so, I added checks for the other combinations too.

Security concerns with this method

See #251 for what could become more of a concern with this method.

Some older commentary before I saw issue 251

This method also does NOT allow users to change or access the constructor of objects. This is because a TNAME is expected, but these properties are seen as TOP's!

expr-eval/src/parser-state.js

Line 321 in a556e27

this.expect(TNAME);

The test "constructor" in this.binaryOps results in true, making the token a TOP even if it would also be seen as a TNAME. For a similar reason this.isOperatorEnabled("constructor") is also guaranteed to be true.

expr-eval/src/token-stream.js

Line 151 in a556e27

if (this.isOperatorEnabled(str) && (str in this.binaryOps || str in this.unaryOps || str in this.ternaryOps)) {

The tokenisation chooses to interpret it this way because of short-circuiting here:

expr-eval/src/token-stream.js

Lines 46 to 48 in a556e27

	this.isNamedOp() ||
	this.isConst() ||
	this.isName()) {

However, this might allow for a way of prototype pollution even if I did not find one. This article on medium links to this vulnerability that has a similar "path assignment based prototype pollution vulnerability". Using the fix linked I'll try to mitigate prototype pollution here for certain.

The code

Below the 3 functions that are different, just so that it might be easier to read than the patch files (which are stored as diffs). The evaluate function is very long because the original is.

Show the full functions…

const IMEMBEREXPR = 'IMEMBEREXPR';

ParserState.prototype.parseVariableAssignmentExpression = function (instr) {
  this.parseConditionalExpression(instr);
  while (this.accept(TOP, '=')) {
    var varName = instr.pop();
    var varValue = [];
    var lastInstrIndex = instr.length - 1;
    if (varName.type === IFUNCALL) {
      if (!this.tokens.isOperatorEnabled('()=')) {
        throw new Error('function definition is not permitted');
      }
      for (var i = 0, len = varName.value + 1; i < len; i++) {
        var index = lastInstrIndex - i;
        if (instr[index].type === IVAR) {
          instr[index] = new Instruction(IVARNAME, instr[index].value);
        }
      }
      this.parseVariableAssignmentExpression(varValue);
      instr.push(new Instruction(IEXPR, varValue));
      instr.push(new Instruction(IFUNDEF, varName.value));
      continue;
    }
    if (varName.type === IOP2 && varName.value === '[') {
      this.parseVariableAssignmentExpression(varValue);
      instr.push(new Instruction(IMEMBEREXPR, '.'));
      instr.push(new Instruction(IEXPR, varValue));
      instr.push(binaryInstruction('='));
      continue;
    }
    if (varName.type === IMEMBER) {
      this.parseVariableAssignmentExpression(varValue);
      instr.push(new Instruction(IMEMBEREXPR, varName.value));
      instr.push(new Instruction(IEXPR, varValue));
      instr.push(binaryInstruction('='));
      continue;
    }
    if (varName.type === IVAR) {
      this.parseVariableAssignmentExpression(varValue);
      instr.push(new Instruction(IVARNAME, varName.value));
      instr.push(new Instruction(IEXPR, varValue));
      instr.push(binaryInstruction('='));
      continue;
    }
    throw new Error('expected variable for assignment');
  }
};

function evaluate(tokens, expr, values) {
  var nstack = [];
  var n1, n2, n3;
  var f, args, argCount;

  if (isExpressionEvaluator(tokens)) {
    return resolveExpression(tokens, values);
  }

  var numTokens = tokens.length;

  for (var i = 0; i < numTokens; i++) {
    var item = tokens[i];
    var type = item.type;
    if (type === INUMBER || type === IVARNAME) {
      nstack.push(item.value);
    } else if (type === IOP2) {
      n2 = nstack.pop();
      n1 = nstack.pop();
      if (item.value === 'and') {
        nstack.push(n1 ? !!evaluate(n2, expr, values) : false);
      } else if (item.value === 'or') {
        nstack.push(n1 ? true : !!evaluate(n2, expr, values));
      } else if (item.value === '=') {
        f = expr.binaryOps[item.value];
        let property;
        if (typeof n1 === 'string' && n1.startsWith('.')) {
          property = n1.slice(1);
          n1 = nstack.pop();
        }
        nstack.push(f(n1, evaluate(n2, expr, values), values, property));
      } else {
        f = expr.binaryOps[item.value];
        nstack.push(f(resolveExpression(n1, values), resolveExpression(n2, values)));
      }
    } else if (type === IOP3) {
      n3 = nstack.pop();
      n2 = nstack.pop();
      n1 = nstack.pop();
      if (item.value === '?') {
        nstack.push(evaluate(n1 ? n2 : n3, expr, values));
      } else {
        f = expr.ternaryOps[item.value];
        nstack.push(f(resolveExpression(n1, values), resolveExpression(n2, values), resolveExpression(n3, values)));
      }
    } else if (type === IVAR) {
      if (item.value in expr.functions) {
        nstack.push(expr.functions[item.value]);
      } else if (item.value in expr.unaryOps && expr.parser.isOperatorEnabled(item.value)) {
        nstack.push(expr.unaryOps[item.value]);
      } else {
        var v = values[item.value];
        if (v !== undefined) {
          nstack.push(v);
        } else {
          throw new Error('undefined variable: ' + item.value);
        }
      }
    } else if (type === IOP1) {
      n1 = nstack.pop();
      f = expr.unaryOps[item.value];
      nstack.push(f(resolveExpression(n1, values)));
    } else if (type === IFUNCALL) {
      argCount = item.value;
      args = [];
      while (argCount-- > 0) {
        args.unshift(resolveExpression(nstack.pop(), values));
      }
      f = nstack.pop();
      if (f.apply && f.call) {
        nstack.push(f.apply(undefined, args));
      } else {
        throw new Error(f + ' is not a function');
      }
    } else if (type === IFUNDEF) {
      // Create closure to keep references to arguments and expression
      nstack.push(
        (function () {
          var n2 = nstack.pop();
          var args = [];
          var argCount = item.value;
          while (argCount-- > 0) {
            args.unshift(nstack.pop());
          }
          var n1 = nstack.pop();
          var f = function () {
            var scope = Object.assign({}, values);
            for (var i = 0, len = args.length; i < len; i++) {
              scope[args[i]] = arguments[i];
            }
            return evaluate(n2, expr, scope);
          };
          // f.name = n1
          Object.defineProperty(f, 'name', {
            value: n1,
            writable: false
          });
          values[n1] = f;
          return f;
        })()
      );
    } else if (type === IEXPR) {
      nstack.push(createExpressionEvaluator(item, expr));
    } else if (type === IEXPREVAL) {
      nstack.push(item);
    } else if (type === IMEMBEREXPR) {
      if (item.value === '.') {
        n1 = nstack.pop();
        nstack.push('.' + n1);
      } else {
        nstack.push('.' + item.value);
      }
    } else if (type === IMEMBER) {
      n1 = nstack.pop();
      nstack.push(n1[item.value]);
    } else if (type === IENDSTATEMENT) {
      nstack.pop();
    } else if (type === IARRAY) {
      argCount = item.value;
      args = [];
      while (argCount-- > 0) {
        args.unshift(nstack.pop());
      }
      nstack.push(args);
    } else {
      throw new Error('invalid Expression');
    }
  }
  if (nstack.length > 1) {
    throw new Error('invalid Expression (parity)');
  }
  // Explicitly return zero to avoid test issues caused by -0
  return nstack[0] === 0 ? 0 : resolveExpression(nstack[0], values);
}

function setVar(nameOrObj, value, variables, property) {
  if (typeof nameOrObj === 'string') {
    if (variables) {
      if (property) {
        variables[nameOrObj][property] = value;
      } else {
        variables[nameOrObj] = value;
      }
    }
  } else {
    if (property) {
      nameOrObj[property] = value;
    } else {
      throw new Error('Cannot change value of reference to array or object');
    }
  }
  return value;
}