test: fix policy-deny tests

nodejs · Jul 27, 2022 · 2978c49 · 2978c49
1 parent 1945c2c
commit 2978c49
Show file tree

Hide file tree

Showing 12 changed files with 160 additions and 250 deletions.
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -783,6 +783,16 @@ unless either the `--pending-deprecation` command-line flag, or the
 are used to provide a kind of selective "early warning" mechanism that
 developers may leverage to detect deprecated API usage.
 
+### `--policy-deny-fs`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+> Stability: 1 - Experimental
+
+TODO
+
 ### `--policy-integrity=sri`
 
 <!-- YAML
@@ -1650,6 +1660,7 @@ Node.js options that are allowed are:
 * `--openssl-config`
 * `--openssl-legacy-provider`
 * `--pending-deprecation`
+* `--policy-deny-fs`
 * `--policy-integrity`
 * `--preserve-symlinks-main`
 * `--preserve-symlinks`

diff --git a/doc/node.1 b/doc/node.1
@@ -309,6 +309,9 @@ Among other uses, this can be used to enable FIPS-compliant crypto if Node.js is
 .It Fl -pending-deprecation
 Emit pending deprecation warnings.
 .
+.It Fl -policy-deny-fs
+Instructs Node.js to restrict access to the given resource type
+.
 .It Fl -policy-integrity Ns = Ns Ar sri
 Instructs Node.js to error prior to running any code if the policy does not have the specified integrity. It expects a Subresource Integrity string as a parameter.
 .

diff --git a/lib/internal/repl/history.js b/lib/internal/repl/history.js
@@ -40,13 +40,6 @@ function setupHistory(repl, historyPath, ready) {
     return ready(null, repl);
   }
 
-  // TODO: make it more granular (fs.in os.homedir())
-  if (!process.policy.check('fs.in') || !process.policy.check('fs.out')) {
-    _writeToOutput(repl, '\nAccess to FileSystemIn/Out is restricted.\n' +
-      'REPL session history will not be persisted.\n');
-    return ready(null, repl);
-  }
-
   if (!historyPath) {
     try {
       historyPath = path.join(os.homedir(), '.node_repl_history');
@@ -60,6 +53,12 @@ function setupHistory(repl, historyPath, ready) {
     }
   }
 
+  if (!process.policy.check('fs.out', historyPath)) {
+    _writeToOutput(repl, '\nAccess to FileSystemOut is restricted.\n' +
+      'REPL session history will not be persisted.\n');
+    return ready(null, repl);
+  }
+
   let timer = null;
   let writing = false;
   let pending = false;

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -1235,8 +1235,13 @@ static void Rename(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue old_path(isolate, args[0]);
   CHECK_NOT_NULL(*old_path);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemOut, *old_path);
+
   BufferValue new_path(isolate, args[1]);
   CHECK_NOT_NULL(*new_path);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemOut, *new_path);
 
   FSReqBase* req_wrap_async = GetReqWrap(args, 2);
   if (req_wrap_async != nullptr) {
@@ -1353,6 +1358,8 @@ static void RMDir(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue path(env->isolate(), args[0]);
   CHECK_NOT_NULL(*path);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemOut, *path);
 
   FSReqBase* req_wrap_async = GetReqWrap(args, 1);  // rmdir(path, req)
   if (req_wrap_async != nullptr) {
@@ -1557,6 +1564,8 @@ static void MKDir(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue path(env->isolate(), args[0]);
   CHECK_NOT_NULL(*path);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemOut, *path);
 
   CHECK(args[1]->IsInt32());
   const int mode = args[1].As<Int32>()->Value();
@@ -1755,7 +1764,7 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
         env, policy::Permission::kFileSystemIn, *path);
     THROW_IF_INSUFFICIENT_PERMISSIONS(
         env, policy::Permission::kFileSystemOut, *path);
-  } else if ((flags &~ (O_RDONLY | O_SYNC)) == 0) {
+  } else if ((flags & ~(O_RDONLY | O_SYNC)) == 0) {
     THROW_IF_INSUFFICIENT_PERMISSIONS(
         env, policy::Permission::kFileSystemIn, *path);
   } else if ((flags & (O_APPEND | O_TRUNC | O_CREAT | O_WRONLY)) != 0) {
@@ -1806,7 +1815,7 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
         env, policy::Permission::kFileSystemIn, *path);
     THROW_IF_INSUFFICIENT_PERMISSIONS(
         env, policy::Permission::kFileSystemOut, *path);
-  } else if ((flags &~ (O_RDONLY | O_SYNC)) == 0) {
+  } else if ((flags & ~(O_RDONLY | O_SYNC)) == 0) {
     THROW_IF_INSUFFICIENT_PERMISSIONS(
         env, policy::Permission::kFileSystemIn, *path);
   } else if ((flags & (O_APPEND | O_TRUNC | O_CREAT | O_WRONLY)) != 0) {
@@ -1843,9 +1852,13 @@ static void CopyFile(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue src(isolate, args[0]);
   CHECK_NOT_NULL(*src);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemIn, *src);
 
   BufferValue dest(isolate, args[1]);
   CHECK_NOT_NULL(*dest);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemOut, *dest);
 
   CHECK(args[2]->IsInt32());
   const int flags = args[2].As<Int32>()->Value();
@@ -2334,6 +2347,8 @@ static void UTimes(const FunctionCallbackInfo<Value>& args) {
 
   BufferValue path(env->isolate(), args[0]);
   CHECK_NOT_NULL(*path);
+  THROW_IF_INSUFFICIENT_PERMISSIONS(
+      env, policy::Permission::kFileSystemOut, *path);
 
   CHECK(args[1]->IsNumber());
   const double atime = args[1].As<Number>()->Value();

diff --git a/src/node_options.cc b/src/node_options.cc
@@ -709,7 +709,6 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
             &PerIsolateOptions::experimental_top_level_await,
             kAllowedInEnvironment);
   AddOption("--harmony-top-level-await", "", V8Option{});
-
   Implies("--experimental-top-level-await", "--harmony-top-level-await");
   Implies("--harmony-top-level-await", "--experimental-top-level-await");
   ImpliesNot("--no-harmony-top-level-await", "--experimental-top-level-await");

diff --git a/src/policy/policy.h b/src/policy/policy.h
@@ -19,7 +19,7 @@ namespace policy {
 
 #define THROW_IF_INSUFFICIENT_PERMISSIONS(env, perm_, resource_, ...)          \
   if (!node::policy::root_policy.is_granted(perm_, resource_)) {               \
-    return node::policy::Policy::ThrowAccessDenied((env), perm_);                     \
+    return node::policy::Policy::ThrowAccessDenied((env), perm_);              \
   }
 
 class Policy {

diff --git a/test/fixtures/policy/deny/regular-file.md b/test/fixtures/policy/deny/regular-file.md
@@ -1,3 +0,0 @@
-# Regular File
-
-Example of a regular file to be used in the PolicyDenyFs module

diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js
@@ -27,6 +27,7 @@ const expectedModules = new Set([
   'Internal Binding options',
   'Internal Binding performance',
   'Internal Binding pipe_wrap',
+  'Internal Binding policy',
   'Internal Binding process_methods',
   'Internal Binding report',
   'Internal Binding serdes',
@@ -100,10 +101,13 @@ const expectedModules = new Set([
   'NativeModule internal/perf/timerify',
   'NativeModule internal/perf/usertiming',
   'NativeModule internal/perf/utils',
+  'NativeModule internal/policy/manifest',
+  'NativeModule internal/policy/sri',
   'NativeModule internal/priority_queue',
   'NativeModule internal/process/esm_loader',
   'NativeModule internal/process/execution',
   'NativeModule internal/process/per_thread',
+  'NativeModule internal/process/policy',
   'NativeModule internal/process/promises',
   'NativeModule internal/process/report',
   'NativeModule internal/process/signal',

diff --git a/test/parallel/test-cli-policy-deny-fs.js b/test/parallel/test-cli-policy-deny-fs.js
@@ -4,12 +4,9 @@ const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
 
-const fixtures = require('../common/fixtures');
-
 const { spawnSync } = require('child_process');
 const assert = require('assert');
-
-const dep = fixtures.path('policy', 'deny', 'check.js');
+const fs = require('fs');
 
 {
   const { status, stdout } = spawnSync(
@@ -18,7 +15,7 @@ const dep = fixtures.path('policy', 'deny', 'check.js');
       '--policy-deny-fs', 'fs', '-e',
       `console.log(process.policy.check("fs"));
        console.log(process.policy.check("fs.in"));
-       console.log(process.policy.check("fs.out"));`
+       console.log(process.policy.check("fs.out"));`,
     ]
   );
 
@@ -37,7 +34,7 @@ const dep = fixtures.path('policy', 'deny', 'check.js');
       '--policy-deny-fs', 'in', '-e',
       `console.log(process.policy.check("fs"));
        console.log(process.policy.check("fs.in"));
-       console.log(process.policy.check("fs.out"));`
+       console.log(process.policy.check("fs.out"));`,
     ]
   );
 
@@ -55,7 +52,7 @@ const dep = fixtures.path('policy', 'deny', 'check.js');
       '--policy-deny-fs', 'out', '-e',
       `console.log(process.policy.check("fs"));
        console.log(process.policy.check("fs.in"));
-       console.log(process.policy.check("fs.out"));`
+       console.log(process.policy.check("fs.out"));`,
     ]
   );
 
@@ -73,7 +70,7 @@ const dep = fixtures.path('policy', 'deny', 'check.js');
       '--policy-deny-fs', 'out,in', '-e',
       `console.log(process.policy.check("fs"));
        console.log(process.policy.check("fs.in"));
-       console.log(process.policy.check("fs.out"));`
+       console.log(process.policy.check("fs.out"));`,
     ]
   );
 
@@ -112,5 +109,5 @@ const dep = fixtures.path('policy', 'deny', 'check.js');
     stderr.toString().includes('Access to this API has been restricted'),
     stderr);
   assert.strictEqual(status, 1);
-  assert.ok(fs.existsSync('policy-deny-example.md'));
+  assert.ok(!fs.existsSync('policy-deny-example.md'));
 }