Add flag for filtering env-variables on Deno.env access

[mechanoid]

While it is great that --allow-env=* raises an exception, when a program uses a not permitted env variable, I think in contrast to other --allow-* Permissions of deno run the pattern is not enough for env access.

In the wild many libraries want to access possibly more Env-Variables than a programm writer want to grant access to. Especiall when you see lib-authors see using Deno.env.toObject() or similar. This results oft in providing access to all the Environment via --allow-env possibly introducing more security vulnerabilities.

I would love to see a deno run option like --grant-env (or something similar), that actually silently does not give access to a variable without failing.

Especially with the npm world becoming more accessible, the deno permission spirit faces lots of libraries written in a different mindset, eagerly reading all env variables without hesitation. I think a solution as described would improve the security of deno apps a lot.

As an example:

export PORT=3001;
echo "console.log(Deno.env.get('PORT'))" | deno run --allow-env=PORT - # => 3001
echo "console.log(Deno.env.get('PORT'))" | deno run - # fails with 'PermissionDenied: Requires env access to "PORT"'

echo "console.log(Deno.env.get('PORT'))" | deno run --grant-env=DEBUG --allow-env - # => null
echo "console.log(Deno.env.get('PORT'))" | deno run --grant-env=PORT --allow-env -  # => 3001

So --allow-env would allow a user to ask for PORT, but when --grant-env is specified, but not defining PORT, it is silently empty and result in an empty string.

[mechanoid]

Hi @kt3k, as you have shown interest in the restrictEnvAccess setting of dotenv earlier, I just wanted to invite you to this discussion. I would highly value your feedback on the problem and on the described idea. Thanks in advance!

[mechanoid]

I'm not a rust dev, but from I see such thing could be implemented in wrapping the collecting of the env vars after the permission check:

deno/runtime/ops/os.rs

Line 105 in cc32a29

Ok(env::vars().collect())

[kt3k]

@mechanoid Thanks for the suggestions!

--grant-env=XYZ looks interesting to me. What happens if --grant-env=DEBUG is specified but --allow-env is not specified? Does it make sense to make --grant-env=DEBUG implying --allow-env implicitly?

Especially with the npm world becoming more accessible, the deno permission spirit faces lots of libraries written in a different mindset, eagerly reading all env variables without hesitation.

Right, we also observe that a lot of npm modules access a lot of env vars even when they are not necessary for them to work correctly.

Currently we also consider adding options like Deno.env.get("FOO", { prompt: false }) / Deno.env.toObject({ prompt: false }) which don't ask permissions to users, but throws instead (and we can use them in getter of process.env)

[mechanoid]

Hey,

If --allow-env is not specified at all one can set --allow-env={GRANTED VARS} implicitly, and I guess that would be a better UX as letting people define the same thing twice 👍

The prompt option seems also a really good idea, but again it leaves people without possibility to use those packages that ask for too much. I think a combination of both would be great though!

Thanks a lot for your feedback, and happy to be able to contribute!

[mechanoid]

Hi @kt3k , @bartlomieju , sorry to interrupt you and approach you this directly. I think this topic is rather critical, as the adoption of npm is increasing. Is there a better way to address/track this, as a discussion? Thx and best regards!

[kt3k]

We are considering to add sync version of Deno.permissions.query with prompt: boolean option (prompt: false disables prompt feature, and returns denied state when permission is not granted), and use it like the below in process.env implementation:

function denoEnvGet(envName) {
  if (Deno.permissions.querySync({ name: "env", variable: envName }).state === "granted") {
    return Deno.env.get(envName);
  }
}

(But we haven't completely decided on this plan yet. Feedbacks are welcome)

[mechanoid]

hey @kt3k, thanks for your quick response!

But I think that this does not address the same problem. This would allow to check if you can access an Env Var at runtime, but I think the major problem is, that there is a huge deal of npm packages out there, that are overly obsessive about reading env variables. As long as you need to rely on such dependency you will end up with a generic --allow-env runtime permisson,
effectively disabling env-var checks for your complete program.

[nandorojo]

In a bit of a similar fashion, I'm running into this issue with firebase-auth. Like many Node libraries, it does some arbitrary checks to process.env. I don't want to give it access to those variables. However, I am fine with it just erroring silently. For example, process.env.HOME etc could just return undefined for the library.

This is common as libraries often do if (process.env.SOME_VAR) doThis(). And I'd like to be able to use those libraries without enabling the env variables they want necessarily.

[mechanoid]

Exactly, this issue is for me still very relevant as it potentially is an unnecessary security risk