Ignore files that have a null byte

[brianmhunt]

Cross-linking rollup/rollup-plugin-commonjs#268

rollup-plugin-commonjs generates a file with a null byte called \u0000commonjsHelpers. This causes fs.stat to fail via the path here (with long paths shortened to ...):

    at nullCheck (fs.js:173:16)
    at Object.fs.stat (fs.js:947:8)
    at FSWatcher.NodeFsHandler._addToNodeFs (.../node_modules/chokidar/lib/nodefs-handler.js:447:20)
    at FSWatcher.<anonymous> (.../node_modules/chokidar/index.js:622:12)
    at .../node_modules/async-each/index.js:16:7
    at Array.forEach (<anonymous>)
    at each (.../node_modules/async-each/index.js:15:11)
    at FSWatcher.add (.../node_modules/chokidar/index.js:621:5)
    at buffer.forEach.m (.../node_modules/karma-rollup-preprocessor/lib/index.js:69:15)

There is probably a better way, but the best I can figure is karma-rollup-preprocessor is the easiest place to fix this, by simply checking for the presence of a null byte before adding a file to the watch list. If a filename has a null byte, ignore it.

A better solution would to be to not have filenames generated in plugins and injected into the watch list, but I'm sure there's a good reason for that.

The test is just if (m.includes('\u0000')) { return } inside the respective forEach functions at Watch.prototype.start and Watch.prototype.clean.

Generally it's considered a security issue if a filename has a null byte, and disallowed in many languages (e.g. Python), so I expect it's a safe change. See eg https://security.stackexchange.com/a/45958/2914

[jlmakes]

Interesting, thanks @brianmhunt

So let's see if I understand—null bytes are a security concern for karma-rollup-preprocessor, but the patch would also serve as a downstream fix for an emergent bug in rollup-plugin-commonjs?

[brianmhunt]

So let's see if I understand—null bytes are a security concern for karma-rollup-preprocessor, but the patch would also serve as a downstream fix for an emergent bug in rollup-plugin-commonjs?

Yes, that is my understanding.

[OverZealous]

To add to this issue, I'm also getting it with babelHelpers now, which is being injected as '\u0000babelHelpers'. I'm assuming that the trend is to use null bytes to identify dynamically generated files so watchers can know to exclude them.

@jlmakes Would you be interested in a PR as @brianmhunt suggested above, to simply ignore any file names with a null byte in them?

This happens to be holding back a major PR for our codebase. 😕

[jlmakes]

If it's just the one-liner he suggested I don't mind implementing it and bumping to 5.1

Edit: Just cut a new release. Let me know if this bug still bites @OverZealous @brianmhunt

[OverZealous]

That would be awesome! It fixed my issue when I added it manually. I just did this:

Watch.prototype.start = function() {
	this.clean()
	this.buffer.forEach(m => {
		if (m.includes('\u0000')) { return } /// <-- here
		if (!this.watchList.has(m)) {

But it could just as easily be merged with existing if.

[OverZealous]

Worked great for me! Thanks a ton!