Environment#init extends the opts arg in-place, leading to side-effects

[zrhmn]

As it says in the title, the Environment class is defined in /nunjucks/src/environment.js. Here is a copy of the code for the Environment class from master.

/* 40  */ class Environment extends Obj {
/* 41  */   init(loaders, opts) {
/* ... */
/* 48  */     opts = this.opts = opts || {};  // <--- Problem starts here...
/* 49  */     this.opts.dev = !!opts.dev;
/* ... */
/* 55  */     this.opts.autoescape = opts.autoescape != null ? opts.autoescape : true;
/* ... */
/* 59  */     this.opts.throwOnUndefined = !!opts.throwOnUndefined;
/* 60  */     this.opts.trimBlocks = !!opts.trimBlocks;
/* 61  */     this.opts.lstripBlocks = !!opts.lstripBlocks;
/* ... */
/* 96  */   }
/* ... */
/* 325 */ }

Now, if I have a setup like:

const assert = require('assert');
const {Environment, FileSystemLoader} = require('nunjucks');
// ...
const config = {
  // ...
  nunjucksOpts: {autoescape: null, trimBlocks: true, /* etc. */}
};
// ...
const env0 = new Environment(new FileSystemLoader(/* ... */), config.nunjucksOpts);
// ...
assert(config.nunjucksOpts.autoescape === true, 'side-effect caught'); // ref: a
config.nunjucksOpts.autoescape = false; // ref: b
// ...
const env1 = new Environment(new FileSystemLoader(/* ... */), config.nunjucksOpts);

You can see how that's a problem, as the first time config.nunjucksOpts was passed to the constructor, the constructor mutated it, and assigned it to itself. When there's an object on the RHS of the operator, JavaScript actually stores a reference to the object, instead of an instance of it. Any changes that Environemnt#init then makes to Environment#opts are reflected in the original config.nunjucksOpts object passed to the constructor (ref a in the example above). Also, if config.nunjucksOpts is then updated by the client code (ref b in the example above), it affects env0 too. So in the above example, env0 and env1 end up being configured with identical options, even though it might not be obvious.

Now, I will concede that the approach taken in the example I provided above is naive. And perhaps it is the client code should be doing something like this to achieve correct behavior:

const env0 = new Environment(new FileSystemLoader(/* ... */, Object.assign({}, config.nunjucksOpts);

However, I still think that it is primarily the responsibility of the constructor (or the Environment#init method which is called by Obj#constructor that Environment extends) to ensure that instead of storing a reference to opts a copy of the opts arg is stored instead. And this safeguard is not complicated to implement. In the implementation of Environment#init, we can:

// opts = this.opts = opts || {}; // instead of this
opts = this.opts = Object.assign({}, opts || {})

I cannot think of any negative side-effects of this change. If anyone can see how that would break something, I'll be happy to rethink it. Otherwise, I can submit a merge request with that single line-of-code change. I'm willing to discuss this here.

I haven't gone through the rest of the code, but there may be other constructors with the same issue, and I think those should be updated too.

Edit: fix formatting 🤦‍♂

[ArmorDarks]

I do not think it is a responsibility of the constructor to clone options. It was a deliberate developer's choice to pass an exactly same reference to two environments. Do not forget that by the reference mutations can be used in some cases for good too.

Besides, if options are huge, deep cloning will take a lot of time. I think it's easier to put the decision about whether it needed on the shoulders of the developer.

opts = this.opts = Object.assign({}, opts || {})

This will work only for non-nested objects (shallow cloning). Second level and above will be passed by reference.

[fdintino]

Since only the top-level attributes are mutated, it should be sufficient to do a shallow copy in order to avoid side-effects. This seems like a legitimate, albeit minor, bug.