fix: proper URL escaping and wrapping (url()) (#627)

lib/loader.js

@@ -83,7 +83,13 @@ module.exports = function(content, map) {
 		}
 
 		cssAsString = cssAsString.replace(result.importItemRegExpG, importItemMatcher.bind(this));
-		if(query.url !== false) {
+
+		// helper for ensuring valid CSS strings from requires
+		var urlEscapeHelper = "";
+
+		if(query.url !== false && result.urlItems.length > 0) {
+			urlEscapeHelper = "var escape = require(" + loaderUtils.stringifyRequest(this, require.resolve("./url/escape.js")) + ");\n";
+
 			cssAsString = cssAsString.replace(result.urlItemRegExpG, function(item) {
 				var match = result.urlItemRegExp.exec(item);
 				var idx = +match[1];
@@ -95,15 +101,14 @@ module.exports = function(content, map) {
 				if(idx > 0) { // idx === 0 is catched by isUrlRequest
 					// in cases like url('webfont.eot?#iefix')
 					urlRequest = url.substr(0, idx);
-					return "\" + require(" + loaderUtils.stringifyRequest(this, urlRequest) + ") + \"" +
+					return "\" + escape(require(" + loaderUtils.stringifyRequest(this, urlRequest) + ")) + \"" +
 							url.substr(idx);
 				}
 				urlRequest = url;
-				return "\" + require(" + loaderUtils.stringifyRequest(this, urlRequest) + ") + \"";
+				return "\" + escape(require(" + loaderUtils.stringifyRequest(this, urlRequest) + ")) + \"";
 			}.bind(this));
 		}
-
-
+
 		var exportJs = compileExports(result, importItemMatcher.bind(this), camelCaseKeys);
 		if (exportJs) {
 			exportJs = "exports.locals = " + exportJs + ";";
@@ -127,7 +132,8 @@ module.exports = function(content, map) {
 		}
 
 		// embed runtime
-		callback(null, "exports = module.exports = require(" +
+		callback(null, urlEscapeHelper +
+			"exports = module.exports = require(" +
 			loaderUtils.stringifyRequest(this, require.resolve("./css-base.js")) +
 			")(" + sourceMap + ");\n" +
 			"// imports\n" +

lib/processCss.js

@@ -104,10 +104,8 @@ var parserPlugin = postcss.plugin("css-loader-parser", function(options) {
 					break;
 				case "url":
 					if (options.url && item.url.replace(/\s/g, '').length && !/^#/.test(item.url) && (isAlias(item.url) || loaderUtils.isUrlRequest(item.url, options.root))) {
-						// Don't remove quotes around url when contain space
-						if (item.url.indexOf(" ") === -1) {
-							item.stringType = "";
-						}
+						// Strip quotes, they will be re-added if the module needs them
+						item.stringType = "";
 						delete item.innerSpacingBefore;
 						delete item.innerSpacingAfter;
 						var url = item.url;

lib/url/escape.js

@@ -0,0 +1,13 @@
+module.exports = function escape(url) {
+    // If url is already wrapped in quotes, remove them
+    if (/^['"].*['"]$/.test(url)) {
+        url = url.slice(1, -1);
+    }
+    // Should url be wrapped?
+    // See https://drafts.csswg.org/css-values-3/#urls
+    if (/["'() \t\n]/.test(url)) {
+        return '"' + url.replace(/"/g, '\\"').replace(/\n/g, '\\n') + '"'
+    }
+
+    return url
+}

test/helpers.js

@@ -12,6 +12,8 @@ function getEvaluated(output, modules) {
 		fn(m, m.exports, function(module) {
 			if(module.indexOf("css-base") >= 0)
 				return require("../lib/css-base");
+			if(module.indexOf("url/escape") >= 0)
+				return require("../lib/url/escape");
 			if(module.indexOf("-!/path/css-loader!") === 0)
 				module = module.substr(19);
 			if(modules && modules[module])

test/moduleTestCases/urls/expected.css

@@ -2,7 +2,7 @@
 	background: url({./module});
 	background: url({./module});
 	background: url("{./module module}");
-	background: url('{./module module}');
+	background: url("{./module module}");
 	background: url({./module});
 	background: url({./module}#?iefix);
 	background: url("#hash");

test/urlTest.js

@@ -19,7 +19,7 @@ describe("url", function() {
 		[1, ".class { background: green url(\"{./img img.png}\") xyz }", ""]
 	]);
 	test("background 2 img contain space in name", ".class { background: green url( 'img img.png' ) xyz }", [
-		[1, ".class { background: green url('{./img img.png}') xyz }", ""]
+		[1, ".class { background: green url(\"{./img img.png}\") xyz }", ""]
 	]);
 	test("background img absolute", ".class { background: green url(/img.png) xyz }", [
 		[1, ".class { background: green url(/img.png) xyz }", ""]
@@ -103,6 +103,28 @@ describe("url", function() {
 	test("external schema-less url", ".class { background: green url(//raw.githubusercontent.com/webpack/media/master/logo/icon.png) xyz }", [
 		[1, ".class { background: green url(//raw.githubusercontent.com/webpack/media/master/logo/icon.png) xyz }", ""]
 	]);
+
+	test("module wrapped in spaces", ".class { background: green url(module) xyz }", [
+		[1, ".class { background: green url(module-wrapped) xyz }", ""]
+	], "", { './module': "\"module-wrapped\"" });
+	test("module with space", ".class { background: green url(module) xyz }", [
+		[1, ".class { background: green url(\"module with space\") xyz }", ""]
+	], "", { './module': "module with space" });
+	test("module with quote", ".class { background: green url(module) xyz }", [
+		[1, ".class { background: green url(\"module\\\"with\\\"quote\") xyz }", ""]
+	], "", { './module': "module\"with\"quote" });
+	test("module with quote wrapped", ".class { background: green url(module) xyz }", [
+		[1, ".class { background: green url(\"module\\\"with\\\"quote\\\"wrapped\") xyz }", ""]
+	], "", { './module': "\"module\"with\"quote\"wrapped\"" });
+	test("module with parens", ".class { background: green url(module) xyz }", [
+		[1, ".class { background: green url(\"module(with-parens)\") xyz }", ""]
+	], "", { './module': 'module(with-parens)' });
+	test("module with newline", ".class { background: green url(module) xyz }", [
+		[1, ".class { background: green url(\"module\\nwith\\nnewline\") xyz }", ""]
+	], "", { './module': "module\nwith\nnewline" });
+	test("module from url-loader", ".class { background: green url(module) xyz }", [
+		[1, ".class { background: green url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAA) xyz }", ""]
+	], "", { './module': "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAA" });
 
 	test("background img with url", ".class { background: green url( \"img.png\" ) xyz }", [
 		[1, ".class { background: green url( \"img.png\" ) xyz }", ""]