bug #561 Add extra checks to the copyFiles feature (Lyrkan)

This PR was merged into the master branch.

Discussion
----------

Add extra checks to the copyFiles feature

This PR adds some extra checks to make sure that files *really* match the pattern used in `copyFiles(...)` before the `file-loader` is called (closes #556).

For a full description of the issue see #556 (comment), but here is a TL;DR:

Currently, if you have a folder that contains a `foo.js` file and calls `copyFile(...)` with a pattern that excludes `.js` files, that file will still be copied.

This is caused by the `resolve.extensions` mechanism of Webpack that also applies to `require.context(...)` calls (on which this feature relies) and includes `./foo` (without the extensions) to the list of files that should be imported.

Commits
-------

eea91a6 Add extra checks to the copyFiles feature

lib/config-generator.js

@@ -183,14 +183,23 @@ class ConfigGenerator {
                         copyTo = this.webpackConfig.useVersioning ? '[path][name].[hash:8].[ext]' : '[path][name].[ext]';
                     }
 
-                    const copyFilesLoader = require.resolve('./webpack/copy-files-loader');
-                    const fileLoader = require.resolve('file-loader');
-                    const copyContext = entry.context ? path.resolve(this.webpackConfig.getContext(), entry.context) : copyFrom;
-                    const requireContextParam = `!${copyFilesLoader}!${fileLoader}?context=${copyContext}&name=${copyTo}!${copyFrom}`;
+                    const copyFilesLoaderPath = require.resolve('./webpack/copy-files-loader');
+                    const copyFilesLoaderConfig = `${copyFilesLoaderPath}?${JSON.stringify({
+                        // file-loader options
+                        context: entry.context ? path.resolve(this.webpackConfig.getContext(), entry.context) : copyFrom,
+                        name: copyTo,
+
+                        // custom copy-files-loader options
+                        // the patternSource is base64 encoded in case
+                        // it contains characters that don't work with
+                        // the "inline loader" syntax
+                        patternSource: Buffer.from(entry.pattern.source).toString('base64'),
+                        patternFlags: entry.pattern.flags,
+                    })}`;
 
                     return buffer + `
                         const context_${index} = require.context(
-                            '${stringEscaper(requireContextParam)}',
+                            '${stringEscaper(`!${copyFilesLoaderConfig}!${copyFrom}`)}',
                             ${!!entry.includeSubdirectories},
                             ${entry.pattern}
                         );

lib/webpack/copy-files-loader.js

@@ -10,8 +10,13 @@
 'use strict';
 
 const LoaderDependency = require('webpack/lib/dependencies/LoaderDependency');
+const fileLoader = require('file-loader');
+const loaderUtils = require('loader-utils');
+const path = require('path');
 
-module.exports = function loader(source) {
+module.exports.raw = true; // Needed to avoid corrupted binary files
+
+module.exports.default = function loader(source) {
     // This is a hack that allows `Encore.copyFiles()` to support
     // JSON files using the file-loader (which is not something
     // that is supported in Webpack 4, see https://github.com/symfony/webpack-encore/issues/535)
@@ -39,5 +44,31 @@ module.exports = function loader(source) {
         this._module.parser = factory.getParser(requiredType);
     }
 
-    return source;
+    const options = loaderUtils.getOptions(this);
+
+    // Retrieve the real path of the resource, relative
+    // to the context used by copyFiles(...)
+    const context = options.context;
+    const resourcePath = this.resourcePath;
+    const relativeResourcePath = path.relative(context, resourcePath);
+
+    // Retrieve the pattern used in copyFiles(...)
+    // The "source" part of the regexp is base64 encoded
+    // in case it contains characters that don't work with
+    // the "inline loader" syntax
+    const pattern = new RegExp(
+        Buffer.from(options.patternSource, 'base64').toString(),
+        options.patternFlags
+    );
+
+    // If the pattern does not match the ressource's path
+    // it probably means that the import was resolved using the
+    // "resolve.extensions" parameter of Webpack (for instance
+    // if "./test.js" was matched by "./test").
+    if (!pattern.test(relativeResourcePath)) {
+        return 'module.exports = "";';
+    }
+
+    // If everything is OK, let the file-loader do the copy
+    return fileLoader.bind(this)(source);
 };

test/functional.js

@@ -2056,9 +2056,10 @@ module.exports = {
                             'foo.css',
                             'foo.js',
                             'foo.json',
+                            'foo.png',
                         ]);
 
-                    for (const file of ['foo.css', 'foo.js', 'foo.json']) {
+                    for (const file of ['foo.css', 'foo.js', 'foo.json', 'foo.png']) {
                         webpackAssert.assertOutputFileContains(
                             file,
                             'This is an invalid content to check that the file is still copied'
@@ -2068,6 +2069,56 @@ module.exports = {
                     done();
                 });
             });
+
+            it('Do not copy files excluded by a RegExp', (done) => {
+                const config = createWebpackConfig('www/build', 'production');
+                config.addEntry('main', './js/no_require');
+                config.setPublicPath('/build');
+
+                // foo.css and foo.js should match this rule
+                // and be versioned
+                config.copyFiles({
+                    from: './copy',
+                    to: './[path][name]-[hash].[ext]',
+                    pattern: /\.(css|js)$/,
+                });
+
+                // foo.css and foo.js should *not* match this rule
+                config.copyFiles({
+                    from: './copy',
+                    to: './[path][name].[ext]',
+                    pattern: /\.(?!(css|js)$)([^.]+$)/
+                });
+
+                // By default the optimize-css-assets-webpack-plugin will
+                // run on ALL emitted CSS files, which includes the ones
+                // handled by `Encore.copyFiles()`.
+                // We disable it for this test since our CSS file will
+                // not be valid and can't be handled by this plugin.
+                config.configureOptimizeCssPlugin(options => {
+                    options.assetNameRegExp = /^$/;
+                });
+
+                testSetup.runWebpack(config, (webpackAssert) => {
+                    expect(config.outputPath).to.be.a.directory()
+                        .with.files([
+                            'entrypoints.json',
+                            'runtime.js',
+                            'main.js',
+                            'manifest.json',
+
+                            // 1st rule
+                            'foo-40095734b7c5293c04603aa78333c23e.css',
+                            'foo-40095734b7c5293c04603aa78333c23e.js',
+
+                            // 2nd rule
+                            'foo.json',
+                            'foo.png',
+                        ]);
+
+                    done();
+                });
+            });
         });
 
         describe('entrypoints.json & splitChunks()', () => {