Fix stringifyRequest messing with loader configs

stringifyRequest used to replace all backslashes with slashes inside the whole request, accidentally also on windows. This fix separates first the config from the path before applying path normalizations.

#37 #54 #55

index.js

@@ -7,6 +7,7 @@ var emojiRegex = /[\uD800-\uDFFF]./;
 var emojiList = require("emojis-list").filter(function(emoji) {
 	return emojiRegex.test(emoji)
 });
+var matchAbsolutePath = /^\/|^[A-Z]:/i; // node 0.10 does not support path.isAbsolute()
 
 var baseEncodeTables = {
 	26: "abcdefghijklmnopqrstuvwxyz",
@@ -130,20 +131,14 @@ exports.stringifyRequest = function(loaderContext, request) {
 	var splitted = request.split("!");
 	var context = loaderContext.context || (loaderContext.options && loaderContext.options.context);
 	return JSON.stringify(splitted.map(function(part) {
-		if(/^\/|^[A-Z]:/i.test(part) && context) {
-			if ( part.indexOf("?") > 0 ) {
-				var splittedPart = part.split("?");
-				part = path.relative(context, splittedPart[0]) + "?" + splittedPart[1];
-			} else {
-				part = path.relative(context, part);
-			}
-			if(/^[A-Z]:/i.test(part)) {
-				return part;
-			} else {
-				return "./" + part.replace(/\\/g, "/");
-			}
+		// First, separate singlePath from query, because the query might contain paths again
+		var splittedPart = part.match(/^(.*?)(\?.*)/);
+		var singlePath = splittedPart ? splittedPart[1] : part;
+		var query = splittedPart ? splittedPart[2] : "";
+		if(matchAbsolutePath.test(singlePath) && context) {
+			singlePath = path.relative(context, singlePath);
 		}
-		return part;
+		return path.normalize(singlePath) + query;
 	}).join("!"));
 };
 

test/index.js

@@ -1,6 +1,10 @@
 var assert = require("assert"),
+	path = require("path"),
 	loaderUtils = require("../");
 
+var n = path.normalize,
+	s = JSON.stringify;
+
 function ExpectedError(regex) { this.regex = regex; }
 ExpectedError.prototype.matches = function (err) {
 	return this.regex.test(err.message);
@@ -69,7 +73,7 @@ describe("loader-utils", function() {
 	describe("#parseString", function() {
 		[
 			["test string", "test string"],
-			[JSON.stringify("!\"§$%&/()=?'*#+,.-;öäü:_test"), "!\"§$%&/()=?'*#+,.-;öäü:_test"],
+			[s("!\"§$%&/()=?'*#+,.-;öäü:_test"), "!\"§$%&/()=?'*#+,.-;öäü:_test"],
 			["'escaped with single \"'", 'escaped with single "'],
 			["invalid \"' string", "invalid \"' string"],
 			["\'inconsistent start and end\"", "\'inconsistent start and end\""]
@@ -233,18 +237,61 @@ describe("loader-utils", function() {
 		]);
 	});
 
-	// request with relative path args
 	describe("#stringifyRequest", function() {
-		var pathArgsString = "path=../../thing/file";
-		var pathArgsObjString = JSON.stringify( { "path-to-file": "../../thing/file" } );
-		[
-			["/path/to/thing", "/path/to/thing/file!/path/to/thing/file?" + pathArgsString, JSON.stringify("./file!./file?" + pathArgsString)],
-			["/path/to/thing", "/path/to/thing/file!/path/to/thing/file?" + pathArgsObjString, JSON.stringify("./file!./file?" + pathArgsObjString)]
-		].forEach(function(test) {
-			it("should stringify " + test[1] + " " + test[2], function() {
-				var request = loaderUtils.stringifyRequest({ context: test[0] }, test[1]);
-				assert.equal(request, test[2]);
-			});
+		// We know that query strings that contain paths and question marks can be problematic.
+		// We must ensure that stringifyRequest is not messing with them
+		var paramQueryString = "?posix=path/to/thing&win=path\\to\\thing&questionMark?";
+		var jsonQueryString = "?" + s({
+			questionMark: "?",
+			posix: "path/to/thing",
+			win: "path\\to\\file"
+		});
+		var cases = [
+			{ request: "a.js", expected: s("a.js") },
+			{ request: "a/b.js", expected: s(n("a/b.js")) },
+			{ request: "a.js" + paramQueryString, expected: s("a.js" + paramQueryString) },
+			{ request: "a.js" + jsonQueryString, expected: s("a.js" + jsonQueryString) },
+			{ context: "/path/to/thing", request: "/path/to/module/a.js", expected: s(n("../module/a.js")) },
+			{
+				context: "/path/to/thing",
+				request: "/path/to/module/a.js" + paramQueryString,
+				expected: s(n("../module/a.js") + paramQueryString)
+			},
+			{
+				request:
+					["a.js", "b.js", "c.js"].join("!"),
+				expected: s(
+					["a.js", "b.js", "c.js"].join("!")
+				)
+			},
+			{
+				request:
+					["a/b.js", "c/d.js", "e/f.js"].join("!"),
+				expected: s(
+					[n("a/b.js"), n("c/d.js"), n("e/f.js")].join("!")
+				)
+			},
+			{
+				request:
+					["a/b.js" + paramQueryString, "c/d.js" + jsonQueryString, "e/f.js"].join("!"),
+				expected: s(
+					[n("a/b.js") + paramQueryString, n("c/d.js") + jsonQueryString, n("e/f.js")].join("!")
+				)
+			},
+			{
+				context: "/path/to/thing",
+				request:
+					["/a/b.js" + paramQueryString, "c/d.js" + jsonQueryString, "/e/f.js"].join("!"),
+				expected: s(
+					[n("../../../a/b.js") + paramQueryString, n("c/d.js") + jsonQueryString, n("../../../e/f.js")].join("!")
+				)
+			},
+			{ request: "a\\b.js", expected: s(n("a\\b.js")) }
+		].forEach(function (testCase) {
+			it("should stringify request " + testCase.request + " to " + testCase.expected + " inside context " + testCase.context, function () {
+				var actual = loaderUtils.stringifyRequest({ context: testCase.context}, testCase.request);
+				assert.equal(actual, testCase.expected);
+			})
 		});
 	});
 });