972795 - NetMonitor should show "Request Headers From Upload Stream" in the Headers tab, not only as part of the Request Payload in the Params tab

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

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

STR:

1. Sign out from https://github.com/
2. Go to https://github.com/login
3. Open Network Monitor
4. Sign in
5. Inspect the POST request to "session" (https://github.com/session)

Notice how these two headers:
> Content-Type: application/x-www-form-urlencoded
> Content-Length: 136
appear only as part of the request payload in the Params tab. They should show up in the Headers tab.

Both Chrome and Firebug correctly handle this, and subsequently parse the post payload as x-www-form-urlencoded, not just displaying it as plain text.

Comment 1

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

(as a sidenote, I wonder if the non SSL version of github.com also sends out your username and password as plain text in the POST body; probably not; I hope not...)

Comment 2

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

Attachment: v1 wip

Works, needs a test.

Comment 3

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

Attachment: v2 wip

Other tests don't fail anymore.

Comment 4

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

Attachment: v3

Rob's going to be sad if I send him another patch to review.

Comment 5

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

Try run: https://tbpl.mozilla.org/?tree=Try&rev=f15c2cf88be5

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8376319 [details] [diff] [review]
v3

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

couple comment nits. Otherwise all good.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +1008,5 @@
>              break;
>            case "requestPostData":
> +            // Search the POST data upload stream for request headers and add
> +            // them to a separate store, different from the classic headers.
> +            // XXX: Be really careful here! We're creating a function inside

do we want to ship an XXX comment?

@@ +2251,5 @@
> +    return promise.all([
> +      gNetwork.getString(postDataLongString),
> +      gNetwork.getString(contentTypeLongString)
> +    ])
> +    .then(([aPostData, aContentType]) => {

slightly awkward line-break. Suppose it preserves indentation, but looks a little weird.

@@ +2252,5 @@
> +      gNetwork.getString(postDataLongString),
> +      gNetwork.getString(contentTypeLongString)
> +    ])
> +    .then(([aPostData, aContentType]) => {
> +      // Handle query strings (poor man's forms, e.g. "?foo=bar&baz=42").

who is this fictional poor man who has forms? Might be confusing for people who aren't familiar with the term. I'd lose that and just include the example.

@@ +2254,5 @@
> +    ])
> +    .then(([aPostData, aContentType]) => {
> +      // Handle query strings (poor man's forms, e.g. "?foo=bar&baz=42").
> +      if (aContentType.contains("x-www-form-urlencoded")) {
> +        for (let section of aPostData.split(/\r\n|\r|\n/)) {

more splits!

::: browser/devtools/netmonitor/test/browser_net_post-data-03.js
@@ +14,5 @@
> +    let { RequestsMenu, NetworkDetails } = NetMonitorView;
> +    let TAB_UPDATED = aMonitor.panelWin.EVENTS.TAB_UPDATED;
> +    RequestsMenu.lazyUpdate = false;
> +
> +    Task.spawn(function() {

function* ?

Comment 7

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

Shit, I need to land this.

Comment 8

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

https://hg.mozilla.org/integration/fx-team/rev/89396a27e89e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89396a27e89e