Closed Bug 1085481 Opened 10 years ago Closed 5 years ago

"Copy as cURL" misses out a bunch of request data

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Version:
35 Branch
Platform:
x86
macOS
Type:
defect

Tracking

(firefox68 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: peterbe, Assigned: glowka.tom)

Details

Attachments

(1 file)

Fix generating curl commands with multipart payload
47 bytes, text/x-phabricator-request
Details | Review 
I have a locally running angularjs app that makes a HTTP POST to the server. It's failing with 400 Bad Request error which is my business to fix. But to make it easier to replicate I use the "Copy as cURL" in the Network panel. Here's what it gives me:

https://gist.github.com/peterbe/2a8d467f1bcde6569d69

This is incomplete.
Clearly it's missing the actual data that is being sent too. Here's the same thing but with using Chrome:

https://gist.github.com/peterbe/a67061d9c386308b2cff

The major difference seems to be the `--data-binary ...` part.
I can reproduce this locally on my laptop but the site is not anywhere on the net.
Firefox 39 (firefox-39.0-8.fc22.x86_64), I can confirm this bug.

"Copy as cURL" ignores request payload.
ff 43.0.4 on OSX can confirm bug

filling out the form at http://www.lightandfitsweepstake.com/app/home/wizard/register.html generates a POST request with an ASCII payload. When you use the dev tool network panel to copy as curl, it sees the post to /api/Sweepstakes, but it ignores the payload. 

payload from network panel:
{"additionalProperties":{},"imageUrl":[null],"firstName":"fname","lastName":"lname","address1":"address1","city":"city","state":"st","postalCode":"zip","birthDate":"mm/dd/yyyy","email":"emailaddress","phone":"phone","agreeToTerms":true,"optIn2":true}

curl request from "copy as curl"
curl 'http://www.lightandfitsweepstake.com/api/Sweepstakes/' -H 'Host: www.lightandfitsweepstake.com' -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:43.0) Gecko/20100101 Firefox/43.0' -H 'Accept: application/json, text/plain, */*' -H 'Accept-Language: en-US,en;q=0.5' --compressed -H 'Content-Type: application/json;charset=utf-8' -H 'Referer: http://www.lightandfitsweepstake.com/' -H 'Content-Length: 291' -H 'Cookie: _ga=GA1.2.823045280.1452805467; AWSELB=497F378F08458425BB411A15E3A4E51856EF49A0F7874E3F4033B2BF5885FBBD958140ABDF36A0D080A4C98030FE899ADF25C84342A403CABE3CDB6CB66D14D4FDA442D4FE; _gat=1' -H 'Connection: keep-alive'
I can confirm as well on the latest aurora. Might be related to https://bugzilla.mozilla.org/show_bug.cgi?id=1031956 (which has been around unfixed for even longer...)
There is also bug 1269468 that requests POST data body to be included in 'Copy as cURL'

Honza
Priority: -- → P2
With both Bug 1031956 and Bug 1269468 fixed, does this bug still exist?
Product: Firefox → DevTools
Can confirm that on ff 60.0.2 (64-bit) macOS 10.13.5.

Sometimes, I get the curl request, but with the body as an empty string, other times, nothing gets copied to the clipboard at all.
I believe the bugs were fixed in FF 62 (e.g. Bug 1452442) - can you check whether the problem exists on the latest FF nightly?
Bug is present in 60.0.2 on linux :-/

When there are post call "save as curl" return empty data like

-H 'Cache-Control: no-cache' --data ''
Indeed, like @WyattJohnson said, sometimes the "Copy as cURL" functionality does not copy anything. There is a workaround I use: right click on the request, then click "Edit and Resend" and send the request again.
There is another small issue if we click "Copy POST Data", the URL parameters are separated by a new line but some security tools like sqlmap require that the POST data is separated by "&" character to replay a request (it looks like it is the standard format used by MITM proxies too).
I guess I should create new reports for these issues?
@baptx The failure to copy should be fixed in FF 62 (Bug 1452442). Can you check the latest nightly and see if it is fixed for you?

For "Copy POST Data", the newline-separated format is intended behaviour. Copy as cURL should be the right way to get the POST data with form encoding (& separators). Hopefully with Bug 1452442 fixed it can be used as intended.
Unfortunately still not fixed.
@didi: Please provide a reproducible example from FF 62 or later of a POST request where the data is not being copied.

As a test, I POSTed this form: http://httpbin.org/forms/post with FF 63 on my Mac, and got the following cURL command:

curl 'http://httpbin.org/post' -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:63.0) Gecko/20100101 Firefox/63.0' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8' -H 'Accept-Language: en-US,en;q=0.5' --compressed -H 'Referer: http://httpbin.org/forms/post' -H 'Content-Type: application/x-www-form-urlencoded' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Cookie: _gauges_unique_hour=1; _gauges_unique_day=1; _gauges_unique_month=1; _gauges_unique_year=1; _gauges_unique=1' -H 'Upgrade-Insecure-Requests: 1' --data 'custname=&custtel=&custemail=&size=medium&topping=bacon&delivery=&comments='

This is the expected output.

@baptx, @Dan, others: if you can, please confirm if this bug is fixed for you?
Flags: needinfo?(info)
Flags: needinfo?(didi)
Flags: needinfo?(baptx.is)
I discovered this while looking at https://app.7mind.de/login. You can just fill in username and password with any junk and look at the post that is sent. The request payload will be

-----------------------------933180046883959081408404421
Content-Disposition: form-data; name="user[email]"

a
-----------------------------933180046883959081408404421
Content-Disposition: form-data; name="user[password]"

a
-----------------------------933180046883959081408404421--

but the curl command that I get from "Copy as cUrl" will be

curl 'https://api.7mind.de/v1/login' -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:64.0) Gecko/20100101 Firefox/64.0' -H 'Accept: */*' -H 'Accept-Language: en-US,en;q=0.5' --compressed -H 'Referer: https://app.7mind.de/' -H 'content-type: multipart/form-data; boundary=---------------------------933180046883959081408404421' -H 'origin: https://app.7mind.de' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'TE: Trailers' --data ''
Flags: needinfo?(didi)
Thanks didi. I found the bug.

In resource://devtools/client/shared/curl.js, starting at line 75:

    // 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) {
      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");
    }

utils.writePostDataTextParams splits the post data into lines and outputs just the last one. I have no idea at all why it's written to do that (+needinfo Honza, who might know more about the logic here), because it seems like this can only result in post data loss. For normal form-encoded data, it works properly, but for multipart data it outputs the final blank line instead of the entire post text.

I *suspect* this is some kind of attempt to avoid outputting e.g. the entire contents of a POSTed binary file - or maybe a mechanism to handle POSTing multipart forms with embedded form-encoded parts, but in this case (of a form that is POSTed with form-data) it does not work properly.

I can think of two ways to fix this:

1) We remove the call to utils.writePostDataTextParams, because escapeString does a good enough job of handling newlines and such, or rewrite that function to be (much) smarter about how it handles the data.
2) We redirect logic so that all multipart forms go through the "if (multipartRequest)", which appears to work properly (although it generates a much longer POST line, e.g. `--data-binary $'-----------------------------9897403161571613505923640\r\n\r\nContent-Disposition: form-data; name="user[email]"\r\n\r\naaa@aaa.aab\r\n-----------------------------9897403161571613505923640\r\n\r\nContent-Disposition: form-data; name="user[password]"\r\n\r\na\r\n-----------------------------9897403161571613505923640--\r\n'`)

Since I'm not particularly familiar with the codebase I'd like Honza to provide some advice; I can develop the relevant patch and make appropriate tests.
Flags: needinfo?(odvarko)
Clearing unnecessary needinfo flags.
Flags: needinfo?(info)
Flags: needinfo?(baptx.is)

@Tom, you've been working on similar bugs before.
Could you help with this?

Thanks!
Honza

Flags: needinfo?(odvarko) → needinfo?(glowka.tom)

Sure, happy to take that. Please assign it :)

Flags: needinfo?(glowka.tom) → needinfo?(odvarko)

Done thanks!
Honza

Assignee: nobody → glowka.tom
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)

Hi Tom. Just a quick check-in to know whether you are still interested in working on this bug. If not, just let me know and I'll un-assign it so others can pick it up.

Flags: needinfo?(glowka.tom)

Hi, thanks for heads-up, I postponed getting to it couple of times, but I think I really will do it this week.

Flags: needinfo?(glowka.tom)

The (In reply to Robert Xiao from comment #15)

Robert, sorry to make you wait that long. I can confirm all your findings – everything works perfect until the lines that you pointed out.

     // Create post data.
     const postData = [];
     if (utils.isUrlEncodedRequest(data) ||
           ["PUT", "POST", "PATCH"].includes(data.method)) {
       ...
     } else if (multipartRequest) {
       ...
     }

It looks like ["PUT", "POST", "PATCH"].includes(data.method) is the broadest condition (kind of default behavior for any postData request), but was put in front rather than after more sophisticated checks as multipartRequest one. So solution 2) is definitely way to go.

To avoid future regressions, it's worth mention there is relevant test that can be extended or be a good base for new one: https://hg.mozilla.org/mozreview/gecko/file/d4319ec6eeb2/devtools/client/netmonitor/test/browser_net_copy_as_curl.js

If you are still interested in providing a patch I can pass this issue to you. Just let me know.

Flags: needinfo?(nneonneo)

I'd be more comfortable letting you take the patch - especially since I don't have free cycles for at least another month.

Flags: needinfo?(nneonneo)

Sure, pretty understandable, just wanted to make sure.

I'll deliver the patch this week.

Fix main bug. Refine output of curl with multipart data payload.
Add missing units tests, including regression tests.

Thanks for the patch Tom!

I tested the patch and it works well for me (fixes STR in comment #14), it looks good code wise,
so let's see if the Try is green

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb6792527884197523f3ab2e4d34a842a44f2fad

Honza

Honza,
I have just fixed one pre-existing test based on the Try (but some tests still running). However I wanted to ask you to run the tests suite for Windows as well — there were some issues with proper quoting in the past, so I thinks it's worth checking upfront.
I filed the bug for restoring Try ssh access after couple of months of inactivity and I am currently waiting. Could you run that for me in the meantime?

Flags: needinfo?(odvarko)

Thanks for running Try!

So Win build failed, but the same way as linux without fix, so Win generally should be OK with the newest revision.

I also tried to make sure my tests are executed and it turns out they are not part of M-e10s(dt) but X xpcshell suite, so I guess we need to run one more Try:

try: -b do -p linux,win64 -u xpcshell,mochitest-dt -t none.

Can you please? :)

Flags: needinfo?(odvarko)

My Try access restored so I could scheduled appropriate job myself:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b84a8dce5c5c4db8499e6e0878ecf00bd97782d

Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3f507baa3de
Fix generating curl commands with multipart payload r=Honza

https://hg.mozilla.org/mozilla-central/rev/c3f507baa3de

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: