Skip to content

modules: enforce extensions and dissalow directories#21572

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
MylesBorins wants to merge 3 commits into from
Closed

modules: enforce extensions and dissalow directories #21572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1125e44
modules: enforce extensions and disallow directories
MylesBorins Jun 28, 2018
afb45d3
doc: document esm limitations
MylesBorins Jun 28, 2018
9c090ea
fixup: @vsemozhetbyt notes
MylesBorins Jun 28, 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
5 changes: 5 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1388,6 +1388,11 @@ OpenSSL crypto support.
An attempt was made to use features that require [ICU][], but Node.js was not
compiled with ICU support.

<a id="ERR_NON_EXPLICIT_SPECIFIER"></a>

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

@vsemozhetbytvsemozhetbytJun 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, sorry, N is before _. But then it should go before ERR_NO_CRYPTO, right?

### ERR_NON_EXPLICIT_SPECIFIER

Modules that are imported must use explicit file extensions.

<a id="ERR_NO_LONGER_SUPPORTED"></a>
### ERR_NO_LONGER_SUPPORTED

Expand Down
12 changes: 10 additions & 2 deletions doc/api/esm.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54,6 +54,14 @@ property:

## Notable differences between `import` and `require`

### Mandatory file extensions

File extension must be provided when using the `import` keyword.

### No importing directories

There is no support for importing directories.

### No NODE_PATH

`NODE_PATH` is not part of resolving `import` specifiers. Please use symlinks
Expand All@@ -78,8 +86,8 @@ Modules will be loaded multiple times if the `import` specifier used to resolve
them have a different query or fragment.

```js
import './foo?query=1'; // loads ./foo with query of "?query=1"
import './foo?query=2'; // loads ./foo with query of "?query=2"
import './foo.mjs?query=1'; // loads ./foo.mjs with query of "?query=1"
import './foo.mjs?query=2'; // loads ./foo.mjs with query of "?query=2"
```

For now, only modules using the `file:` protocol can be loaded.
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/errors.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -740,6 +740,9 @@ E('ERR_NAPI_INVALID_TYPEDARRAY_ALIGNMENT',
'start offset of %s should be a multiple of %s', RangeError);
E('ERR_NAPI_INVALID_TYPEDARRAY_LENGTH',
'Invalid typed array length', RangeError);
E('ERR_NON_EXPLICIT_SPECIFIER',
'The file %s does not include an extension. Imported modules must declare' +
' an extension', Error);
E('ERR_NO_CRYPTO',
'Node.js is not compiled with OpenSSL crypto support', Error);
E('ERR_NO_ICU',
Expand Down
7 changes: 6 additions & 1 deletion lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -42,6 +42,7 @@ const {
const preserveSymlinks = !!process.binding('config').preserveSymlinks;
const preserveSymlinksMain = !!process.binding('config').preserveSymlinksMain;
const experimentalModules = !!process.binding('config').experimentalModules;
const hasLoader = !!process.binding('config').userLoader;

const {
ERR_INVALID_ARG_TYPE,
Expand DownExpand Up@@ -728,7 +729,11 @@ if (experimentalModules) {
// bootstrap main module.
Module.runMain = function() {
// Load the main module--the command line argument.
if (experimentalModules) {
const base = path.basename(process.argv[1]);
const ext = path.extname(base);
const isESM = ext === '.mjs';

if (experimentalModules && (isESM || hasLoader)) {
if (asyncESM === undefined) lazyLoadESM();
asyncESM.loaderPromise.then((loader) => {
return loader.import(getURLFromFilePath(process.argv[1]).pathname);
Expand Down
7 changes: 6 additions & 1 deletion lib/internal/modules/esm/default_resolve.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -11,7 +11,8 @@ const preserveSymlinksMain = !!process.binding('config').preserveSymlinksMain;
const {
ERR_MISSING_MODULE,
ERR_MODULE_RESOLUTION_LEGACY,
ERR_UNKNOWN_FILE_EXTENSION
ERR_UNKNOWN_FILE_EXTENSION,
ERR_NON_EXPLICIT_SPECIFIER
} = require('internal/errors').codes;
const { resolve: moduleWrapResolve } = internalBinding('module_wrap');
const StringStartsWith = Function.call.bind(String.prototype.startsWith);
Expand All@@ -24,6 +25,10 @@ function search(target, base) {
// We cannot search without a base.
throw new ERR_MISSING_MODULE(target);
}
if ((target[0] === '.' || target[0] === '/') &&

@guybedfordguybedfordJun 28, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This check should be target[0] === '.' && (target[1] === '/' || target[1] === '.' && target[2] === '/') || target[0] === '/' to ensure support for filenames starting with .. Alternatively just the regex /^\.{0,2}\//.test(target).

I understand this is an initial implementation, but note that this implementation still supports "main"s and URLs getting default extensions. Happy to do a deeper review in due course.

@YurySolovyovYurySolovyovJun 28, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

maybe String.prototype.startsWith can be used?

@bmeckbmeckJun 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we do use a method, i'd prefer the non-taintable approaches used elsewhere for robustness

(extname(target) <= 1)) {

@guybedfordguybedfordJun 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It may be a little clearer to have an explicit .length check here instead of relying on JavaScript magic.

throw new ERR_NON_EXPLICIT_SPECIFIER(target);
}
try {
return moduleWrapResolve(target, base);
} catch (e) {
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-basic-imports.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
// Flags: --experimental-modules
import '../common';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import assert from 'assert';
import ok from '../fixtures/es-modules/test-esm-ok.mjs';
import okShebang from './test-esm-shebang.mjs';
Expand Down
5 changes: 3 additions & 2 deletions test/es-module/test-esm-cyclic-dynamic-import.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
// Flags: --experimental-modules
import '../common';
import('./test-esm-cyclic-dynamic-import');
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import('./test-esm-cyclic-dynamic-import.mjs');
5 changes: 3 additions & 2 deletions test/es-module/test-esm-double-encoding.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
// Flags: --experimental-modules
import '../common';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';

// Assert we can import files with `%` in their pathname.

import '../fixtures/es-modules/test-esm-double-encoding-native%2520.js';
import '../fixtures/es-modules/test-esm-double-encoding-native%2520.mjs';
3 changes: 2 additions & 1 deletion test/es-module/test-esm-encoded-path.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
// Flags: --experimental-modules
import '../common';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import assert from 'assert';
// ./test-esm-ok.mjs
import ok from '../fixtures/es-modules/test-%65%73%6d-ok.mjs';
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-forbidden-globals.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
// Flags: --experimental-modules
import '../common';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';

// eslint-disable-next-line no-undef
if (typeof arguments !== 'undefined') {
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-import-meta.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
// Flags: --experimental-modules
/* eslint-disable node-core/required-modules */

import '../common';
import '../common/index.mjs';
import assert from 'assert';

assert.strictEqual(Object.getPrototypeOf(import.meta), null);
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-json.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
// Flags: --experimental-modules
import '../common';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import assert from 'assert';
import ok from '../fixtures/es-modules/test-esm-ok.mjs';
import json from '../fixtures/es-modules/json.json';
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-live-binding.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
// Flags: --experimental-modules
/* eslint-disable node-core/required-modules */

import '../common';
import '../common/index.mjs';
import assert from 'assert';

import fs, { readFile, readFileSync } from 'fs';
Expand Down
6 changes: 0 additions & 6 deletions test/es-module/test-esm-main-lookup.mjs
View file
Open in desktop

This file was deleted.

3 changes: 2 additions & 1 deletion test/es-module/test-esm-named-exports.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/builtin-named-exports-loader.mjs
import '../common';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import { readFile } from 'fs';
import assert from 'assert';
import ok from '../fixtures/es-modules/test-esm-ok.mjs';
Expand Down
4 changes: 3 additions & 1 deletion test/es-module/test-esm-namespace.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
// Flags: --experimental-modules
import '../common';
/* eslint-disable node-core/required-modules */

import '../common/index.mjs';
import * as fs from 'fs';
import assert from 'assert';
import Module from 'module';
Expand Down
2 changes: 1 addition & 1 deletion test/es-module/test-esm-preserve-symlinks-not-found.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/not-found-assert-loader.mjs
/* eslint-disable node-core/required-modules */
import './not-found';
import './not-found.mjs';
3 changes: 2 additions & 1 deletion test/es-module/test-esm-require-cache.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
// Flags: --experimental-modules
import '../common';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import '../fixtures/es-module-require-cache/preload.js';
import '../fixtures/es-module-require-cache/counter.js';
import assert from 'assert';
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-shared-loader-dep.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-shared-dep.mjs
import '../common';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import assert from 'assert';
import '../fixtures/es-modules/test-esm-ok.mjs';
import dep from '../fixtures/es-module-loaders/loader-dep.js';
Expand Down
3 changes: 2 additions & 1 deletion test/es-module/test-esm-shebang.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
#! }]) // isn't js
// Flags: --experimental-modules
import '../common';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';

const isJs = true;
export default isJs;
7 changes: 4 additions & 3 deletions test/es-module/test-esm-snapshot.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
// Flags: --experimental-modules
import '../common';
import '../fixtures/es-modules/esm-snapshot-mutator';
import one from '../fixtures/es-modules/esm-snapshot';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import '../fixtures/es-modules/esm-snapshot-mutator.js';
import one from '../fixtures/es-modules/esm-snapshot.js';
import assert from 'assert';

assert.strictEqual(one, 1);
2 changes: 1 addition & 1 deletion test/es-module/test-esm-symlink-main.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,7 @@ const fs = require('fs');
tmpdir.refresh();

const realPath = path.resolve(__dirname, '../fixtures/es-modules/symlink.mjs');
const symlinkPath = path.resolve(tmpdir.path, 'symlink.js');
const symlinkPath = path.resolve(tmpdir.path, 'symlink.mjs');

try {
fs.symlinkSync(realPath, symlinkPath);
Expand Down
10 changes: 4 additions & 6 deletions test/es-module/test-esm-symlink.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -12,8 +12,8 @@ const tmpDir = tmpdir.path;

const entry = path.join(tmpDir, 'entry.mjs');
const real = path.join(tmpDir, 'index.mjs');
const link_absolute_path = path.join(tmpDir, 'absolute');
const link_relative_path = path.join(tmpDir, 'relative');
const link_absolute_path = path.join(tmpDir, 'absolute.mjs');
const link_relative_path = path.join(tmpDir, 'relative.mjs');
const link_ignore_extension = path.join(tmpDir,
'ignore_extension.json');
const link_directory = path.join(tmpDir, 'directory');
Expand All@@ -22,15 +22,13 @@ fs.writeFileSync(real, 'export default [];');
fs.writeFileSync(entry, `
import assert from 'assert';
import real from './index.mjs';
import absolute from './absolute';
import relative from './relative';
import absolute from './absolute.mjs';
import relative from './relative.mjs';
import ignoreExtension from './ignore_extension.json';
import directory from './directory';

assert.strictEqual(absolute, real);
assert.strictEqual(relative, real);
assert.strictEqual(ignoreExtension, real);
assert.strictEqual(directory, real);
`);

try {
Expand Down
2 changes: 1 addition & 1 deletion test/es-module/test-esm-throw-undefined.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,7 +6,7 @@ import assert from 'assert';
async function doTest() {
await assert.rejects(
async () => {
await import('../fixtures/es-module-loaders/throw-undefined');
await import('../fixtures/es-module-loaders/throw-undefined.mjs');
},
(e) => e === undefined
);
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/es-module-loaders/syntax-error-import.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
import { foo, notfound } from './module-named-exports';
import { foo, notfound } from './module-named-exports.mjs';
2 changes: 1 addition & 1 deletion test/fixtures/es-modules/loop.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
import { message } from './message';
import { message } from './message.mjs';

var t = 1;
var k = 1;
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/es-modules/pjson-main/main.js
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions test/fixtures/es-modules/pjson-main/package.json
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions test/fixtures/es-modules/test-esm-double-encoding-native%20.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
'use strict';

// Trivial test to assert we can load files with `%` in their pathname.
// Imported by `test-esm-double-encoding.mjs`.

export default 42;
6 changes: 3 additions & 3 deletions test/message/esm_display_syntax_error_import.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
// Flags: --experimental-modules
/* eslint-disable no-unused-vars */
import '../common';
/* eslint-disable no-unused-vars, node-core/required-modules */
import '../common/index.mjs';
import {
foo,
notfound
} from '../fixtures/es-module-loaders/module-named-exports';
} from '../fixtures/es-module-loaders/module-named-exports.mjs';
2 changes: 1 addition & 1 deletion test/message/esm_display_syntax_error_import.out
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,5 +2,5 @@
file:///*/test/message/esm_display_syntax_error_import.mjs:6
notfound
^^^^^^^^
SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-exports' does not provide an export named 'notfound'
SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-exports.mjs' does not provide an export named 'notfound'
at ModuleJob._instantiate (internal/modules/esm/module_job.js:*:*)
5 changes: 3 additions & 2 deletions test/message/esm_display_syntax_error_import_module.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
// Flags: --experimental-modules
import '../common';
import '../fixtures/es-module-loaders/syntax-error-import';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import '../fixtures/es-module-loaders/syntax-error-import.mjs';
4 changes: 2 additions & 2 deletions test/message/esm_display_syntax_error_import_module.out
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
(node:*) ExperimentalWarning: The ESM module loader is experimental.
file:///*/test/fixtures/es-module-loaders/syntax-error-import.mjs:1
import { foo, notfound } from './module-named-exports';
import { foo, notfound } from './module-named-exports.mjs';
^^^^^^^^
SyntaxError: The requested module './module-named-exports' does not provide an export named 'notfound'
SyntaxError: The requested module './module-named-exports.mjs' does not provide an export named 'notfound'
at ModuleJob._instantiate (internal/modules/esm/module_job.js:*:*)
5 changes: 3 additions & 2 deletions test/message/esm_display_syntax_error_module.mjs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
// Flags: --experimental-modules
import '../common';
import '../fixtures/es-module-loaders/syntax-error';
/* eslint-disable node-core/required-modules */
import '../common/index.mjs';
import '../fixtures/es-module-loaders/syntax-error.mjs';
2 changes: 1 addition & 1 deletion test/parallel/test-module-main-extension-lookup.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,6 +6,6 @@ const { execFileSync } = require('child_process');
const node = process.argv[0];

execFileSync(node, ['--experimental-modules',
fixtures.path('es-modules', 'test-esm-ok')]);
fixtures.path('es-modules', 'test-esm-ok.mjs')]);
execFileSync(node, ['--experimental-modules',
fixtures.path('es-modules', 'noext')]);