permission: refactor permission checks and improve test assertions for environment variables

Signed-off-by: nabeel378 <[email protected]>

Author: nabeel378

src/node_env_var.cc

@@ -465,11 +465,10 @@ static Intercepted EnvSetter(Local<Name> property,
 
   if (property->IsString()) {
     Utf8Value key(env->isolate(), property);
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env,
-        permission::PermissionScope::kEnvVar,
-        key.ToStringView(),
-        Intercepted::kYes);
+    THROW_IF_INSUFFICIENT_PERMISSIONS(env,
+                                      permission::PermissionScope::kEnvVar,
+                                      key.ToStringView(),
+                                      Intercepted::kYes);
   }
 
   // calling env->EmitProcessEnvWarning() sets a variable indicating that
@@ -533,11 +532,10 @@ static Intercepted EnvDeleter(Local<Name> property,
   CHECK(env->has_run_bootstrapping_code());
   if (property->IsString()) {
     Utf8Value key(env->isolate(), property);
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env,
-        permission::PermissionScope::kEnvVar,
-        key.ToStringView(),
-        Intercepted::kYes);
+    THROW_IF_INSUFFICIENT_PERMISSIONS(env,
+                                      permission::PermissionScope::kEnvVar,
+                                      key.ToStringView(),
+                                      Intercepted::kYes);
 
     env->env_vars()->Delete(env->isolate(), property.As<String>());
 
@@ -564,10 +562,9 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
         Local<Value> elem;
         if (!ret->Get(env->context(), i).ToLocal(&elem)) continue;
         Utf8Value key(env->isolate(), elem);
-        if (env->permission()->is_granted(
-                env,
-                permission::PermissionScope::kEnvVar,
-                key.ToStringView())) {
+        if (env->permission()->is_granted(env,
+                                          permission::PermissionScope::kEnvVar,
+                                          key.ToStringView())) {
           filtered.push_back(elem);
         }
       }

src/permission/permission.h

@@ -6,14 +6,14 @@
 #include "debug_utils.h"
 #include "node_diagnostics_channel.h"
 #include "node_options.h"
-#include "permission/permission_base.h"
 #include "permission/addon_permission.h"
 #include "permission/child_process_permission.h"
 #include "permission/env_var_permission.h"
 #include "permission/ffi_permission.h"
 #include "permission/fs_permission.h"
 #include "permission/inspector_permission.h"
 #include "permission/net_permission.h"
+#include "permission/permission_base.h"
 #include "permission/wasi_permission.h"
 #include "permission/worker_permission.h"
 #include "v8.h"

src/permission/permission_base.h

@@ -36,8 +36,7 @@ namespace permission {
   V(Addon, "addon", PermissionsRoot, "--allow-addons")
 
 #define FFI_PERMISSIONS(V) V(FFI, "ffi", PermissionsRoot, "--allow-ffi")
-#define ENV_VAR_PERMISSIONS(V)                                                 \
-  V(EnvVar, "env", PermissionsRoot, "--allow-env")
+#define ENV_VAR_PERMISSIONS(V) V(EnvVar, "env", PermissionsRoot, "--allow-env")
 
 #define PERMISSIONS(V)                                                         \
   FILESYSTEM_PERMISSIONS(V)                                                    \
@@ -47,7 +46,10 @@ namespace permission {
   INSPECTOR_PERMISSIONS(V)                                                     \
   NET_PERMISSIONS(V)                                                           \
   ADDON_PERMISSIONS(V)                                                         \
-  FFI_PERMISSIONS(V)
+  FFI_PERMISSIONS(V)                                                           \
+  ENV_VAR_PERMISSIONS(V)
+
+enum class PermissionScope {
 #define V(name, _, __, ___) k##name,
   kPermissionsRoot = -1,
   PERMISSIONS(V) kPermissionsCount

test/parallel/test-permission-env-allow-all.js

@@ -14,8 +14,8 @@ const assert = require('assert');
 {
   assert.ok(process.permission.has('env'));
   // Should not throw for any env var
-  const home = process.env.HOME;
-  const path = process.env.PATH;
+  assert.ok(process.env.HOME !== undefined);
+  assert.ok(process.env.PATH !== undefined);
   process.env.TEST_PERMISSION_VAR = 'test';
   assert.strictEqual(process.env.TEST_PERMISSION_VAR, 'test');
   delete process.env.TEST_PERMISSION_VAR;

test/parallel/test-permission-env-cli.js

@@ -15,8 +15,8 @@ const assert = require('assert');
 }
 
 {
-  const home = process.env.HOME;
-  const path = process.env.PATH;
+  assert.ok(process.env.HOME !== undefined);
+  assert.ok(process.env.PATH !== undefined);
   assert.ok(process.permission.has('env', 'HOME'));
   assert.ok(process.permission.has('env', 'PATH'));
 }