test: added input validation test for fchmod #18217

lucamaraschi · 2018-01-18T00:15:15Z

Added a test to ensure input validation for FD and mode for fs.fchmod.
Removed check for values lower than 0 for mode as it's already checked
by validateUint32.

Checklist

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

Affected core subsystem(s)

test, fs

jasnell · 2018-01-18T04:52:07Z

Hey look @cjihrig ... @lucamaraschi submitted a test case! How many more does he have to go ;-)

cjihrig · 2018-01-18T14:12:36Z

@jasnell if my math is right, 90 once this lands.

BridgeAR · 2018-01-18T14:39:02Z

@jasnell @cjihrig I am working on reducing that number:

BridgeAR [7:46 PM]
If you write at least three tests each day, I will not remind you at all! Think about that! :smile:
Luca Maraschi [7:47 PM]
3 tests a day, keep Ruben away!

addaleax · 2018-01-18T15:00:28Z

CI: https://ci.nodejs.org/job/node-test-commit/15502/

addaleax · 2018-01-18T16:14:22Z

CI complains about a syntax error:

/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-fs-fchmod.js:47
common.mustCall(() => fs.fchmod(1, modeUpperBoundaryValue);
                                                         ^

SyntaxError: missing ) after argument list
    at new Script (vm.js:50:7)
    at createScript (vm.js:137:10)
    at Object.runInThisContext (vm.js:185:10)
    at Module._compile (module.js:628:28)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:577:32)
    at tryModuleLoad (module.js:517:12)
    at Function.Module._load (module.js:509:3)
    at Function.Module.runMain (module.js:701:10)
    at startup (bootstrap_node.js:194:16)

jasnell · 2018-01-18T16:17:00Z

test/parallel/test-fs-fchmod.js

+// Check for mode values range
+const modeUpperBoundaryValue = 0o777;
+common.mustCall(() => fs.fchmod(1, modeUpperBoundaryValue);
+common.mustCall(() => fs.fchmodSync(1, modeUpperBoundaryValue);


nit: missing parens at the end of both of these lines.

targos · 2018-01-20T16:49:02Z

test/parallel/test-fs-fchmod.js

+// Check for mode values range
+const modeUpperBoundaryValue = 0o777;
+common.mustCall(() => fs.fchmod(1, modeUpperBoundaryValue));
+common.mustCall(() => fs.fchmodSync(1, modeUpperBoundaryValue));


Is it me or something is missing? The function returned by common.mustCall is never called, so I expect the test to fail.

Good point. And neither of these two entries should require being wrapped in a mustCall.

Added a test to ensure input validation for FD and mode for fs.fchmod. Removed check for values lower than 0 for `mode` as it's already checked by `validateUint32`.

BridgeAR · 2018-02-01T16:37:32Z

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

Added a test to ensure input validation for FD and mode for fs.fchmod. Removed check for values lower than 0 for `mode` as it's already checked by `validateUint32`. PR-URL: nodejs#18217 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

BridgeAR · 2018-02-06T11:15:16Z

Landed in 075eef5

@lucamaraschi you are very much behind with those tests!

MylesBorins · 2018-02-21T00:19:51Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 18, 2018

jasnell approved these changes Jan 18, 2018

View reviewed changes

cjihrig approved these changes Jan 18, 2018

View reviewed changes

BridgeAR approved these changes Jan 18, 2018

View reviewed changes

addaleax approved these changes Jan 18, 2018

View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 18, 2018

jasnell reviewed Jan 18, 2018

View reviewed changes

lucamaraschi force-pushed the test-fs-fchmod branch from 572c760 to b16a6e5 Compare January 18, 2018 16:33

targos reviewed Jan 20, 2018

View reviewed changes

lucamaraschi added 3 commits January 20, 2018 12:27

test: added input validation test for fchmod

6cd50ac

Added a test to ensure input validation for FD and mode for fs.fchmod. Removed check for values lower than 0 for `mode` as it's already checked by `validateUint32`.

Fixed ) nit

cc739e1

Fixed assert

61d47f2

lucamaraschi force-pushed the test-fs-fchmod branch from b16a6e5 to 61d47f2 Compare January 20, 2018 20:28

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018

BridgeAR closed this Feb 6, 2018

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

targos added backported-to-v9.x and removed backport-requested-v9.x labels Mar 30, 2018

This was referenced Apr 7, 2024

[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1365

Open

[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1366

Open

baby636 mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 baby636/node#145

Open

ryan-ally mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 6.8.0 to 9.0.0 ryan-ally/node#106

Open

aliscco mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 3.19.0 to 9.0.0 aliscco/alisco-node#1372

Open

leonardoadame mentioned this pull request Apr 7, 2024

[Snyk] Security upgrade eslint from 3.19.0 to 9.0.0 leonardoadame/node#770

Open

This was referenced Apr 8, 2024

[Snyk] Security upgrade eslint from 4.19.1 to 9.0.0 aliscco/alisco-node#1375

Open

[Snyk] Security upgrade eslint from 7.32.0 to 9.0.0 aliscco/alisco-node#1376

Open

[Snyk] Security upgrade eslint from 5.16.0 to 9.0.0 aliscco/alisco-node#1377

Open

wedataintelligence mentioned this pull request Oct 9, 2024

[Snyk] Fix for 3 vulnerabilities wedataintelligence/node#127

Open

ron-a20 mentioned this pull request Oct 9, 2024

[Snyk] Fix for 3 vulnerabilities ron-a20/node#142

Open

aliceUnhinged613 mentioned this pull request Oct 11, 2024

[Snyk] Fix for 4 vulnerabilities aliceUnhinged613/node#146

Open

This was referenced Oct 21, 2024

[Snyk] Fix for 6 vulnerabilities Graysonbarton/node#38

Open

[Snyk] Fix for 16 vulnerabilities Graysonbarton/node#61

Open

[Snyk] Fix for 5 vulnerabilities Graysonbarton/node#93

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: added input validation test for fchmod #18217

test: added input validation test for fchmod #18217

lucamaraschi commented Jan 18, 2018

jasnell commented Jan 18, 2018

cjihrig commented Jan 18, 2018

BridgeAR commented Jan 18, 2018

addaleax commented Jan 18, 2018

addaleax commented Jan 18, 2018

jasnell Jan 18, 2018

targos Jan 20, 2018

BridgeAR Jan 20, 2018

BridgeAR commented Feb 1, 2018

BridgeAR commented Feb 6, 2018

MylesBorins commented Feb 21, 2018

test: added input validation test for fchmod #18217

test: added input validation test for fchmod #18217

Conversation

lucamaraschi commented Jan 18, 2018

Checklist

Affected core subsystem(s)

jasnell commented Jan 18, 2018

cjihrig commented Jan 18, 2018

BridgeAR commented Jan 18, 2018

addaleax commented Jan 18, 2018

addaleax commented Jan 18, 2018

jasnell Jan 18, 2018

Choose a reason for hiding this comment

targos Jan 20, 2018

Choose a reason for hiding this comment

BridgeAR Jan 20, 2018

Choose a reason for hiding this comment

BridgeAR commented Feb 1, 2018

BridgeAR commented Feb 6, 2018

MylesBorins commented Feb 21, 2018