851538 - "inspect network requests" concatenates multiple header field instances

Status: RESOLVED WONTFIX

(DevTools :: Console, defect, P3)

Description

[Julian Reschke]

Given a response containing the header fields:

  WWW-Authenticate: Basic realm=foo
  WWW-Authenticate: Basic realm=bar

The network inspector shows

  WWW-Authenticate:Basic realm=foo Basic realm=bar

(copying and pasting shows that the whitespace between "realm=foo" and "Basic" is actually a CR)

The expected result is that the actually received fields are displayed.

Comment 1

[Julian Reschke]

Note this is not security-sensitive; somebody who's authorized please remove the flag.

Comment 2

[Julian Reschke]

Link to test case that actually sends multiple header fields, in this case WWW-Authenticate:

http://greenbytes.de/tech/tc/httpauth/#multibasicunknown2mf

Comment 3

[Julian Reschke]

Attachment: draft patch

Here's a patch that yields the "right" result. However, it appears that the right way to fix this would be to fix the result of the headers member of httpactivity.

Comment 4

[Patrick McManus [:mcmanus]]

Thanks julian!

Comment 5

[Julian Reschke]

(In reply to Julian Reschke from comment #3)
> Created attachment 727544 [details] [diff] [review]
> draft patch
> 
> Here's a patch that yields the "right" result. However, it appears that the
> right way to fix this would be to fix the result of the headers member of
> httpactivity.

Expanding on that: the headers list obtained from the HttpActivity currently has one entry per header field name, not per header field instance.

If a header field was received multiple times (as in my example), the returned value just contains the concatenation of the fields, separated by \n.

I *believe* this is the wrong thing to do in the first place and should be changed to actually put multiple entries for the same header field name into the list. However, I have no idea if and what other parts of the code would break.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 727544 [details] [diff] [review]
draft patch

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

Thank you for the bug report and for the patch!

The patch is fine, except we should probably make this change in the web console server code. See the NetworkMonitor in WebConsoleUtils.jsm.

However, I'd like to better understand this bug. Is this really a bug? See HTTP 1/1 specification (RFC 2616), section 4.2, last paragraph. [1] We get the merged header values from Gecko - it's not the Web Console that does the merging (IIANM).

I'm not yet sure if we want this fix. How is this case handled by other developer tools in other web browsers?

[1] http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

Comment 7

[Julian Reschke]

(In reply to Mihai Sucan [:msucan] from comment #6)
> Comment on attachment 727544 [details] [diff] [review]
> draft patch
> 
> Review of attachment 727544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the bug report and for the patch!
> 
> The patch is fine, except we should probably make this change in the web
> console server code. See the NetworkMonitor in WebConsoleUtils.jsm.
> 
> However, I'd like to better understand this bug. Is this really a bug? See
> HTTP 1/1 specification (RFC 2616), section 4.2, last paragraph. [1] We get
> the merged header values from Gecko - it's not the Web Console that does the
> merging (IIANM).

Whoever does the merging does it in a non-compliant way; using CR instead of ",".
 
> I'm not yet sure if we want this fix. How is this case handled by other
> developer tools in other web browsers?

I'm pretty sure it should be fixed; what's current displayed is totally incorrect.

Chrome displays it correctly.

Comment 8

[Mihai Sucan [:msucan]]

This works as intended, for now. We only show the parsed headers. We can also add the raw headers, as an option. Also see bug 859133.

Victor: this is something we can do at the NetworkEventActor level, and in NetworkMonitor. We have Gecko APIs that allow us to also retrieve the raw headers.

Julian: do you want to work on this? I can explain what needs to be done, on IRC or here.

Comment 9

[Julian Reschke]

(In reply to Mihai Sucan [:msucan] from comment #8)
> This works as intended, for now. We only show the parsed headers. We can
> also add the raw headers, as an option. Also see bug 859133.
> ...

How can it be "intended" to show a misleading and incorrect field value?

Comment 11

[Mihai Sucan [:msucan]]

Do you still plan to work on this? If yes, a better use of your time would be if you could fix bug 859133. I can mentor you on how to fix it.

If you no longer plan to work on this bug, we should reset the assignee.

Thank you!

Comment 12

[Tim Nguyen :ntim]

This popup has recently been removed.