Repository: nodejs/node
Pull request: fs: special input `-1` on `chown`, `lchown` and `fchown`

Fixes: https://github.com/nodejs/node/issues/58826

`-1` is a special input for chown, means won't change anything according to the spec.

https://stackoverflow.com/questions/21370094/is-the-uid-t-type-signed-or-unsigned

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: src/node_file.cc
patch: @@ -2853,10 +2853,10 @@ static void Chown(const FunctionCallbackInfo<Value>& args) {
   ToNamespacedPath(env, &path);
 
   CHECK(IsSafeJsInt(args[1]));
-  const uv_uid_t uid = FromV8Value<uv_uid_t>(args[1]);
+  const auto uid = FromV8Value<uv_uid_t, true>(args[1]);
 
   CHECK(IsSafeJsInt(args[2]));
-  const uv_gid_t gid = FromV8Value<uv_gid_t>(args[2]);
+  const auto gid = FromV8Value<uv_gid_t, true>(args[2]);
 
   if (argc > 3) {  // chown(path, uid, gid, req)
     FSReqBase* req_wrap_async = GetReqWrap(args, 3);
@@ -2898,10 +2898,10 @@ static void FChown(const FunctionCallbackInfo<Value>& args) {
   }
 
   CHECK(IsSafeJsInt(args[1]));
-  const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Integer>()->Value());
+  const auto uid = FromV8Value<uv_uid_t, true>(args[1]);
 
   CHECK(IsSafeJsInt(args[2]));
-  const uv_gid_t gid = static_cast<uv_gid_t>(args[2].As<Integer>()->Value());
+  const auto gid = FromV8Value<uv_gid_t, true>(args[2]);
 
   if (argc > 3) {  // fchown(fd, uid, gid, req)
     FSReqBase* req_wrap_async = GetReqWrap(args, 3);
@@ -2928,10 +2928,10 @@ static void LChown(const FunctionCallbackInfo<Value>& args) {
   ToNamespacedPath(env, &path);
 
   CHECK(IsSafeJsInt(args[1]));
-  const uv_uid_t uid = FromV8Value<uv_uid_t>(args[1]);
+  const auto uid = FromV8Value<uv_uid_t, true>(args[1]);
 
   CHECK(IsSafeJsInt(args[2]));
-  const uv_gid_t gid = FromV8Value<uv_gid_t>(args[2]);
+  const auto gid = FromV8Value<uv_gid_t, true>(args[2]);
 
   if (argc > 3) {  // lchown(path, uid, gid, req)
     FSReqBase* req_wrap_async = GetReqWrap(args, 3);

filename: src/util-inl.h
patch: @@ -627,27 +627,36 @@ constexpr std::string_view FastStringKey::as_string_view() const {
 
 // Converts a V8 numeric value to a corresponding C++ primitive or enum type.
 template <typename T,
+          bool loose = false,
           typename = std::enable_if_t<std::numeric_limits<T>::is_specialized ||
                                       std::is_enum_v<T>>>
 T FromV8Value(v8::Local<v8::Value> value) {
   if constexpr (std::is_enum_v<T>) {
     using Underlying = std::underlying_type_t<T>;
-    return static_cast<T>(FromV8Value<Underlying>(value));
+    return static_cast<T>(FromV8Value<Underlying, loose>(value));
   } else if constexpr (std::is_integral_v<T> && std::is_unsigned_v<T>) {
     static_assert(
         std::numeric_limits<T>::max() <= std::numeric_limits<uint32_t>::max() &&
             std::numeric_limits<T>::min() >=
                 std::numeric_limits<uint32_t>::min(),
         "Type is out of unsigned integer range");
-    CHECK(value->IsUint32());
+    if constexpr (!loose) {
+      CHECK(value->IsUint32());
+    } else {
+      CHECK(value->IsNumber());
+    }
     return static_cast<T>(value.As<v8::Uint32>()->Value());
   } else if constexpr (std::is_integral_v<T> && std::is_signed_v<T>) {
     static_assert(
         std::numeric_limits<T>::max() <= std::numeric_limits<int32_t>::max() &&
             std::numeric_limits<T>::min() >=
                 std::numeric_limits<int32_t>::min(),
         "Type is out of signed integer range");
-    CHECK(value->IsInt32());
+    if constexpr (!loose) {
+      CHECK(value->IsInt32());
+    } else {
+      CHECK(value->IsNumber());
+    }
     return static_cast<T>(value.As<v8::Int32>()->Value());
   } else {
     static_assert(std::is_floating_point_v<T>,

filename: test/parallel/test-fs-chown-negative-one.js
patch: @@ -0,0 +1,32 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const testFile = path.join(tmpdir.path, 'chown-test-file.txt');
+
+fs.writeFileSync(testFile, 'test content for chown');
+
+const stats = fs.statSync(testFile);
+const uid = stats.uid;
+const gid = stats.gid;
+
+// -1 for uid and gid means "don't change the value"
+{
+  fs.chown(testFile, -1, -1, common.mustSucceed(() => {
+    const stats = fs.statSync(testFile);
+    assert.strictEqual(stats.uid, uid);
+    assert.strictEqual(stats.gid, gid);
+  }));
+}
+{
+  fs.chownSync(testFile, -1, -1);
+  const stats = fs.statSync(testFile);
+  assert.strictEqual(stats.uid, uid);
+  assert.strictEqual(stats.gid, gid);
+}

filename: test/parallel/test-fs-fchown-negative-one.js
patch: @@ -0,0 +1,42 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const testFilePath = path.join(tmpdir.path, 'fchown-test-file.txt');
+
+fs.writeFileSync(testFilePath, 'test content for fchown');
+
+{
+  const fd = fs.openSync(testFilePath, 'r+');
+  const stats = fs.fstatSync(fd);
+  const uid = stats.uid;
+  const gid = stats.gid;
+
+  fs.fchown(fd, -1, -1, common.mustSucceed(() => {
+    const stats = fs.fstatSync(fd);
+    assert.strictEqual(stats.uid, uid);
+    assert.strictEqual(stats.gid, gid);
+    fs.closeSync(fd);
+  }));
+}
+
+// Test sync fchown with -1 values
+{
+  const fd = fs.openSync(testFilePath, 'r+');
+  const stats = fs.fstatSync(fd);
+  const uid = stats.uid;
+  const gid = stats.gid;
+
+  fs.fchownSync(fd, -1, -1);
+  const statsAfter = fs.fstatSync(fd);
+  assert.strictEqual(statsAfter.uid, uid);
+  assert.strictEqual(statsAfter.gid, gid);
+
+  fs.closeSync(fd);
+}

filename: test/parallel/test-fs-lchown-negative-one.js
patch: @@ -0,0 +1,34 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const testFile = path.join(tmpdir.path, 'lchown-test-file.txt');
+const testLink = path.join(tmpdir.path, 'lchown-test-link');
+
+fs.writeFileSync(testFile, 'test content for lchown');
+fs.symlinkSync(testFile, testLink);
+
+const stats = fs.lstatSync(testLink);
+const uid = stats.uid;
+const gid = stats.gid;
+
+// -1 for uid and gid means "don't change the value"
+{
+  fs.lchown(testLink, -1, -1, common.mustSucceed(() => {
+    const stats = fs.lstatSync(testLink);
+    assert.strictEqual(stats.uid, uid);
+    assert.strictEqual(stats.gid, gid);
+  }));
+}
+{
+  fs.lchownSync(testLink, -1, -1);
+  const stats = fs.lstatSync(testLink);
+  assert.strictEqual(stats.uid, uid);
+  assert.strictEqual(stats.gid, gid);
+}