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

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
VERIFIED FIXED
3 years ago
2 months ago

People

(Reporter: warner, Assigned: Stefan Yohansson, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

31 Branch
Firefox 55
x86
Mac OS X
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [netmonitor-reserve])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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?
(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
This would be super useful.

Comment 4

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

Comment 5

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

Comment 6

a year ago
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

a year ago
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.
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 :)
Assignee: nobody → colin
Status: NEW → ASSIGNED
Priority: -- → P2
See Also: → bug 1220758

Comment 9

4 months ago
colin, any new progress on this?
Flags: needinfo?(colin)

Updated

4 months ago
Blocks: 1307743
Whiteboard: [netmonitor]

Updated

4 months ago
Flags: qe-verify?
Priority: P2 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]

Updated

4 months ago
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu

Comment 10

4 months ago
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?
Assignee: colin → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(guohao.yan)

Updated

4 months ago
Mentor: gasolin@mozilla.com
Keywords: good-first-bug
(Assignee)

Comment 11

3 months ago
Created attachment 8848749 [details] [diff] [review]
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.
Attachment #8848749 - Flags: review?(gasolin)
(Assignee)

Updated

3 months ago
Assignee: nobody → sy.fen0

Updated

3 months ago
Status: NEW → ASSIGNED

Comment 12

3 months ago
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)) {
`
Attachment #8848749 - Flags: review?(odvarko)
Attachment #8848749 - Flags: review?(gasolin)
Attachment #8848749 - Flags: feedback+

Updated

3 months ago
Flags: needinfo?(guohao.yan)
Flags: needinfo?(colin)
(Assignee)

Comment 13

3 months ago
Created attachment 8849063 [details] [diff] [review]
bug1031956.patch

Updated code style.
Attachment #8848749 - Attachment is obsolete: true
Attachment #8848749 - Flags: review?(odvarko)
Attachment #8849063 - Flags: review?(gasolin)

Comment 14

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

Looks good to me, thanks!
Attachment #8849063 - Flags: review?(odvarko)
Attachment #8849063 - Flags: review?(gasolin)
Attachment #8849063 - Flags: review+
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
Attachment #8849063 - Flags: review?(odvarko) → review+

Updated

3 months ago
Duplicate of this bug: 1336485
Comment hidden (mozreview-request)

Comment 18

3 months ago
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

3 months ago
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?
Flags: needinfo?(sy.fen0)
(Assignee)

Comment 20

3 months ago
Created attachment 8850923 [details] [diff] [review]
bug1031956.patch

Thanks for the heads-up, I have updated it.
Attachment #8849063 - Attachment is obsolete: true
Flags: needinfo?(sy.fen0)
Attachment #8850923 - Flags: review?(gasolin)

Comment 21

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

Thanks!
Attachment #8850923 - Flags: review?(gasolin) → review+

Comment 22

3 months ago
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
Attachment #8850883 - Attachment is obsolete: true

Updated

3 months ago
Keywords: checkin-needed

Comment 23

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49b8b413df8
"Copy as cURL" is building GET when it should be POST. r=gasolin
Keywords: checkin-needed

Comment 24

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a49b8b413df8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

3 months ago
Iteration: --- → 55.2 - Apr 3
Priority: P3 → P1

Updated

3 months ago
Blocks: 1353535
> ...
> 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!
Flags: needinfo?(sy.fen0)
(Assignee)

Comment 26

2 months ago
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).
Flags: needinfo?(sy.fen0)
No problem, thanks for the clarification! Marking here as verified fixed per comment 25.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.