Deny setting operations on __ENV variables from JS code

[mstoykov]

Update — the current agreement from the comments:

https://github.com/grafana/k6/issues/1722#issuecomment-796752500

So, yeah, I think we should either close this issue and https://github.com/grafana/k6/pull/1877, or "fix" it by freezing __ENV so any attempts to change it won't silently fail, but would throw an exception. The script pattern could be used for having default values of script options, instead of modifying __ENV:

const myScriptOption = (typeof __ENV.something !== 'undefined') ? __ENV.something : 'defaultValue';

This has been broken for a while, but very few people have complained and there's an easy workaround shown above, so my vote is for "fixing" the current behavior in the sense of making it permanent and explicit by freezing __ENV 😅

Check the original issue description

if (typeof __ENV.something == "undefined")  {
 __ENV.something= "test";
}
export default function () {
    console.log(__ENV.something);
}

This code should print "test" and it did up to https://github.com/loadimpact/k6/commit/e1a5c0af001977b6a10945791f5b16288937d391 and if the golang version is go1.13.8 but not if it is 1.15.3 (or 1.14.6)

This is fairly common pattern to fix your __ENV variables in cases where you didn't provide them but you use them through the script.

[vhanich-es]

Confirmed, I have the same issue.

[mstoykov]

The breakage due to the go version seems to be a caching issue - I can't reproduce it and the k6 logo in the second screenshot looks different and we did that and was added in https://github.com/loadimpact/k6/pull/1615 which was a few months after e1a5c0a.

Looking at the screenshots I don't know what I did wrong, but this is good news :rofl:

[na--]

After thinking more about this, reviewing https://github.com/loadimpact/k6/pull/1877, and discussing it a bit on Slack, I am of the opinion that we shouldn't "fix" this. That is, I think that __ENV should not be modifiable from inside of the script, at all. Only the setting of actual environment variables (if --include-system-env-vars is enabled) and --env should be able to modify the contents of __ENV.

My reasons for that are that there are a ton of corner cases that have to be handled here, for some dubious benefit. For example:

• __ENV keys and values should be strings, but what happens when someone tries to do something like __ENV[Symbol('a')] = 12.3? How about __ENV[foo] = function() { /* ... */ }?
• if you have custom logic in each init context, every VU (or k6 instance, in a distributed test) might have different environment variables, which is probably not what we want
• when you execute a script bundle, you usually expect the environment variables in the metadata.json file to be the ones that are used, but this might break that expectation

So, yeah, I think we should either close this issue and https://github.com/loadimpact/k6/pull/1877, or "fix" it by freezing __ENV so any attempts to change it won't silently fail, but would throw an exception. The script pattern could be used for having default values of script options, instead of modifying __ENV:

const myScriptOption = (typeof __ENV.something !== 'undefined') ? __ENV.something : 'defaultValue';

This has been broken for a while, but very few people have complained and there's an easy workaround shown above, so my vote is for "fixing" the current behavior in the sense of making it permanent and explicit by freezing __ENV :sweat_smile:

[mstoykov]

yeah after we talked about it I also am of the opinion that "freezing" it is better. It will likely not be object.Freeze though ;), but goja.DynamicObject as we need to change environment variables in a VU between different scenarios.

[na--]

I'm removing this from the v0.32.0 milestone and marking it as not a bug then.

[mstoykov]

Also adding here that currently we keep setting a new __ENV object on each activation of the VU, while this probably should just change the underlying object

https://github.com/grafana/k6/blob/c949fd87d2c650a2e397e760a4d8a4a68b5f232d/js/runner.go#L669

This currently also technically can return an error which is not checked adn the ActivateVU is not able to return error at this point. So removing thsi might also be beneficial.