lib: mask mode_t type of arguments with 0o777 #20636

joyeecheung · May 9, 2018

doc: document file mode caveats on Windows

On Windows only the write permission (read-only attribute) can be
manipulated, and there is no distinction among owner, group
or others (no implementation to model the ACL after UNIX permissions).
mkdir on Windows does not support the mode argument.

lib: mask mode_t type of arguments with 0o777

Introduce the validateAndMaskMode validator that
validates mode_t arguments and mask them with 0o777
if they are 32-bit unsigned integer or octal string
to be more consistent with POSIX APIs.
Use the validator in fs APIs and process.umask for
consistency.
Add tests for 32-bit unsigned modes larger than 0o777.

Fixes: #20498

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

joyeecheung · May 9, 2018

joyeecheung reviewed May 9, 2018

View changes

joyeecheung · May 9, 2018

test/parallel/test-fs-chmod.js

@@ -62,7 +62,7 @@ function closeSync() {
 }


-// On Windows chmod is only able to manipulate read-only bit


It feels pretty strange to talk about the read-only attribute on windows in this context (attempting to model the Windows permission model after the unix one) so I just changed this to write permission.

joyeecheung · May 9, 2018

cc @nodejs/fs
CI: https://ci.nodejs.org/job/node-test-pull-request/14752/

joyeecheung · May 9, 2018

joyeecheung reviewed May 9, 2018

View changes

joyeecheung · May 9, 2018

doc/api/fs.md

@@ -1089,6 +1089,9 @@ For example, the octal value `0o765` means:
 * The group may read and write the file.
 * Others may read and execute the file.

+Caveats: on Windows only the write permission can be changed, and there is


I have not tried all the combinations but I think on Windows effectively you can only change the permission to either 666 or 444 with our API

vsemozhetbyt · May 9, 2018

vsemozhetbyt reviewed May 9, 2018

View changes

vsemozhetbyt · May 9, 2018

doc/api/fs.md

@@ -1987,7 +1990,7 @@ changes:
 -->

 * `path` {string|Buffer|URL}
-* `mode` {integer} **Default:** `0o777`
+* `mode` {integer} **Default:** `0o777`. Not supported on Windows.


Nit: Default section usually goes last (and doctools expect it to be the last).

mscdex · May 9, 2018

mscdex reviewed May 9, 2018

View changes

mscdex · May 9, 2018

lib/internal/validators.js

+  }
+
+  if (typeof value === 'string') {
+    if (!octalReg.test(value)) {


Why do we need this? If the number is not a parseable octal value, NaN will be returned, which we check for anyway.

parseInt will not return NaN for strings like '666abc'.
But I agree there seems to be redundancy. Could there be cases where the string passes the regex and the value cannot be parsed? I don't think so.

@mscdex @targos Yes there are existing test cases ('123x') in our code base, it is currently only enforced in process.umask though. It does not seem quite right to remove the test IMO. Passing a number followed by invalid characters looks like a mistake that the user may want to know.

By the way, https://github.com/tc39/proposal-number-fromstring (currently in Stage 1) is designed for this situation

@joyeecheung what I meant is that the Number.isNaN(parsed) check can be removed. Unless I'm missing an edge case, it can never be true after the regex check.

mscdex · May 9, 2018

mscdex reviewed May 9, 2018

View changes

mscdex · May 9, 2018

lib/internal/validators.js

+
+  if (typeof value === 'number') {
+    if (!Number.isInteger(value)) {
+      throw new ERR_OUT_OF_RANGE(name, 'an integer', value);


Should we really be using an out of range error type when it's a value type issue?

@mscdex JavaScript only has one number type (at least for now) so it could be seen both ways. I went with ERR_OUT_OF_RANGE so this can still be semver-patch.

joyeecheung · May 14, 2018

Updated the docs to move the defaults to the end of the explanation of arguments and removed the abundant isNaN check. @vsemozhetbyt @targos @mscdex PTAL, thanks!

CI: https://ci.nodejs.org/job/node-test-pull-request/14862/

vsemozhetbyt · May 14, 2018

Doc LGTM)

joyeecheung · May 16, 2018

CI is green..can I have some reviews please? cc @nodejs/fs

jasnell · May 16, 2018

jasnell approved these changes May 16, 2018

View changes

apapirovski · May 16, 2018

apapirovski approved these changes May 16, 2018

View changes

joyeecheung · May 16, 2018

CITGM just in case:

master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1414/
this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1415/

addaleax · May 16, 2018

@joyeecheung Apart from CITGM, this is ready to land, right?

joyeecheung · May 16, 2018

@addaleax Yes

joyeecheung · May 17, 2018

CITGM failures looks irrelevant, two no space left on device failures, one rename with EPERM, and another known failures of thread-sleep

joyeecheung · May 17, 2018

#19811 added tests that expect mode > 0o777 to be rejected. Replaced to expect those operations not to fail. It could use some better coverage to make sure the chmod in fs promises actually work but those can be done in a later PR IMO, since it doesn't do that for 0o666 anyway.

joyeecheung · May 17, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/14922/

joyeecheung · May 17, 2018

Landed in 0d9500d...f9de6f5, thanks!

nodejs-github-bot added the lib / src label May 9, 2018

vsemozhetbyt added the fs label May 9, 2018

joyeecheung changed the title ~~Mode validator~~ lib: mask mode_t type of arguments with 0o777 May 9, 2018

joyeecheung force-pushed the joyeecheung:mode-validator branch 2 times, most recently from 470ce3d to 48f9e8a May 14, 2018

cjihrig referenced this pull request May 16, 2018
Merged
v10.2.0 proposal #20724

cjihrig referenced this pull request May 16, 2018
Merged
fs: improve fchmod{Sync} validation #20588
2 of 2 tasks complete

joyeecheung force-pushed the joyeecheung:mode-validator branch from 48f9e8a to 37af759 May 17, 2018

joyeecheung closed this May 17, 2018

targos added backported-to-v10.x and removed backport-requested-v10.x labels Jun 7, 2018

targos referenced this pull request Jun 19, 2018
Merged
v10.5.0 proposal #21400

This was referenced Jun 20, 2018

Closed

2018-06-20 Version 10.5.0 (Current) @targos beevelop/docker-android-nodejs#216

Closed

2018-06-20 Version 10.5.0 (Current) @targos beevelop/docker-nodejs#201

targos added this to Backported in v10.x Sep 23, 2018

nodejs/node

Join GitHub today

lib: mask mode_t type of arguments with 0o777 #20636

Conversation

joyeecheung commented May 9, 2018 • edited

doc: document file mode caveats on Windows

lib: mask mode_t type of arguments with 0o777

Checklist

nodejs-github-bot added the lib / src label May 9, 2018

joyeecheung reviewed May 9, 2018 View changes

This comment has been minimized.

joyeecheung May 9, 2018 • edited

This comment has been minimized.

joyeecheung commented May 9, 2018

vsemozhetbyt added the fs label May 9, 2018

joyeecheung reviewed May 9, 2018 View changes

This comment has been minimized.

joyeecheung May 9, 2018 • edited

vsemozhetbyt reviewed May 9, 2018 View changes

This comment has been minimized.

vsemozhetbyt May 9, 2018 • edited

mscdex reviewed May 9, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joyeecheung May 9, 2018 • edited

This comment has been minimized.

This comment has been minimized.

mscdex reviewed May 9, 2018 View changes

This comment has been minimized.

This comment has been minimized.

joyeecheung changed the title Mode validator lib: mask mode_t type of arguments with 0o777 May 9, 2018

joyeecheung force-pushed the joyeecheung:mode-validator branch 2 times, most recently from 470ce3d to 48f9e8a May 14, 2018

This comment has been minimized.

joyeecheung commented May 14, 2018

This comment has been minimized.

vsemozhetbyt commented May 14, 2018 • edited

This comment has been minimized.

joyeecheung commented May 16, 2018

jasnell approved these changes May 16, 2018 View changes

apapirovski approved these changes May 16, 2018 View changes

This comment has been minimized.

joyeecheung commented May 16, 2018

cjihrig referenced this pull request May 16, 2018

This comment has been minimized.

addaleax commented May 16, 2018

This comment has been minimized.

joyeecheung commented May 16, 2018

cjihrig referenced this pull request May 16, 2018

This comment has been minimized.

joyeecheung commented May 17, 2018

joyeecheung added some commits May 9, 2018

This comment has been minimized.

joyeecheung commented May 17, 2018

This comment has been minimized.

joyeecheung commented May 17, 2018 • edited

joyeecheung force-pushed the joyeecheung:mode-validator branch from 48f9e8a to 37af759 May 17, 2018

This comment has been minimized.

joyeecheung commented May 17, 2018

joyeecheung closed this May 17, 2018

targos added a commit that referenced this pull request Jun 7, 2018

targos added a commit that referenced this pull request Jun 7, 2018

targos added backported-to-v10.x and removed backport-requested-v10.x labels Jun 7, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

targos added a commit that referenced this pull request Jun 13, 2018

targos added a commit that referenced this pull request Jun 13, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

targos referenced this pull request Jun 19, 2018

targos added a commit that referenced this pull request Jun 20, 2018

targos added a commit that referenced this pull request Jun 20, 2018

targos added a commit that referenced this pull request Jun 20, 2018

This was referenced Jun 20, 2018

calebeby added a commit to Pigmice2733/scouting-frontend that referenced this pull request Jun 20, 2018

This was referenced Jun 20, 2018

joyeecheung commented May 9, 2018 •

edited

joyeecheung reviewed May 9, 2018

View changes

joyeecheung May 9, 2018 •

edited

joyeecheung reviewed May 9, 2018

View changes

joyeecheung May 9, 2018 •

edited

vsemozhetbyt reviewed May 9, 2018

View changes

vsemozhetbyt May 9, 2018 •

edited

mscdex reviewed May 9, 2018

View changes

joyeecheung May 9, 2018 •

edited

mscdex reviewed May 9, 2018

View changes

joyeecheung force-pushed the joyeecheung:mode-validator branch 2 times, most recently from `470ce3d` to `48f9e8a` May 14, 2018

vsemozhetbyt commented May 14, 2018 •

edited

jasnell approved these changes May 16, 2018

View changes

apapirovski approved these changes May 16, 2018

View changes

joyeecheung commented May 17, 2018 •

edited

joyeecheung force-pushed the joyeecheung:mode-validator branch from `48f9e8a` to `37af759` May 17, 2018