js: add support for the 'node:' prefix for importing internal modules

[kaeluka]

Context

import fs from 'node:fs/promises';

Is equivalent to

import fs from 'fs/promises';

if there is no node_modules/fs folder (which would be extremely bad practice).

References

https://nodejs.org/api/esm.html#node-imports
https://2ality.com/2021/12/node-protocol-imports.html

[erik-krogh]

Could you add a test. (Maybe test that a require("node:fs").writeFileSync(sink, xx) works with path-injection).

And an evaluation.
You are touching something that affects basically all queries, my usual goto for that is nightly + security-extended.

[kaeluka]

Thanks, @erik-krogh!

The change is now tested (I also confirmed that the test fails when undoing my change — manually). But I didn't make a new test, rather modified an existing one. Otherwise, the new test would've more been about the path-injection than about the import syntax.

I also started an experiment.

[erik-krogh]

The change is now tested (I also confirmed that the test fails when undoing my change — manually).

I would rather have a some new code, such that both cases are tested.
Adding this to the bottom of the file should work:

var nodefs = require('node:fs');

var server2 = http.createServer(function(req, res) {
  let path = url.parse(req.url, true).query.path;
  
  nodefs.readFileSync(path); // NOT OK
});

[kaeluka]

Alright, @erik-krogh, I've added that snippet instead and amended the .expected file.

[erik-krogh]

👍

I was wondering for a while whether supporting the node: prefix on all module imports was the right thing to do.
But I think it's fine for now, and we can consider what we want to do long term when such prefixes begin seeing more adoption.

[kaeluka]

My thinking is that it's pbly better because it will make potential problems visible at least (FPs are easier to find than FNs). But we can easily undo the change if it proves wrong.