826798 - Network inspector does not correctly render cookie

Status: RESOLVED FIXED

(DevTools :: Console, defect, P2)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The stackoverflow.com site sets the following cookie:

Name: usr
Value: t=KxxxxqBWXEmn&s=n7IxxxwwU2A

This is rendered in the Inspect Network Request panel as:

usr=t

It probably incorrectly splits on the '=' somewhere.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix

Surprisingly I had this bug fix for parsing Set-Cookie, but I forgot to fix it for Cookie. Fixed it now and I added a test.

Please review. Thanks!

We might also want to land this in aurora.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 698752 [details] [diff] [review]
proposed fix

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

Looks good. parseSetCookieHeader could also use the |equal| local variable.

::: toolkit/devtools/webconsole/test/test_network_longstring.html
@@ +178,5 @@
>  
>    checkHeadersOrCookies(aResponse.cookies, {
>      foobar: "fooval",
>      omgfoo: "bug768096",
>    });

Shouldn't you add a 3rd cookie here?

Comment 3

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #2)
> Comment on attachment 698752 [details] [diff] [review]
> proposed fix
> 
> Review of attachment 698752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. parseSetCookieHeader could also use the |equal| local variable.

Sure.

> ::: toolkit/devtools/webconsole/test/test_network_longstring.html
> @@ +178,5 @@
> >  
> >    checkHeadersOrCookies(aResponse.cookies, {
> >      foobar: "fooval",
> >      omgfoo: "bug768096",
> >    });
> 
> Shouldn't you add a 3rd cookie here?

I didn't bother adding the 3rd cookies to all tests - as long as there's at least one test does what it needs to do. Will fix.

Thank you for the quick review!

Comment 4

[Mihai Sucan [:msucan]]

Attachment: updated patch

Comment 5

[Paul Rouget [:paul]]

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

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 699403 [details] [diff] [review]
updated patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 673148
User impact if declined: the web console network panel will continue to show incorrect values for some cookies that contain unescaped '='.
Testing completed (on m-c, etc.): landed in fx-team and m-c.
Risk to taking this patch (and alternatives if risky): close to no risks. alternatives: not really.
String or UUID changes made by this patch: no such changes.

Thank you!

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 699403 [details] [diff] [review]
updated patch

The minimal user impact here means this change can ride the trains.