feat(permissions): allow env permission to take values

[crowlKats]

closes #3137

[kt3k]

I tried simple script like the below on my end with and without combinations of perm flags.

await Deno.permissions.request({ name: 'env', variable: 'FOO' })
console.log(Deno.env.get('FOO'));

The basic feature seems working to me. Good feature to have! 👍

[bartlomieju]

In general this PR looks good to me, but I have one comment that makes me wanna skip it for 1.9: with this change --allow-env allows to provide a list of variables that can be read and set - I'm thinking that maybe we should split this flag into to, one for reading and one for setting (similar to --allow-read and --allow-write).

Before making decision I will wait for other contributors opinions.

[lucacasonato]

I'm thinking that maybe we should split this flag into to, one for reading and one for setting

I don't think this is needed as setting env vars only has an effect on you, and your processes children, but not siblings or parent processes.

[nayeemrmn]

I don't think this is needed as setting env vars only has an effect on you, and your processes children, but not siblings or parent processes.

Hmm, doesn't that also mean that variables should be settable without permission and allowlisted thereafter?

[bartlomieju]

I don't think this is needed as setting env vars only has an effect on you, and your processes children, but not siblings or parent processes.

Hmm, doesn't that also mean that variables should be settable without permission and allowlisted thereafter?

@nayeemrmn I don't understand the question, could you provide an example?

[nayeemrmn]

I realised since setting env variables within the process doesn't affect the outside, there's no reason for it to require permission.

Deno.env.get("foo"); // Requires `--allow-env=foo`.

Deno.env.set("foo", "bar"); // Shouldn't require permission.
Deno.env.get("foo"); // Shouldn't require permission.

[bartlomieju]

I realised since setting env variables within the process doesn't affect the outside, there's no reason for it to require permission.

Deno.env.get("foo"); // Requires `--allow-env=foo`.

Deno.env.set("foo", "bar"); // Shouldn't require permission.
Deno.env.get("foo"); // Shouldn't require permission.

@nayeemrmn so this suggestion alters current behavior, which would be a breaking change. I'm also not sure how would it be implemented, but I'm more than happy to discuss it for 2.0.

[nayeemrmn]

If you take away a permission gate because it's not actually a security issue, I'm not sure it's a breaking change. But either way it certainly doesn't need to be part of this PR 👍

(EDIT: I suppose it would be breaking for workers? ¯_(ツ)_/¯)

[ry]

LGTM - nice work @crowlKats !

[piscisaureus]

I think sufficient effort was put into this PR to address Windows quirks.

Of course, it is not a given that windows uses case folding that is 100% compatible with .toUpperCase(). Who knows, this could lead to some obscure bugs down the line, or not.

@bartlomieju