Closed Bug 972795 Opened 9 years ago Closed 8 years ago

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

Categories

(DevTools :: Netmonitor, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: vporof, Assigned: vporof)

Details

Attachments

(1 file, 2 obsolete files)

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.
(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...)
Status: NEW → ASSIGNED
QA Contact: vporof
Attached patch v1 wip (obsolete) — Splinter Review
Works, needs a test.
Assignee: nobody → vporof
Attached patch v2 wip (obsolete) — Splinter Review
Other tests don't fail anymore.
Attachment #8376186 - Attachment is obsolete: true
Attached patch v3Splinter Review
Rob's going to be sad if I send him another patch to review.
Attachment #8376279 - Attachment is obsolete: true
Attachment #8376319 - Flags: review?(dcamp)
Attachment #8376319 - Flags: review?(dcamp) → review?(rcampbell)
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* ?
Attachment #8376319 - Flags: review?(rcampbell) → review+
Shit, I need to land this.
https://hg.mozilla.org/mozilla-central/rev/89396a27e89e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
QA Whiteboard: [qa-]
QA Contact: vporof
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.