Bug 1085481 - Fix generating curl commands with multipart payload r=Honza
authorTom Glowka <glowka.tom@gmail.com>
Thu, 04 Apr 2019 09:21:20 +0000
changeset 467933 c3f507baa3dea6e86274de6d57d3e638f2b3bb47
parent 467932 73388d92a46f87cd923799d94a629db5f4c7e1e4
child 467934 957cc8c178c0ff51070ac45d1b0f140e7d5a26c8
push id112667
push useraiakab@mozilla.com
push dateThu, 04 Apr 2019 16:12:45 +0000
treeherdermozilla-inbound@230bb363f2f3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersHonza
bugs1085481
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1085481 - Fix generating curl commands with multipart payload r=Honza Fix main bug. Refine output of curl with multipart data payload. Add missing units tests, including regression tests. Differential Revision: https://phabricator.services.mozilla.com/D25890
devtools/client/netmonitor/test/browser_net_curl-utils.js
devtools/client/shared/curl.js
devtools/client/shared/test/unit/test_curl.js
devtools/client/shared/test/unit/xpcshell.ini
--- a/devtools/client/netmonitor/test/browser_net_curl-utils.js
+++ b/devtools/client/netmonitor/test/browser_net_curl-utils.js
@@ -135,48 +135,49 @@ function testRemoveBinaryDataFromMultipa
   const text = data.postDataText;
   const binaryRemoved =
     CurlUtils.removeBinaryDataFromMultipartText(text, generatedBoundary);
   const boundary = "--" + generatedBoundary;
 
   const EXPECTED_POSIX_RESULT = [
     "$'",
     boundary,
-    "\\r\\n\\r\\n",
+    "\\r\\n",
     "Content-Disposition: form-data; name=\"param1\"",
     "\\r\\n\\r\\n",
     "value1",
     "\\r\\n",
     boundary,
-    "\\r\\n\\r\\n",
+    "\\r\\n",
     "Content-Disposition: form-data; name=\"file\"; filename=\"filename.png\"",
     "\\r\\n",
     "Content-Type: image/png",
     "\\r\\n\\r\\n",
     boundary + "--",
     "\\r\\n",
     "'",
   ].join("");
 
   const EXPECTED_WIN_RESULT = [
-    '"' + boundary + '"^',
-    "\u000d\u000A\u000d\u000A",
-    '"Content-Disposition: form-data; name=""param1"""^',
-    "\u000d\u000A\u000d\u000A",
-    '"value1"^',
-    "\u000d\u000A",
-    '"' + boundary + '"^',
-    "\u000d\u000A\u000d\u000A",
-    '"Content-Disposition: form-data; name=""file""; filename=""filename.png"""^',
-    "\u000d\u000A",
-    '"Content-Type: image/png"^',
-    "\u000d\u000A\u000d\u000A",
-    '"' + boundary + '--"^',
-    "\u000d\u000A",
-    '""',
+    '"',
+    boundary,
+    '"^\u000d\u000A\u000d\u000A"',
+    'Content-Disposition: form-data; name=""param1""',
+    '"^\u000d\u000A\u000d\u000A""^\u000d\u000A\u000d\u000A"',
+    "value1",
+    '"^\u000d\u000A\u000d\u000A"',
+    boundary,
+    '"^\u000d\u000A\u000d\u000A"',
+    'Content-Disposition: form-data; name=""file""; filename=""filename.png""',
+    '"^\u000d\u000A\u000d\u000A"',
+    "Content-Type: image/png",
+    '"^\u000d\u000A\u000d\u000A""^\u000d\u000A\u000d\u000A"',
+    boundary + "--",
+    '"^\u000d\u000A\u000d\u000A"',
+    '"',
   ].join("");
 
   if (Services.appinfo.OS != "WINNT") {
     is(CurlUtils.escapeStringPosix(binaryRemoved), EXPECTED_POSIX_RESULT,
       "The mulitpart request payload should not contain binary data.");
   } else {
     is(CurlUtils.escapeStringWin(binaryRemoved), EXPECTED_WIN_RESULT,
       "WinNT: The mulitpart request payload should not contain binary data.");
@@ -235,17 +236,17 @@ function testEscapeStringWin() {
     "Percent signs should be escaped.");
 
   const backslashes = "\\A simple string\\";
   is(CurlUtils.escapeStringWin(backslashes), '"\\\\A simple string\\\\"',
     "Backslashes should be escaped.");
 
   const newLines = "line1\r\nline2\r\nline3";
   is(CurlUtils.escapeStringWin(newLines),
-    '"line1"^\u000d\u000A"line2"^\u000d\u000A"line3"',
+    '"line1"^\u000d\u000A\u000d\u000A"line2"^\u000d\u000A\u000d\u000A"line3"',
     "Newlines should be escaped.");
 }
 
 async function createCurlData(selected, getLongString, requestData) {
   const { id, url, method, httpVersion } = selected;
 
   // Create a sanitized object for the Curl command generator.
   const data = {
--- a/devtools/client/shared/curl.js
+++ b/devtools/client/shared/curl.js
@@ -69,29 +69,36 @@ const Curl = {
     // Add URL.
     command.push(escapeString(data.url));
 
     let postDataText = null;
     const multipartRequest = utils.isMultipartRequest(data);
 
     // Create post data.
     const postData = [];
-    if (utils.isUrlEncodedRequest(data) ||
-          ["PUT", "POST", "PATCH"].includes(data.method)) {
-      postDataText = data.postDataText;
-      postData.push("--data");
-      postData.push(escapeString(utils.writePostDataTextParams(postDataText)));
-      ignoredHeaders.add("content-length");
-    } else if (multipartRequest) {
+    if (multipartRequest) {
+      // WINDOWS KNOWN LIMITATIONS: Due to the specificity of running curl on
+      // cmd.exe even correctly escaped windows newline \r\n will be
+      // treated by curl as plain local newline. It corresponds in unix
+      // to single \n and that's what curl will send in payload.
+      // It may be particularly hurtful for multipart/form-data payloads
+      // which composed using \n only, not \r\n, may be not parsable for
+      // peers which split parts of multipart payload using \r\n.
       postDataText = data.postDataText;
       postData.push("--data-binary");
       const boundary = utils.getMultipartBoundary(data);
       const text = utils.removeBinaryDataFromMultipartText(postDataText, boundary);
       postData.push(escapeString(text));
       ignoredHeaders.add("content-length");
+    } else if (utils.isUrlEncodedRequest(data) ||
+          ["PUT", "POST", "PATCH"].includes(data.method)) {
+      postDataText = data.postDataText;
+      postData.push("--data");
+      postData.push(escapeString(utils.writePostDataTextParams(postDataText)));
+      ignoredHeaders.add("content-length");
     }
     // curl generates the host header itself based on the given URL
     ignoredHeaders.add("host");
 
     // Add -I (HEAD)
     // For servers that supports HEAD.
     // This will fetch the header of a document only.
     if (data.method == "HEAD") {
@@ -275,19 +282,19 @@ const CurlUtils = {
         continue;
       }
       contentDispositionLine = contentDispositionLine.toLowerCase();
       if (contentDispositionLine.includes("content-disposition: form-data")) {
         if (contentDispositionLine.includes("filename=")) {
           // The header lines and the binary blob is separated by 2 CRLF's.
           // Add only the headers to the result.
           const headers = part.split("\r\n\r\n")[0];
-          result += boundary + "\r\n" + headers + "\r\n\r\n";
+          result += boundary + headers + "\r\n\r\n";
         } else {
-          result += boundary + "\r\n" + part;
+          result += boundary + part;
         }
       }
     }
     result += boundary + "--\r\n";
 
     return result;
   },
 
@@ -382,18 +389,23 @@ const CurlUtils = {
        variable value. So %% becomes "%""%". Even if an env variable ""
        (2 doublequotes) is declared, the cmd.exe will not
        substitute it with its value.
 
        Replace each backslash with double backslash to make sure
        MS Crt arguments parser won't collapse them.
 
        Replace new line outside of quotes since cmd.exe doesn't let
-       to do it inside.
+       to do it inside. At the same time it gets duplicated,
+       because first newline is consumed by ^.
+       So for quote: `"Text-start\r\ntext-continue"`,
+       we get: `"Text-start"^\r\n\r\n"text-continue"`,
+       where `^\r\n` is just breaking the command, the `\r\n` right
+       after is actual escaped newline.
     */
     return "\"" + str.replace(/"/g, "\"\"")
                      .replace(/%/g, "\"%\"")
                      .replace(/\\/g, "\\\\")
-                     .replace(/[\r\n]+/g, "\"^$&\"") + "\"";
+                     .replace(/[\r\n]{1,2}/g, "\"^$&$&\"") + "\"";
   },
 };
 
 exports.CurlUtils = CurlUtils;
new file mode 100644
--- /dev/null
+++ b/devtools/client/shared/test/unit/test_curl.js
@@ -0,0 +1,246 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Tests utility functions contained in `source-utils.js`
+ */
+
+const {require} = ChromeUtils.import("resource://devtools/shared/Loader.jsm");
+const curl = require("devtools/client/shared/curl");
+const Curl = curl.Curl;
+const CurlUtils = curl.CurlUtils;
+
+const Services = require("Services");
+
+// Test `Curl.generateCommand` headers forwarding/filtering
+add_task(async function() {
+  const request = {
+    url: "https://example.com/form/",
+    method: "GET",
+    headers: [
+      {name: "Host", value: "example.com"},
+      {
+        name: "User-Agent",
+        value: "Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0",
+      },
+      {name: "Accept", value: "*/*"},
+      {name: "Accept-Language", value: "en-US,en;q=0.5"},
+      {name: "Accept-Encoding", value: "gzip, deflate, br"},
+      {name: "Origin", value: "https://example.com"},
+      {name: "Connection", value: "keep-alive"},
+      {name: "Referer", value: "https://example.com/home/"},
+      {name: "Content-Type", value: "text/plain"},
+    ],
+    httpVersion: "HTTP/2.0",
+  };
+
+  const cmd = Curl.generateCommand(request);
+  const curlParams = parseCurl(cmd);
+
+  ok(!headerTypeInParams(curlParams, "Host"),
+    "host header ignored - to be generated from url");
+  ok(exactHeaderInParams(curlParams, "Accept: */*"),
+    "accept header present in curl command");
+  ok(exactHeaderInParams(curlParams,
+    "User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0"),
+    "user-agent header present in curl command"
+  );
+  ok(exactHeaderInParams(curlParams, "Accept-Language: en-US,en;q=0.5"),
+    "accept-language header present in curl output");
+  ok(!headerTypeInParams(curlParams, "Accept-Encoding") &&
+    inParams(curlParams, "--compressed"),
+    '"--compressed" param replaced accept-encoding header');
+  ok(exactHeaderInParams(curlParams, "Origin: https://example.com"),
+    "origin header present in curl output");
+  ok(exactHeaderInParams(curlParams, "Connection: keep-alive"),
+    "connection header present in curl output");
+  ok(exactHeaderInParams(curlParams, "Referer: https://example.com/home/"),
+    "referer header present in curl output");
+  ok(exactHeaderInParams(curlParams, "Content-Type: text/plain"),
+    "content-type header present in curl output");
+  ok(!inParams(curlParams, "--data"), "no data param in GET curl output");
+});
+
+// Test `Curl.generateCommand` data POSTing
+add_task(async function() {
+  const request = {
+    url: "https://example.com/form/",
+    method: "POST",
+    headers: [
+      {name: "Content-Length", value: "1000"},
+      {name: "Content-Type", value: "text/plain"},
+    ],
+    httpVersion: "HTTP/2.0",
+    postDataText: "A piece of plain payload text",
+  };
+
+  const cmd = Curl.generateCommand(request);
+  const curlParams = parseCurl(cmd);
+
+  ok(!headerTypeInParams(curlParams, "Content-Length"),
+    "content-length header ignored - curl generates new one");
+  ok(exactHeaderInParams(curlParams, "Content-Type: text/plain"),
+    "content-type header present in curl output");
+  ok(inParams(curlParams, "--data"), '"--data" param present in curl output');
+  ok(inParams(curlParams, `--data ${quote(request.postDataText)}`),
+    "proper payload data present in output");
+});
+
+// Test `Curl.generateCommand` multipart data POSTing
+add_task(async function() {
+  const boundary = "----------14808";
+  const request = {
+    url: "https://example.com/form/",
+    method: "POST",
+    headers: [
+      {
+        name: "Content-Type",
+        value: `multipart/form-data; boundary=${boundary}`,
+      },
+    ],
+    httpVersion: "HTTP/2.0",
+    postDataText: [
+      `--${boundary}`,
+      "Content-Disposition: form-data; name=\"field_one\"",
+      "",
+      "value_one",
+      `--${boundary}`,
+      "Content-Disposition: form-data; name=\"field_two\"",
+      "",
+      "value two",
+      `--${boundary}--`,
+      "",
+    ].join("\r\n"),
+  };
+
+  const cmd = Curl.generateCommand(request);
+
+  // Check content type
+  const contentTypePos = cmd.indexOf(headerParamPrefix("Content-Type"));
+  const contentTypeParam = headerParam(
+    `Content-Type: multipart/form-data; boundary=${boundary}`
+  );
+  ok(contentTypePos !== -1, "content type header present in curl output");
+  equal(cmd.substr(contentTypePos, contentTypeParam.length), contentTypeParam,
+    "proper content type header present in curl output"
+  );
+
+  // Check binary data
+  const dataBinaryPos = cmd.indexOf("--data-binary");
+  const dataBinaryParam =
+    `--data-binary ${isWin() ? "" : "$"}${escapeNewline(quote(request.postDataText))}`;
+  ok(dataBinaryPos !== -1, "--data-binary param present in curl output");
+  equal(cmd.substr(dataBinaryPos, dataBinaryParam.length), dataBinaryParam,
+    "proper multipart data present in curl output"
+  );
+});
+
+// Test `CurlUtils.removeBinaryDataFromMultipartText` doesn't change text data
+add_task(async function() {
+  const boundary = "----------14808";
+  const postTextLines = [
+    `--${boundary}`,
+    "Content-Disposition: form-data; name=\"field_one\"",
+    "",
+    "value_one",
+    `--${boundary}`,
+    "Content-Disposition: form-data; name=\"field_two\"",
+    "",
+    "value two",
+    `--${boundary}--`,
+    "",
+  ];
+
+  const cleanedText =
+    CurlUtils.removeBinaryDataFromMultipartText(postTextLines.join("\r\n"), boundary);
+  equal(cleanedText, postTextLines.join("\r\n"),
+    "proper non-binary multipart text unchanged");
+});
+
+// Test `CurlUtils.removeBinaryDataFromMultipartText` removes binary data
+add_task(async function() {
+  const boundary = "----------14808";
+  const postTextLines = [
+    `--${boundary}`,
+    "Content-Disposition: form-data; name=\"field_one\"",
+    "",
+    "value_one",
+    `--${boundary}`,
+    "Content-Disposition: form-data; name=\"field_two\"; filename=\"file_field_two.txt\"",
+    "",
+    "file content",
+    `--${boundary}--`,
+    "",
+  ];
+
+  const cleanedText =
+    CurlUtils.removeBinaryDataFromMultipartText(postTextLines.join("\r\n"), boundary);
+  postTextLines.splice(7, 1);
+  equal(
+    cleanedText, postTextLines.join("\r\n"),
+    "file content removed from multipart text"
+  );
+});
+
+function isWin() {
+  return Services.appinfo.OS === "WINNT";
+}
+
+const QUOTE = isWin() ? "\"" : "'";
+
+// Quote a string, escape the quotes inside the string
+function quote(str) {
+  let escaped;
+  if (isWin()) {
+    escaped = str.replace(new RegExp(QUOTE, "g"), `${QUOTE}${QUOTE}`);
+  } else {
+    escaped = str.replace(new RegExp(QUOTE, "g"), `\\${QUOTE}`);
+  }
+  return QUOTE + escaped + QUOTE;
+}
+
+function escapeNewline(txt) {
+  if (isWin()) {
+    // Add `"` to close quote, then escape newline outside of quote, then start new quote
+    return txt.replace(/[\r\n]{1,2}/g, '"^$&$&"');
+  }
+  return txt.replace(/\r/g, "\\r").replace(/\n/g, "\\n");
+}
+
+// Header param is formatted as -H "Header: value" or -H 'Header: value'
+function headerParam(h) {
+  return "-H " + quote(h);
+}
+
+// Header param prefix is formatted as `-H "HeaderName` or `-H 'HeaderName`
+function headerParamPrefix(headerName) {
+  return `-H ${QUOTE}${headerName}`;
+}
+
+// If any params startswith `-H "HeaderName` or `-H 'HeaderName`
+function headerTypeInParams(curlParams, headerName) {
+  return curlParams.some(
+    param => param.toLowerCase().startsWith(headerParamPrefix(headerName).toLowerCase())
+  );
+}
+
+function exactHeaderInParams(curlParams, header) {
+  return curlParams.some(param => param === headerParam(header));
+}
+
+function inParams(curlParams, param) {
+  return curlParams.some(p => p.startsWith(param));
+}
+
+// Parse complete curl command to array of params. Can be applied to simple headers/data,
+// but will not on WIN with sophisticated values of --data-binary with e.g. escaped quotes
+function parseCurl(curlCmd) {
+  // This monster regexp parses the command line into an array of arguments,
+  // recognizing quoted args with matching quotes and escaped quotes inside:
+  // [ "curl 'url'", "--standalone-arg", "-arg-with-quoted-string 'value\'s'" ]
+  const matchRe = /[-A-Za-z1-9]+(?: \$?([\"'])(?:\\\1|.)*?\1)?/g;
+  return curlCmd.match(matchRe);
+}
+
--- a/devtools/client/shared/test/unit/xpcshell.ini
+++ b/devtools/client/shared/test/unit/xpcshell.ini
@@ -13,16 +13,17 @@ support-files =
 [test_bezierCanvas.js]
 [test_cssAngle.js]
 [test_cssColor-01.js]
 [test_cssColor-02.js]
 [test_cssColor-03.js]
 [test_cssColor-8-digit-hex.js]
 [test_cssColorDatabase.js]
 [test_cubicBezier.js]
+[test_curl.js]
 [test_escapeCSSComment.js]
 [test_parseDeclarations.js]
 [test_parsePseudoClassesAndAttributes.js]
 [test_parseSingleValue.js]
 [test_rewriteDeclarations.js]
 [test_source-utils.js]
 [test_suggestion-picker.js]
 [test_undoStack.js]