From f945b37825a85c92b1b528e33ec848a3d5c6af09 Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Wed, 19 Jan 2022 00:47:05 +0200 Subject: [PATCH 1/6] fix(arborist): shrinkwrap throws trying to read a folder without permissions Fix an issue where shrinkwrap throws an error when trying to read a folder that it doesn't have permissions to, instead of returning a correct object with an error --- workspaces/arborist/lib/shrinkwrap.js | 8 ++++++-- workspaces/arborist/test/shrinkwrap.js | 13 +++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/shrinkwrap.js b/workspaces/arborist/lib/shrinkwrap.js index a7a68c98c6d6d..1354b851a3a23 100644 --- a/workspaces/arborist/lib/shrinkwrap.js +++ b/workspaces/arborist/lib/shrinkwrap.js @@ -472,8 +472,12 @@ class Shrinkwrap { // all good! hidden lockfile is the newest thing in here. return data }).catch(er => { - const rel = relpath(this.path, this.filename) - this.log.verbose('shrinkwrap', `failed to load ${rel}`, er) + if (typeof this.filename === 'string') { + const rel = relpath(this.path, this.filename) + this.log.verbose('shrinkwrap', `failed to load ${rel}`, er) + } else { + this.log.verbose('shrinkwrap', `failed to load ${this.path}`, er) + } this.loadingError = er this.loadedFromDisk = false this.ancientLockfile = false diff --git a/workspaces/arborist/test/shrinkwrap.js b/workspaces/arborist/test/shrinkwrap.js index f752c724a35e7..d21b30c7bd7d5 100644 --- a/workspaces/arborist/test/shrinkwrap.js +++ b/workspaces/arborist/test/shrinkwrap.js @@ -1597,4 +1597,17 @@ t.test('setting lockfileVersion from the file contents', async t => { }) t.equal(Shrinkwrap.defaultLockfileVersion, 2, 'default is 2') + + t.test('load should return error correctly when it cant access folder', async t => { + const dir = t.testdir({}) + try { + fs.chmodSync(dir, '000') + const res = await Shrinkwrap.load({ path: dir }) + t.ok(res.loadingError, 'loading error should exist') + t.strictSame(res.loadingError.errno, -13) + t.strictSame(res.loadingError.code, 'EACCES') + } finally { + fs.chmodSync(dir, '777') + } + }) }) From bfbd139adca0d9d8667a95b2afe98650ac29cb30 Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Wed, 19 Jan 2022 13:08:42 +0200 Subject: [PATCH 2/6] fixup! fix(arborist): shrinkwrap throws trying to read a folder without permissions --- workspaces/arborist/test/shrinkwrap.js | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/workspaces/arborist/test/shrinkwrap.js b/workspaces/arborist/test/shrinkwrap.js index d21b30c7bd7d5..8b302ec69f4e4 100644 --- a/workspaces/arborist/test/shrinkwrap.js +++ b/workspaces/arborist/test/shrinkwrap.js @@ -5,6 +5,7 @@ const calcDepFlags = require('../lib/calc-dep-flags.js') const fs = require('fs') const Arborist = require('../lib/arborist/index.js') const rimraf = require('rimraf') +const { execSync } = require('child_process') const t = require('tap') @@ -1601,13 +1602,30 @@ t.test('setting lockfileVersion from the file contents', async t => { t.test('load should return error correctly when it cant access folder', async t => { const dir = t.testdir({}) try { - fs.chmodSync(dir, '000') + const err = removePermissions(dir) const res = await Shrinkwrap.load({ path: dir }) t.ok(res.loadingError, 'loading error should exist') - t.strictSame(res.loadingError.errno, -13) - t.strictSame(res.loadingError.code, 'EACCES') + t.strictSame(res.loadingError.code, err) } finally { - fs.chmodSync(dir, '777') + restorePermissions(dir) + } + + function removePermissions(dir) { + if (process.platform === 'win32') { + execSync(`icacls ${dir} /deny "everyone:R"`) + return 'EPERM' + } + + fs.chmodSync(dir, '000') + return 'EACCES' + } + + function restorePermissions(dir) { + if (process.platform === 'win32') { + execSync(`icacls ${dir} /grant "everyone:R"`) + } else { + fs.chmodSync(dir, '777') + } } }) }) From 63187f74d989dcdf8f0e6a9de2a1ab6e3af1c3af Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Thu, 20 Jan 2022 22:57:41 +0200 Subject: [PATCH 3/6] revert test + skip --- workspaces/arborist/test/shrinkwrap.js | 41 ++++++++------------------ 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/workspaces/arborist/test/shrinkwrap.js b/workspaces/arborist/test/shrinkwrap.js index 8b302ec69f4e4..cab19d4b7961d 100644 --- a/workspaces/arborist/test/shrinkwrap.js +++ b/workspaces/arborist/test/shrinkwrap.js @@ -1599,33 +1599,18 @@ t.test('setting lockfileVersion from the file contents', async t => { t.equal(Shrinkwrap.defaultLockfileVersion, 2, 'default is 2') - t.test('load should return error correctly when it cant access folder', async t => { - const dir = t.testdir({}) - try { - const err = removePermissions(dir) - const res = await Shrinkwrap.load({ path: dir }) - t.ok(res.loadingError, 'loading error should exist') - t.strictSame(res.loadingError.code, err) - } finally { - restorePermissions(dir) - } - - function removePermissions(dir) { - if (process.platform === 'win32') { - execSync(`icacls ${dir} /deny "everyone:R"`) - return 'EPERM' - } - - fs.chmodSync(dir, '000') - return 'EACCES' - } - - function restorePermissions(dir) { - if (process.platform === 'win32') { - execSync(`icacls ${dir} /grant "everyone:R"`) - } else { - fs.chmodSync(dir, '777') + t.test('load should return error correctly when it cant access folder', + { skip: process.platform === 'win32' ? 'skip chmod in windows' : false }, + async t => { + const dir = t.testdir({}) + try { + fs.chmodSync(dir, '000') + const res = await Shrinkwrap.load({ path: dir }) + t.ok(res.loadingError, 'loading error should exist') + t.strictSame(res.loadingError.errno, -13) + t.strictSame(res.loadingError.code, 'EACCES') + } finally { + fs.chmodSync(dir, '666') } - } - }) + }) }) From 4ff8b90d69fa394da0103021f19c9fa4947bbffd Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Fri, 21 Jan 2022 15:38:59 +0200 Subject: [PATCH 4/6] lint --- workspaces/arborist/test/shrinkwrap.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/workspaces/arborist/test/shrinkwrap.js b/workspaces/arborist/test/shrinkwrap.js index cab19d4b7961d..1aff99bfbd239 100644 --- a/workspaces/arborist/test/shrinkwrap.js +++ b/workspaces/arborist/test/shrinkwrap.js @@ -5,7 +5,6 @@ const calcDepFlags = require('../lib/calc-dep-flags.js') const fs = require('fs') const Arborist = require('../lib/arborist/index.js') const rimraf = require('rimraf') -const { execSync } = require('child_process') const t = require('tap') @@ -1600,7 +1599,7 @@ t.test('setting lockfileVersion from the file contents', async t => { t.equal(Shrinkwrap.defaultLockfileVersion, 2, 'default is 2') t.test('load should return error correctly when it cant access folder', - { skip: process.platform === 'win32' ? 'skip chmod in windows' : false }, + { skip: process.platform === 'win32' ? 'skip chmod in windows' : false }, async t => { const dir = t.testdir({}) try { From dec9c74484bfa9471d156e830cdbb6efadf95630 Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Fri, 21 Jan 2022 15:46:14 +0200 Subject: [PATCH 5/6] add istanbul ignore --- workspaces/arborist/lib/shrinkwrap.js | 1 + 1 file changed, 1 insertion(+) diff --git a/workspaces/arborist/lib/shrinkwrap.js b/workspaces/arborist/lib/shrinkwrap.js index 1354b851a3a23..ebff7f8528b59 100644 --- a/workspaces/arborist/lib/shrinkwrap.js +++ b/workspaces/arborist/lib/shrinkwrap.js @@ -472,6 +472,7 @@ class Shrinkwrap { // all good! hidden lockfile is the newest thing in here. return data }).catch(er => { + /* istanbul ignore else */ if (typeof this.filename === 'string') { const rel = relpath(this.path, this.filename) this.log.verbose('shrinkwrap', `failed to load ${rel}`, er) From 71fdb5c0cedd46976bf92529133f41fe3c58363e Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Mon, 24 Jan 2022 21:49:57 +0200 Subject: [PATCH 6/6] fix test indentation --- workspaces/arborist/test/shrinkwrap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/arborist/test/shrinkwrap.js b/workspaces/arborist/test/shrinkwrap.js index 1aff99bfbd239..d47266d30e1c1 100644 --- a/workspaces/arborist/test/shrinkwrap.js +++ b/workspaces/arborist/test/shrinkwrap.js @@ -1599,7 +1599,7 @@ t.test('setting lockfileVersion from the file contents', async t => { t.equal(Shrinkwrap.defaultLockfileVersion, 2, 'default is 2') t.test('load should return error correctly when it cant access folder', - { skip: process.platform === 'win32' ? 'skip chmod in windows' : false }, + { skip: process.platform === 'win32' ? 'skip chmod in windows' : false }, async t => { const dir = t.testdir({}) try {