Skip to content

test: add test for loading read-only modules #20138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
billti wants to merge 2 commits into from
Closed

test: add test for loading read-only modules #20138

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
767b86a
test: add test for loading read-only modules
billti Apr 18, 2018
ccab982
test: updated formatting per feedback
billti Apr 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
test: add test for loading read-only modules 
Adds a test-case to cover loading modules the user does not have permission
to write to.
Covers issue logged in #20112
  • Loading branch information
@billti
billti committed Apr 18, 2018
commit 767b86a1e1710ee2067d9ab59bfda0911eeb1ea5
46 changes: 46 additions & 0 deletions test/parallel/test-module-readonly.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const path = require('path');
const cp = require('child_process');

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

if (common.isWindows) {
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Apr 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be more optimal to add this check at the beginning, something like:

const common = require('../common');

// TODO: Similar checks on *nix-like systems (e.g using chmod or the like)
if (!common.isWindows)
  common.skip('test only runs on Windows');

const assert = require('assert');
// ...

This way the test will not waste the time and resources loading all the other modules. common.skip() makes the script exit, so no else is required after it and we can save one indentation level as well.

// Create readOnlyMod.js and set to read only
const readOnlyMod = path.join(tmpdir.path, 'readOnlyMod');
const readOnlyModRelative = path.relative(__dirname, readOnlyMod);
const readOnlyModFullPath = readOnlyMod + '.js';
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually prefer template literals in such cases, so maybe:

const readOnlyModFullPath = `${readOnlyMod}.js`;

but feel free to ignore this and next stylistic notes)


fs.writeFileSync(readOnlyModFullPath, 'module.exports = 42;');
// Removed any inherited ACEs, and any explicitly granted ACEs for the
// current user
cp.execSync('icacls.exe "' + readOnlyModFullPath +
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Apr 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

cp.execSync(
  `icacls.exe "${readOnlyModFullPath}" /inheritance:r /remove "%USERNAME%"`);

Copy link
Copy Markdown
Contributor Author

@billti billti Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did originally have template literals, but a) I wasn't sure they were supported all the way back to all LTS releases (I see now they are), and b) One long string was exceeded the lint line limit (but I could wrap it as shown above). I can add them back if desired.

Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I find template literals in these cases more clear, they have less syntactic noise (operators, quotes), but this can be my personal taste, so you can decide freely) We did not set strict linting rule for this, but we had many PRs that have replaced concatenations with templates, so templates may be prevalent style in tests now.

'" /inheritance:r /remove "%USERNAME%"');

// Grant the current user read & execute only
cp.execSync('icacls.exe "' + readOnlyModFullPath +
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

cp.execSync(`icacls.exe "${readOnlyModFullPath}" /grant "%USERNAME%":RX`);

'" /grant "%USERNAME%":RX');

let except = null;
try {
// Attempt to load the module. Will fail if write access is required
require(readOnlyModRelative);
} catch (err) {
except = err;
}

// Remove the expliclty granted rights, and reenable inheritance
cp.execSync('icacls.exe "' + readOnlyModFullPath +
Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:

cp.execSync(
  `icacls.exe "${readOnlyModFullPath}" /remove "%USERNAME%" /inheritance:e`);

'" /remove "%USERNAME%" /inheritance:e');
// Delete the test module (note: tmpdir should get cleaned anyway)
fs.unlinkSync(readOnlyModFullPath);

assert.ifError(except);
} else {
// TODO: Similar checks on *nix-like systems (e.g using chmod or the like)
common.skip('test only runs on Windows');
}