1031956 - "Copy as cURL" is building GET when it should be POST

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Brian Warner [:warner :bwarner]]

I love the new "Copy as cURL" devtools feature added in bug 859059. I was trying it out in FF31 beta (about:buildconfig says https://hg.mozilla.org/releases/mozilla-beta/rev/a04918ac3197) on one of my webapps, and I noticed that using it on a POST request resulted in a cURL command line that actually does a GET. It's also lacking the "-d" option to include the JSON data that my app was submitting with the request.

The network panel had a row that said:

 200 POST eventchannel-create

the Headers pane said:

 Request URL: http://localhost:59276/api/v1/eventchannel-create
 Request method: POST
 .. and a bunch of Request Headers

and the Params pane said:

 {"token":"b3vito63bkux2ghgtfcbun5smu4mwhkxmrkp77qcspaaqofg76vq"}

After hitting "Copy as cURL", the clipboard contained the following string:

 curl 'http://localhost:59276/api/v1/eventchannel-create' -H 'Host: localhost:59276' -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0' -H 'Accept: application/json,*/*' -H 'Accept-Language: en-US,en;q=0.5' -H 'Accept-Encoding: gzip, deflate' -H 'DNT: 1' -H 'Referer: http://localhost:59276/control' -H 'Content-Length: 64' -H 'Content-Type: text/plain; charset=UTF-8' -H 'Connection: keep-alive' -H 'Pragma: no-cache' -H 'Cache-Control: no-cache'

The curl command includes a -H option for all the headers, and gets the URL right, but I expected it to include a '-X POST' and a -d '{"token":"b3vito63bkux2ghgtfcbun5smu4mwhkxmrkp77qcspaaqofg76vq"}' to represent the POST data properly. Interestingly, it *does* have a "Content-Length: 64", which implies that it's aware of the data. (actually using this curl command results in a stall, as the server waits for 64 bytes of data that will never come, even though it's a GET, which isn't supposed to have a request body).

I can reproduce this pretty easily.. let me know if I can help out at all.

Comment 1

[Brian Warner [:warner :bwarner]]

Digging into this a bit deeper, it looks like
browser/devtools/shared/Curl.jsm is a bit too eager to treat the request as a
form-post. The following patch fixes my observed problem and doesn't seem to
fail any tests:

diff --git a/browser/devtools/shared/Curl.jsm b/browser/devtools/shared/Curl.jsm
index 615c3fc..e3cd2f6 100644
--- a/browser/devtools/shared/Curl.jsm
+++ b/browser/devtools/shared/Curl.jsm
@@ -78,7 +78,7 @@ this.Curl = {
 
     // Create post data.
     let data = [];
-    if (utils.isUrlEncodedRequest(aData) || aData.method == "PUT") {
+    if (utils.isUrlEncodedRequest(aData) || aData.method == "PUT" || aData.method == "POST") {
       postDataText = aData.postDataText;
       data.push("--data");
       data.push(escapeString(utils.writePostDataTextParams(postDataText)));


But I'd like to write a test that exercises my case (and similar ones) more
directly. I'm thinking of something that just passes an "aData" string into
Curl.generateCommand() and examines the command string that it returns. I
couldn't find an existing test that behaves this way: I only found
browser_net_curl-utils.js (which seems to focus on constructing "aData" from
various real requests), and browser_net_copy_as_curl.js (which also uses a
real request, but only exercises a single GET).

I don't know a lot about the testing frameworks available here. If someone
points me at some examples, I might be able to add a few. (is an "XPCShell
test" the right tool for something that doesn't need a real browser window?
is there one in devtools/ or netmonitor/ that I could enhance?). I'd like to
write tests that exercise:

* plain GET
* POSTs created by various kinds of forms
* POSTs/PUTs created by XHR, with arbitrary (maybe binary) request bodies

I care the most about the last one (in my apps, I tend to use POST APIs which
accept and return JSON exclusively), but I know that traditional form
submission is still in use out there :).

I'm not really sure if it'd make sense for curl, but I'm inclined to think
that we could avoid doing very much processing and be more explicit with what
we tell curl to do. The code in isUrlEncodedRequest() that looks for
x-www-form-urlencoded in the request body makes me nervous (I'm thinking
about false positives when the JSON or JPEG or who knows what just happens to
contain that string), as does the test for multipart/form-data. Could we
instead just always pass -X METHOD and --data-binary with the exact contents
of the request body?

Comment 2

[Victor Porof [:vporof][:vp]]

(In reply to Brian Warner [:warner :bwarner] from comment #1)

Thanks for tracking this down Brian!

>      // Create post data.
>      let data = [];
> -    if (utils.isUrlEncodedRequest(aData) || aData.method == "PUT") {
> +    if (utils.isUrlEncodedRequest(aData) || aData.method == "PUT" ||
> aData.method == "POST") {
>        postDataText = aData.postDataText;
>        data.push("--data");

This looks like a harmless enough change to me.

> But I'd like to write a test that exercises my case (and similar ones) more
> directly. I'm thinking of something that just passes an "aData" string into
> Curl.generateCommand() and examines the command string that it returns. I
> couldn't find an existing test that behaves this way: I only found
> browser_net_curl-utils.js (which seems to focus on constructing "aData" from
> various real requests), and browser_net_copy_as_curl.js (which also uses a
> real request, but only exercises a single GET).
> 

I'm reasonably sure those are the only two tests available. The feature was introduced in bug 859059, and, at the time, it seemed Good Enough™ to land. However, the curl utility could surely use a few more tests.

> I don't know a lot about the testing frameworks available here. If someone
> points me at some examples, I might be able to add a few. (is an "XPCShell
> test" the right tool for something that doesn't need a real browser window?
> is there one in devtools/ or netmonitor/ that I could enhance?). I'd like to
> write tests that exercise:
> 
> * plain GET
> * POSTs created by various kinds of forms
> * POSTs/PUTs created by XHR, with arbitrary (maybe binary) request bodies
> 

I think the best way forward here is to add additional browser mochitest that exercise the scenarios described above, by cloning the existing tests, or creating new ones. You'll have to add the newly created files to browser.ini [0] and run them via

./mach mochitest-devtools browser/devtools/netmonitor/test/browser_net_...
 
> I'm not really sure if it'd make sense for curl, but I'm inclined to think
> that we could avoid doing very much processing and be more explicit with what
> we tell curl to do. The code in isUrlEncodedRequest() that looks for
> x-www-form-urlencoded in the request body makes me nervous (I'm thinking
> about false positives when the JSON or JPEG or who knows what just happens to
> contain that string), as does the test for multipart/form-data. Could we
> instead just always pass -X METHOD and --data-binary with the exact contents
> of the request body?

There was a very long ongoing discussion in bug 859059 about these methods, and we carefully looked at Firebug's and Blink's implementations as inspiration. Skim through that bug if you have the time, however I agree that some of this stuff is more empirical rather than scientific.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/browser.ini

Comment 3

[(not reading bugmail) Nick Desaulniers [:\n]]

This would be super useful.

Comment 4

[colin]

I'd love to see this build the full/correct cURL command!

Comment 5

[Karthik T]

I just hit this bug as well. Sad to see it has been unfixed for so long

Comment 6

[dindog]

What's going on? It a confirmed bug and seem some progress had been made, but don't know why stay that way for almost 2 years?

Comment 7

[ygormutti]

As a web developer that uses Aurora as main browser for some time, I'm concerned that such an obvious and simple bug takes so long to be fixed. It may just be the case the developers lost track of this bug because of its "NEW" status.

In Firefox Developer Edition we can't use Firebug 2.0 with e10s enabled, one of the best early-access features of this edition. Firebug 3 is still alpha and looks like it won't bring any real improvement over DevTools, which isn't yet up to par with Firebug 2.0.

Ironically, it seems like Firefox Developer Edition is the worst Firefox for developers.

Comment 8

[Michael Kohler [:mkohler]]

Assigning to Colin since he has worked on this today. The patch is more or less ready, he just needs to upload it from what I know.

@Colin: feel free to reach out to me if you need any help with that :)

Comment 9

[[e-mail]]

colin, any new progress on this?

Comment 10

[[e-mail]]

Since its been unanswered for a while, I think this issue is ready for other candidate to pick.

FYR, curl has been re-factored to
http://searchfox.org/mozilla-central/source/devtools/client/shared/curl.js


To solve this issue, you have to add POST method in curl.js condition

- if (utils.isUrlEncodedRequest(data) || data.method == "PUT") {
+ if (utils.isUrlEncodedRequest(data) || data.method == "PUT" || data.method == "POST") {

and add a test in browser_net_curl-utils.js
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_curl-utils.js

to make sure the result of requests.post contains related `--data` content


guohao, are you interest in solve this issue?

Comment 11

[Stefan Yohansson]

Attachment: bug1031956.patch

Hey, I have added the POST on conditional and added test to check --data on generated command, since testWritePostDataTextParams function is checking serialized --data value.

Please let me know if I need to change something.

Comment 12

[[e-mail]]

Comment on attachment 8848749 [details] [diff] [review]
bug1031956.patch

Review of attachment 8848749 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Overall it looks good and the test is passed

just need to fix one style nit

::: devtools/client/shared/curl.js
@@ +76,5 @@
>  
>      // Create post data.
>      let postData = [];
> +    if (utils.isUrlEncodedRequest(data)
> +        || ["PUT", "POST"].includes(data.method)) {

nit: we would put || at the end of line,
```
utils.isUrlEncodedRequest(data) ||
  ["PUT", "POST"].includes(data.method)) {
`

Comment 13

[Stefan Yohansson]

Attachment: bug1031956.patch

Updated code style.

Comment 14

[[e-mail]]

Comment on attachment 8849063 [details] [diff] [review]
bug1031956.patch

Looks good to me, thanks!

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8849063 [details] [diff] [review]
bug1031956.patch

Review of attachment 8849063 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

R+ assuming try is green

(we should push to try yet)

Honza

Comment 17

[[e-mail]]

Attachment: Bug 1031956 - is building GET when it should be POST

Review commit: https://reviewboard.mozilla.org/r/123394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123394/

Comment 18

[[e-mail]]

Hi, Stefan,

I push a new PR to test it on try server, if all test green we can land it into gecko, thanks!

Comment 19

[[e-mail]]

The test file has changed a bit therefore your patch cause conflict with the current gecko version.

Could you update your patch based on current gecko version?

Comment 20

[Stefan Yohansson]

Attachment: bug1031956.patch

Thanks for the heads-up, I have updated it.

Comment 21

[[e-mail]]

Comment on attachment 8850923 [details] [diff] [review]
bug1031956.patch

Thanks!

Comment 22

[[e-mail]]

Comment on attachment 8850883 [details]
Bug 1031956 -  is building GET when it should be POST

Try all green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05f46503fed8

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49b8b413df8
"Copy as cURL" is building GET when it should be POST. r=gasolin

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 25

[Ciprian Georgiu, Desktop QA]

> ...
> The curl command includes a -H option for all the headers, and gets the URL
> right, but I expected it to include a '-X POST' and a -d
> '{"token":"b3vito63bkux2ghgtfcbun5smu4mwhkxmrkp77qcspaaqofg76vq"}' to
> represent the POST data properly. Interestingly, it *does* have a
> "Content-Length: 64", which implies that it's aware of the data. (actually
> using this curl command results in a stall, as the server waits for 64 bytes
> of data that will never come, even though it's a GET, which isn't supposed
> to have a request body).
> 

I tested this with latest Nightly 55.0a1 2017-04-19 on a POST request but the cURL command line does not seem to include a "-X POST" and a "-d" option as mentioned in the above comment.

This are the strings from the clipboard after hitting "Copy as cURL" in netmonitor:

- before the fix - Nightly (54.0a1 2017-02-01)
curl "https://www.google.ro/gen_204?atyp=i&ct=slh&cad=&ei=Z3v3WIPgAcf6Uq7agIgE&s=2&v=2&pv=0.662386097514339&me=7:1492614009078,V,0,0,0,0:361,U,361:0,V,0,0,1550,307:22341,e,H&zx=1492614034384" --2.0 -H "Host: www.google.ro" -H "User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.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: https://www.google.ro/?gws_rd=ssl" -H "Content-Length: 0" -H "Content-Type: text/plain;charset=UTF-8" -H "Cookie: NID=101=btMHyF-3rJys24tnh_D90EfVQtBRFnZIhUYHkXZGvZxaLPEXtIsNkUObXJxK02xwFQ4lI3jkx-Bh6Ob3e3QQCq3y7MqJ-fMJpr10edp3T48Cinz9gn-MK5v2kwj8qUfa; CONSENT=WP.25f588; DV=Ui_rRtAOtngVpH3lGaNxj3QfU3UNtwI" -H "Connection: keep-alive"

- after the fix - latest Nightly (55.0a1 2017-04-19)
curl "https://www.google.ro/gen_204?atyp=i&ct=slh&cad=&ei=t3n3WPSxNsXxUtespcgN&s=2&v=2&pv=0.70000686085948&me=10:1492614007244,V,0,0,0,0:405,U,405:0,V,0,0,1920,396:12690,h,1,43,i:102,h,1,43,o:11850,h,1,43,i:79,h,1,43,o:5469,e,H&zx=1492614040348" --2.0 -H "Host: www.google.ro" -H "User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.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: https://www.google.ro/" -H "Content-Type: text/plain;charset=UTF-8" -H "Cookie: NID=101=cQqg99-ROiMqP2bikkdr_6sxcpsLGo-lAwkZLCViqae-r-iXBLelkUT_cZQCh4V4YDM270DwXDY7dmuyb60fjvh0vRfOjio3DxKSyt70wTt9Cd0e89kHN0arvZOvejq9; CONSENT=WP.25f568; DV=UqKG8KJ5IhMR3MwX6pa--3mIgXUNtwI" -H "Connection: keep-alive" --data ""

Maybe, I'm missing something here. Stefan, what do you think about this? 
Thanks!

Comment 26

[Stefan Yohansson]

Sorry for take ages to reply, I was busy in another project.

The "--data" on curl command is what I have added. Basically if "--data" (-d) is present, it'll be a POST request (-X POST).

Comment 27

[Ciprian Georgiu, Desktop QA]

No problem, thanks for the clarification! Marking here as verified fixed per comment 25.