deno fix(ext/node): Removed Deno Permissions for Env Variables (temp message: for 1.41, do not move to next)

Fixed remove Deno permissions for Env variables. Fixed Issue #18665

Aug 20 '23 06:08 itsajay1029

All committers have signed the CLA.

Aug 20 '23 06:08 CLAassistant

@ry @bartlomieju We now have A option (Allow all same kind permissions, in this case all 'env') in permission prompt. I think removing this check is now reasonable. What do you think?

Aug 21 '23 09:08 kt3k

@kt3k Please merge the PR

Aug 26 '23 06:08 itsajay1029

@kt3k I'll review this week, need to better understand problem at hand

Aug 26 '23 11:08 bartlomieju

@bartlomieju More context. We wanted to reduce the number of env permission prompts in node compat (invoked from process.env[NAME]) in the past because, for example, npm:express asks excessive number of envs (like ~10, and most are somethings nobody cares about) and it was very annoying. We started checking permission before actually calling Deno.env.get in https://github.com/denoland/deno_std/pull/3178, and only access to it when permission is already given. That resulted in another issue such as #18665

Aug 28 '23 05:08 kt3k

@itsajay1029 thanks! I think this is a good change to do. The CI is pointing out that some tests are now failing due to this change though. Would you be able to fix them? Once those tests are fixed then we'll be able to land this.

Dec 05 '23 20:12 dsherret

@itsajay1029 are you still wanting to contribute this one? There are some tests failures that just need to be fixed. I can take it over if not. It's unfortunate this one didn't get in the 1.40 release so it will have to land in 1.41 now.

Jan 25 '24 14:01 dsherret

I'll take over this PR to get it landed today.

Feb 20 '24 18:02 dsherret

Oh wait, just noticed https://github.com/denoland/deno/pull/22487 opened

Feb 20 '24 18:02 dsherret

This was superceeded by the approach in #22487 to permission prompt, but not throw.

Feb 20 '24 21:02 dsherret